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 @@ -114,6 +114,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **`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.
- **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.
- **`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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ termcolor = "1.1"
anyhow = "1.0"
chrono = { version = "0.4", features = ["serde"] }
tempfile = "3.27"
# Cross-platform file-identity comparison (inode on unix, a file handle on
# Windows) for request_log.rs's audit-file collision guard (issue #1694) —
# already in the dependency graph transitively via `walkdir`.
same-file = "1.0"
reqwest = { version = "0.13", features = ["form", "json", "multipart", "stream"] }
tokio = { version = "1.52", features = ["full"] }
axum = "0.8"
Expand Down
18 changes: 18 additions & 0 deletions docs/adrs/adr-0080.md
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,24 @@ and `-f/--follow` all apply unchanged, so `omni-dev log --audit --query
`--audit`, since exemption from pruning is the point. This is a CLI-surface
change and lands under `update-snapshots` in Phase 1.

The exemption is enforced against the **resolved file**, not merely the
`--audit` flag or the default path spelling (issue #1694): `prune` and
size-capped rotation both refuse outright when the path they were handed
resolves to the same file as `audit_file_path()` — via the `same-file` crate
(inode on unix, a file handle on Windows) where both sides exist, else by a
lexically-`..`-collapsed, canonicalized comparison — so a `OMNI_DEV_LOG_FILE`
naming `audit.jsonl` directly, via a `..` segment, or via a symlink can't be
pruned or rotated away either. `record_audit`'s own
collision guard uses the same file-identity comparison rather than a raw
path equality, for the same reason. One direction is deliberately left open:
a colliding `OMNI_DEV_LOG_FILE` can still cause a best-effort record to land
*in* `audit.jsonl` — only the destructive operations are guarded, since
silently dropping the user's request logging would be its own surprise, and
the next `record_audit` call still fails loudly on the same collision.
Separately, a `OMNI_DEV_AUDIT_LOG_FILE` pointed at one of rotation's own
*sibling* paths (`<log>.1`, `<log>.lock`, …) is not covered by this guard —
a contrived misconfiguration distinct from the reported one.

**Fail-closed inverts this codebase's usual ordering.** `request_log`'s
existing writers are deliberately best-effort and non-blocking — a logging
failure must never change the caller's exit code (module doc,
Expand Down
18 changes: 13 additions & 5 deletions docs/log.md
Original file line number Diff line number Diff line change
Expand Up @@ -445,8 +445,8 @@ described:
|---|---|---|
| On a write failure | Swallowed; the command's exit code is unaffected | Propagated; the caller aborts the operation it was about to audit |
| `OMNI_DEV_LOG_DISABLE=1` | Suppresses all writes | No effect |
| `omni-dev log prune` | Trims by age/size | Refuses `--audit` outright (`log prune does not support --audit`) |
| `OMNI_DEV_LOG_MAX_SIZE` rotation | Applies | Never applies, regardless of the setting |
| `omni-dev log prune` | Trims by age/size | Refuses `--audit` outright (`log prune does not support --audit`), **and** refuses any resolved log path that names the audit file — e.g. `OMNI_DEV_LOG_FILE` pointed at `audit.jsonl` directly, via a `..` segment, or via a symlink |
| `OMNI_DEV_LOG_MAX_SIZE` rotation | Applies | Never applies, regardless of the setting or how `OMNI_DEV_LOG_FILE` is spelled |
| Path override | `OMNI_DEV_LOG_FILE` | `OMNI_DEV_AUDIT_LOG_FILE` |
| Location when unset | `<state dir>/omni-dev/log.jsonl` | `<state dir>/omni-dev/audit.jsonl` |

Expand All @@ -455,10 +455,18 @@ separately, on `prune` (`omni-dev log prune --audit`, always refused, above);
placed before a subcommand name instead — `omni-dev log --audit prune ...` —
it is refused rather than silently ignored, since a subcommand never
consults it. `record_audit` also refuses to write if `OMNI_DEV_LOG_FILE` and
`OMNI_DEV_AUDIT_LOG_FILE` are configured to resolve to the same path: the two
sinks are siblings by default but not otherwise mutually exclusive by
`OMNI_DEV_AUDIT_LOG_FILE` are configured to resolve to the **same file** —
compared by file identity (the `same-file` crate — inode on unix, a file
handle on Windows — falling back to a normalized path when one side doesn't
exist yet), not merely by identical spelling, so a `..` segment,
a relative-vs-absolute spelling, or a symlink can't slip past the check: the
two sinks are siblings by default but not otherwise mutually exclusive by
construction, so misconfiguration is caught at write time instead of
silently blending the fail-closed sink into the best-effort one.
silently blending the fail-closed sink into the best-effort one. That guard
only stops writes *into* `log.jsonl`; a colliding `OMNI_DEV_LOG_FILE` can
still cause a best-effort record to *land in* `audit.jsonl` (it just can no
longer be pruned or rotated away, per the table above) — the next
`record_audit` call still fails loudly, surfacing the misconfiguration.

Every record carries `kind: "audit"` and, in `context`, an `integration` key
(`"drive"` today, so a later integration's audit trail is additive rather
Expand Down
Loading
Loading