fix(storage): enforce root ownership and detect account corruption - #1937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds privacy-safe SQLite integrity probing, exposes it through account sessions, schedules it in account workers, and enforces exclusive Marmot root ownership in CLI and daemon paths. Tests and documentation cover ownership isolation, probe outcomes, incident findings, and recovery requirements. ChangesIntegrity and ownership hardening
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Datawav
left a comment
Datawav
left a comment
There was a problem hiding this comment.
Automated review
Actionable findings:
- [P1] TUI bypasses the new root-ownership gate
Location: crates/cli/src/lib.rs:297
Trigger:wn tuiis started for a home whose root lease is already held bywn-agentorwnd; the earlier TUI return never reaches this acquisition.
Impact: Root exclusivity remains unenforced for a storage-backed direct CLI entry point, permitting concurrent hydrated runtimes against one Marmot home and preserving the corruption risk this PR addresses.
Fix: AcquireMarmotRootRuntimeLeasefor the resolved home before callingtui::run_tui, or route TUI through the owning runtime.
Confidence: high
|
Reply to Datawav’s review (no inline thread was present). @Datawav @erskingardner Addressed the P1 TUI startup finding in signed commit
Limitation: this is a startup-only check, not a reservation for the UI lifetime. There remains a TOCTOU window between concurrent TUI startups; both may pass the startup check. Their storage-backed children still acquire their own runtime leases. Routing the TUI through the owning runtime is a candidate follow-up. Lease handoff is not implemented: it requires new ownership/subprocess semantics beyond this review fix. This scoped design was explicitly accepted by the operator and manager. The regression starts a disposable real Validation on this commit (all exit 0):
These are the full requested crate suites, not a claim of full workspace |
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
Reviewed with the branch checked out: built it, ran the new tests, ran clippy on the touched crates, and benchmarked the integrity probe.
The ownership mechanism is implemented correctly and the code is clean. But it lands a large undocumented behavioral regression, and the corruption probe is undersized for the exact database that motivated it. Requesting changes on those two.
Blocking
1. Concurrent wn invocations now fail hard, with no retry and no distinguishable error code
Measured — six concurrent wn --json accounts list against one home:
master: 6/6 {"ok":true,"result":{"accounts":[]}}
PR: 1/6 ok
5/6 {"error":{"code":"command_failed",
"message":"marmot runtime root is already in use"},"ok":false}
try_acquire is nonblocking — no backoff, no --wait. It also gates read-only commands, though the stated hazard is two hydrated runtimes mutating.
AppError::code() already yields "runtime_busy" (crates/marmot-app/src/error.rs:396), but the CLI collapses it to command_failed, so automation has to string-match "already in use" — which is exactly what the new tests do. Please plumb the real code through.
wn logout is now unreachable while wnd runs: daemon_socket_for_client refuses to forward Logout without an explicit socket (crates/cli/src/lib.rs:752), and the lease blocks local execution. This PR inverts daemon_running_does_not_auto_forward_logout to assert exactly that.
The incident doc in this PR notes "The babysitter invokes the same CLI binary" — that automation will break.
crates/cli/CHANGELOG.md documents only the TUI change. The much larger "all direct commands now require exclusive ownership" has no entry at all.
2. The lease is root-wide, so one home can no longer serve two accounts concurrently
The evidence is in this PR's own test churn: stream_start_quic_chunks_and_final_payload_verify_through_mls_messages had to be split into home + bob_home because bob's long-lived watcher now blocks alice. --account multi-account homes are a supported feature, and this removes concurrent use of them silently. Needs either an explicit decision plus docs, or a per-account lease.
3. The daemon still opens a second hydrated runtime against its own leased root
crates/cli/src/daemon/mod.rs:608 calls run_cli_local, which builds an independent MarmotApp on the same root while the daemon's own app runtime is live. Same process, lease not re-acquired, so it cannot be caught.
That is precisely the hazard in this PR's root-cause paragraph, in the deployment where the incident occurred. The description discloses it as out of scope, which is fair — but it means this change probably does not close the reported failure mode, and the title reads like it does. Worth making that explicit up front.
Should fix
4. The 250 ms probe budget is too small for the database it was written for
Benchmarked PRAGMA quick_check(1) on SQLCipher — warm cache, local NVMe, single-table schema, i.e. best case:
| size | on-disk | full quick_check | with 250 ms budget |
|---|---|---|---|
| 8 MB | 9.4 MB | 95 ms | Healthy |
| 38 MB | 44.8 MB | 348 ms | Healthy — 249.9 ms, right at the edge |
| 100 MB | 118 MB | 445 ms | Incomplete (interrupted) |
The incident database was 37,994,496 bytes. A real 78-table schema with indexes, a cold cache, or mobile flash all push it over. So the detector silently stops detecting as history grows, emitting status=incomplete every 120 s forever.
Suggest tracking consecutive incompletes and alerting on that — today "never completed a check" and "one slow tick" are indistinguishable to an operator.
Two things I checked that are sound: interruption yields SQLITE_INTERRUPT → Incomplete, never a false Corrupt; and SQLite's checkProgress() does fire the progress handler inside sqlite3BtreeIntegrityCheck, so the budget genuinely bounds the single OP_IntegrityCk opcode.
5. The probe blocks the account's only connection on a tokio worker thread
crates/cgka-session/src/lib.rs:345 — "The engine and this handle share one underlying connection." So the probe holds the single ConnectionSlot mutex for 250 ms of VM time plus unbounded, non-preemptible I/O, every 120 s. That stalls engine work, projections, and SqliteAccountStorage::close() — the call required to release locks before iOS suspension.
lifecycle.is_stopping() is checked before the probe, but a probe that started 1 ms earlier makes shutdown wait it out. Consider spawn_blocking, or skipping when a close is pending.
6. Duplicate classifier
stateless_stream_command (crates/cli/src/lib.rs:561) has byte-identical match arms to the existing client_hosted_stream_command (crates/cli/src/lib.rs:762). The description says "one shared classifier defines that exemption and execution path", but there are now two that must stay in sync — and drift means either a spurious hard failure or a storage-opening command gaining a lease exemption. Reuse client_hosted_stream_command(...).is_some().
The exemption itself is safe today: I traced stream_command_local for Receive and Send { start_event_id: None } — QUIC only, never account storage, and the arms exactly match the dispatch they replaced.
Minor
storage_integrity.rs's_ =>foldsErr(StorageError::Closed)into a warning, so every shutdown racing a tick logs a spurious "health is unknown". MatchIncompleteexplicitly and keepClosedquiet.tracing::info!on healthy probes is ~720 lines/day/account at INFO.debug!for healthy, keeperror!/warn!.- If
progress_handler(0, None)fails, the code returnsIncompletewhile leaving the always-interrupt closure installed — every later query on that connection would returnSQLITE_INTERRUPTpermanently.check_owned()effectively cannot fail here, but the code already branches on it, so it should not be able to leave the connection armed. docs/incident-2026-09-18-session-corruption.md: worth deciding whether an operational incident record belongs in the public repo. It carries SHA-256 digests and byte sizes of production database snapshots, row and page counts, file-mode remediation notes, and internal tracker IDs (btq-harness-2e48d60f…) that mean nothing here. No key material is exposed, so this is a scope/disclosure judgment rather than a leak — and the repo-relevant parts (supported access modes, probe semantics and limits) are already inlocal-artifact-safety.md.
Verified clean
cargo clippy -p storage-sqlite -p marmot-app -p wn-cli -p cgka-session --all-targets— clean.crates/cli/tests/root_ownership.rs2/2 pass;storage-sqlite integrity::3/3 pass.- All four commits have verified signatures.
- No
Cargo.lockor workspace-version churn. The rusqlitehooksfeature is pure Rust and adds noSQLITE_ENABLE_*compile flags. - Daemon lease is correctly placed before
remove_stale_socket/remove_stale_pid. - Lease bindings are
_root_lease, not a bare_, so they actually live for the scope. - Lock file creation is
openat(..., O_NOFOLLOW | O_CLOEXEC, 0600)relative to a prepared directory fd — no symlink swap, even though everywnrun now creates it. - The TUI really is a subprocess shell (
crates/cli/src/tui/client.rs:31) with no in-process storage, so acquire-then-drop is honest. Still a TOCTOU advisory check, which the comment and CHANGELOG both say. - New tracing is privacy-compliant: fixed
targetandmethod, status only, and the raw SQLite diagnostic string is discarded on theCorruptpath.
3 participants
Direct
wncommands andwndstartup bypassed the root lease already held bywn-agent, allowing independent hydrated runtimes to open one Marmot home. Acquire that lease before local execution or daemon socket cleanup, including fallback after an abandoned socket. Existing socket clients continue through their owning daemon. Raw QUIC receive and unanchored send remain independent of account storage; one shared classifier defines that exemption and execution path.Add a periodic structural integrity probe on the existing keyed account connection, with a SQLite VM interruption budget and fixed healthy/corrupt/incomplete telemetry. This detects damage without returning private diagnostic rows or attempting repair. The incident report records the ownership defect, incomplete recovery evidence, supported access modes, and the manager deployment handoff. The historical cause of the HMAC failure remains unresolved; the retained rebuild has 2,803 foreign-key violations and is not a verified restore candidate.
Validation:
just fast-cipassed; all three commits have verified signatures. Complete touched-crate tests pass: CLI 627 unit + 104 integration + two ownership tests; application 1,680 tests (12 ignored); session 22 tests; storage 776 unit + two integration tests (seven ignored). Documentation checks pass. Ownership tests cover logout rejection until daemon shutdown, raw streams while a root lease is held, and anchored streams between separate homes; all three integrity regressions pass.Earlier runs exposed storage setup/subprocess failures and one stale compiled test fixture that expected an integer from
cipher_page_size. The committed fixture parses the returned string; the full current-source storage rerun passes. Ignored tests remain ignored, and the full workspacejust cimatrix was not run locally.Deployment handled by manager. A coherent binary cohort and sender routing through the owning socket facade are required;
WN_SOCKETuses the wnd protocol, not the wn-agent protocol. Merge, recovery, deployment, and live messaging verification remain open.Summary by CodeRabbit
Bug Fixes
New Features
Documentation