diff --git a/CHANGELOG.md b/CHANGELOG.md index 2feebc72..dbfb22c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **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. +- **`drive lease restore` no longer duplicates a sheet it has already restored** ([#1689](https://github.com/rust-works/omni-dev/issues/1689), [ADR-0080](docs/adrs/adr-0080.md) §10): re-running restore on a token whose deleted-sheet restore had already succeeded silently copied the sheet in *again*, one "Copy of …" per run, each costing a Touch ID prompt and a fresh Drive backup copy. `spreadsheets.sheets.copyTo` assigns the destination a fresh `sheetId`, so the backup sheet's own id stayed missing from the live spreadsheet and the structural detection ADR-0080 §10 relies on kept firing; the pre-write recheck compared against that same original id rather than the one the previous restore created, so it passed too. Live spreadsheet state alone cannot settle it — a restored sheet is structurally indistinguishable from a live sheet the user happened to give the backup sheet's title, and §10 deliberately restores *through* the latter — so the ledger row now records `restored_sheet_id`, the live id each restore creates, and a repeat run is refused (`sheet-already-restored`) **before** the authentication prompt and the fresh backup copy, naming the sheet and the still-live lease the earlier restore minted. The guard keys on that id still being live rather than on `restored_at` being set, so re-deleting the restored sheet and re-running legitimately restores it again; the recheck immediately before the write applies the same test, for a concurrent restore landing during the up-to-two-minute prompt window. The `Bytes` restore path was already idempotent and is unchanged. `restored_sheet_id` is additive — ledgers written by earlier builds keep loading. - **`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/adrs/adr-0080.md b/docs/adrs/adr-0080.md index 4200e9bb..6b805ece 100644 --- a/docs/adrs/adr-0080.md +++ b/docs/adrs/adr-0080.md @@ -571,7 +571,20 @@ one fails outright rather than needing a separate mime-type gate. `copyTo` always assigns the destination a fresh `sheetId`, so no id collision with a live sheet is possible; a title collision is resolved by a best-effort rename back to the original title only when it's currently free, otherwise -the copy keeps Sheets' own default ("Copy of {title}"). Scope stays +the copy keeps Sheets' own default ("Copy of {title}"). + +That fresh id is also what makes the detection **non-idempotent on its own** +(issue #1689): the backup sheet's own id stays missing-live even after a +successful restore, so re-running would report the same deleted sheet again +and add a second copy. Live state cannot settle it either — a restored sheet +and a live sheet the user happened to give the backup sheet's title are +structurally identical, and the paragraph above commits to restoring +*through* the latter. So the ledger row records `restored_sheet_id`, the live +id the restore created, and a later run refuses (`sheet-already-restored`, +before the prompt and before the fresh backup copy) while that id is still +live. Keyed on the id being live rather than on `restored_at` being set, so +deleting the restored sheet and re-running legitimately restores it again. +Scope stays whole-sheet: a deleted row/column/range is below what this diff can detect, and is not attempted. Where no typed path exists, `restore` reports the backup's location and stops — an honest limitation in the same spirit as diff --git a/docs/drive.md b/docs/drive.md index dcff91af..0b14fe76 100644 --- a/docs/drive.md +++ b/docs/drive.md @@ -936,7 +936,7 @@ Same gate, scope requirement, and logging behavior as [Create](#create). ```bash $ omni-dev drive lease acquire 1ExistingFileId lease-abc123... -Backed up to /home/user/.local/state/omni-dev/drive-backups/20260911T000000Z-1ExistingFileId-report.pdf (expires 2026-09-11T00:30:00Z) +Backed up to /home/user/.local/state/omni-dev/drive-backups/20260911T000000Z-1ExistingFileId-report.pdf (expires 2026-09-11 00:30:00 UTC) $ omni-dev drive edit 1ExistingFileId --content ./new-report.pdf --lease lease-abc123... Edited: 1ExistingFileId @@ -994,7 +994,7 @@ Same request-log behavior as [Create](#create)/[Upload](#upload). ```bash $ omni-dev drive lease acquire 1ExistingFileId lease-abc123... -Backed up to /home/user/.local/state/omni-dev/drive-backups/20260911T000000Z-1ExistingFileId-report.pdf (expires 2026-09-11T00:30:00Z) +Backed up to /home/user/.local/state/omni-dev/drive-backups/20260911T000000Z-1ExistingFileId-report.pdf (expires 2026-09-11 00:30:00 UTC) ``` Before `drive edit` can write, it needs a **lease**: a token bound to a @@ -1129,7 +1129,7 @@ having used the waiver, so it stays visible after the fact. ```bash $ omni-dev drive lease restore lease-abc123... lease-def456... -Restored. Backed up the pre-restore content to /home/user/.local/state/omni-dev/drive-backups/20260912T000000Z-1ExistingFileId-report.pdf (expires 2026-09-12T00:30:00Z) +Restored. Backed up the pre-restore content to /home/user/.local/state/omni-dev/drive-backups/20260912T000000Z-1ExistingFileId-report.pdf (expires 2026-09-12 00:30:00 UTC) ``` `drive lease restore ` restores a file from the backup a lease @@ -1164,9 +1164,26 @@ title when that title is currently free: ```bash $ omni-dev drive lease restore lease-native789... lease-def456... -Restored sheet 'Q3 Numbers' (id 1481923) back into spreadsheet 1SpreadsheetId. Backed up the pre-restore content to Drive copy 1FreshBackupCopyId (expires 2026-09-12T00:30:00Z) +Restored sheet 'Q3 Numbers' (id 1481923) back into spreadsheet 1SpreadsheetId. Backed up the pre-restore content to Drive copy 1FreshBackupCopyId (expires 2026-09-12 00:30:00 UTC) ``` +**Restoring the same sheet backup twice is refused, not repeated.** +`copyTo` gives the restored sheet a *fresh* id, so the backup sheet's own id +stays missing from the live spreadsheet and the structural diff above would +happily fire again — silently adding another "Copy of …" every run. The +ledger row records the id each restore creates, and a re-run is refused while +that sheet is still there, before the authentication prompt and before the +fresh backup copy: + +```bash +$ omni-dev drive lease restore lease-native789... +Refused: this backup's deleted sheet was already restored on 2026-09-12 00:00:00 UTC into spreadsheet 1SpreadsheetId as 'Q3 Numbers' (id 1481923), which is still there — restoring again would only add a second copy. Delete that sheet first if you do want another one. No fresh lease was minted, no Touch ID was spent. +``` + +The check keys on that sheet still being live, not on the backup merely +having been restored from before — so if the restored sheet is deleted +*again*, re-running restores it again as normal. + **Everything else native has no typed restore path.** Zero or more than one sheet missing (nothing to restore this way, or ambiguous — this never guesses), a Docs/Slides backup, or anything below whole-sheet granularity diff --git a/docs/log.md b/docs/log.md index d0472ddf..c293ea17 100644 --- a/docs/log.md +++ b/docs/log.md @@ -541,6 +541,8 @@ once minted (or the existing one, for `already-leased`), and `` argument — so `--query 'restored_from_lease_id:'` finds every restore attempt made from one backup regardless of outcome. `verdict` matches `drive lease restore`'s own reported status: `restored`, +`restored-sheet`, `restored-headless-waiver`, +`restored-sheet-headless-waiver`, `sheet-already-restored`, `no-such-backup-token`, `no-typed-restore-path`, `backup-too-large-for-simple-upload`, `refused-no-visible-parents`, `blocked`, `already-leased`, `refused-native-document`, `denied`, diff --git a/src/cli/drive/lease.rs b/src/cli/drive/lease.rs index 7adff58d..b280854e 100644 --- a/src/cli/drive/lease.rs +++ b/src/cli/drive/lease.rs @@ -427,6 +427,32 @@ fn print_restore_result(result: &RestoreResult) { ); } } + RestoreResult::SheetAlreadyRestored { + spreadsheet_id, + sheet_id, + sheet_title, + restored_at, + live_lease, + } => { + let when = restored_at.map_or_else(String::new, |at| format!(" on {at}")); + eprintln!( + "Refused: this backup's deleted sheet was already restored{when} into \ + spreadsheet {} as '{}' (id {sheet_id}), which is still there — restoring \ + again would only add a second copy. Delete that sheet first if you do want \ + another one. No fresh lease was minted, no Touch ID was spent.", + sanitize_for_terminal(spreadsheet_id), + sanitize_for_terminal(sheet_title) + ); + if let Some(lease) = live_lease { + println!("{}", lease.token); + eprintln!( + "A lease is still live for this file (expires {}, not necessarily minted \ + by that earlier restore) — present it to `--lease` rather than spending \ + another prompt", + lease.expires_at + ); + } + } RestoreResult::NoSuchBackupToken => { eprintln!( "Refused: no lease in this ledger was ever acquired with that token — check it \ @@ -708,6 +734,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); @@ -765,6 +792,25 @@ mod tests { sheet_title: "Deleted".to_string(), headless_waiver: false, }, + // Both halves of the #1689 refusal: with a live lease to name, + // and without one (the common case, once it has expired). + RestoreResult::SheetAlreadyRestored { + spreadsheet_id: "sheet-1".to_string(), + sheet_id: 999, + sheet_title: "Deleted".to_string(), + restored_at: Some(chrono::Utc::now()), + live_lease: Some(restore::LiveLease { + token: "tok-6".to_string(), + expires_at: chrono::Utc::now(), + }), + }, + RestoreResult::SheetAlreadyRestored { + spreadsheet_id: "sheet-1".to_string(), + sheet_id: 999, + sheet_title: "Deleted".to_string(), + restored_at: None, + live_lease: None, + }, RestoreResult::NoSuchBackupToken, RestoreResult::NoTypedRestorePath { backup_location: "copy-1".to_string(), @@ -1069,6 +1115,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::days(9), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); @@ -1120,6 +1167,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::hours(2), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.insert(crate::drive::lease::ledger::LeaseRecord { token: "new-token".to_string(), @@ -1135,6 +1183,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); @@ -1183,6 +1232,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::days(9), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); diff --git a/src/cli/drive/sheets/write.rs b/src/cli/drive/sheets/write.rs index b36ef7f3..9832bf9c 100644 --- a/src/cli/drive/sheets/write.rs +++ b/src/cli/drive/sheets/write.rs @@ -442,6 +442,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); token diff --git a/src/drive/content_edit.rs b/src/drive/content_edit.rs index ea378da5..8044dc99 100644 --- a/src/drive/content_edit.rs +++ b/src/drive/content_edit.rs @@ -433,6 +433,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token @@ -1048,6 +1049,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); diff --git a/src/drive/docs/write.rs b/src/drive/docs/write.rs index b79024cb..12962167 100644 --- a/src/drive/docs/write.rs +++ b/src/drive/docs/write.rs @@ -823,6 +823,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token diff --git a/src/drive/lease/acquire.rs b/src/drive/lease/acquire.rs index 71da50df..ff125ab3 100644 --- a/src/drive/lease/acquire.rs +++ b/src/drive/lease/acquire.rs @@ -391,6 +391,7 @@ async fn finish_acquisition( expires_at, released_at: None, restored_at: None, + restored_sheet_id: None, }; // Synchronous ledger I/O (lock, load, save) on the async runtime's // current thread — `block_in_place` hands its other queued tasks off @@ -465,8 +466,10 @@ enum InsertOutcome { Inserted, /// A live lease already covered this record's `file_id` — nothing was /// inserted or overwritten. Carries that existing record so the caller - /// can return its token instead. - AlreadyLeased(LeaseRecord), + /// can return its token instead — boxed, since it otherwise dwarfs the + /// dataless `Inserted` variant (`clippy::large_enum_variant`), the same + /// reasoning `restore.rs`'s `GateCheck::Ok` documents. + AlreadyLeased(Box), } /// Downloads a binary file's bytes and writes them to `backup_dir` @@ -576,7 +579,7 @@ fn write_backup(path: &Path, bytes: &[u8]) -> anyhow::Result<()> { fn insert_record(record: LeaseRecord, ledger_path: &Path) -> anyhow::Result { LeaseLedger::mutate_locked(ledger_path, |ledger| { if let Some(existing) = ledger.live_lease_for_file(&record.file_id, Utc::now()) { - return InsertOutcome::AlreadyLeased(existing.clone()); + return InsertOutcome::AlreadyLeased(Box::new(existing.clone())); } ledger.insert(record); InsertOutcome::Inserted @@ -1718,6 +1721,7 @@ mod tests { expires_at: Utc::now() + ChronoDuration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); }) .unwrap(); diff --git a/src/drive/lease/check.rs b/src/drive/lease/check.rs index f5f6cc85..d7a2b25e 100644 --- a/src/drive/lease/check.rs +++ b/src/drive/lease/check.rs @@ -495,6 +495,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); } diff --git a/src/drive/lease/ledger.rs b/src/drive/lease/ledger.rs index 1e238b69..d44eecb8 100644 --- a/src/drive/lease/ledger.rs +++ b/src/drive/lease/ledger.rs @@ -123,6 +123,19 @@ pub(crate) struct LeaseRecord { /// expected common case (§4), not something this field forbids. #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) restored_at: Option>, + /// The `sheetId` the most recent successful sheet restore from this + /// row's backup created in the live spreadsheet (issue #1689) — absent + /// for a `Bytes` restore, and for a row never restored from. + /// + /// `spreadsheets.sheets.copyTo` assigns the destination a *fresh* id, + /// so the backup sheet's own id stays missing-live forever and the + /// structural diff `restore` detects a deletion by keeps firing. This + /// is the only durable way to tell "already restored" from "a live + /// sheet that merely shares the backup sheet's title", which by + /// [ADR-0080](../../../docs/adrs/adr-0080.md) §10 is a state a restore + /// is expected to proceed through. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) restored_sheet_id: Option, } impl LeaseRecord { @@ -234,13 +247,26 @@ impl LeaseLedger { } /// Marks `token`'s row as having been restored from (ADR-0080 §4/§10), - /// stamped with `at`. A no-op if the token is absent. Idempotent: a - /// second restore from the same backup just overwrites the timestamp, - /// since the row already carries no count of how many times it has - /// been used — `drive lease restore` is not rate-limited by this field. - pub(crate) fn mark_restored(&mut self, token: &str, at: DateTime) { + /// stamped with `at` and, for a sheet restore, the `sheetId` it created + /// live. A no-op if the token is absent. + /// + /// Both fields describe the *most recent* restore, not a history: a + /// second restore from the same backup overwrites them, since the row + /// carries no count of how many times it has been used. Plain + /// assignment rather than a don't-clobber merge is deliberate — a + /// `DriveCopy` row's successful restore always yields `Some`, a + /// `Bytes` row's always `None`, so a row can never regress from one to + /// the other. The id is what [`super::restore`] reads back to refuse a + /// duplicate (issue #1689); the timestamp is for the audit trail. + pub(crate) fn mark_restored( + &mut self, + token: &str, + at: DateTime, + restored_sheet_id: Option, + ) { if let Some(rec) = self.0.get_mut(token) { rec.restored_at = Some(at); + rec.restored_sheet_id = restored_sheet_id; } } @@ -494,6 +520,7 @@ mod tests { expires_at: Utc::now() + ChronoDuration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, } } @@ -728,14 +755,36 @@ mod tests { ledger.save(&path).unwrap(); let returned = LeaseLedger::mutate(&path, |ledger| { - ledger.mark_restored("t1", Utc::now()); + ledger.mark_restored("t1", Utc::now(), Some(999)); "ok" }) .unwrap(); assert_eq!(returned, "ok"); let reloaded = LeaseLedger::load(&path).unwrap(); - assert!(reloaded.get("t1").unwrap().restored_at.is_some()); + let record = reloaded.get("t1").unwrap(); + assert!(record.restored_at.is_some()); + assert_eq!( + record.restored_sheet_id, + Some(999), + "the restored sheet's live id must survive the save/load round-trip — it is what \ + a later restore reads back to refuse a duplicate (#1689)" + ); + } + + #[test] + fn a_ledger_line_predating_restored_sheet_id_still_parses() { + // The field is additive (`serde(default)`), so a ledger written by + // a build before #1689 must keep loading rather than turning every + // existing lease into an unparseable line. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("lease-ledger.jsonl"); + let mut without = serde_json::to_value(sample_record("t1")).unwrap(); + without.as_object_mut().unwrap().remove("restored_sheet_id"); + std::fs::write(&path, format!("{without}\n")).unwrap(); + + let record = LeaseLedger::load(&path).unwrap(); + assert_eq!(record.get("t1").unwrap().restored_sheet_id, None); } #[test] diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs index d04be460..5371b760 100644 --- a/src/drive/lease/prune.rs +++ b/src/drive/lease/prune.rs @@ -402,6 +402,7 @@ mod tests { expires_at, released_at: None, restored_at: None, + restored_sheet_id: None, } } @@ -418,6 +419,7 @@ mod tests { expires_at, released_at: None, restored_at: None, + restored_sheet_id: None, } } diff --git a/src/drive/lease/restore.rs b/src/drive/lease/restore.rs index 0e88b2fc..21e52483 100644 --- a/src/drive/lease/restore.rs +++ b/src/drive/lease/restore.rs @@ -130,6 +130,32 @@ pub enum RestoreResult { /// name. headless_waiver: bool, }, + /// An earlier restore from this same backup already copied its deleted + /// sheet back in, and that copy is still live (issue #1689). Refused + /// *before* the fresh lease's authentication prompt and before its + /// backup copy, since `spreadsheets.sheets.copyTo` would otherwise + /// happily make a second, independent duplicate — it assigns a fresh + /// id every time, so the backup sheet's own id stays missing-live and + /// the structural detection keeps firing. Delete the sheet named here + /// and re-run to restore it again. + SheetAlreadyRestored { + /// The live spreadsheet the earlier restore copied into. + spreadsheet_id: String, + /// The id that restore created there, still present. + sheet_id: i64, + /// That sheet's current title — read live, so a rename since the + /// restore shows up rather than the backup's own title. + sheet_title: String, + /// When the earlier restore was recorded. Absent only if the row + /// somehow carries an id but no timestamp — the two are written + /// together. + #[serde(skip_serializing_if = "Option::is_none")] + restored_at: Option>, + /// The lease that earlier restore minted, when it is still live — + /// present it to `--lease` rather than spending a fresh prompt. + #[serde(skip_serializing_if = "Option::is_none")] + live_lease: Option, + }, /// `` names no row this ledger has ever recorded. NoSuchBackupToken, /// The backup is a native-document Drive copy, and either it isn't a @@ -227,6 +253,15 @@ pub enum RestoreResult { }, } +/// A lease that is still live, named on a refusal so its token isn't lost. +#[derive(Debug, Clone, Serialize)] +pub struct LiveLease { + /// The lease's token. + pub token: String, + /// When it expires. + pub expires_at: DateTime, +} + impl JsonlSerialize for RestoreResult { fn write_jsonl(&self, out: &mut dyn std::io::Write) -> Result<(), anyhow::Error> { crate::cli::drive::format::write_scalar_jsonl(self, out) @@ -253,6 +288,7 @@ impl RestoreResult { .. } => "restored-sheet-headless-waiver", Self::RestoredSheet { .. } => "restored-sheet", + Self::SheetAlreadyRestored { .. } => "sheet-already-restored", Self::NoSuchBackupToken => "no-such-backup-token", Self::NoTypedRestorePath { .. } => "no-typed-restore-path", Self::BackupTooLargeForSimpleUpload { .. } => "backup-too-large-for-simple-upload", @@ -341,13 +377,36 @@ async fn restore_inner( let sheets_api = SheetsApi::new(sheets); let plan = match &backup_record.backup { LeaseBackup::DriveCopy { file_id: copy_id } => { - match detect_deleted_sheet(&sheets_api, copy_id, &file_id).await { - Some((sheet_id, sheet_title)) => RestorePlan::Sheet(SheetRestorePlan { + let previously_restored_sheet_id = backup_record.restored_sheet_id; + match detect_sheet_restore(&sheets_api, copy_id, &file_id, previously_restored_sheet_id) + .await + { + SheetDetection::Deleted { + sheet_id, + sheet_title, + } => RestorePlan::Sheet(SheetRestorePlan { backup_spreadsheet_id: copy_id.clone(), sheet_id, sheet_title, + previously_restored_sheet_id, }), - None => { + // Refused here, before `acquire` — so a repeat run spends + // no authentication prompt and takes no fresh Drive backup + // copy, which is the whole cost of the duplicate this + // guards against (issue #1689). + SheetDetection::AlreadyRestored { + sheet_id, + sheet_title, + } => { + return RestoreResult::SheetAlreadyRestored { + spreadsheet_id: file_id.clone(), + sheet_id, + sheet_title, + restored_at: backup_record.restored_at, + live_lease: live_lease_for(&opts.ledger_path, &file_id), + } + } + SheetDetection::None => { return RestoreResult::NoTypedRestorePath { backup_location: copy_id.clone(), } @@ -528,6 +587,7 @@ async fn restore_inner( backup_spreadsheet_id, sheet_id, sheet_title, + previously_restored_sheet_id, }) => { // A race with the initial detection: the sheet could have been // manually recreated in the roughly two minutes the fresh @@ -537,14 +597,28 @@ async fn restore_inner( // from before the prompt" discipline as the mime-type re-check // above. match sheets_api.get_spreadsheet(&file_id).await { - Ok(live) if live.sheet_ids().contains(sheet_id) => { - return record_failure( - "a sheet with the same id already exists again in the live \ - spreadsheet; nothing to restore" - .to_string(), - ); + Ok(live) => { + let live_ids = live.sheet_ids(); + if live_ids.contains(sheet_id) { + return record_failure( + "a sheet with the same id already exists again in the live \ + spreadsheet; nothing to restore" + .to_string(), + ); + } + // The same window can also have seen a *concurrent + // restore* from this very backup land — free to check, + // since the live workbook is already fetched here, and + // the id it would have created is the one the + // pre-prompt guard read (issue #1689). + if previously_restored_sheet_id.is_some_and(|id| live_ids.contains(&id)) { + return record_failure( + "this backup's deleted sheet was already restored into the live \ + spreadsheet; nothing to restore" + .to_string(), + ); + } } - Ok(_) => {} Err(err) => return record_failure(err.to_string()), } let copied = match sheets_api @@ -585,6 +659,13 @@ async fn restore_inner( // "transition"), best-effort — the restore itself already succeeded or // failed by this point, so a failure to stamp this is logged, not // surfaced as a failed restore. + // A sheet restore also records the id `copyTo` just created, which is + // the only thing that lets a later run tell "already restored" from "a + // live sheet that merely shares the title" (issue #1689). + let restored_sheet_id = match &result { + RestoreResult::RestoredSheet { sheet_id, .. } => Some(*sheet_id), + _ => None, + }; if matches!( result, RestoreResult::Restored { .. } | RestoreResult::RestoredSheet { .. } @@ -594,7 +675,9 @@ async fn restore_inner( // off to the runtime's other workers for the duration, the same // reasoning `acquire.rs`'s own ledger writes document (issue #1664 // review finding). - tokio::task::block_in_place(|| mark_backup_restored(&opts.ledger_path, &opts.token)); + tokio::task::block_in_place(|| { + mark_backup_restored(&opts.ledger_path, &opts.token, restored_sheet_id); + }); } result @@ -602,7 +685,7 @@ async fn restore_inner( /// The sheet-restore payload shared verbatim by [`RestorePlan::Sheet`] and /// [`PreparedRestore::Sheet`] — a sheet present in `backup_spreadsheet_id` -/// but missing from the live spreadsheet (see [`detect_deleted_sheet`]), +/// but missing from the live spreadsheet (see [`detect_sheet_restore`]), /// pending its `spreadsheets.sheets.copyTo` restore. One struct rather than /// two field-for-field-identical enum variants, so a field added to one /// can't be forgotten in the other (unlike the `Bytes` variants, whose @@ -612,10 +695,15 @@ struct SheetRestorePlan { backup_spreadsheet_id: String, sheet_id: i64, sheet_title: String, + /// The live id an earlier restore from this same backup created, if + /// any — carried so the pre-write recheck can refuse a duplicate that + /// appeared during the authentication prompt, not just a sheet whose + /// *original* id came back (issue #1689). + previously_restored_sheet_id: Option, } /// Which write `restore_inner` is about to perform, decided once from the -/// backup's own shape (and, for a `DriveCopy` backup, [`detect_deleted_sheet`]) +/// backup's own shape (and, for a `DriveCopy` backup, [`detect_sheet_restore`]) /// before the write-permission gate — kept as data rather than re-deciding /// at each later step, so the gate re-check and the write itself can never /// disagree about which path they're on. @@ -637,36 +725,106 @@ enum PreparedRestore { Sheet(SheetRestorePlan), } -/// Diffs `backup_spreadsheet_id`'s sheet-id set against -/// `live_spreadsheet_id`'s, returning the one sheet id (and its title) that -/// is present in the backup but missing live — only when that difference has -/// exactly one element. Any error (wrong resource type — a Docs/Slides -/// backup fails `spreadsheets.get` outright; a spreadsheet that's vanished; -/// a network failure) or an ambiguous (zero or more than one) diff collapses -/// to `None`, never `RestoreResult::Failed`: this is a best-effort detection -/// whose failure mode is always the existing, safe `NoTypedRestorePath` -/// fallback, never a wrong guess. -async fn detect_deleted_sheet( +/// What a `DriveCopy` backup's two sheet lists say this restore should do. +enum SheetDetection { + /// Exactly one sheet is present in the backup but missing live — the + /// deleted sheet, to be restored via `copyTo`. + Deleted { + /// Its id *in the backup* — never the id a restore will create. + sheet_id: i64, + /// Its title in the backup, for the best-effort rename-back. + sheet_title: String, + }, + /// An earlier restore from this same backup already copied the sheet + /// in, and that copy is still live (issue #1689). + AlreadyRestored { + /// The live id that earlier restore created. + sheet_id: i64, + /// That live sheet's current title — read fresh, so a rename since + /// the restore is reflected rather than the backup's title assumed. + sheet_title: String, + }, + /// Nothing to restore this way, ambiguous, or the detection reads + /// failed — all collapse to the same safe fallback. + None, +} + +/// Decides which [`SheetDetection`] applies, from the backup spreadsheet's +/// and the live spreadsheet's sheet lists alone. +/// +/// `previously_restored_sheet_id` is the live id an earlier restore from +/// this same backup created ([`LeaseRecord::restored_sheet_id`]). It is +/// checked **first**, and that order is load-bearing: `copyTo` assigns the +/// destination a fresh id, so the backup sheet's own id stays missing-live +/// even after a successful restore and the diff below would happily report +/// "exactly one deleted sheet" a second time (issue #1689). When that id is +/// *gone* from live — the restored sheet was deleted again — this falls +/// through and restores it once more, which is the legitimate flow a blunt +/// "refuse whenever `restored_at` is set" gate would have blocked. +/// +/// Any error (wrong resource type — a Docs/Slides backup fails +/// `spreadsheets.get` outright; a spreadsheet that's vanished; a network +/// failure) or an ambiguous (zero or more than one) diff collapses to +/// [`SheetDetection::None`], never `RestoreResult::Failed`: this is a +/// best-effort detection whose failure mode is always the existing, safe +/// `NoTypedRestorePath` fallback, never a wrong guess. +/// +/// [`LeaseRecord::restored_sheet_id`]: super::ledger::LeaseRecord::restored_sheet_id +async fn detect_sheet_restore( sheets_api: &SheetsApi<'_>, backup_spreadsheet_id: &str, live_spreadsheet_id: &str, -) -> Option<(i64, String)> { - let backup = sheets_api - .get_spreadsheet(backup_spreadsheet_id) - .await - .ok()?; - let live = sheets_api.get_spreadsheet(live_spreadsheet_id).await.ok()?; + previously_restored_sheet_id: Option, +) -> SheetDetection { + let Ok(backup) = sheets_api.get_spreadsheet(backup_spreadsheet_id).await else { + return SheetDetection::None; + }; + let Ok(live) = sheets_api.get_spreadsheet(live_spreadsheet_id).await else { + return SheetDetection::None; + }; + if let Some(restored_id) = previously_restored_sheet_id { + if let Some(sheet) = live + .sheets + .iter() + .find(|sheet| sheet.sheet_id() == Some(restored_id)) + { + return SheetDetection::AlreadyRestored { + sheet_id: restored_id, + sheet_title: sheet.title().to_string(), + }; + } + } let live_ids = live.sheet_ids(); let mut missing = backup.sheets.iter().filter_map(|sheet| { let props = sheet.properties.as_ref()?; let id = props.sheet_id?; (!live_ids.contains(&id)).then(|| (id, props.title.clone())) }); - let first = missing.next()?; + let Some((sheet_id, sheet_title)) = missing.next() else { + return SheetDetection::None; + }; if missing.next().is_some() { - return None; + return SheetDetection::None; } - Some(first) + SheetDetection::Deleted { + sheet_id, + sheet_title, + } +} + +/// The live (unexpired, unreleased) lease covering `file_id`, if any — +/// read back purely to surface its token on a refusal that would otherwise +/// leave the caller with no way to find it (issue #1689's +/// [`RestoreResult::SheetAlreadyRestored`]; there is no `drive lease list` +/// verb). Best-effort: an unreadable ledger yields `None`, since this is +/// decoration on a refusal already decided, never itself a gate. +fn live_lease_for(ledger_path: &Path, file_id: &str) -> Option { + let ledger = LeaseLedger::load(ledger_path).ok()?; + let record = ledger.live_lease_for_file(file_id, Utc::now())?; + Some(LiveLease { + token: record.token.clone(), + expires_at: record.expires_at, + }) } /// Best-effort: renames the just-copied sheet back to `original_title` if @@ -811,7 +969,8 @@ fn verify_and_read_backup(path: &Path, expected_sha256: &str) -> Result, Ok(bytes) } -/// Best-effort: stamps `token`'s row with `restored_at` (ADR-0080 §4). +/// Best-effort: stamps `token`'s row with `restored_at` and, for a sheet +/// restore, the id `copyTo` created live (ADR-0080 §4, issue #1689). /// /// Goes through [`LeaseLedger::mutate_locked`] — this runs after /// `restore_inner` has already released its own lock (acquired via @@ -822,9 +981,9 @@ fn verify_and_read_backup(path: &Path, expected_sha256: &str) -> Result, /// two calls saves last would silently discard the other's change (issue /// #1664 review finding) — exactly the class of bug the lock exists to /// prevent everywhere else in this module. -fn mark_backup_restored(ledger_path: &Path, token: &str) { +fn mark_backup_restored(ledger_path: &Path, token: &str, restored_sheet_id: Option) { let result = LeaseLedger::mutate_locked(ledger_path, |ledger| { - ledger.mark_restored(token, Utc::now()); + ledger.mark_restored(token, Utc::now(), restored_sheet_id); }); if let Err(err) = result { tracing::debug!( @@ -880,6 +1039,7 @@ fn record_attempt(opts: &RestoreOptions, result: &RestoreResult) { | RestoreResult::Unavailable { detail } | RestoreResult::Failed { detail } => (None, Some(detail.clone())), RestoreResult::NoSuchBackupToken + | RestoreResult::SheetAlreadyRestored { .. } | RestoreResult::NoTypedRestorePath { .. } | RestoreResult::BackupTooLargeForSimpleUpload { .. } | RestoreResult::RefusedNoVisibleParents @@ -1096,6 +1256,7 @@ mod tests { expires_at: Utc::now() - ChronoDuration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token @@ -1193,7 +1354,7 @@ mod tests { ) { // Covers issue #1676's "does the scope stay Sheets-only" question: // a Docs/Slides backup fails `spreadsheets.get` outright (wrong - // resource type), which `detect_deleted_sheet` folds into `None` + // resource type), which `detect_sheet_restore` folds into `None` // with no separate mime-type gate needed. let server = wiremock::MockServer::start().await; let client = client_with_bootstrapped_token(&server).await; @@ -1393,6 +1554,302 @@ mod tests { assert_eq!(body["requests"].as_array().unwrap().len(), 1); } + #[tokio::test(flavor = "multi_thread")] + async fn restoring_the_same_sheet_backup_twice_refuses_instead_of_duplicating_it() { + // Regression test for issue #1689. `copyTo` assigns the + // destination a *fresh* sheet id, so the backup sheet's own id + // stays missing-live even after a successful restore and the + // structural diff happily fires a second time — silently adding a + // "Copy of Deleted" duplicate per run, each costing a Touch ID + // prompt and a fresh Drive backup copy. The second run must refuse + // before spending either. + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let sheets = sheets_client_for(&server, &client); + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditLogGuard::redirect(dir.path()); + let test_opts = opts(dir.path(), ""); + let old_token = seed_backup_lease( + &test_opts.ledger_path, + "sheet-1", + LeaseBackup::DriveCopy { + file_id: "copy-1".to_string(), + }, + ); + + // ── Run 1: the ordinary successful restore. ── + mount_spreadsheet("copy-1", &[(1, "Sheet1"), (2, "Deleted")]) + .mount(&server) + .await; + mount_spreadsheet("sheet-1", &[(1, "Sheet1")]) + .mount(&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("POST")) + .and(wiremock::matchers::path("/drive/v3/files/sheet-1/copy")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-2", "name": "backup", "mimeType": GOOGLE_SHEET_MIME_TYPE + })), + ) + .mount(&server) + .await; + mount_copy_to("copy-1", 2, 999, "Copy of Deleted") + .mount(&server) + .await; + mount_batch_update("sheet-1").mount(&server).await; + + let mut restore_opts = opts(dir.path(), &old_token); + restore_opts.native_backup_folder_id = Some("backup-folder".to_string()); + let first = restore( + &client, + &sheets, + &restore_opts, + &FakeAuthenticator(AuthOutcome::Authorized), + &[allow_rule("parent-1")], + ) + .await; + let RestoreResult::RestoredSheet { + sheet_id: first_sheet_id, + new_token: fresh_token, + .. + } = first + else { + panic!("expected RestoredSheet on the first run, got {first:?}"); + }; + assert_eq!(first_sheet_id, 999); + assert_eq!( + LeaseLedger::load(&test_opts.ledger_path) + .unwrap() + .get(&old_token) + .unwrap() + .restored_sheet_id, + Some(999), + "the first restore must record the live id it created" + ); + + // ── Run 2: the live spreadsheet now holds the restored sheet. ── + // Re-mounted from scratch (the cached OAuth token survives the + // reset, so no `/token` mock is needed again). Every mutating call + // is mounted with `.expect(0)`: reaching one is the bug. + server.reset().await; + mount_spreadsheet("copy-1", &[(1, "Sheet1"), (2, "Deleted")]) + .mount(&server) + .await; + mount_spreadsheet("sheet-1", &[(1, "Sheet1"), (999, "Deleted")]) + .mount(&server) + .await; + mount_file("sheet-1", GOOGLE_SHEET_MIME_TYPE, &["parent-1"]) + .mount(&server) + .await; + mount_folder("parent-1").mount(&server).await; + for path in [ + "/drive/v3/files/sheet-1/copy", + "/v4/spreadsheets/copy-1/sheets/2:copyTo", + "/v4/spreadsheets/sheet-1:batchUpdate", + ] { + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path(path)) + .respond_with(wiremock::ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + } + + // `PanicsIfCalled` proves no second authentication prompt is spent. + let second = restore( + &client, + &sheets, + &restore_opts, + &PanicsIfCalled, + &[allow_rule("parent-1")], + ) + .await; + + let RestoreResult::SheetAlreadyRestored { + spreadsheet_id, + sheet_id, + sheet_title, + restored_at, + live_lease, + } = second + else { + panic!("expected SheetAlreadyRestored on the second run, got {second:?}"); + }; + assert_eq!(spreadsheet_id, "sheet-1"); + assert_eq!(sheet_id, 999); + assert_eq!(sheet_title, "Deleted"); + assert!(restored_at.is_some()); + let live_lease = live_lease.expect("the first run's lease is still live"); + assert_eq!( + live_lease.token, fresh_token, + "the refusal must name the still-live lease the first restore minted, since \ + there is no other way to recover its token" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn a_restored_sheet_deleted_again_is_restored_again() { + // The other half of #1689: the guard keys on whether the id the + // earlier restore created is *still live*, not on the mere fact + // that a restore happened — so re-deleting the restored sheet and + // re-running restores it once more, which a blunt "refuse whenever + // `restored_at` is set" gate would have wrongly blocked. + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let sheets = sheets_client_for(&server, &client); + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditLogGuard::redirect(dir.path()); + let test_opts = opts(dir.path(), ""); + let old_token = seed_backup_lease( + &test_opts.ledger_path, + "sheet-1", + LeaseBackup::DriveCopy { + file_id: "copy-1".to_string(), + }, + ); + LeaseLedger::mutate(&test_opts.ledger_path, |ledger| { + ledger.mark_restored(&old_token, Utc::now(), Some(999)); + }) + .unwrap(); + + mount_spreadsheet("copy-1", &[(1, "Sheet1"), (2, "Deleted")]) + .mount(&server) + .await; + // Neither the backup sheet's original id (2) nor the id the + // earlier restore created (999) is live — both were deleted. + mount_spreadsheet("sheet-1", &[(1, "Sheet1")]) + .mount(&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("POST")) + .and(wiremock::matchers::path("/drive/v3/files/sheet-1/copy")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-2", "name": "backup", "mimeType": GOOGLE_SHEET_MIME_TYPE + })), + ) + .mount(&server) + .await; + mount_copy_to("copy-1", 2, 1001, "Copy of Deleted") + .expect(1) + .mount(&server) + .await; + mount_batch_update("sheet-1").expect(1).mount(&server).await; + + let mut restore_opts = opts(dir.path(), &old_token); + restore_opts.native_backup_folder_id = Some("backup-folder".to_string()); + let result = restore( + &client, + &sheets, + &restore_opts, + &FakeAuthenticator(AuthOutcome::Authorized), + &[allow_rule("parent-1")], + ) + .await; + + let RestoreResult::RestoredSheet { sheet_id, .. } = result else { + panic!("expected RestoredSheet, got {result:?}"); + }; + assert_eq!(sheet_id, 1001); + assert_eq!( + LeaseLedger::load(&test_opts.ledger_path) + .unwrap() + .get(&old_token) + .unwrap() + .restored_sheet_id, + Some(1001), + "the row must now point at the newest restore, not the stale 999" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn a_concurrent_restore_landing_during_the_prompt_is_caught_at_the_recheck() { + // #1689's post-prompt half: the pre-prompt guard saw the recorded + // id absent, but by write time a concurrent restore from the same + // backup had put it back. The authentication prompt can take two + // minutes to answer (ADR-0080 §7), so the recheck must not trust + // the earlier detection here either. + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let sheets = sheets_client_for(&server, &client); + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditLogGuard::redirect(dir.path()); + let test_opts = opts(dir.path(), ""); + let old_token = seed_backup_lease( + &test_opts.ledger_path, + "sheet-1", + LeaseBackup::DriveCopy { + file_id: "copy-1".to_string(), + }, + ); + LeaseLedger::mutate(&test_opts.ledger_path, |ledger| { + ledger.mark_restored(&old_token, Utc::now(), Some(999)); + }) + .unwrap(); + + mount_spreadsheet("copy-1", &[(1, "Sheet1"), (2, "Deleted")]) + .mount(&server) + .await; + // First live read (detection): 999 is absent, so the plan is made. + mount_spreadsheet("sheet-1", &[(1, "Sheet1")]) + .up_to_n_times(1) + .with_priority(1) + .mount(&server) + .await; + // Every read after it (the pre-write recheck): 999 is back. + mount_spreadsheet("sheet-1", &[(1, "Sheet1"), (999, "Deleted")]) + .with_priority(2) + .mount(&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("POST")) + .and(wiremock::matchers::path("/drive/v3/files/sheet-1/copy")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "copy-2", "name": "backup", "mimeType": GOOGLE_SHEET_MIME_TYPE + })), + ) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path( + "/v4/spreadsheets/copy-1/sheets/2:copyTo", + )) + .respond_with(wiremock::ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + + let mut restore_opts = opts(dir.path(), &old_token); + restore_opts.native_backup_folder_id = Some("backup-folder".to_string()); + let result = restore( + &client, + &sheets, + &restore_opts, + &FakeAuthenticator(AuthOutcome::Authorized), + &[allow_rule("parent-1")], + ) + .await; + + let RestoreResult::FreshLeaseButWriteFailed { detail, .. } = &result else { + panic!("expected FreshLeaseButWriteFailed, got {result:?}"); + }; + assert!( + detail.contains("was already restored"), + "must name the duplicate, not some other refusal: {detail}" + ); + } + #[tokio::test(flavor = "multi_thread")] async fn a_live_spreadsheet_fetch_failure_at_the_sheet_recheck_reports_fresh_lease_but_write_failed( ) { @@ -2068,7 +2525,7 @@ mod tests { let _held = LedgerLock::acquire(&ledger_path).unwrap(); // Must not panic despite the lock already being held. - mark_backup_restored(&ledger_path, "backup-token"); + mark_backup_restored(&ledger_path, "backup-token", Some(999)); let ledger = LeaseLedger::load(&ledger_path).unwrap(); assert!( @@ -2299,6 +2756,7 @@ mod tests { expires_at: Utc::now() + ChronoDuration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&test_opts.ledger_path).unwrap(); // `mount_file`/`mount_folder` are still needed: the write-permission @@ -2870,6 +3328,44 @@ mod tests { assert!(text.contains("tok-2"), "{text}"); } + #[test] + fn a_sheet_already_restored_refusal_serializes_to_jsonl() { + let mut buf = Vec::new(); + RestoreResult::SheetAlreadyRestored { + spreadsheet_id: "sheet-1".to_string(), + sheet_id: 999, + sheet_title: "Deleted".to_string(), + restored_at: Some(Utc::now()), + live_lease: Some(LiveLease { + token: "tok-6".to_string(), + expires_at: Utc::now(), + }), + } + .write_jsonl(&mut buf) + .unwrap(); + let text = String::from_utf8(buf).unwrap(); + assert!( + text.contains("\"status\":\"sheet-already-restored\""), + "{text}" + ); + assert!(text.contains("tok-6"), "{text}"); + } + + #[test] + fn verdict_names_the_already_restored_refusal() { + assert_eq!( + RestoreResult::SheetAlreadyRestored { + spreadsheet_id: "sheet-1".to_string(), + sheet_id: 999, + sheet_title: "Deleted".to_string(), + restored_at: None, + live_lease: None, + } + .verdict(), + "sheet-already-restored" + ); + } + #[test] fn verdict_distinguishes_a_headless_waived_restore() { let restored = |headless_waiver| RestoreResult::Restored { diff --git a/src/drive/sheets/format.rs b/src/drive/sheets/format.rs index 5f26ff57..45733903 100644 --- a/src/drive/sheets/format.rs +++ b/src/drive/sheets/format.rs @@ -1285,6 +1285,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token @@ -2856,6 +2857,7 @@ mod tests { expires_at: chrono::Utc::now() - chrono::Duration::hours(1), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(&ledger_path).unwrap(); diff --git a/src/drive/sheets/protection.rs b/src/drive/sheets/protection.rs index 739c832d..5c8d4fa4 100644 --- a/src/drive/sheets/protection.rs +++ b/src/drive/sheets/protection.rs @@ -1139,6 +1139,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token diff --git a/src/drive/sheets/structure.rs b/src/drive/sheets/structure.rs index de1b0b7e..a87527c3 100644 --- a/src/drive/sheets/structure.rs +++ b/src/drive/sheets/structure.rs @@ -2192,6 +2192,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token diff --git a/src/drive/sheets/validation.rs b/src/drive/sheets/validation.rs index e7189836..5921e873 100644 --- a/src/drive/sheets/validation.rs +++ b/src/drive/sheets/validation.rs @@ -892,6 +892,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token diff --git a/src/drive/sheets/write.rs b/src/drive/sheets/write.rs index 1c6b060f..1d6b6f01 100644 --- a/src/drive/sheets/write.rs +++ b/src/drive/sheets/write.rs @@ -826,6 +826,7 @@ mod tests { expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), released_at: None, restored_at: None, + restored_sheet_id: None, }); ledger.save(ledger_path).unwrap(); token