fix(storage): surface degraded session database state - #86762
Conversation
trevorgordon981
left a comment
There was a problem hiding this comment.
Sound, well-scoped fix. The process-local latch design is the right call — _storage_status_by_path with intentional reset-on-restart so a completed offline repair is adopted without a hidden persistent marker. The StorageDegradedError on write to a latched-store is a clean way to stop a write retry storm. Tests including test_unrepaired_malformed_write_latches_storage_and_pauses_later_writes are good. A couple of notes.
1. The "process-wide" guarantee is per-process, not cross-process
mark_storage_degraded latches into a module-global dict in the current process. Long-running backends may run as more than one process (a gateway writing state.db plus a second gateway, or a desktop poller). If corruption is first observed in process A, only A latches; process B, on its next write, redisovers via its own is_malformed_db_error path and latches independently. The docstring's framing — "Long-running backends create short-lived SessionDBs ... while the gateway keeps another instance" — is all one process, so within-process it's exactly right. Just be aware the guarantee doesn't extend across processes; if two writers exist, the write-retry-storm pause only kicks in after each process individually observes structural failure. Probably acceptable, but worth stating explicitly given "process-wide" reads as stronger than it is.
2. Latch only triggers on write/heal, not on a read-only surface
The latch fires through the write and schema-heal paths (mark_storage_degraded on is_malformed_db_error). A purely read-only SELECT against a malformed DB — the sidebar poll, say — may not hit that path, so the first sighting of corruption by a read-only process won't latch. The desktop banner relies on get_storage_status, which only reports "degraded" if some process latched it. If the first (and only) observer is read-only, the banner stays "ok" until a write occurs. Worth confirming the malformed-repair path also marks on read-side detection.
3. Minor
get_storage_statusreturns"ok"for an absent store too (key missing → "ok"). An uninitialized DB is not corrupted, so that's fine, but a caller can't distinguish "genuinely ok" from "never seen." Acceptable given the desktop only needs the degraded red flag.
Tests
Good coverage of the core: latch-on-malformed-write, pause-later-writes, plus the web-server/health-probe wiring in test_web_server.py and the TS hook/store tests including the caller-side storage: 'degraded' → $storageStatus propagation. The gap is cross-process (finding #1) — nothing exercises two independent SessionDB instances/processes seeing corruption.
|
Rebased onto current |
9f61f36 to
f364df2
Compare
fix(storage): surface degraded session database state
|
trevorgordon981
left a comment
There was a problem hiding this comment.
Blocking finding from re-reviewing the current head (f364df22): the PR's own FTS-rebuild test suite is red, and CI slice 4/12 fails because of it. This is a genuine internal regression, not a pre-existing flake.
What fails: tests/state/test_fts_runtime_rebuild.py — 6 of 11 tests fail (reproduced locally on this head):
- test_second_corruption_fails_open_and_rebuilds_on_reopen
- test_failed_in_place_rebuild_fails_open
- test_stale_search_preserves_not_semantics
- test_existing_peer_observes_fail_open_marker
- test_failed_startup_rebuild_keeps_fts_detached
- test_legacy_inline_fts_fails_open_and_recovers
Root cause: hermes_state.py:4069-4079 now raises StorageDegradedError as soon as a write hits an FTS-corruption error class after an in-place rebuild fails:
if is_malformed_db_error(exc):
mark_storage_degraded(self.db_path, exc)
raise StorageDegradedError(...) # hard-fails persistence
But the whole point of the existing FTS fail-open path (. _try_runtime_fts_rebuild, then . _enter_fts_fail_open) is that when the one-shot rebuild fails, the DB should detach the derived FTS indexes and continue in a degraded-but-working mode, preserving canonical message rows (the warning log at 4218/4226 documents exactly this intent). The new degrade-latch now turns that recovery into a hard StorageDegradedError that blocks all persistence — which is precisely what the fail-open tests assert must not happen.
Notably, the author's own summary says "test_fts_runtime_rebuild.py ... pass (28 passed)" — that's not the case here; 6/11 fail, and CI slice 4/12 is red on this exact file.
Suggested direction: the degraded-surface feature and the FTS fail-open recovery need to coexist. The latch should mark the storage degraded (so the banner/API surface it) without raising past the fail-open recovery — i.e. let _enter_fts_fail_open / the no-FTS degraded mode proceed, and have mark_storage_degraded reflect the partially-degraded state rather than hard-failing the write. That also lines up with my earlier finding #2 (the banner should report degraded on read-side observation, not only when a write hard-fails).
Happy to dig into the exact call-flow if useful.
What does this PR do?
Adds the phase-one failure semantics for a corrupt
state.db: a confirmed unrecoverable malformed write latches the affected profile as degraded, pauses subsequent persistence, exposes the state to API clients, and shows a non-dismissable Desktop recovery notice. A successful in-place derived-FTS rebuild remains healthy; only a rebuild that cannot recover the write enters the latch.The existing
hermes sessions repaircommand remains the guided recovery path. Full cross-process quiescing and offline recovery orchestration are a separate phase, so this PR uses a non-closing reference.Related Issue
Refs #72046
Addressing maintainer feedback
Type of Change
Changes Made
hermes_state.py: adds a process-wide per-database degraded-state latch and short-circuits later writes after unrecoverable malformed corruption.hermes_cli/web_server.py: latches failed session-store heals and adds the selected profile's storage state to/api/status.hermes_cli/web_routers/sessions.py: includesstoragein the/api/sessionsresponse envelope.apps/desktop/src/store/storage-status.ts: owns the renderer's storage-status atom.apps/desktop/src/components/storage-degraded-banner.tsx: renders persistent repair guidance while writes are paused.apps/desktop/src/app/contrib/wiring.tsx: mounts the Desktop banner globally.apps/desktop/src/app/shell/hooks/use-status-snapshot.ts: synchronizes the REST status field into the renderer atom.apps/desktop/src/types/hermes.ts: declares the new REST fields.tests/test_state_db_malformed_repair.py: proves an unrepaired malformed write latches storage and blocks later appends.tests/hermes_cli/test_web_server.py: proves session and status APIs surface the same degraded state.apps/desktop/src/app/shell/hooks/use-status-snapshot.test.ts: proves Desktop publishes the status field to the persistent-warning state.How to Test
PATH="$VIRTUAL_ENV/bin:$PATH" /opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' sh./opt/homebrew/bin/timeout -k 30 480 "$VIRTUAL_ENV/bin/python" -m pytest tests/test_state_db_malformed_repair.py::test_unrepaired_malformed_write_latches_storage_and_pauses_later_writes tests/hermes_cli/test_web_server.py::TestWebServerEndpoints::test_storage_degraded_is_exposed_to_session_and_status_clients -q -x --timeout=60.StorageDegradedError, later writes are refused without retrying SQLite,/api/statusand/api/sessionsreturnstorage: "degraded", and Desktop displays the repair guidance.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A.
Screenshots / Logs
Focused backend coverage passed:
2 passedin 8.36s. The local Desktop Node dependencies are absent (vitest: command not found), so its new hook coverage is included for CI. The scoped Python lint passed; the Windows-footgun scanner found only four pre-existing encoding warnings outside this diff intests/hermes_cli/test_web_server.py.