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 @@ -117,6 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **`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.
- **A `drive lease` command with a broken `settings.json` now warns instead of silently falling back to defaults** ([#1695](https://github.com/rust-works/omni-dev/issues/1695)): `LeaseFlags::resolve` called `Settings::load().unwrap_or_default()`, so a `settings.json` that failed to parse silently dropped every `lease.*` setting — including `biometrics_only`, whose default is the *less*-secure direction, so a broken config could silently downgrade the auth policy with no indication anything was wrong. A parse failure now logs a `tracing::warn!` naming the error and which settings are being ignored before falling back to defaults.
- **`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
76 changes: 74 additions & 2 deletions src/cli/drive/lease.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,18 @@ impl LeaseFlags {
// and `Settings::load_lease()` each independently re-read it;
// `SettingsEnv::from_settings` was added for exactly this (issue
// #1533), so both views come from the same parse (issue #1677 review
// finding).
let loaded = Settings::load().unwrap_or_default();
// finding). A parse failure warns rather than silently falling back
// (issue #1695) — `biometrics_only`'s default is the less-secure
// direction, so a broken settings.json must not silently downgrade
// it.
let loaded = Settings::load().unwrap_or_else(|e| {
tracing::warn!(
"{e:#}; falling back to default settings for this invocation — any \
`lease.*` config in settings.json (backup_dir, default_expiry_minutes, \
biometrics_only, allow_headless) is being ignored"
);
Settings::default()
});
let lease = loaded.lease.clone();
let profile = crate::utils::settings::active_profile_from(&crate::utils::env::SystemEnv);
let env = SettingsEnv::from_settings(loaded, profile.as_deref());
Expand Down Expand Up @@ -868,6 +878,68 @@ mod tests {
);
}

/// Thread-scoped log buffer, mirroring the `CaptureWriter`/`capture_info`
/// pattern in `src/gmail/chrome_profile.rs`.
#[derive(Clone, Default)]
struct CaptureWriter(std::sync::Arc<std::sync::Mutex<Vec<u8>>>);

impl std::io::Write for CaptureWriter {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.0.lock().unwrap().extend_from_slice(buf);
Ok(buf.len())
}
fn flush(&mut self) -> std::io::Result<()> {
Ok(())
}
}

impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for CaptureWriter {
type Writer = Self;
fn make_writer(&'a self) -> Self::Writer {
self.clone()
}
}

/// Runs `f` under a thread-local WARN-level subscriber and returns
/// everything it logged. `f` must be fully synchronous on this thread.
fn capture_warn(f: impl FnOnce()) -> String {
let writer = CaptureWriter::default();
let subscriber = tracing_subscriber::fmt()
.with_max_level(tracing::Level::WARN)
.with_ansi(false)
.with_writer(writer.clone())
.finish();
tracing::subscriber::with_default(subscriber, f);
let logs = String::from_utf8_lossy(&writer.0.lock().unwrap()).into_owned();
logs
}

#[test]
fn resolve_warns_and_falls_back_when_settings_json_fails_to_parse() {
// A missing settings.json resolves to defaults with no warning
// (the ordinary case, covered implicitly by every other test in
// this module) — this covers the "file exists but doesn't parse"
// case (issue #1695), which must warn rather than silently drop
// `lease.biometrics_only`/`backup_dir`/`default_expiry_minutes`.
let guard = crate::drive::test_support::EnvGuard::take();
let dir = guard.clear_credentials();
let settings_dir = dir.path().join(".omni-dev");
std::fs::create_dir_all(&settings_dir).unwrap();
std::fs::write(settings_dir.join("settings.json"), "{not valid json").unwrap();

let flags = LeaseFlags {
backup_dir: None,
expiry_minutes: None,
biometrics_only: false,
allow_headless: false,
};
let logs = capture_warn(|| {
let resolved = flags.resolve().unwrap();
assert_eq!(resolved.auth_policy, AuthPolicy::DeviceOwner);
});
assert!(logs.contains("settings.json"), "{logs}");
}

#[test]
fn print_result_does_not_panic_for_any_variant() {
for result in [
Expand Down
Loading