fix(state): divert the turn batch on a raw SQLITE_CANTOPEN (deleted WAL generation)

[clempat]

What this fixes

A raw SQLITE_CANTOPEN (sqlite3.OperationalError: unable to open database file) on the flush path lost the turn's batch silently. _db_flush_failed (agent/session_persistence.py) only diverted to JSONL on isinstance(e, (StateDbReplacedError, StateDbCorruptError)); a raw CANTOPEN matches neither, so the batch was neither written to SQLite nor kept on disk — the turn's messages vanished.

It now diverts whenever the classified cause is deleted_wal or disk, and the cause carries an honest storage label instead of a generation claim.

Fail-closed is unchanged: the flush still returns False → session_persistence_failed; no reopen/replay is added.

Review follow-up (197f71f47)

Two reviews (P1 from @andrexibiza, measurements from @strzhao) converged on the same point, and I agree with it: the divert belongs at the flush site, and mapping generic SQLITE_CANTOPEN to deleted_wal claimed a proof the error does not carry.

• Generic CANTOPEN is no longer deleted_wal. It classifies as disk, placed after the locked/busy bucket, so unable to open database file: database is locked stays retryable, while a missing parent directory or an unreadable file reads disk rather than unknown or deleted_wal. The proven retired-generation case keeps its own type (DeletedWalGenerationError) and its sidecar-identity / proc-fd evidence.
• The divert is decided at _db_flush_failed(), on the classified cause — not by widening a bucket upstream.
• The stated mechanism does not reproduce. Independent measurement on Linux (Python 3.13.13, SQLite 3.51.2): a clean close with another connection live does not unlink -wal/-shm (SQLite unlinks only at the last connection's close), and unlinking both sidecars under a live handle still allows insert, commit, wal_checkpoint(TRUNCATE) and a fresh connect. Every CANTOPEN I could induce came from a missing directory or an unreadable file — the same result @strzhao got on darwin. This PR therefore no longer asserts the retired-generation mechanism: it preserves the batch on the raw symptom, whatever produces it.
• The observed production path is unchanged: Session DB append_message failed: unable to open database file → reason=session_persistence_failed. That is what the divert now covers.
• Not fixed here, deliberately: the deleted_wal / replaced / corrupt copy still says "a copy is kept in {home}/sessions/" although the divert's own exception is swallowed. That gap is pre-existing and fix: refuse held state.db publish; replay diverted transcripts #110179 (_last_diverted_transcript_path) owns it; duplicating it here would conflict with that carrier. Say the word if you would rather have it folded in.

Changes

• agent/session_persistence.py — _db_flush_failed diverts the batch to sessions/<id>.jsonl when the cause is deleted_wal or disk, exactly like the replaced/corrupt cases.
• hermes_state_errors.py — "unable to open database file" maps to disk, after locked.

Tests

• tests/agent/test_flush_diverts_on_cantopen.py — a raw CANTOPEN diverts the batch to JSONL and fails closed; a negative control asserts that a failing divert (unwritable sessions/ path) stays quiet, fail-closed, and fabricates nothing; a third test pins the no-reopen contract.
• tests/hermes_state/test_deleted_wal_generation_guard.py — generic CANTOPEN classifies as disk, with the locked negative control staying locked.

Measured against the base f5d192611: 4 failed with these tests. On this head: 20 passed, 7 skipped.

Verdict: level 1 only — level 2 (reopen + replay) deliberately declined

The follow-up asked to detect the lost WAL generation and, if cleanly checkpointed, reopen the handle and replay the append once. I declined that level after tracing the recovery machinery, for three reasons:

1. Reopening on a retired WAL generation is exactly what the store already refuses to do. refuse_deleted_wal_generation / _halt_if_db_generation_changed exist to prevent minting a second WAL over a deleted generation (split-brain → intermittent SQLITE_CORRUPT/IOERR, WAL generation split-brain: DeletedWalGenerationError guarded writes, but the graceful-shutdown checkpoint then corrupted state.db (field report) #105670). A turn-level reopen reintroduces that failure mode.
2. "Cleanly checkpointed" is not reliably knowable from the surviving writer's side. The raw CANTOPEN escapes the write-path guard precisely because the writer's recorded sidecar identity doesn't see the loss; it cannot distinguish "gateway checkpointed then unlinked" (safe to reopen) from "operator deleted the -wal with uncommitted frames" (committed data loss). Reopening in the latter case would silently drop data — the exact silent failure the recovery was meant to avoid.
3. The safe recovery already exists and stays the remediation: stop the writers, hermes sessions recover --source <dir>/state.db, with capture_retired_wal_generation preserving the retired frames for the operator to decide whether they belong on the main file. A turn-level auto-reopen would duplicate this, more riskily, without that decision.

Net effect: the batch is never silently lost (level 1, non-negotiable); the turn still aborts rather than risk a corrupt recovery. Whatever emits the raw CANTOPEN (worker drain ordering vs the gateway's clean close) is tracked separately and is not addressed here.

Out of scope

• No deployment, no restart — draft only.
• Worker drain / close-ordering as an emitter of the raw CANTOPEN is a separate problem.

[andrexibiza]

Reviewed exact head fed9c1dc81afc2e912c9718c79dc3fd6481ca6c7 against current main f5d192611032025d2757b07ad838921872126182 (2 commits ahead, 0 behind, currently mergeable).

The level-1 direction is right: preserve the failed turn batch somewhere durable, keep the turn failed, and do not auto-reopen/replay a writer whose generation cannot be proven safe. That is the correct failure posture for the real retired-WAL class.

I do have one P1 data-integrity blocker before this classification can land, because the new predicate turns an indirect SQLite symptom into a specific generation proof and then exposes that proof through user-facing recovery/settlement copy.

P1 — SQLITE_CANTOPEN is not proof of a deleted WAL generation

"unable to open database file" is SQLite's generic SQLITE_CANTOPEN text. The repository already has a concrete counterexample in #29610 / wyjuven: the exact same sqlite3.OperationalError: unable to open database file occurs after descriptor exhaustion ([Errno 24] Too many open files) from leaked Kanban SQLite handles. A missing/uncreatable parent directory produces the same SQLite error as well. Neither condition establishes that state.db-wal / state.db-shm was retired.

The consequence is stronger than a diagnostic mislabel. With this head:

1. classify_persistence_error() turns any matching CANTOPEN into deleted_wal.
2. _db_flush_failed() therefore attempts the JSONL divert.
3. If CANTOPEN came from FD exhaustion, the divert itself can fail opening sessions/<id>.jsonl; that exception is swallowed after a warning.
4. _last_persistence_error_cause nevertheless remains deleted_wal, and the existing turn explainer for that cause says "a copy is kept in …/sessions/".

That can produce a false durability receipt: the user is told the unsaved message was preserved when the fallback write never landed. This is the same architectural shape that the WAL campaign has already been eliminating: an indirect signal is accepted as authority for a stronger state claim.

The fix does not require giving up the level-1 behavior. Keep generic CANTOPEN generic (or give it its own cause) unless the failing boundary has direct sidecar-generation evidence: DeletedWalGenerationError, an identity/holder check, or a provenance-bearing wrapper raised after that check. If the desired policy is “attempt a transcript divert on every SQLITE_CANTOPEN,” make that decision independently at _db_flush_failed() using the actual SQLite error code; do not widen the deleted_wal classification to get there. And bind any “copy kept” claim to the actual diversion receipt/path, not merely to the error bucket.

The open #110179 / ngpestelos is especially relevant here and should be composed rather than duplicated: it already changes this same persistence seam to retain _last_diverted_transcript_path from divert_session_transcript_jsonl() (or None on failure) and adds replay/inspection of that exact artifact. Preserve that authorship and settle merge order if both carriers proceed.

Interlocks / provenance

• #105670 / maurohdev is genuine deleted-WAL-generation evidence and the predecessor containment/capture lineage should remain the semantic source for that bucket.
• Merged #108082 / teknium1 is the most important architectural precedent: it closed several state/WAL false positives whose shared root was trusting a single indirect signal as proof of a permanent condition. This head should not reintroduce that class through generic CANTOPEN prose.
• #110179 / ngpestelos is complementary and directly file-overlapping (agent/session_persistence.py, agent/turn_explainers.py): it owns truthful diverted-artifact path/replay work, not this CANTOPEN classification.
• #109766 / Sahilvishnaliya + #109687 are adjacent self-heal work for a proven lost generation, not duplicates of this level-1 preservation carrier.
• #29610 / wyjuven is the concrete hostile witness showing this exact CANTOPEN text has another reachable cause inside Hermes.

Acceptance

Hosted acceptance is currently absent on both surviving commits. Parent 2db7bc18a82f3e912ec5a6b0f30e84ec701c60a6 has CI 35201160136, Docker 35201159651, and Nix 35201159653, all action_required. Exact head has CI 35206547413, Docker 35206547071, and Nix 35206547094, also all action_required; the exact-head CI run created zero jobs. So this is 0/2 surviving commits green.

Required closure: narrow the cause classification to evidence that actually proves the WAL generation state; keep the level-1 divert independent if desired; make the “copy kept” settlement conditional on a successful durable divert; add negative controls for non-WAL CANTOPEN plus divert failure; compose the overlapping receipt/replay work with #110179 without erasing credit; then establish exact-head and every-surviving-commit green acceptance.

The core fail-closed instinct here is good. The blocker is specifically that the proof and the durability claim are currently wider than the evidence.

[andrexibiza]

[P1] Don't promote generic SQLITE_CANTOPEN prose into deleted-WAL proof. "unable to open database file" is not generation-specific: #29610 records this exact SQLite error under FD exhaustion, and an invalid/missing parent path produces it too. Because this global classifier feeds _db_flush_failed() and the user-facing deleted_wal explainer, a non-WAL CANTOPEN can be mislabeled as generation loss; if the JSONL divert also fails (very plausible under EMFILE), Hermes still retains deleted_wal and can tell the user a copy was kept when no durable fallback exists. Keep this bucket tied to direct generation evidence (DeletedWalGenerationError / identity-holder proof). If you want level-1 diversion for every CANTOPEN, detect SQLITE_CANTOPEN at the flush boundary independently and bind the preservation claim to the actual divert receipt/path (the complementary #110179 already carries that receipt shape).

[strzhao]

Checked against a clean origin/main (98f758ae7e) and the PR head (fed9c1dc81af), Python 3.11.16 / SQLite 3.53.1.

The flush half holds up. Your two tests, copied onto an unmodified origin/main worktree, fail there (2 failed: sessions/live.jsonl never written, cause stays unknown) and pass on the head (2 passed). Diverting the batch while the flush still reports failure preserves the invariant the existing replaced / corrupt branches preserve.

Where I would push back is the phrase mapping. unable to open database file is SQLITE_CANTOPEN, the generic "could not open it" result, and in the phrase table it now outranks every later bucket. Feeding classify_persistence_error the same real, induced failures on both revisions:

induced failure / input	origin/main	PR head
sqlite3.connect("<tmp>/nope/state.db") (parent dir missing)	unknown	deleted_wal
db file chmod 000 (unreadable)	unknown	deleted_wal
"unable to open database file: database is locked"	locked	deleted_wal

Rows 1 and 2 are storage that is unreachable or unwritable, nothing was retired, and the user-facing copy for this bucket (hermes_state_user_copy.py, agent/turn_explainers.py) tells the user the session database "was changed or replaced while Hermes was running" and to stop Hermes and run hermes doctor. Row 3 flips a retryable busy into capture inspection.

I also could not reproduce the stated mechanism on POSIX. With a live WAL connection, unlinking state.db-wal / state.db-shm (parent process, child process, and same-process) left every following insert, commit and checkpoint succeeding: a live handle keeps its fds, and a fresh connection just creates new sidecars. Every CANTOPEN I could induce came from paths and permissions instead. Measured on darwin; a Linux-only path I did not exercise may exist. But if the fleet symptom really was CANTOPEN, replaced (main file gone or moved) or disk look like the honest labels for those cases.

The divert itself is worth keeping for any open failure; it is the cause bucket I would scope more narrowly. Reporting the measurements rather than asking for a rewrite.

[clempat]

Addressed in 197f71f47. Both reviews converged on the same point and I agree with it: the divert belongs at the flush site, and the classification widening claimed a proof the error does not carry.

What changed

• "unable to open database file" no longer classifies as deleted_wal. It classifies as disk, placed after the locked/busy bucket, so unable to open database file: database is locked stays retryable (@strzhao's row 3) and a missing parent dir / unreadable file reads disk instead of unknown or deleted_wal (rows 1 and 2).
• The divert is now decided at _db_flush_failed() on the classified cause — the decision @andrexibiza asked to move out of the bucket — so a raw CANTOPEN still keeps its batch on disk without widening a classification upstream.
• Negative controls added: a locked+open failure stays locked; a failing divert (unwritable sessions/ path) stays quiet, fail-closed, and fabricates nothing.

On the stated mechanism

I could not reproduce it either, on Linux (Python 3.13.13 / SQLite 3.51.2). A clean close while another connection is live does not unlink -wal/-shm — SQLite unlinks only at the last connection's close — and unlinking both sidecars under a live handle still allows insert, commit, wal_checkpoint(TRUNCATE) and a fresh connect. Every CANTOPEN I could induce came from a missing directory or an unreadable file, matching your darwin measurements. The PR no longer asserts the retired-generation mechanism; it preserves the batch on the raw symptom, whatever produces it.

What is unchanged: Session DB append_message failed: unable to open database file → reason=session_persistence_failed is the observed production path, and that is what the divert now covers. The #29610 FD-exhaustion reachability is why the label moved and the divert did not: that case now reads disk, not deleted_wal.

Not in this PR

The "a copy is kept in {home}/sessions/" copy is still emitted even when the divert itself fails (_db_flush_failed swallows that exception). That is pre-existing on replaced/corrupt/deleted_wal, and #110179 (_last_diverted_transcript_path) owns it; I did not duplicate it here to avoid conflicting with that carrier. Tell me if you would rather have it folded in and the merge order settled the other way.

Evidence

• Tests: tests/agent/test_flush_diverts_on_cantopen.py, tests/hermes_state/test_deleted_wal_generation_guard.py — 4 failed against the base f5d192611, 20 passed / 7 skipped on 197f71f47.
• CI: the three workflows on this head (CI 35226252642, Nix 35226252315, Docker 35226252320) are all action_required with zero jobs, same as the parent commits — a maintainer has to approve the workflow run before any of this can be green.

[andrexibiza]

Re-review — 197f71f47b96225afd30e97e68298915ba2652ba

The CANTOPEN classification P1 from my previous review is resolved on this head. The correction separates the storage symptom from a claim about WAL-generation identity, while retaining the failed-batch diversion. This addresses the classification counterexamples in strzhao’s measurements, without adding speculative recovery.

I found no new runtime blocker in the four-file diff. There is one P2 test-contract correction, inline below. This is not yet a merge-ready acceptance result: the PR remains draft and 0/3 surviving commits have green CI/Docker/Nix acceptance.

What is now correct

• Cause precedence is preserved. In the classifier, typed lease/compression/generation errors and FTS-scoped corruption retain their existing precedence. Generic CANTOPEN becomes disk; the combined unable to open database file: database is locked control remains locked. A failed open is no longer promoted into evidence of a retired WAL.
• The new behavior belongs to the failure boundary. _db_flush_failed attempts diversion without reporting SQLite success. The surrounding flush still returns False, and the success-only message-marker stamping remains after append_messages_batch. The patch does not add reconnect, replay, or a new retry path; the existing typed compression-continuation retry is separate.
• The introduced false-copy claim is removed for CANTOPEN. The disk explainer does not say “a copy is kept.” A failed CANTOPEN diversion therefore no longer reaches the unconditional deleted_wal copy claim that made the prior classification dangerous. The remaining unconditional wording for replaced/deleted_wal is pre-existing; it should not be represented as a new defect introduced by this revision.

Verification and its limits

I read all four changed files, the complete flush implementation, the actual JSONL writer, the cause-to-user-copy mapping, the existing discussion, and the current-main versions of the two production seams.

I also ran an isolated, 25-case source-excerpt harness against the selected classifier and failure/diversion definitions: 25 passed, 0 failed; exit 0 on Python 3.13.5 / SQLite 3.46.1. Imports and the temporary home were wired by the harness; SQLite error generation and JSONL filesystem reads/writes were real. Controls included native missing-parent SQLITE_CANTOPEN, native read-only rejection, native SQLITE_BUSY, injected ENOSPC/FULL/IOERR, typed generation/lease/compression precedence, FTS precedence, multi-row JSON round-trip, and diversion failures with a directory at the artifact path or a file at the sessions-directory path.

Those are boundary checks, not execution of the repository’s AIAgent/SessionDB suites or its public turn loop. The reported 20 passed / 7 skipped remains the author’s repository-suite receipt, not a result I independently reproduced. No packaged, gateway, or end-to-end acceptance is claimed here.

P2 — make the no-reopen test observe the prohibited effect

test_raw_cantopen_fails_closed_without_reopen currently checks only False and one occurrence of the saved payload. An implementation that reopens/retries, fails again, and then diverts once would still satisfy both assertions. The current production implementation does not do that, but the test does not guard the contract its name and comment claim.

After fixture setup, forbid or spy on connection creation/reopening and premature close, count append attempts, and assert the original DB/connection identity survives the flush. Keep the existing failure/result and parsed JSONL assertions. Also parameterize the flush test over the other newly admitted disk causes: this predicate now covers FULL, read-only, and IOERR, not only CANTOPEN; keep locked, FTS, and lease failures as negative controls.

Interlock and current-main integration

#110179, by ngpestelos, is still open and unmerged, at 4696d9d48131d842199db16ae87bb101eaabeaa1. Its actual persistence patch captures the diversion return path and clears it on exception. It is complementary, not a replacement for this PR. Keep that receipt/replay work there rather than duplicating it here. When the two are composed, retain both this broadened predicate and the receipt assignment/clear inside it; restoring either whole older handler would lose the other behavior. This narrow CANTOPEN correction need not absorb that larger PR to resolve the original classification P1.

The reviewed merge base is f5d192611032025d2757b07ad838921872126182. The separately fetched live main was 5dd70d7cb6560c3ff8aff294ec44ec4f8d1558e5: 3 ahead / 3,208 behind. Current main still lacks this CANTOPEN change, so it is not already superseded there. Its surrounding persistence code has moved, including notification-row handling and checkpoint cleanup; preserve those changes when bringing this branch current. The reported mergeable flag is not a substitute for testing that combined tree.

Hosted acceptance remains outstanding

Fresh reads return action_required for every workflow linked below, not passing checks. The exact-head CI run has zero jobs.

Surviving commit	CI	Docker	Nix
2db7bc18a82f	35201160136	35201159651	35201159653
fed9c1dc81af	35206547413	35206547071	35206547094
197f71f47b96	35226252642	35226252320	35226252315

Before acceptance, establish green results for the surviving commits and the actual integrated head, including the touched flush/WAL suites and adjacent turn-explainer/file-identity coverage. Also narrow the PR-body guarantee from “the batch is never silently lost” to best-effort preservation when the JSONL write succeeds: the negative control deliberately demonstrates that the fallback itself can fail.

Bottom line: the earlier P1 is closed by this revision; retain fail-closed, no-auto-reopen behavior, strengthen the no-reopen regression, and complete hosted/integration acceptance without duplicating the separate recovery carrier.

[andrexibiza]

[P2] Observe reopening/retry, not just the final JSONL count. These assertions establish False plus one saved payload, but would also pass if a future implementation closed/reopened the handle, retried the always-failing append_messages_batch, and only then diverted once. Nothing here observes connection creation, close, append-attempt count, or handle identity, so the comment’s “No retry/reopen path was taken” is not established. After creating the fixture DB, forbid/spy on reconnect and premature close, assert exactly one append attempt, and assert the original DB/connection remains attached. Keep the failure and JSONL checks. The current implementation has no added reopen path; this is a gap in the regression guard for that explicit contract.