Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<ledger>.lock` `flock`, kernel-released on process death, so a crashed holder never leaves a stale lock and `Drop` never unlinks anything — nothing advises deleting the file any more (on Unix; non-Unix has no `flock` and falls back to the old marker-plus-unlink scheme, so this guarantee doesn't extend there). Every leased write now *waits* for a busy lock (printing a one-line notice, `OMNI_DEV_LEASE_LOCK_WAIT_SECS`-bounded) instead of hard-failing; the lock remains ledger-global, so this narrows "hard-fail" to "wait", it does not add per-file scope (tracked as a follow-up). `drive lease prune` now secures the lock *before* touching a row's backup rather than after, so a lock collision leaves both the row and its backup untouched instead of stranding a row that points at an irrecoverably deleted backup. The identical fix applies to `gmail insert`'s own lock (`src/cli/gmail/insert/ledger.rs`), which stays non-waiting since it is held for a whole multi-message run. See [docs/drive.md](docs/drive.md#concurrent-access) and [ADR-0080](docs/adrs/adr-0080.md) §4.
- **`drive sheets format-cells`/`update-borders`/etc. and the `structure` verbs no longer orphan a `pending` audit record when building the request fails after the lease gate** ([#1688](https://github.com/rust-works/omni-dev/issues/1688), [ADR-0080](docs/adrs/adr-0080.md) §11): `gate_leased_write` fsyncs the write-ahead `pending` record and only the mutating call's own `allowed`/`failed` outcome concludes it, but `format.rs`/`structure.rs` built their `batchUpdate` request (`parse_hex_color`, a resolved sheet's missing `sheetId`) *after* the gate — so a request that could never be issued (e.g. `update-borders --color ZZZZZZ` under an otherwise-valid `--lease`) still opened a `pending` record with nothing to close it, indistinguishable from a process that died mid-write. Both engines now build the request before the gate, matching `protection.rs`'s existing shape (`validation.rs`/`sheets/write.rs`/`docs/write.rs`/`content_edit.rs` were never affected — each either builds first already or has no fallible step between the gate and its mutating call). One accepted side effect: a malformed request under a bad or absent lease now reports the request-build refusal rather than the lease refusal, which is the ordering both engines already use ahead of the gate for every other pre-lease refusal.
- **`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.

Expand Down
15 changes: 14 additions & 1 deletion docs/adrs/adr-0080.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 21 additions & 4 deletions docs/drive.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 <TOKEN>` restores a file from the backup a lease
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions docs/log.md
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,8 @@ once minted (or the existing one, for `already-leased`), and
`<TOKEN>` argument — so `--query 'restored_from_lease_id:<token>'` 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`,
Expand Down
50 changes: 50 additions & 0 deletions src/cli/drive/lease.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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(),
Expand All @@ -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();

Expand Down Expand Up @@ -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();

Expand Down
1 change: 1 addition & 0 deletions src/cli/drive/sheets/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading