diff --git a/CHANGELOG.md b/CHANGELOG.md index bbdeb08c..12576cb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`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. - **The audit log's exemption from `log prune`/rotation now holds against the *resolved* file, not just the `--audit` flag or the default path spelling** ([#1694](https://github.com/rust-works/omni-dev/issues/1694), [ADR-0080](docs/adrs/adr-0080.md) §11): `omni-dev log prune` refused `--audit` outright, but nothing stopped `OMNI_DEV_LOG_FILE` from being pointed at `audit.jsonl` directly, via a `..` segment, or via a symlink — the obvious workaround, which let `log prune` or `OMNI_DEV_LOG_MAX_SIZE` rotation destroy the fail-closed audit trail ADR-0080 §11 promises is unprunable. `request_log::prune` and size-capped rotation now both refuse outright when the path they were handed resolves to the audit file, compared by **file identity** (the [`same-file`](https://docs.rs/same-file) crate — inode on unix, a file handle on Windows — where both sides exist, else a lexically-`..`-collapsed, canonicalized path comparison so a not-yet-created symlink target is still caught) rather than a raw path or spelling. `record_audit`'s own collision guard (refusing to write when `OMNI_DEV_AUDIT_LOG_FILE` and `OMNI_DEV_LOG_FILE` name the same file) is upgraded to the same file-identity comparison, closing the same `..`/symlink gap there. This only guards the destructive operations — a colliding `OMNI_DEV_LOG_FILE` can still cause a best-effort record to land in `audit.jsonl`, which the next `record_audit` call still catches and fails loudly on. +- **A relative `drive lease` `--backup-dir` is now absolutized before it's stored in the ledger** ([#1692](https://github.com/rust-works/omni-dev/issues/1692)): a relative value from `--backup-dir`, `OMNI_DEV_DRIVE_LEASE_BACKUP_DIR`, or `settings.json`'s `lease.backup_dir` was stored **relative** in the ledger row, so a later `drive lease prune`/`restore` run from a different cwd would resolve it against the wrong directory. `prune`'s `clear_backup` would get `NotFound`, treat the backup as already gone, and drop the ledger row while reporting `bytes_freed` — leaving the real backup on disk with no row pointing at it; `restore` from the other cwd would fail outright with "Failed to read backup". `resolve_backup_dir` (`src/drive/lease/settings.rs`) now routes every non-default source through a new `absolutize()` helper (`std::path::absolute`, touching neither the filesystem nor requiring the directory to exist), so the ledger always stores an absolute path regardless of the caller's cwd. The hard-coded default (already absolute, under the state/data directory) is unaffected. - **`gmail sync-all`'s rate-limit retry notices no longer corrupt the live progress bars or go unattributed** ([#1651](https://github.com/rust-works/omni-dev/issues/1651)): the shared retry loop (`retry_if`, `src/utils/http.rs`) printed a raw `eprintln!` while waiting out a 429/403, so on a `sync-all` run — where every account's bars share one `indicatif::MultiProgress` ([#1504](https://github.com/rust-works/omni-dev/issues/1504)) — a rate limit on any account teared the render and named no account, leaving no way to tell which one was throttled. `retry_if`/`retry_429` gain an optional `notify` callback (`RetryNotifyFn`) invoked in place of the `eprintln!`; Atlassian, Datadog and Drive keep the default fallback unchanged (they pass `None`), while `GmailClient` exposes `set_retry_notify` so `gmail sync`/`sync-all` can register one once a progress channel exists. The notice now renders as a transient message on the throttled account's own fetch bar — already prefixed with the account label in `sync-all` — via a new `SyncProgressEvent::RateLimited` event, and clears itself once that fetch's retry resolves. - **`worktrees ui`'s embedded-terminal shutdown could still hang forever, this time on the kernel rather than a child process** ([#1611](https://github.com/rust-works/omni-dev/issues/1611)): [#1605](https://github.com/rust-works/omni-dev/issues/1605)/[#1610](https://github.com/rust-works/omni-dev/issues/1610) bounded the child-side wait (a SIGHUP-ignoring shell) by reaping the whole process group with escalation before joining the PTY thread — but `TerminalTab::shutdown` ran that join, and the `Pty` drop inside it, on `tokio::task::spawn_blocking`. Nobody ever awaited that task, yet `tokio::Runtime::drop()` still blocks until every outstanding blocking-pool task finishes, so anything that made the join-then-drop take unboundedly long could still wedge a shutdown — reintroducing the exact class of bug #1605 fixed, from a new cause. Live `lldb` debugging of a hung process (reading the actual `pid` argument off the blocked `wait4()` syscall, confirmed on two independent reproductions) found that cause: a `TabKind::Shell` tab's child is `/usr/bin/login` on macOS, and a killed `login` process can land in the kernel's own exit teardown (`ps` reports state `Es+`) and never finish it — the SIGKILL genuinely lands, but the reap that would collect the resulting zombie simply never completes. Nothing in-process can bound a stuck kernel-side teardown, so `shutdown` now joins the PTY thread and drops the `Pty` on a plain, detached `std::thread` instead of `spawn_blocking` — untracked by the runtime, so a stuck reap can no longer hold up `Runtime::drop`, whether in a test's per-test runtime or the real CLI's in `main.rs`. This was a real bug, not a test artifact: `shutdown` also runs on ordinary app quit, so a user's own Shell tab could in principle wedge `omni-dev worktrees ui` on exit the same way. Verified with a 20-run soak of `cargo test --lib worktrees::ui` at default parallelism (0 hangs, versus roughly 2 in 3 runs hanging beforehand); a new test pins the structural property the fix relies on — a detached thread that never finishes cannot block a runtime's drop, unlike a `spawn_blocking` task, which an adjoining `#[ignore]`d test documents by demonstrating the hang it *would* cause if run. diff --git a/src/drive/lease/settings.rs b/src/drive/lease/settings.rs index 2039f4e1..a33f8877 100644 --- a/src/drive/lease/settings.rs +++ b/src/drive/lease/settings.rs @@ -131,6 +131,20 @@ pub(crate) fn resolve_expiry_minutes( Ok(value) } +/// Resolves `dir` to an absolute path against the current working +/// directory, without touching the filesystem or requiring `dir` to exist — +/// a relative `--backup-dir`/env/settings.json value must not be stored +/// relative in the ledger, since a later `prune`/`restore` can run from a +/// different cwd (issue #1692). +fn absolutize(dir: PathBuf) -> Result { + std::path::absolute(&dir).with_context(|| { + format!( + "Failed to resolve backup directory {} to an absolute path", + dir.display() + ) + }) +} + /// Resolves `--backup-dir`. pub(crate) fn resolve_backup_dir( explicit: Option, @@ -138,13 +152,13 @@ pub(crate) fn resolve_backup_dir( settings: &LeaseSettings, ) -> Result { if let Some(dir) = explicit { - return Ok(dir); + return absolutize(dir); } if let Some(dir) = non_empty_var(env, LEASE_BACKUP_DIR_ENV) { - return Ok(PathBuf::from(dir)); + return absolutize(PathBuf::from(dir)); } if let Some(dir) = settings.backup_dir.clone() { - return Ok(dir); + return absolutize(dir); } default_backup_dir() } @@ -316,6 +330,39 @@ mod tests { assert!(resolved.ends_with("omni-dev/drive-backups")); } + #[test] + fn backup_dir_explicit_relative_is_absolutized() { + let resolved = resolve_backup_dir( + Some(PathBuf::from("relative/cli")), + &MapEnv::new(), + &settings(), + ) + .unwrap(); + assert!(resolved.is_absolute(), "{}", resolved.display()); + assert!(resolved.ends_with("relative/cli"), "{}", resolved.display()); + } + + #[test] + fn backup_dir_env_relative_is_absolutized() { + let env = MapEnv::new().with(LEASE_BACKUP_DIR_ENV, "relative/env"); + let resolved = resolve_backup_dir(None, &env, &settings()).unwrap(); + assert!(resolved.is_absolute(), "{}", resolved.display()); + assert!(resolved.ends_with("relative/env"), "{}", resolved.display()); + } + + #[test] + fn backup_dir_settings_relative_is_absolutized() { + let mut s = settings(); + s.backup_dir = Some(PathBuf::from("relative/settings")); + let resolved = resolve_backup_dir(None, &MapEnv::new(), &s).unwrap(); + assert!(resolved.is_absolute(), "{}", resolved.display()); + assert!( + resolved.ends_with("relative/settings"), + "{}", + resolved.display() + ); + } + // ── resolve_auth_policy ── #[test]