fix(state): refuse checkpoint and repair on unprovable live-writer state (#103339)

[Halldrix]

What does this PR do?

Closes the two evidenced second-writer holes on the live WAL state.db with ~35 lines, per the slim-redo direction:

1. hermes doctor --fix ran a raw wal_checkpoint(PASSIVE) on a bare sqlite3.connect with no holder guard — under a running gateway that second-writer handling corrupts state.db. The checkpoint is now routed through hermes_state_holders.live_writer_holds_db and skipped with an actionable finding while the database cannot be proven quiet. The skip message deliberately asserts no fact it cannot prove ("cannot prove state.db is quiet" — a True can mean a live holder or an unreadable file). The connection is also closed via contextlib.closing instead of a bare close() after the statement, so a failed checkpoint never leaves its connection open.
2. The repair EXCLUSIVE probe is left as-is: an earlier revision failed it closed on DatabaseError, but that was verified to refuse header-destroyed DBs with a misleading "live writer" message, so it was dropped. Fail-closed is enforced one layer down — the exclusive guard opens its own connection and refuses surgery honestly (test_repair_refuses_when_guard_cannot_open pins this).

Checkpoint-lock premise credit to @UncleCatWu's #40177 alongside the gap analysis from #103339.

Related Issue

Refs #103339 (partial: doctor-checkpoint + repair-probe here; hosted-rooms routing and VACUUM/optimize/migration gates out of scope).

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Changes Made

• hermes_cli/doctor_state.py (_state_db_wal): holder-guard skip + checkpoint run on the held exclusive-guard connection (no check-then-act window).
• hermes_state_holders.py: untouched — the preflight keeps its fail-open-on-unknown contract; the guard below is the authority.
• hermes_state_repair.py: untouched.
• Tests (4 doctor + 1 repair): skip-pins under live holder / unprovable connect / post-probe writer admission, quiet-success through the held guard, and guard-refusal with an honest message — each sabotage-proven red/green.

How to Test

scripts/run_tests.sh tests/test_state_db_repair_live_writer_guard.py tests/hermes_cli/test_doctor_wal_holder_guard.py tests/test_state_db_repair_non_destructive.py tests/test_wal_checkpoint_strategy.py tests/hermes_state/test_deleted_wal_checkpoint_guard.py

Result on this host: 49 passed, 0 failed, 3 skipped (requires_wal skips: host SQLite 3.46.1 is in the WAL-reset vulnerable range — pre-existing environmental). Every new test sabotage-proven red/green individually (without the doctor guard, the checkpoint demonstrably runs under the live writer; without the guard-held checkpoint, a post-probe writer races it). Pre-publish adversarial gate: 2 voices (APPROBAR-CONDICIONADO with the honest-message condition applied; no blockers).

Checklist

• I have read the Contributing Guide
• I have followed Conventional Commits
• I have searched for duplicates (no open PR covers this)
• I have run the relevant tests (scripts/run_tests.sh, suites above)
• I have added regression tests (2, sabotage-proven)
• git diff --check clean; check-windows-footguns clean on all touched files
• ruff check — no ruff binary on this host; leaving to CI
• Platform: Debian GNU/Linux 13 (trixie), x86_64 — Hermes Agent v0.21.1 (v2026.9.7), upstream 8d79c2ff57

[ehz0ah]

Request changes conclusion (formal request unavailable without explicit repository access)

Reviewed exact head 6c69680084d5f301f8699ddc8bc29d23e2e57525.

Motivation

This PR addresses two real second-writer hazards around state.db: doctor --fix can checkpoint a live WAL, and the shared repair probe previously treated an unprovable database error as proof that the database was quiet. The intended invariant is correct: checkpoint or repair work must not begin unless exclusive access is proven.

Approach

The patch reuses live_writer_holds_db() before the doctor checkpoint, changes one DatabaseError path to fail closed, closes the checkpoint connection through contextlib.closing, and adds focused regressions. The scope is small and the connection cleanup is correct.

Concrete changes

_state_db_wal() now skips the checkpoint when the shared holder helper reports a live or unprovable writer state. live_writer_holds_db() now returns True for its plain sqlite3.DatabaseError branch. The repair docstring is updated to describe that behavior. The new tests cover a held WAL and a directly raised DatabaseError.

Main-branch risk

Two blockers remain.

1. hermes_state_holders.py:276-278 catches sqlite3.OperationalError before sqlite3.DatabaseError. Non-lock operational failures such as unable to open database file and disk I/O error therefore return False, even though exclusive ownership was not proven. I reproduced doctor --fix continuing to open SQLite, execute PRAGMA wal_checkpoint(PASSIVE), and increment fixed after that exact probe failure. The repair caller receives the same unsafe result. The new regression raises only the base DatabaseError, so it misses this subclass branch. Make every unprovable SQLite probe failure fail closed, while preserving the known quiet result only after all probe statements succeed. Add coverage that asserts both doctor and repair refuse a non-lock OperationalError.

2. The doctor path has a check-then-act race. live_writer_holds_db() closes its exclusive probe before _state_db_wal() opens the separate checkpoint connection at hermes_cli/doctor_state.py:262. I reproduced a writer opening after the helper returned False and before sqlite3.connect; doctor still executed the checkpoint and reported success while that holder was live. The quiescence authority must remain held across the checkpoint operation. A boolean preflight cannot provide that lifetime. Use a scoped exclusive guard or an equivalent authority token that is acquired atomically and released only after the checkpoint connection closes. Add a deterministic interleaving regression for quiet probe, writer admission attempt, checkpoint effect.

Overall assessment

Request changes. The patch improves failure handling and resource cleanup, but it does not yet enforce its central fail-closed and proof-lifetime guarantees. Both gaps can authorize the exact second-writer checkpoint this PR is intended to prevent.

Validation performed:

• Focused state repair and checkpoint suites: 35 passed, 12 skipped
• Holder and FTS admission suites: 27 passed, 1 skipped
• Non-lock OperationalError reproduction: doctor executed the checkpoint and reported one fix
• Check-then-act reproduction: doctor opened the checkpoint connection after a writer entered
• Ruff on all changed Python files: passed
• Windows footgun scan on all changed files: passed
• git diff --check: passed
• Hosted checks: none reported for this head

English verdict: REQUEST_CHANGES at exact head 6c69680084d5f301f8699ddc8bc29d23e2e57525.

[Halldrix]

Reopening — my previous close was premature and wrong: this PR stays open for its remaining review items. The #108076 salvage covers the base doctor hunk; the two blockers below are still being addressed here. Disregard my "superseded" comment above.

[Halldrix]

Thanks for the review, @ehz0ah — verified point by point, with one revision below.

1. Check-then-act (doctor_state): confirmed — the probe connection closed before the checkpoint opened. Fixed without new policy: the checkpoint now runs on the existing _exclusive_repair_db_guard connection (verified: the statement executes there, and a concurrent writer gets database is locked while it's held). The boolean preflight stays as the cheap foreign-scan skip; guard-acquisition failure takes the same skip path. Added the deterministic interleaving regression (quiet preflight → writer admission → guard refuses → fixed == 0).
2. Non-lock OperationalError (holders probe): partially revised after empirical verification. Collapsing the probe to fail-closed was verified to refuse header-destroyed DBs too (garbage → True), which misdirects the repair class into a "live writer" message — so the preflight collapse is dropped from this PR. The system still fail-closes: the exclusive guard opens its own connection and refuses surgery with an honest message (test_repair_refuses_when_guard_cannot_open pins this, sabotage-proven), and the doctor path above is safe via the held guard regardless of the preflight reading.

Pushed as feb95d8e19; neighbor suites (49 passed, 3 environmental skips) + per-hunk sabotage re-proven. Hosted CI still pending fork approval.

[ehz0ah]

Request changes conclusion (formal request unavailable without explicit repository access)

Reviewed exact head feb95d8 against current upstream main 8706517.

Motivation

This PR addresses a valid P1 state-corruption path. Before this change, hermes doctor --fix opened a separate SQLite connection and checkpointed a large live WAL without holding quiescence authority.

Approach and behavior change

The revised path keeps the existing holder scan as an early skip, then acquires _exclusive_repair_db_guard() and executes PRAGMA wal_checkpoint(PASSIVE) on that same connection. A writer that enters after the preliminary probe now prevents guard acquisition. Once the guard is acquired, competing readers and writers remain blocked until its connection closes. The prior check-then-act and non-lock probe blockers are therefore resolved.

Remaining risk

One checkpoint failure path still reports a false clean result. Details are inline.

Validation

• Focused exact-head suites: 40 passed, 12 platform skips
• Focused current-main composition: 40 passed, 12 platform skips
• Direct guard-lifetime probes: competing readers and writers were locked until guard close
• Injected checkpoint disk I/O failure: fixed=0, issues=[], final summary reported All checks passed
• Ruff on all changed files: passed
• Windows footgun scan: passed
• git diff --check: passed
• Current-main synthetic merge: clean
• Hosted checks: none reported

Overall assessment

Request changes. The scoped guard fixes the core authority race, but doctor must preserve a failed checkpoint as an unresolved issue instead of reporting a clean result.

[ehz0ah]

[Bug] (blocking) A checkpoint error is silently converted into a clean doctor result. This call is inside the existing warn_on_error("") block, so sqlite3.OperationalError or DatabaseError from guard.execute() is discarded. I reproduced a disk I/O error here with a 51 MB WAL: fixed stayed 0, issues stayed empty, the WAL remained 51 MB, and _print_summary() printed All checks passed. Record an actionable unresolved issue when the guarded checkpoint or its post-checkpoint size read fails, and add a regression for the final Finding or summary behavior.

[teknium1]

Landed on main in #108076 (902ecca). The checkpoint-gate hunk was cherry-picked with authorship intact; thank you @Halldrix. The second hunk (fail-closed on any DatabaseError) refused every header-destroyed DB the repair exists for, so it was left out; the existing repair-lock ownership check covers that path.