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 @@ -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 `<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.
- **`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
29 changes: 19 additions & 10 deletions docs/log.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
7 changes: 6 additions & 1 deletion src/drive/docs/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. }));
Expand Down
Loading
Loading