fix(drive,docs): build request before the lease gate

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 `<ledger>.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.
 

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<'_>,

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,20 +2322,29 @@ 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,
             ..Default::default()
         };
         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 } => {
                 assert!(detail.contains("not a color"), "{detail}");
             }
             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 ───────────────

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());
+    }
 }