From 5e311314dce42f943ffee1073c9d937e448a2d7f Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 01:13:02 +1000 Subject: [PATCH 1/2] fix(drive,docs): refuse an already-leased file before backing up drive lease acquire's only live-lease check lived inside insert_record, after the Touch ID prompt and the full backup - the normal outcome of any second acquire on an already-leased file, not just a rare race, and the orphaned backup was unreachable by drive lease prune (#1678), which only ever iterates ledger rows. Add a lock-free live_lease_for_file pre-check before authenticating at all, refusing the common case for free. Leave the ledger lock itself unheld across the prompt - it's a non-blocking, machine-wide lock, so holding it across a 120s human prompt would fail every other concurrent lease op - so insert_record's lock-held check stays the sole authoritative gate for the pre-check's narrow remaining race. Reclaim the backup this attempt took on every exit but Acquired, reusing prune's own clear_backup, as belt-and-braces for that race and for the other outcomes that can still follow a real backup: a post-backup metadata-fetch failure, a missing post-backup version, or insert_record itself losing to a concurrently held lock. Warn and audit a reclamation failure with a -backup-orphaned verdict suffix naming the surviving backup's location, rather than silently orphaning it. Fixes #1690. --- CHANGELOG.md | 1 + docs/log.md | 29 +- src/drive/lease/acquire.rs | 557 +++++++++++++++++++++++++++++++------ src/drive/lease/ledger.rs | 20 ++ src/drive/lease/prune.rs | 26 +- src/drive/lease/restore.rs | 19 +- 6 files changed, 540 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 634ed5e0..2feebc72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,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. +- **`drive lease acquire` no longer spends a Touch ID prompt and a real backup on a file that's already leased, and the backup this fixes** ([#1690](https://github.com/rust-works/omni-dev/issues/1690)): the only live-lease check used to live inside `insert_record`, *after* the prompt and the full backup — the normal outcome of any second `acquire` on an already-leased file (a "did I already lease this?" retry, a lost token), not merely a rare race, and the just-taken backup was neither deleted nor recorded in any ledger row, so `drive lease prune` ([#1678](https://github.com/rust-works/omni-dev/issues/1678), which only ever iterates ledger rows) could never reclaim it. A new lock-free `live_lease_for_file` pre-check now runs before authenticating at all, refusing the common case for free; the ledger lock itself is deliberately *not* held across the prompt (`LedgerLock::acquire` is a non-blocking, machine-wide `create_new` lock, so holding it across a 120s human prompt would fail every other concurrent `drive lease` operation), so `insert_record`'s own lock-held check remains the sole authoritative gate for the pre-check's narrow remaining race. Belt-and-braces for that race, and for the other outcomes that can still follow a real backup (a post-backup `files.get` failure, a missing post-backup `version`, or `insert_record` itself failing against a concurrently-held lock): the backup this attempt took is now reclaimed (deleted/trashed, via the same `clear_backup` `drive lease prune` uses) on every exit but `Acquired`, and a reclamation failure is both warned and audited with a `-backup-orphaned` verdict suffix naming the surviving backup's location, rather than silently orphaning it as before. - **`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/docs/log.md b/docs/log.md index 50c64520..d0472ddf 100644 --- a/docs/log.md +++ b/docs/log.md @@ -470,18 +470,27 @@ leased-write lifecycle, landing across that same issue's phases. **`drive lease acquire`** writes one record per attempt, `command: ["drive", "lease-acquire"]`, regardless of outcome — `verdict` is `acquired`, +`acquired-headless-waiver` (ADR-0080 §8/§13, issue #1677: proceeded under the +headless opt-out, no human ever prompted), `already-leased`, `refused-native-document`, `denied`, `unavailable`, or `failed`, matching -`AcquireResult`'s own kebab-case status. An `acquired` record additionally -carries `lease_id` (the token), `version_after`/`modified_time_after` (the -Drive state actually recorded into the ledger — re-read after the backup, -not the pre-authentication snapshot, for the same TOCTOU reason the ledger -itself does), `backup_location` (a local path for a byte backup, or the -backup copy's own file id for a native-document backup), +`AcquireResult`'s own kebab-case status — plus, on `already-leased`/`failed`, +the `-backup-orphaned` suffix (issue #1690): this attempt took a real backup +before discovering it isn't referenced by any ledger row, and reclaiming +that backup itself then failed, so it is now truly orphaned (`drive lease +prune` cannot see it either). An `acquired` record additionally carries +`lease_id` (the token), `version_after`/`modified_time_after` (the Drive +state actually recorded into the ledger — re-read after the backup, not the +pre-authentication snapshot, for the same TOCTOU reason the ledger itself +does), `backup_location` (a local path for a byte backup, or the backup +copy's own file id for a native-document backup), `backup_sha256`/`backup_size` (byte backups only), and `auth_policy` -(`device-owner` or `biometrics-only`). Unlike a leased *write*'s own record -(below), this one is best-effort rather than write-ahead/fail-closed: -acquiring mutates no Drive content — by the time the record is written the -consent, the backup and the ledger row have already durably happened, so a +(`device-owner` or `biometrics-only`). An `already-leased`/`failed` record +also carries `backup_location` whenever this attempt took a backup, whether +or not reclaiming it succeeded — the `-backup-orphaned` suffix is what +distinguishes the two. Unlike a leased *write*'s own record (below), this +one is best-effort rather than write-ahead/fail-closed: acquiring mutates no +Drive content — by the time the record is written the consent, the backup +and the ledger row have already durably happened, so a logging failure is warned and does not turn a successful acquisition into a reported failure. diff --git a/src/drive/lease/acquire.rs b/src/drive/lease/acquire.rs index e21e3b16..9c659f4d 100644 --- a/src/drive/lease/acquire.rs +++ b/src/drive/lease/acquire.rs @@ -4,7 +4,13 @@ //! a Google-native document (Sheet/Doc/Slide) backs up as a lossless //! Drive-side `files.copy` into the account's configured backup folder //! (§3's fidelity split) — refused outright, before authenticating at all, -//! when no backup folder is configured for this account. +//! when no backup folder is configured for this account, or when a live +//! lease already covers the target file (issue #1690). The latter refusal +//! is a fast path, not the authoritative gate — see [`acquire_inner`] — so +//! the narrow remaining race still spends a prompt and a backup; that +//! backup is then reclaimed (deleted/trashed) rather than left orphaned, +//! since `drive lease prune` can only ever see backups a ledger row +//! points at. use std::path::{Path, PathBuf}; @@ -78,8 +84,12 @@ pub enum AcquireResult { /// second writer silently clobber the first's write (issue #1664 /// review finding) — see /// [`LeaseLedger::live_lease_for_file`](super::ledger::LeaseLedger::live_lease_for_file)'s - /// doc comment. A fresh Touch ID prompt was already spent finding this - /// out; the backup this attempt took (if any) is orphaned. + /// doc comment. A lock-free pre-check in [`acquire_inner`] refuses most + /// of these *before* authenticating at all (issue #1690); only the + /// narrow remaining race — another process inserting its own lease + /// between that pre-check and the authoritative, lock-held check — + /// still spends a prompt and a backup first, and that backup is + /// reclaimed (deleted/trashed) automatically rather than left orphaned. AlreadyLeased { /// The existing lease's token — present this to `--lease` instead. token: String, @@ -132,8 +142,8 @@ pub async fn acquire( opts: &AcquireOptions, authenticator: &dyn Authenticator, ) -> AcquireResult { - let result = acquire_inner(client, opts, authenticator).await; - record_attempt(opts, &result); + let (result, disposition) = acquire_inner(client, opts, authenticator).await; + record_attempt(opts, &result, disposition.as_ref()); result } @@ -148,42 +158,104 @@ pub(crate) const MIN_EXPIRY_MINUTES: i64 = 1; /// See [`MIN_EXPIRY_MINUTES`]. pub(crate) const MAX_EXPIRY_MINUTES: i64 = 24 * 60; +/// What became of a backup this attempt took, once [`finish_acquisition`] +/// decided it is referenced by no ledger row — the belt-and-braces half of +/// issue #1690's fix, for the narrow race the lock-free pre-check in +/// [`acquire_inner`] cannot close. `None` everywhere in +/// [`acquire_inner`]'s return value means this attempt never took a +/// backup at all (refused/denied/unavailable before ever reaching step 2). +enum BackupDisposition { + /// Deleted (bytes) or trashed (Drive copy) before returning. + Reclaimed(LeaseBackup), + /// Reclaiming it itself failed; the backup is still on disk/Drive, + /// now truly orphaned — `drive lease prune` cannot see it either, + /// since no ledger row points at it. Carried so the audit record can + /// still name its location for a human to clean up. + ReclaimFailed(LeaseBackup), +} + async fn acquire_inner( client: &DriveClient, opts: &AcquireOptions, authenticator: &dyn Authenticator, -) -> AcquireResult { +) -> (AcquireResult, Option) { let expiry_minutes = opts.expiry.num_minutes(); if !(MIN_EXPIRY_MINUTES..=MAX_EXPIRY_MINUTES).contains(&expiry_minutes) { - return AcquireResult::Failed { - detail: format!( - "expiry must be between {MIN_EXPIRY_MINUTES} and {MAX_EXPIRY_MINUTES} minutes, \ - got {expiry_minutes}" - ), - }; + return ( + AcquireResult::Failed { + detail: format!( + "expiry must be between {MIN_EXPIRY_MINUTES} and {MAX_EXPIRY_MINUTES} \ + minutes, got {expiry_minutes}" + ), + }, + None, + ); } let files_api = FilesApi::new(client); let target = match files_api.get_metadata(&opts.file_id).await { Ok(target) => target, Err(err) => { - return AcquireResult::Failed { - detail: err.to_string(), - } + return ( + AcquireResult::Failed { + detail: err.to_string(), + }, + None, + ) } }; let is_native = target.is_google_native(); if is_native && opts.native_backup_folder_id.is_none() { - return AcquireResult::RefusedNativeDocument; + return (AcquireResult::RefusedNativeDocument, None); } if target.version.is_none() { - return AcquireResult::Failed { - detail: "Drive did not return a `version` for this file; refusing to lease it \ - without a staleness check" - .to_string(), - }; + return ( + AcquireResult::Failed { + detail: "Drive did not return a `version` for this file; refusing to lease it \ + without a staleness check" + .to_string(), + }, + None, + ); + } + + // Lock-free live-lease pre-check (issue #1690): cheap, and + // deliberately *not* the authoritative gate — `insert_record`, taken + // under the ledger lock inside `finish_acquisition` below, remains + // that (see its own doc comment). This one's only job is refusing + // *before* spending the (up to 120s) Touch ID prompt and a real + // backup on the common case this closes: a second `acquire` on a + // file that already has a live lease — a "did I already lease this?" + // retry, a lost token — not merely a rare race. A lease that goes + // live or expires in the window between this check and the + // authoritative one is still decided correctly there either way: this + // fast path can only ever be *more* conservative than the real gate + // (refusing a lease that in fact expires a moment later, which just + // means the caller retries), never less, so the two can never + // disagree in the direction that would matter. + let pre_check = tokio::task::block_in_place(|| LeaseLedger::load(&opts.ledger_path)); + match pre_check { + Ok(ledger) => { + if let Some(existing) = ledger.live_lease_for_file(&opts.file_id, Utc::now()) { + return ( + AcquireResult::AlreadyLeased { + token: existing.token.clone(), + expires_at: existing.expires_at, + }, + None, + ); + } + } + Err(err) => { + return ( + AcquireResult::Failed { + detail: err.to_string(), + }, + None, + ) + } } // 1. Authenticate — consent gates the action, not merely possession of @@ -212,7 +284,7 @@ async fn acquire_inner( tokio::task::block_in_place(|| authenticator.authenticate(&reason, opts.auth_policy)); let headless_waiver = match auth_outcome { AuthOutcome::Authorized => false, - AuthOutcome::Denied(detail) => return AcquireResult::Denied { detail }, + AuthOutcome::Denied(detail) => return (AcquireResult::Denied { detail }, None), AuthOutcome::Unavailable(detail) => { // ADR-0080 §8/§13: an explicit, per-installation opt-out lets // this proceed with no human ever having been prompted, rather @@ -220,7 +292,7 @@ async fn acquire_inner( // `Acquired` result (and so the audit record) is what makes // this waiver durably visible. if !opts.allow_headless { - return AcquireResult::Unavailable { detail }; + return (AcquireResult::Unavailable { detail }, None); } true } @@ -234,22 +306,50 @@ async fn acquire_inner( match native_backup(&files_api, &opts.file_id, folder_id, &target.name).await { Ok(backup) => backup, Err(err) => { - return AcquireResult::Failed { - detail: err.to_string(), - } + return ( + AcquireResult::Failed { + detail: err.to_string(), + }, + None, + ) } } } else { match byte_backup(&files_api, &opts.file_id, &opts.backup_dir, &target.name).await { Ok(backup) => backup, Err(err) => { - return AcquireResult::Failed { - detail: err.to_string(), - } + return ( + AcquireResult::Failed { + detail: err.to_string(), + }, + None, + ) } } }; + // From here on a backup exists, so every remaining exit must account + // for it: `Acquired` because a ledger row now references it, every + // other outcome by reclaiming it (issue #1690's belt-and-braces half, + // for the pre-check's narrow remaining race). + let result = finish_acquisition(&files_api, opts, backup.clone(), headless_waiver).await; + if matches!(result, AcquireResult::Acquired { .. }) { + return (result, None); + } + let disposition = reclaim_backup(&files_api, backup).await; + (result, Some(disposition)) +} + +/// Steps 3–4: re-fetches metadata, checks the file's live lease under the +/// ledger lock, and mints the lease. Split out of `acquire_inner` so every +/// exit past the backup step shares that function's one reclaim wrapper — +/// this function only ever decides mint-or-refuse, never reclaims. +async fn finish_acquisition( + files_api: &FilesApi<'_>, + opts: &AcquireOptions, + backup: LeaseBackup, + headless_waiver: bool, +) -> AcquireResult { // 3. Ledger record. `version`/`modified_time` are re-fetched here // rather than reused from the `target` metadata read at the very top — // that read happened before the (up to 120s) Touch ID prompt and @@ -296,13 +396,13 @@ async fn acquire_inner( // the `authenticate` call above documents. match tokio::task::block_in_place(|| insert_record(record, &opts.ledger_path)) { Ok(InsertOutcome::Inserted) => {} - // Refuse rather than mint a second, independent lease on a file - // that already has a live one — see `AcquireResult::AlreadyLeased`'s - // doc comment. The backup just taken above is orphaned: it was - // never inserted into the ledger, so `drive lease prune` (#1678, - // which only ever iterates existing ledger rows) cannot discover - // or reclaim it — a known gap this race leaves open, distinct from - // the expired/released-row growth `lease prune` does bound. + // Refuse rather than mint a second, independent lease — see + // `AcquireResult::AlreadyLeased`'s and `insert_record`'s own doc + // comments for why this stays the authoritative gate even with + // the lock-free pre-check in `acquire_inner`. Reclaiming the + // backup this attempt just took happens there, once it sees this + // isn't `Acquired` — not here, since this function's only job is + // deciding mint-or-refuse. Ok(InsertOutcome::AlreadyLeased(existing)) => { return AcquireResult::AlreadyLeased { token: existing.token, @@ -325,6 +425,38 @@ async fn acquire_inner( } } +/// Deletes/trashes a backup this attempt itself just took, once +/// `acquire_inner` has decided it ends up referenced by no ledger row — +/// the belt-and-braces half of issue #1690's fix, for the narrow race the +/// lock-free pre-check above cannot close by itself. Reuses +/// [`super::prune::clear_backup`], which already handles both +/// [`LeaseBackup`] variants and tolerates an already-absent backup. Safe +/// specifically because `backup` is always one this very call just +/// created moments ago under a fresh identity — `write_backup`'s +/// `create_new`/`O_EXCL` open, or `native_backup`'s brand-new `files.copy` +/// id — never one an existing ledger row (live, expired, or already +/// pruned) could still point at. +/// +/// A reclamation failure is warned, not surfaced: the primary outcome +/// (`AlreadyLeased`/`Failed`) is unaffected either way, and the orphan's +/// location still ends up in the best-effort audit record +/// ([`record_attempt`]) for `drive lease prune` — or a human — to find. +async fn reclaim_backup(files_api: &FilesApi<'_>, backup: LeaseBackup) -> BackupDisposition { + match super::prune::clear_backup(files_api, &backup).await { + Ok(()) => BackupDisposition::Reclaimed(backup), + Err(err) => { + let (location, ..) = backup.audit_fields(); + tracing::warn!( + "drive lease acquire: failed to reclaim an orphaned backup at {}: {err} — \ + `drive lease prune` cannot see it either, since no ledger row references it; \ + remove it manually", + location.unwrap_or_default() + ); + BackupDisposition::ReclaimFailed(backup) + } + } +} + /// What [`insert_record`] did. enum InsertOutcome { /// The record was inserted; the lease is minted. @@ -449,10 +581,38 @@ fn insert_record(record: LeaseRecord, ledger_path: &Path) -> anyhow::Result-backup-orphaned` +/// so an operator can grep for a leaked backup `drive lease prune` cannot +/// see (issue #1690) — see [`BackupDisposition`]. Returns `base_verdict` +/// unchanged, with no `backup_location`, when this attempt never took a +/// backup at all. +fn orphan_verdict( + base_verdict: &str, + disposition: Option<&BackupDisposition>, +) -> (String, Option) { + let backup = match disposition { + None => return (base_verdict.to_string(), None), + Some(BackupDisposition::Reclaimed(backup)) => backup, + Some(BackupDisposition::ReclaimFailed(backup)) => { + let (location, ..) = backup.audit_fields(); + return (format!("{base_verdict}-backup-orphaned"), location); + } + }; + let (location, ..) = backup.audit_fields(); + (base_verdict.to_string(), location) +} + /// Builds and writes the `kind: "audit"` record for one acquire attempt. /// See [`acquire`]'s own doc comment for why this is best-effort rather than -/// write-ahead/fail-closed. -fn record_attempt(opts: &AcquireOptions, result: &AcquireResult) { +/// write-ahead/fail-closed. `disposition` is `Some` only when this attempt +/// took a backup that turned out to be reclaimed or orphaned rather than +/// referenced by a ledger row (issue #1690) — see [`BackupDisposition`]. +fn record_attempt( + opts: &AcquireOptions, + result: &AcquireResult, + disposition: Option<&BackupDisposition>, +) { let auth_policy = Some( match opts.auth_policy { AuthPolicy::DeviceOwner => "device-owner", @@ -467,14 +627,7 @@ fn record_attempt(opts: &AcquireOptions, result: &AcquireResult) { expires_at: _, headless_waiver, } => { - let (backup_location, backup_sha256, backup_size) = match backup { - LeaseBackup::Bytes { path, sha256, size } => ( - Some(path.display().to_string()), - Some(sha256.clone()), - Some(*size), - ), - LeaseBackup::DriveCopy { file_id } => (Some(file_id.clone()), None, None), - }; + let (backup_location, backup_sha256, backup_size) = backup.audit_fields(); // Re-read the just-written ledger row for the version/ // modified_time actually recorded, rather than widening // `AcquireResult::Acquired` (a public, `--output json` wire @@ -511,15 +664,19 @@ fn record_attempt(opts: &AcquireOptions, result: &AcquireResult) { ..Default::default() } } - AcquireResult::AlreadyLeased { token, .. } => crate::request_log::AuditOutcome { - command: vec!["drive".to_string(), "lease-acquire".to_string()], - integration: "drive", - file_id: opts.file_id.clone(), - lease_id: Some(token.clone()), - verdict: "already-leased".to_string(), - auth_policy, - ..Default::default() - }, + AcquireResult::AlreadyLeased { token, .. } => { + let (verdict, backup_location) = orphan_verdict("already-leased", disposition); + crate::request_log::AuditOutcome { + command: vec!["drive".to_string(), "lease-acquire".to_string()], + integration: "drive", + file_id: opts.file_id.clone(), + lease_id: Some(token.clone()), + verdict, + backup_location, + auth_policy, + ..Default::default() + } + } AcquireResult::RefusedNativeDocument => crate::request_log::AuditOutcome { command: vec!["drive".to_string(), "lease-acquire".to_string()], integration: "drive", @@ -546,15 +703,19 @@ fn record_attempt(opts: &AcquireOptions, result: &AcquireResult) { auth_policy, ..Default::default() }, - AcquireResult::Failed { detail } => crate::request_log::AuditOutcome { - command: vec!["drive".to_string(), "lease-acquire".to_string()], - integration: "drive", - file_id: opts.file_id.clone(), - verdict: "failed".to_string(), - error: Some(detail.clone()), - auth_policy, - ..Default::default() - }, + AcquireResult::Failed { detail } => { + let (verdict, backup_location) = orphan_verdict("failed", disposition); + crate::request_log::AuditOutcome { + command: vec!["drive".to_string(), "lease-acquire".to_string()], + integration: "drive", + file_id: opts.file_id.clone(), + verdict, + error: Some(detail.clone()), + backup_location, + auth_policy, + ..Default::default() + } + } }; if let Err(err) = crate::request_log::record_audit_event(outcome) { tracing::warn!("drive lease acquire: failed to write audit record: {err}"); @@ -842,7 +1003,15 @@ mod tests { } #[tokio::test(flavor = "multi_thread")] - async fn a_ledger_insert_failure_is_reported_as_failed() { + async fn a_ledger_insert_failure_reclaims_the_backup_just_taken() { + // A concurrent lease op (or a leftover lock from a crash) holds the + // ledger lock across this attempt's own `insert_record` — the + // routine, non-race way `Failed` still follows a full backup + // (issue #1690): the lock-free pre-check can't see this, since it + // takes no lock itself. The lock file is pre-created here, mirroring + // `LedgerLock`'s own `.lock` naming, so + // `LedgerLock::acquire` fails exactly like it would against a + // genuinely concurrent holder. let server = wiremock::MockServer::start().await; wiremock::Mock::given(wiremock::matchers::method("GET")) .and(wiremock::matchers::path("/drive/v3/files/f1")) @@ -864,9 +1033,9 @@ mod tests { let root = tempfile::tempdir().unwrap(); let _audit = AuditGuard::redirect(root.path()); let test_opts = opts(root.path()); - // A directory in place of the ledger file makes `LeaseLedger::load` - // fail, which `insert_record` surfaces as an `anyhow::Error`. - std::fs::create_dir_all(&test_opts.ledger_path).unwrap(); + let mut lock_name = test_opts.ledger_path.as_os_str().to_owned(); + lock_name.push(".lock"); + std::fs::write(PathBuf::from(lock_name), b"").unwrap(); let result = acquire( &client, @@ -876,6 +1045,24 @@ mod tests { .await; assert!(matches!(result, AcquireResult::Failed { .. })); + assert!( + std::fs::read_dir(&test_opts.backup_dir) + .unwrap() + .next() + .is_none(), + "the backup this attempt took must be reclaimed, not left orphaned" + ); + let contents = std::fs::read_to_string(root.path().join("audit.jsonl")).unwrap(); + let rec: crate::request_log::LogRecord = serde_json::from_str(contents.trim_end()).unwrap(); + assert_eq!( + rec.context.get("verdict").map(String::as_str), + Some("failed"), + "a successfully reclaimed backup takes no verdict suffix" + ); + assert!( + rec.context.contains_key("backup_location"), + "a reclaimed backup's location must still be audited" + ); } #[test] @@ -1197,6 +1384,13 @@ mod tests { !test_opts.ledger_path.exists(), "no ledger row must be written" ); + assert!( + std::fs::read_dir(&test_opts.backup_dir) + .unwrap() + .next() + .is_none(), + "the backup this attempt took must be reclaimed, not left orphaned (issue #1690)" + ); } #[tokio::test(flavor = "multi_thread")] @@ -1238,6 +1432,13 @@ mod tests { !test_opts.ledger_path.exists(), "no ledger row must be written" ); + assert!( + std::fs::read_dir(&test_opts.backup_dir) + .unwrap() + .next() + .is_none(), + "the backup this attempt took must be reclaimed, not left orphaned (issue #1690)" + ); } // ── the audit sink (ADR-0080 §11) ────────────────────────────────── @@ -1417,7 +1618,10 @@ mod tests { // acquired leases on the same file would otherwise each capture the // same version and each pass their own staleness check against it, // letting the second writer's write silently clobber the first's. - // Refusing to mint the second lease at all closes that gap. + // Refusing to mint the second lease at all closes that gap. The + // second attempt's authenticator panics if called at all — proving + // the lock-free pre-check (issue #1690) refuses it before ever + // spending a Touch ID prompt, not merely before minting. let server = wiremock::MockServer::start().await; wiremock::Mock::given(wiremock::matchers::method("GET")) .and(wiremock::matchers::path("/drive/v3/files/f1")) @@ -1454,23 +1658,28 @@ mod tests { panic!("expected Acquired, got {first:?}"); }; - // A distinct `backup_dir` for the second attempt: `backup_name`'s - // timestamp has only whole-second precision (see `write_backup`'s - // own doc comment), so two backups of the same file within one - // test's runtime would otherwise collide on the same path. let mut second_opts = test_opts.clone(); second_opts.backup_dir = root.path().join("backups2"); - let second = acquire( - &client, - &second_opts, - &FakeAuthenticator(AuthOutcome::Authorized), - ) - .await; + struct PanicsIfCalled; + impl Authenticator for PanicsIfCalled { + fn authenticate(&self, _reason: &str, _policy: AuthPolicy) -> AuthOutcome { + panic!( + "must not authenticate: the lock-free pre-check must refuse a second \ + live lease before spending a prompt" + ); + } + } + + let second = acquire(&client, &second_opts, &PanicsIfCalled).await; let AcquireResult::AlreadyLeased { token, .. } = second else { panic!("expected AlreadyLeased, got {second:?}"); }; assert_eq!(token, first_token); + assert!( + !second_opts.backup_dir.exists(), + "the pre-check must refuse before ever taking a backup" + ); // Only the first lease is live in the ledger. let ledger = LeaseLedger::load(&test_opts.ledger_path).unwrap(); @@ -1482,6 +1691,196 @@ mod tests { ); } + // ── the belt-and-braces reclaim for the pre-check's narrow race (#1690) ── + + /// An authenticator that, as a side effect of authenticating, inserts a + /// live lease for `file_id` into the ledger at `ledger_path` — used to + /// simulate another process's `acquire` winning the race between this + /// attempt's lock-free pre-check and its own `insert_record`, which the + /// pre-check is not designed to close (only to make rare). + struct InsertsALiveLeaseDuringAuth { + ledger_path: PathBuf, + file_id: String, + } + impl Authenticator for InsertsALiveLeaseDuringAuth { + fn authenticate(&self, _reason: &str, _policy: AuthPolicy) -> AuthOutcome { + LeaseLedger::mutate_locked(&self.ledger_path, |ledger| { + ledger.insert(LeaseRecord { + token: "racer".to_string(), + file_id: self.file_id.clone(), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::DriveCopy { + file_id: "racer-backup".to_string(), + }, + acquired_at: Utc::now(), + expires_at: Utc::now() + ChronoDuration::minutes(30), + released_at: None, + restored_at: None, + }); + }) + .unwrap(); + AuthOutcome::Authorized + } + } + + #[tokio::test(flavor = "multi_thread")] + async fn a_lease_inserted_during_the_prompt_reclaims_this_attempts_bytes_backup() { + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .and(wiremock::matchers::query_param_is_missing("alt")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "f1", "name": "report.pdf", "mimeType": "application/pdf", "version": "1" + })), + ) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .and(wiremock::matchers::query_param("alt", "media")) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_bytes(b"hello".to_vec())) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let root = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(root.path()); + let test_opts = opts(root.path()); + + let result = acquire( + &client, + &test_opts, + &InsertsALiveLeaseDuringAuth { + ledger_path: test_opts.ledger_path.clone(), + file_id: test_opts.file_id.clone(), + }, + ) + .await; + + let AcquireResult::AlreadyLeased { token, .. } = result else { + panic!("expected AlreadyLeased, got {result:?}"); + }; + assert_eq!(token, "racer"); + assert!( + std::fs::read_dir(&test_opts.backup_dir) + .unwrap() + .next() + .is_none(), + "this attempt's own now-orphaned backup must be reclaimed" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn a_lease_inserted_during_the_prompt_reclaims_this_attempts_native_backup() { + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "f1", "name": "Budget", "mimeType": "application/vnd.google-apps.spreadsheet", + "version": "7" + }))) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path("/drive/v3/files/f1/copy")) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-1", "name": "backup", "mimeType": "application/vnd.google-apps.spreadsheet" + }))) + .expect(1) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/copy-1")) + .and(wiremock::matchers::body_json( + serde_json::json!({"trashed": true}), + )) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-1", "name": "backup", "trashed": true, + })), + ) + .expect(1) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let root = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(root.path()); + let mut test_opts = opts(root.path()); + test_opts.native_backup_folder_id = Some("backup-folder".to_string()); + + let result = acquire( + &client, + &test_opts, + &InsertsALiveLeaseDuringAuth { + ledger_path: test_opts.ledger_path.clone(), + file_id: test_opts.file_id.clone(), + }, + ) + .await; + + let AcquireResult::AlreadyLeased { token, .. } = result else { + panic!("expected AlreadyLeased, got {result:?}"); + }; + assert_eq!(token, "racer"); + // The PATCH mock's `.expect(1)` above is the real assertion: + // reclaiming this attempt's own orphaned Drive-copy backup means + // trashing it. + } + + #[tokio::test(flavor = "multi_thread")] + async fn a_reclamation_failure_is_audited_as_backup_orphaned_but_still_reports_already_leased() + { + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "f1", "name": "Budget", "mimeType": "application/vnd.google-apps.spreadsheet", + "version": "7" + }))) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path("/drive/v3/files/f1/copy")) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-1", "name": "backup", "mimeType": "application/vnd.google-apps.spreadsheet" + }))) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/copy-1")) + .respond_with(wiremock::ResponseTemplate::new(500)) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let root = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(root.path()); + let mut test_opts = opts(root.path()); + test_opts.native_backup_folder_id = Some("backup-folder".to_string()); + + let result = acquire( + &client, + &test_opts, + &InsertsALiveLeaseDuringAuth { + ledger_path: test_opts.ledger_path.clone(), + file_id: test_opts.file_id.clone(), + }, + ) + .await; + + assert!(matches!(result, AcquireResult::AlreadyLeased { .. })); + let contents = std::fs::read_to_string(root.path().join("audit.jsonl")).unwrap(); + let rec: crate::request_log::LogRecord = serde_json::from_str(contents.trim_end()).unwrap(); + assert_eq!( + rec.context.get("verdict").map(String::as_str), + Some("already-leased-backup-orphaned") + ); + assert_eq!( + rec.context.get("backup_location").map(String::as_str), + Some("copy-1") + ); + } + #[tokio::test(flavor = "multi_thread")] async fn a_second_acquisition_after_the_first_expires_mints_a_fresh_lease() { let server = wiremock::MockServer::start().await; diff --git a/src/drive/lease/ledger.rs b/src/drive/lease/ledger.rs index e5957b74..1e238b69 100644 --- a/src/drive/lease/ledger.rs +++ b/src/drive/lease/ledger.rs @@ -59,6 +59,26 @@ pub enum LeaseBackup { }, } +impl LeaseBackup { + /// The `(location, sha256, size)` triple every acquire/prune audit + /// record maps a backup into: `location` is a local path for a byte + /// backup or the backup copy's own Drive file id for a + /// native-document backup (always `Some`); `sha256`/`size` are + /// byte-backup-only. Shared by `acquire::record_attempt` and + /// `prune::record_prune_attempt` so their mapping can never drift + /// apart, as it once risked doing when each held an independent copy. + pub(crate) fn audit_fields(&self) -> (Option, Option, Option) { + match self { + Self::Bytes { path, sha256, size } => ( + Some(path.display().to_string()), + Some(sha256.clone()), + Some(*size), + ), + Self::DriveCopy { file_id } => (Some(file_id.clone()), None, None), + } + } +} + /// One row of the ledger — a lease's full operational state. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub(crate) struct LeaseRecord { diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs index 11040abe..d04be460 100644 --- a/src/drive/lease/prune.rs +++ b/src/drive/lease/prune.rs @@ -104,10 +104,19 @@ fn is_drive_not_found(err: &anyhow::Error) -> bool { ) } -/// Deletes/trashes one candidate's backup. `Ok(())` means the row's backup -/// is gone (deleted just now, or already absent) and the row may be -/// dropped; an `Err` means it is left in place for a future prune. -async fn clear_backup(files_api: &FilesApi<'_>, backup: &LeaseBackup) -> Result<()> { +/// Deletes/trashes a backup by path/id: a local file for [`LeaseBackup::Bytes`] +/// (`std::fs::remove_file`), a Drive Trash move for [`LeaseBackup::DriveCopy`] +/// ([`FilesApi::trash`]). `Ok(())` means the backup is gone (deleted just +/// now, or already absent); an `Err` means it is left in place. +/// +/// Two callers: `prune`, for a row's backup once that row is a removal +/// candidate, and `acquire::reclaim_backup`, for a backup an acquire +/// attempt itself just took but that turned out to be referenced by no +/// ledger row (issue #1690). Both callers hold the same invariant that +/// makes deleting *by path* safe here: they only ever pass a backup they +/// have just proven, by their own logic, to be unreferenced by any +/// surviving ledger row — never one merely believed to be unused. +pub(super) async fn clear_backup(files_api: &FilesApi<'_>, backup: &LeaseBackup) -> Result<()> { match backup { LeaseBackup::Bytes { path, .. } => match std::fs::remove_file(path) { Ok(()) => Ok(()), @@ -140,14 +149,7 @@ fn account_removal(outcome: &mut PruneOutcome, backup: &LeaseBackup) { /// since the destructive step (the backup already cleared, or refused to /// be) has already happened by the time this is called. fn record_prune_attempt(rec: &LeaseRecord, verdict: &str, error: Option) { - let (backup_location, backup_sha256, backup_size) = match &rec.backup { - LeaseBackup::Bytes { path, sha256, size } => ( - Some(path.display().to_string()), - Some(sha256.clone()), - Some(*size), - ), - LeaseBackup::DriveCopy { file_id } => (Some(file_id.clone()), None, None), - }; + let (backup_location, backup_sha256, backup_size) = rec.backup.audit_fields(); let outcome = crate::request_log::AuditOutcome { command: vec!["drive".to_string(), "lease-prune".to_string()], integration: "drive", diff --git a/src/drive/lease/restore.rs b/src/drive/lease/restore.rs index ad9eee54..0e88b2fc 100644 --- a/src/drive/lease/restore.rs +++ b/src/drive/lease/restore.rs @@ -2301,26 +2301,23 @@ mod tests { restored_at: None, }); ledger.save(&test_opts.ledger_path).unwrap(); + // `mount_file`/`mount_folder` are still needed: the write-permission + // gate runs *before* `restore_inner` ever calls `acquire`. No + // `mount_download`, though — the fresh acquire's lock-free + // pre-check (issue #1690) refuses this before it ever authenticates + // or takes a backup, so `byte_backup`'s own download is never + // reached. This is now the common "did I already lease this?" + // case, not the narrow race that still spends a prompt. mount_file("file-1", "text/plain", &["parent-1"]) .mount(&server) .await; mount_folder("parent-1").mount(&server).await; - // The fresh acquire authenticates and takes its own backup *before* - // discovering the already-live lease at the ledger-insert step - // (`acquire.rs`'s own sequence) — that backup ends up orphaned, but - // still needs a mock, or the acquire fails earlier than the - // `AlreadyLeased` check this test means to exercise. No PATCH mock: - // reaching the restore write itself would mean the short-circuit - // failed to prevent it. - mount_download("file-1", b"orphaned backup content") - .mount(&server) - .await; let result = restore( &client, &sheets, &opts(dir.path(), &old_token), - &FakeAuthenticator(AuthOutcome::Authorized), + &PanicsIfCalled, &[allow_rule("parent-1")], ) .await; From 2c984d6f42b688bd54953d8d0051ef48f7977359 Mon Sep 17 00:00:00 2001 From: John Ky Date: Fri, 18 Sep 2026 02:55:51 +1000 Subject: [PATCH 2/2] fix(drive): repair lock-acquisition-failure tests broken by the flock migration a_ledger_insert_failure_reclaims_the_backup_just_taken pre-created an empty .lock file to simulate a busy lock, which stopped working once issue #1687 switched to flock(2): a bare file's existence holds nothing under flock, only an actual lock does, so insert_record succeeded instead of failing. Fixed by holding a real LedgerLock, matching restore.rs's own already-correct pattern. The same pre-existing-file pattern was used by six reports_a_lock_ acquisition_failure_as_failed tests across sheets/format, sheets/write, sheets/validation, sheets/protection, sheets/structure and docs/write. These route through check_and_lock_lease, which #1687 changed to *wait* for a busy lock rather than hard-fail, so simply holding a real LedgerLock here would make the tests block for the full lock-wait timeout instead of failing fast. Fixed by creating a directory at the lock path instead: an open() for write against it fails immediately with a genuine I/O error, which acquire_waiting never retries. --- src/drive/docs/write.rs | 7 ++++++- src/drive/lease/acquire.rs | 15 ++++++++------- src/drive/sheets/format.rs | 7 ++++++- src/drive/sheets/protection.rs | 7 ++++++- src/drive/sheets/structure.rs | 7 ++++++- src/drive/sheets/validation.rs | 7 ++++++- src/drive/sheets/write.rs | 7 ++++++- 7 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/drive/docs/write.rs b/src/drive/docs/write.rs index b148143b..b79024cb 100644 --- a/src/drive/docs/write.rs +++ b/src/drive/docs/write.rs @@ -1429,9 +1429,14 @@ mod tests { .await; let opts = replace_opts(false); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = opts.ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let outcome = write(&drive, &docs, &opts, &[allow_rule("folder-1")]).await; assert!(matches!(outcome.result, WriteResult::Failed { .. })); diff --git a/src/drive/lease/acquire.rs b/src/drive/lease/acquire.rs index 9c659f4d..71da50df 100644 --- a/src/drive/lease/acquire.rs +++ b/src/drive/lease/acquire.rs @@ -24,6 +24,8 @@ use crate::cli::format::sanitize_for_terminal; use crate::drive::client::DriveClient; use crate::drive::files_api::FilesApi; use crate::drive::lease::authenticate::{AuthOutcome, AuthPolicy, Authenticator}; +#[cfg(test)] +use crate::drive::lease::ledger::LedgerLock; use crate::drive::lease::ledger::{LeaseBackup, LeaseLedger, LeaseRecord}; /// Per-call options for `drive lease acquire`. @@ -1008,10 +1010,11 @@ mod tests { // ledger lock across this attempt's own `insert_record` — the // routine, non-race way `Failed` still follows a full backup // (issue #1690): the lock-free pre-check can't see this, since it - // takes no lock itself. The lock file is pre-created here, mirroring - // `LedgerLock`'s own `.lock` naming, so - // `LedgerLock::acquire` fails exactly like it would against a - // genuinely concurrent holder. + // takes no lock itself. A held `LedgerLock`, not a bare + // `File::create` on the lock path (issue #1687): under `flock`, + // the file's mere existence holds nothing — only an actual lock + // does, and `flock` conflicts against a second `open()` even from + // this same process. let server = wiremock::MockServer::start().await; wiremock::Mock::given(wiremock::matchers::method("GET")) .and(wiremock::matchers::path("/drive/v3/files/f1")) @@ -1033,9 +1036,7 @@ mod tests { let root = tempfile::tempdir().unwrap(); let _audit = AuditGuard::redirect(root.path()); let test_opts = opts(root.path()); - let mut lock_name = test_opts.ledger_path.as_os_str().to_owned(); - lock_name.push(".lock"); - std::fs::write(PathBuf::from(lock_name), b"").unwrap(); + let _held = LedgerLock::acquire(&test_opts.ledger_path).unwrap(); let result = acquire( &client, diff --git a/src/drive/sheets/format.rs b/src/drive/sheets/format.rs index 5520a969..5f26ff57 100644 --- a/src/drive/sheets/format.rs +++ b/src/drive/sheets/format.rs @@ -2741,9 +2741,14 @@ mod tests { let rules = vec![allow_rule("folder-1")]; let opts = format_cells_opts(false); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = opts.ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let outcome = format(&drive, &sheets, &opts, &rules).await; assert!(matches!(outcome.result, FormatResult::Failed { .. })); diff --git a/src/drive/sheets/protection.rs b/src/drive/sheets/protection.rs index b5465203..739c832d 100644 --- a/src/drive/sheets/protection.rs +++ b/src/drive/sheets/protection.rs @@ -2399,9 +2399,14 @@ mod tests { mount_workbook(serde_json::json!([])).mount(&server).await; let rules = vec![allow_rule("folder-1")]; let (lease_token, ledger_path) = leased_opts_for("sheet-1"); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let opts = ProtectionOptions { spreadsheet_id: "sheet-1".to_string(), diff --git a/src/drive/sheets/structure.rs b/src/drive/sheets/structure.rs index cb653fe4..de1b0b7e 100644 --- a/src/drive/sheets/structure.rs +++ b/src/drive/sheets/structure.rs @@ -4518,9 +4518,14 @@ mod tests { mount_workbook().mount(&server).await; let o = opts(rename(), false); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = o.ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let outcome = structure(&drive, &sheets, &o, &[allow_rule("parent-1")]).await; assert!(matches!(outcome.result, StructureResult::Failed { .. })); diff --git a/src/drive/sheets/validation.rs b/src/drive/sheets/validation.rs index 6370dfdc..e7189836 100644 --- a/src/drive/sheets/validation.rs +++ b/src/drive/sheets/validation.rs @@ -1542,9 +1542,14 @@ mod tests { mount_workbook().mount(&server).await; let rules = vec![allow_rule("folder-1")]; let (lease_token, ledger_path) = leased_opts_for("sheet-1"); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let opts = ValidationOptions { spreadsheet_id: "sheet-1".to_string(), diff --git a/src/drive/sheets/write.rs b/src/drive/sheets/write.rs index 8534b840..1c6b060f 100644 --- a/src/drive/sheets/write.rs +++ b/src/drive/sheets/write.rs @@ -1224,9 +1224,14 @@ mod tests { mount_folder("parent-1").mount(&server).await; let o = opts(WriteVerb::Write, false); + // Under `flock` (issue #1687), a busy lock now waits rather than + // hard-failing (`check_and_lock_lease` -> `acquire_waiting`), so a + // held `LedgerLock` no longer reproduces an immediate failure here. + // A directory at the lock path does: opening it for write fails + // outright with an I/O error, which is never retried. let mut lock_path = o.ledger_path.clone().into_os_string(); lock_path.push(".lock"); - std::fs::write(std::path::PathBuf::from(lock_path), b"").unwrap(); + std::fs::create_dir(std::path::PathBuf::from(lock_path)).unwrap(); let outcome = write(&drive, &sheets, &o, &[allow_rule("parent-1")]).await; assert!(matches!(outcome.result, WriteResult::Failed { .. }));