fix(state): serialize state.db schema surgery across processes + backup-refusal hard stop - #84882
Merged
Merged
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 (#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 #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>
The Aug 2026 incident in #69603 documented a fail-open: when the pre-repair backup was refused (another same-process handle open), repair_state_db_schema() recorded backup_path=None and proceeded — leaving the writable_schema surgery, FTS-schema deletion, REINDEX and VACUUM strategies reachable against the only remaining copy of the damaged DB. _backup_db_file() now returns (path, reason) and the repair path treats any refused/failed backup as an unconditional hard stop: abort before the first mutating strategy and surface the reason in report['error']. Explicit backup=False (CLI --no-backup) is unchanged — that is the operator opting out, not a silent failure. Three new tests: refusal hard-stops with source bytes untouched, OS-level copy failure hard-stops with the reason surfaced, and backup=False still repairs.
Contributor
૮ >ﻌ< ა ci reviewran on 87876ca — chore(contributors): map ernst-bablick email for PR #69609 s ❌ Job failuresOS-specific tests / Windows-only tests · View jobJob OS-specific tests / Windows-only tests failed.
|
alt-glitch
added
type/bug
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
state.dbschema repair is now serialized across processes, so gateway + CLI + cron + Desktop can no longer racewritable_schemasurgery and re-corrupt the file — the exact repair/re-corrupt cascade in #69603 (P1), reproduced live today on a real corrupted install (5 failed repair attempts in 2.5h with concurrent openers).Salvage of #69609 by @ernst-bablick (authorship preserved), widened with a backup-refusal hard stop from the incident report in the issue thread.
Changes
hermes_state.py(from fix(state): serialize state.db schema surgery across processes #69609): cross-process file lock aroundrepair_state_db_schema()(fcntl on POSIX, msvcrt on Windows — no module-top fcntl import) +_bump_schema_cookie()after bothsqlite_masteredit sites (PRAGMA schema_version=N+1under writable_schema — SQLite does NOT auto-bump the cookie on direct sqlite_master UPDATEs, so other processes' prepared statements never invalidated)_backup_db_filereturns(path, reason); repair hard-stops before ANY mutating strategy (FTS rebuild, REINDEX, surgery, VACUUM) when the pre-repair backup is refused or fails, surfacing the reason inreport['error']. Explicitbackup=False(CLI--no-backup) unchanged.backup=Falsestill repairsValidation
messages_ftssqlite_master row (the #69603 class), two simultaneous subprocesses calling repair: strategies['dedup_schema','already_healthy'], exactly 1 backup file, integrity ok, all 20 messages intact, zero tracebacksFixes #69603.
Infographic
https://files.catbox.moe/dnkur0.png