diff --git a/CHANGELOG.md b/CHANGELOG.md index 08cf7043..634ed5e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **The Drive lease ledger's lock is now a `flock(2)`, not a `create_new` marker that a holder's own `Drop` unlinked by path** ([#1687](https://github.com/rust-works/omni-dev/issues/1687)): the old lock was ledger-global and non-waiting — a leased write to one file hard-failed a concurrent leased write to an *unrelated* file for the whole duration of the first's upload — and its own error advice, "remove the lock file and retry", reopened a lease-token double-spend if issued while the original holder was still live (the second acquirer's new marker would then be deleted by the first holder's `Drop`, with no identity check, letting a third acquirer in mid-write). A SIGKILLed holder also left a permanent lock with no stale-lock detection. The lock is now a persistent-sibling `.lock` `flock`, kernel-released on process death, so a crashed holder never leaves a stale lock and `Drop` never unlinks anything — nothing advises deleting the file any more (on Unix; non-Unix has no `flock` and falls back to the old marker-plus-unlink scheme, so this guarantee doesn't extend there). Every leased write now *waits* for a busy lock (printing a one-line notice, `OMNI_DEV_LEASE_LOCK_WAIT_SECS`-bounded) instead of hard-failing; the lock remains ledger-global, so this narrows "hard-fail" to "wait", it does not add per-file scope (tracked as a follow-up). `drive lease prune` now secures the lock *before* touching a row's backup rather than after, so a lock collision leaves both the row and its backup untouched instead of stranding a row that points at an irrecoverably deleted backup. The identical fix applies to `gmail insert`'s own lock (`src/cli/gmail/insert/ledger.rs`), which stays non-waiting since it is held for a whole multi-message run. See [docs/drive.md](docs/drive.md#concurrent-access) and [ADR-0080](docs/adrs/adr-0080.md) §4. +- **`drive sheets format-cells`/`update-borders`/etc. and the `structure` verbs no longer orphan a `pending` audit record when building the request fails after the lease gate** ([#1688](https://github.com/rust-works/omni-dev/issues/1688), [ADR-0080](docs/adrs/adr-0080.md) §11): `gate_leased_write` fsyncs the write-ahead `pending` record and only the mutating call's own `allowed`/`failed` outcome concludes it, but `format.rs`/`structure.rs` built their `batchUpdate` request (`parse_hex_color`, a resolved sheet's missing `sheetId`) *after* the gate — so a request that could never be issued (e.g. `update-borders --color ZZZZZZ` under an otherwise-valid `--lease`) still opened a `pending` record with nothing to close it, indistinguishable from a process that died mid-write. Both engines now build the request before the gate, matching `protection.rs`'s existing shape (`validation.rs`/`sheets/write.rs`/`docs/write.rs`/`content_edit.rs` were never affected — each either builds first already or has no fallible step between the gate and its mutating call). One accepted side effect: a malformed request under a bad or absent lease now reports the request-build refusal rather than the lease refusal, which is the ordering both engines already use ahead of the gate for every other pre-lease refusal. - **`gmail sync-all`'s rate-limit retry notices no longer corrupt the live progress bars or go unattributed** ([#1651](https://github.com/rust-works/omni-dev/issues/1651)): the shared retry loop (`retry_if`, `src/utils/http.rs`) printed a raw `eprintln!` while waiting out a 429/403, so on a `sync-all` run — where every account's bars share one `indicatif::MultiProgress` ([#1504](https://github.com/rust-works/omni-dev/issues/1504)) — a rate limit on any account teared the render and named no account, leaving no way to tell which one was throttled. `retry_if`/`retry_429` gain an optional `notify` callback (`RetryNotifyFn`) invoked in place of the `eprintln!`; Atlassian, Datadog and Drive keep the default fallback unchanged (they pass `None`), while `GmailClient` exposes `set_retry_notify` so `gmail sync`/`sync-all` can register one once a progress channel exists. The notice now renders as a transient message on the throttled account's own fetch bar — already prefixed with the account label in `sync-all` — via a new `SyncProgressEvent::RateLimited` event, and clears itself once that fetch's retry resolves. - **`worktrees ui`'s embedded-terminal shutdown could still hang forever, this time on the kernel rather than a child process** ([#1611](https://github.com/rust-works/omni-dev/issues/1611)): [#1605](https://github.com/rust-works/omni-dev/issues/1605)/[#1610](https://github.com/rust-works/omni-dev/issues/1610) bounded the child-side wait (a SIGHUP-ignoring shell) by reaping the whole process group with escalation before joining the PTY thread — but `TerminalTab::shutdown` ran that join, and the `Pty` drop inside it, on `tokio::task::spawn_blocking`. Nobody ever awaited that task, yet `tokio::Runtime::drop()` still blocks until every outstanding blocking-pool task finishes, so anything that made the join-then-drop take unboundedly long could still wedge a shutdown — reintroducing the exact class of bug #1605 fixed, from a new cause. Live `lldb` debugging of a hung process (reading the actual `pid` argument off the blocked `wait4()` syscall, confirmed on two independent reproductions) found that cause: a `TabKind::Shell` tab's child is `/usr/bin/login` on macOS, and a killed `login` process can land in the kernel's own exit teardown (`ps` reports state `Es+`) and never finish it — the SIGKILL genuinely lands, but the reap that would collect the resulting zombie simply never completes. Nothing in-process can bound a stuck kernel-side teardown, so `shutdown` now joins the PTY thread and drops the `Pty` on a plain, detached `std::thread` instead of `spawn_blocking` — untracked by the runtime, so a stuck reap can no longer hold up `Runtime::drop`, whether in a test's per-test runtime or the real CLI's in `main.rs`. This was a real bug, not a test artifact: `shutdown` also runs on ordinary app quit, so a user's own Shell tab could in principle wedge `omni-dev worktrees ui` on exit the same way. Verified with a 20-run soak of `cargo test --lib worktrees::ui` at default parallelism (0 hangs, versus roughly 2 in 3 runs hanging beforehand); a new test pins the structural property the fix relies on — a detached thread that never finishes cannot block a runtime's drop, unlike a `spawn_blocking` task, which an adjoining `#[ignore]`d test documents by demonstrating the hang it *would* cause if run. diff --git a/src/drive/lease/check.rs b/src/drive/lease/check.rs index 2b22b26f..f5f6cc85 100644 --- a/src/drive/lease/check.rs +++ b/src/drive/lease/check.rs @@ -305,6 +305,15 @@ pub(crate) enum LeaseGateRefusal { /// it alive across the mutating call and into [`finish_leased_write`]/ /// [`finish_leased_native_write`]) inside its [`LeaseGrant`], or the /// reason for refusal. +/// +/// A caller must build its mutating request *before* calling this — this +/// must be the last fallible step before the mutating call itself. A +/// success here fsyncs the write-ahead `pending` audit record, and nothing +/// else concludes it except the mutating call's own `allowed`/`failed` +/// outcome; a fallible step still ahead of the caller (building the +/// request, resolving its target) can refuse *after* that record is +/// written, leaving it orphaned as though the process had died mid-write +/// (#1688). pub(crate) async fn gate_leased_write( write: LeasedWrite<'_>, files_api: &FilesApi<'_>, diff --git a/src/drive/sheets/format.rs b/src/drive/sheets/format.rs index 92c55bd8..5520a969 100644 --- a/src/drive/sheets/format.rs +++ b/src/drive/sheets/format.rs @@ -607,6 +607,17 @@ async fn format_inner( }); } + // Built before the gate, not after: `build_request` (in particular its + // `parse_hex_color` call) is the one fallible step between here and the + // mutating call, and the gate must be the *last* fallible step before + // it (see `gate_leased_write`'s doc comment) — otherwise a request that + // was always going to fail to build would still fsync a `pending` + // audit record for a write that never happens (#1688). + let request = match build_request(&opts.verb, &resolved) { + Ok(request) => request, + Err(detail) => return gated(FormatResult::RefusedInvalidRange { detail }), + }; + // The lease check (ADR-0080 §9) sits here: after the permission gate // and the `--dry-run` branch, before the mutating call — see // `content_edit.rs::edit_inner`'s doc comment for the full reasoning, @@ -634,11 +645,6 @@ async fn format_inner( None }; - let request = match build_request(&opts.verb, &resolved) { - Ok(request) => request, - Err(detail) => return gated(FormatResult::RefusedInvalidRange { detail }), - }; - let result = match api.batch_update(&opts.spreadsheet_id, vec![request]).await { Ok(_response) => { if let (Some(token), Some(grant)) = (&opts.lease_token, &lease_grant) { @@ -2316,6 +2322,8 @@ mod tests { // No batchUpdate mock mounted: `describe_effect` doesn't validate // the color (only `sides.any()`), so this proves the request-build // step's own `parse_hex_color` call is the one that catches it. + let dir = tempfile::tempdir().unwrap(); + let audit = crate::test_support::AuditLogGuard::redirect(dir.path()); let rules = vec![allow_rule("folder-1")]; let sides = BorderSides { top: true, @@ -2323,6 +2331,8 @@ mod tests { }; let verb = update_borders_verb(sides, "SOLID", Some("ZZZZZZ")); + // `opts()` seeds a live, matching lease, so this exercises the real + // gate — not just a verb that never reaches it. let outcome = format(&drive, &sheets, &opts(verb, false), &rules).await; match outcome.result { FormatResult::RefusedInvalidRange { detail } => { @@ -2330,6 +2340,11 @@ mod tests { } other => panic!("expected RefusedInvalidRange, got {other:?}"), } + + // #1688: `build_request` now runs before the gate, so a request + // that can never be issued must never open a `pending` audit + // record in the first place — there is nothing to conclude it. + assert!(audit.records().is_empty(), "{:?}", audit.records()); } // ── format_inner: full non-dry-run round trips per verb ─────────────── diff --git a/src/drive/sheets/structure.rs b/src/drive/sheets/structure.rs index b4cc3702..cb653fe4 100644 --- a/src/drive/sheets/structure.rs +++ b/src/drive/sheets/structure.rs @@ -677,6 +677,19 @@ async fn structure_inner( return gated(StructureResult::WouldChange { sheet, sheet_count }); } + // ── The request ──────────────────────────────────────────────────── + // Built before the gate, not after: a sheet that resolved by title but + // reported no `sheetId` (a server contract violation `build_request` + // refuses on) is the one fallible step between here and the mutating + // call, and the gate must be the *last* fallible step before it (see + // `gate_leased_write`'s doc comment) — otherwise a request that was + // always going to fail to build would still fsync a `pending` audit + // record for a write that never happens (#1688). + let request = match build_request(&opts.verb, sheet.as_ref()) { + Ok(request) => request, + Err(detail) => return gated(StructureResult::Failed { detail }), + }; + // The lease check (ADR-0080 §9) sits here: after the permission gate // and the `--dry-run` branch, before the mutating call — see // `content_edit.rs::edit_inner`'s doc comment for the full reasoning, @@ -709,12 +722,6 @@ async fn structure_inner( None }; - // ── The mutation ─────────────────────────────────────────────────── - let request = match build_request(&opts.verb, sheet.as_ref()) { - Ok(request) => request, - Err(detail) => return gated(StructureResult::Failed { detail }), - }; - let result = match api.batch_update(&opts.spreadsheet_id, vec![request]).await { Ok(response) => { if let (Some(token), Some(grant)) = (&opts.lease_token, &lease_grant) { @@ -5186,4 +5193,54 @@ mod tests { // see which verb ran without joining back to `log.jsonl`. assert_eq!(records[0].command, ["drive", "sheets-rename-sheet"]); } + + /// #1688: a sheet that resolved by title but reported no `sheetId` (a + /// server contract violation, see [`build_request_fails_rather_than_guessing_a_missing_sheet_id`]) + /// is exactly the case `build_request` refuses on — and it must never + /// open a `pending` audit record for a write that can never be issued. + /// `build_request_fails_rather_than_guessing_a_missing_sheet_id` covers + /// the pure function; this covers the engine path through it, which was + /// untested before this fix (the bug this test guards against left the + /// `pending` record unconcluded). + #[tokio::test] + async fn a_missing_sheet_id_fails_before_opening_the_audit_pair() { + let server = wiremock::MockServer::start().await; + let (drive, sheets) = clients(&server).await; + mount_file("sheet-1", GOOGLE_SHEET_MIME_TYPE, &["parent-1"]) + .mount(&server) + .await; + mount_folder("parent-1").mount(&server).await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(wiremock::matchers::path("/v4/spreadsheets/sheet-1")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "spreadsheetId": "sheet-1", + "properties": {"title": "Budget"}, + "sheets": [ + {"properties": {"title": "Q2", "index": 0}} + ], + })), + ) + .mount(&server) + .await; + // No batchUpdate mock mounted: `build_request`'s own missing-`sheetId` + // check is the one that must catch this, before any mutating call. + let dir = tempfile::tempdir().unwrap(); + let audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + + let outcome = structure( + &drive, + &sheets, + &opts(rename(), false), + &[allow_rule("parent-1")], + ) + .await; + match outcome.result { + StructureResult::Failed { detail } => { + assert!(detail.contains("sheetId"), "{detail}"); + } + other => panic!("expected Failed, got {other:?}"), + } + assert!(audit.records().is_empty(), "{:?}", audit.records()); + } }