fix(state): serialize state.db schema surgery across processes - #69609
fix(state): serialize state.db schema surgery across processes#69609ernst-bablick wants to merge 1 commit into
Conversation
`repair_state_db_schema()` performs `PRAGMA writable_schema=ON` + `sqlite_master` surgery + `VACUUM` on a private connection. The only guard around it is `_repair_attempt_lock`, a `threading.Lock`, whose docstring claims it "serialises concurrent web_server / gateway opens" — but a threading lock covers threads inside one interpreter, not processes. A normal host runs four independent processes against the same state.db: the gateway service, the Desktop app's own `hermes serve` backend (it spawns one per launch, not a thin client), interactive CLI sessions, and the TUI slash worker. When two of them hit a malformed DB, both entered the critical section and each ran the full surgery while the other was mid-rewrite. Observed as a repair/re-corrupt cascade: the DB is repaired, then re-corrupts minutes later, repeatedly. Two fixes: 1. Wrap the surgery in a bounded `flock` on `<db>.repair.lock`. `flock` is the right primitive — the kernel drops it when the holder dies, so a crashed repairer cannot wedge future repairs the way a pidfile would. The acquire is bounded (NousResearch#36644's failure shape) and, unlike the kanban init lock, a caller that times out must NOT proceed: here "proceed anyway" is exactly the unsafe interleaving. It re-probes instead, and reports success if the holder already healed the file. Under the lock, the existing `_db_opens_cleanly()` check becomes a double-check: a queued process finds the DB healthy and returns `already_healthy` rather than re-running surgery on a repaired DB. 2. Bump the schema cookie after direct `sqlite_master` edits. Ordinary DDL bumps it for free and every other connection compares it before running a prepared statement — that is how they learn to drop a cached schema. Editing `sqlite_master` under `writable_schema=ON` does not, so live connections in other processes kept writing `messages` rows through triggers into `messages_fts*` shadow tables the surgery had just deleted. SQLite's writable_schema docs call out incrementing `schema_version` as the required companion to such an edit. Tests: four new cases in tests/test_state_db_malformed_repair.py, all using real child processes and a real flock. All four fail on main and pass with this change; the concurrency case asserts exactly one `malformed-backup-*` file is produced by two simultaneous repairers (two on main). Full state suite: 558 passed. Complements NousResearch#43742, which makes the *in-process* claim loser retry rather than raise; it explicitly leaves `repair_state_db_schema()` unchanged and does nothing cross-process. The two are independent and compose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Full suite now run on this branch, not just the The 3 failures are host-environment artifacts, not regressions — all reproduce on
Nothing in the run touches the |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this to the repair path; the current-main premise is confirmed. hermes_state.py:923-954 provides only a process-local threading.Lock, while repair_state_db_schema() still performs direct sqlite_master surgery at hermes_state.py:1326-1360 without a schema-cookie bump.
Problems
hermes_state.py:597fails open when the sidecar lock cannot be opened, then permits the destructive repair without cross-process serialization. The existing quarantine path instead fails closed on lock failure (hermes_state.py:1645-1657); this path needs the same safety property.- The simultaneous-repair test does not synchronize both children at the relevant race boundary (
tests/test_state_db_malformed_repair.py:726-740), so a serial scheduler can make it pass without exercising concurrent repairers. - The schema-cookie test closes its probe before repair and opens a new connection after it (
tests/test_state_db_malformed_repair.py:767-781), so it does not test the stated live-connection behavior.
Suggested changes
- Fail closed if opening/acquiring the repair lock is impossible.
- Make the concurrency and live-connection checks deterministic.
The related #71982 backup-copy race is separate, as noted in #69603 discussion. This is an automated hermes-sweeper review.
| "Could not open state.db repair lock %s (%s) — proceeding with " | ||
| "in-process serialisation only.", lock_path, exc, | ||
| ) | ||
| yield True |
There was a problem hiding this comment.
This must not yield True: when the lock file cannot be opened, this permits the same uncoordinated schema surgery the lock is meant to prevent. Please fail closed (yield False / return a repair error), consistent with the quarantine lock's refusal behavior on current main.
| probe.execute("PRAGMA writable_schema=ON") | ||
| before = probe.execute("PRAGMA schema_version").fetchone()[0] | ||
| finally: | ||
| probe.close() |
There was a problem hiding this comment.
This closes the only probe before repair, so the test only observes a persisted schema-version change through a fresh connection. Keep a connection alive across repair and execute a previously prepared schema-dependent operation to cover the claimed cache invalidation.
…try transient EIO on journal-mode probe Salvaged remainder of PR #82280 (state.db hardening rollup): - Runtime connection corruption: a sibling process replacing/truncating the backing file breaks the live write connection — every subsequent write raises 'file is not a database' and the gateway wedges permanently (messages pile up in memory). Add a bounded one-shot reconnect on the write path: close the broken connection, reopen the DB file (re-running WAL activation + schema reconciliation), retry the failed write once. - _on_disk_journal_mode: retry transient 'disk i/o error' (virtualized block devices) a few times before returning None, so a one-shot EIO doesn't push callers onto the fail-closed unknown-mode branch. The rollup's write-lock machinery, checkpoint-strategy changes, and repair serialization are intentionally NOT included — superseded by PRs #84277 and #69609, or wrong-direction per the POSIX lock-cancellation findings (#71724 lineage).
Fixes #69603.
Problem
repair_state_db_schema()runsPRAGMA writable_schema=ON+sqlite_mastersurgery +VACUUMon a private connection. The only guard around it is_repair_attempt_lock, athreading.Lockwhose docstring claims it "serialises concurrent web_server / gateway opens" — but a threading lock covers threads inside one interpreter, not processes.A normal host runs four independent processes against the same
state.db:hermes-gateway.servicehermes servebackend (it spawns one per launch — it is not a thin client)When two of them hit a malformed DB, both enter the critical section and each runs the full surgery while the other is mid-rewrite. Observed on my host as a repair/re-corrupt cascade — the DB is repaired, then re-corrupts minutes later, repeatedly (18:03, 22:05, 22:17 the same evening), each cycle logging:
Fix
1. Cross-process
flockaround the destructive path. An advisory lock on a sidecar file next tostate.db, so it covers every process on the host regardless of interpreter. Under the lock, the existing_db_opens_cleanly()check becomes a double-check: a queued process finds the DB already healthy and returnsalready_healthyinstead of re-running surgery on a freshly repaired DB.2. Bump the schema cookie after direct
sqlite_masteredits. Ordinary DDL bumpsschema_versionfor free, and every other connection compares it before running a prepared statement — that is how they learn to drop a cached schema. Editingsqlite_masterunderwritable_schema=ONdoes not bump it, so live connections in other processes kept writingmessagesrows through triggers intomessages_fts*shadow tables the surgery had just deleted. SQLite's writable_schema documentation calls out incrementingschema_versionas the required companion to such an edit.Tests
Four new cases in
tests/test_state_db_malformed_repair.py, using real child processes and a realflock(no mocking of the concurrency):malformed-backup-*file (two on main)already_healthysqlite_mastersurgeryAll four fail on
mainand pass with this change. Full-k statesuite: 1043 passed, 13 skipped.Relation to #43742
#43742 makes the in-process claim loser retry rather than raise; it explicitly leaves
repair_state_db_schema()unchanged and does nothing cross-process. The two are independent and compose.🤖 Generated with Claude Code