fix(sessions): salvage header-damaged state.db in the lost_and_found lane

[TaoMasterCoder]

Fixes #106667

Symptom (real incident)

An OOM kill followed by SIGKILL mid-write left this Hermes install's state.db in a shape the existing hermes sessions recover --allow-partial page-level lane cannot touch: page 1 itself is damaged, so SQLite refuses the file outright —

sqlite3.DatabaseError: file is not a database

and sqlite3 <file> .recover fails with the same error, because .recover opens the file like any other client (rc=26, SQLITE_NOTADB). Yet the data pages below page 1 are intact — 335 MB file, header bytes 16-17 damaged to 3250 (not a power of two), magic string gone. Before this change the lane reported "page-level .recover salvage failed" and gave back zero rows.

Fix

Extend the lost_and_found lane with a header-salvage pre-pass in session_lost_and_found.py:

• detect_database_open_error() separates header rejection (file is not a database / unsupported file format, SQLITE_NOTADB) from opens-but-malformed (database disk image is malformed) — the latter keeps using the untouched plain path, no behavior change.
• When the file is refused, a valid 100-byte SQLite header (produced by letting SQLite itself write a minimal donor DB at the target page size) is spliced onto a private copy — the source is never modified, the copy lives inside the recovery snapshot directory that is removed at the end.
• Page-size selection follows the file's own bytes 16-17 when they are a legal power of two (512..65536, 1→65536); otherwise a candidate sweep (4096 first, then 8192, 16384, 65536, 2048, 1024, 512) picks the first size for which the existing .recover flow produces a usable lost_and_found database. A candidate whose header is accepted but whose pages still won't recover is a real failure and short-circuits the sweep.
• The file's own version-number bytes (18/19) are preserved when they are a consistent legal pair (rollback 1/1 or WAL 2/2); otherwise the spliced header defaults to rollback mode — a WAL header without its -wal sidecar buys nothing.
• Everything downstream (mapping, stubbing, FTS rebuild, verification, report) is the existing code, reused as-is. No new commands, tables, or config keys. The recovery report gains a sqlite3_cli.header_salvage section and a "header salvage" verification warning when the pre-pass fires, so the extra step is auditable.

Testing (RED → GREEN)

New tests in tests/hermes_cli/test_session_recovery_lost_and_found.py build real header damage with the genuine sqlite3 CLI (no mocks; skipped when the shell lacks .recover):

• premise guard: the fixture really reproduces file is not a database
• open-error probe distinguishes refused vs malformed vs healthy
• donor-header contract (magic, page size incl. the 1→65536 encoding, sanitized version bytes, spliced file opens)
• E2E recover_session_database(..., allow_partial=True) on header-damaged sources — both variants: page-size bytes surviving, and the incident shape where bytes 16-17 are also destroyed (3250) — asserting exact session/message counts, content payloads via FTS, integrity_check/fk_check, and that the output opens as a live SessionDB
• the plain malformed-but-openable path records no header_salvage (no behavior change)
• a beyond-repair source still raises LostAndFoundError (no lying, no hanging)

RED: 5 failed (the 5 new salvage tests; 9 pre-existing pass) on the pre-fix code — failing exactly with dump rc=26 ... file is not a database, the incident symptom.
GREEN: 18 passed (test_session_recovery_lost_and_found.py + test_session_recovery.py) after the fix.

On the real 335 MB incident file this approach went from file is not a database to a complete salvage of 136,392 rows including the lost_and_found table (validated manually with the same splice + .recover pipeline the code automates).

Branch base note

Branch based on fork main snapshot (no workflow scope on the PR token); fix applies cleanly to current upstream main — verified on origin/main via 3-way apply of the source changes, with the only friction being two tests appending at the same tail of the shared test file. Happy to rebase.

One pre-existing upstream test (test_lost_and_found_lane_refuses_to_verify_a_physically_shifted_source) fails identically on clean origin/main with a self-built recover-capable sqlite CLI under this environment's Python/sqlite combination — unrelated to this change.

[shellybotmoyer]

🔀 Merge Conflict Diagnosis

This PR currently shows mergeable_state: DIRTY — it cannot be merged until conflicts are resolved.

PR: #102808 — fix(sessions): salvage header-damaged state.db in the lost_and_found lane
Branch: u/nick/nick/salvage-db → main

The fix extends session_lost_and_found.py with a detect_database_open_error() function that separates header rejection (file is not a database / SQLITE_NOTADB) from other open failures, then applies a .recover pre-pass to salvage data pages when page 1 itself is damaged.

To complete the merge, the author should either:

1. Rebase onto current main to pick up any new session_lost_and_found.py changes that may have landed since the branch was cut, or
2. Manually resolve any overlapping modifications to session_lost_and_found.py if both sides modified it.

Happy to re-test once conflicts are resolved.

[TaoMasterCoder]

Filed #106667 to track the production incident behind this PR (OOM SIGKILL mid-write → page-1 header damage → file is not a database, unreachable by the existing page-level --allow-partial lane). Body now carries Fixes #106667 so triage can pair them.

[strzhao]

Review comment draft — PR #102808 (v3 final)

Independent verification of the header-salvage pre-pass, read against origin/main@ee7fc3bc06 (2026-09-09) and PR head 357e9e659f; no claims beyond file:line receipts.

The approach is sound and the incident shape is real. Damaged page-1 makes SQLite refuse the file outright (SQLITE_NOTADB, rc 26), and sqlite3 .recover opens files like any other client — so the existing lane could not reach the intact pages below page 1. Splicing a valid 100-byte header onto a private copy and running the untouched recovery flow against the copy is the right minimal shape: _splice_header writes to lf_path.with_name(...) inside the snapshot directory the caller removes on exit, so the source is never modified (the tests assert repaired[100:] == original[100:]).

This now composes with the quarantine that landed after this PR's base — #106587 (merged 09-09, 9e84ce5bae) moves a state.db whose first page is not SQLite aside to <stem>.notadb-<ts>-<pid>.bak together with its -wal/-shm (hermes_state_dbfile.py:245-292), before anything opens it (the pre-open decision itself is a non-opening pread probe, hermes_state_dbfile.py:168-202). In production the header-damaged file therefore reaches recovery via hermes sessions recover --source <bak>, not in place — the startup quarantine message points there explicitly (hermes_state.py:578-586: "salvage the preserved bytes with hermes sessions recover --source {qpath}"). Two things follow for the rebase:

1. The E2E should exercise the quarantined-source path. The current tests damage a file in place and recover it directly — a flow current main no longer produces (a startup SessionDB open now quarantines the header-damaged file first). Recovering from a copy named/located like the quarantine output (or at least through --source on a .bak) would cover the actual production handoff.

2. A surviving -wal is outside the salvage source — worth stating as a documented boundary or investigating. Hermes DBs default to journal_mode=wal (config_defaults.py:27-30), and A state.db with a clobbered first page is quarantined with its WAL instead of opened destructively (refs #102198, salvage #102212) #106587 deliberately preserves the sidecars next to the .bak. _splice_header copies only the main-file bytes, so any committed-but-uncheckpointed frames in a preserved -wal never participate in .recover. If the OOM kill landed during a checkpoint write-back the -wal may already be drained and little is lost; if frames remain, the tail of the session history is outside the recovered set. This is a structural question rather than a defect claim: recovering WAL frames onto a donor-header copy is not trivially "also copy the sidecar" (the donor header will not satisfy the WAL's expectations), so the honest options are (a) state the boundary in the salvage report ("recovers up to the last checkpoint; a preserved -wal is not included") or (b) treat WAL-aware salvage as a follow-up.

Rebase note: since this branch's base (09-04) the recovery core moved substantially — streamed salvaged population (ead7e91dab), physical layout inferred from schema history (17c43aba06), lost_and_found cells mapped by physical column names (7704712168), plus a central pre-open header probe (_preopen_header / has_invalid_sqlite_header_preopen, hermes_state_dbfile.py:168-202). "Downstream reused as-is" should be revalidated against that core, and detect_database_open_error() (a connect+PRAGMA probe) overlaps the new _preopen_header — reusing it after rebase keeps "notadb" meaning one thing across startup quarantine and recovery. One mechanism note in case it informs the choice: the PR's probe opens the file (sqlite3.connect + PRAGMA journal_mode), whereas the main-side probe deliberately preads the header so probing has no open-time side effects (hermes_state_dbfile.py:168-202 docstring).

Adjacent, for whoever converges this: .recover-era orphan FTS5 shadow hygiene is being worked in #56824 and #103868 (SessionDB open self-heal; the production .recover field report tying them together is #103840) — worth a glance if any recovered intermediate is opened as a live SessionDB.

[TaoMasterCoder]

Thank you for the thorough review — the quarantine composition, WAL-boundary framing, and the pread probe reuse are all on point.

The rebase is substantial (upstream's recovery core has been restructured significantly since this PR's base: streamed salvaged population, physical layout inference from schema history, preopen header probe, and the #106587 quarantine). I'm planning to:

1. Switch the detection from sqlite3.connect + PRAGMA journal_mode to has_invalid_sqlite_header_preopen(path, force=True) (pread-based, no side effects — aligns with the quarantine probe's invariant).
2. Add an E2E that exercises the quarantined-source path (recover from a .notadb-*.bak via --source), so the test covers the actual production handoff.
3. State the WAL boundary explicitly in the salvage report: "recovers up to the last checkpoint; a preserved -wal is not included."
4. Revalidate the downstream mapping/stubbing/FTS reuse against the new core (streamed population, physical layout inference).

Will push a rebased branch once the conflicts are resolved and tests pass.

— TaoMasterCoder (via agent)

[TaoMasterCoder]

Rebased onto current main and addressed all four points, @strzhao — thank you for the review.

The branch now carries two commits on top of the old head (357e9e659f):

• 35d4ca0ff9 — merge of upstream/main (67764dc086). The recovery core was restructured substantially, so I resolved the conflicts by keeping upstream's shape and re-attaching the header-salvage lane on top (no force-push; the old history is intact).
• 790a797735 — the four-point follow-up below.

1. Detection switched to the shared preopen probe. detect_database_open_error() (hermes_cli/session_lost_and_found.py:239) no longer opens the file at all — it delegates to hermes_state_dbfile.has_invalid_sqlite_header_preopen(path, force=True) (hermes_state_dbfile.py:197), the same side-effect-free pread probe the startup quarantine uses. force=True is the honest contract here: this lane only ever probes offline files (the recovery flow's own snapshot copy, or a closed quarantined .bak), never a live connection. As a bonus the probe can no longer fabricate -wal/-shm sidecars in the snapshot dir. An invariant test pins the alignment across four shapes (header-damaged / zeroed / malformed-but-openable / healthy): recovery fires exactly when the quarantine probe does — test_detection_shares_the_quarantine_notadb_invariant (tests/hermes_cli/test_session_recovery_lost_and_found.py:1236).

2. Quarantine-source E2E added. test_quarantine_source_recovers_via_sessions_recover_cli (tests/hermes_cli/test_session_recovery_lost_and_found.py:1456) walks the real #106587 handoff: quarantine_invalid_state_db() moves a header-damaged state.db aside as <stem>.notadb-<ts>-<pid>.bak, then the genuine CLI runs as a subprocess against a temp HERMES_HOME — hermes sessions recover --source <bak> --allow-partial — and asserts the rows come back, the .bak bytes are unchanged (sha256-pinned), and the report carries the salvage record.

3. WAL boundary stated in the salvage report, in both shapes: the machine-readable header_salvage.wal_boundary field (hermes_cli/session_lost_and_found.py:416) and the human-facing verification warning (hermes_cli/session_recovery.py:1026): "recovers up to the last checkpoint; a preserved -wal is not included." The copy .recover runs against is the main database file alone (the -wal/-shm sidecars are not spliced in), so that wording is the exact guarantee the lane makes rather than an aspirational one.

4. Downstream reuse re-validated against the restructured core. The merge resolution kept upstream's streamed salvaged population (ead7e91dab), schema-history physical layout (17c43aba06) and physical-column mapping (7704712168) intact, and the parametrized E2E (test_header_salvage_lane_recovers_file_not_a_database_source) now exercises the salvage through those paths — sessions/messages/FTS payloads all reconcile against the fixture. And as per point 1, detect_database_open_error() is now a thin adapter over _preopen_header semantics, so "notadb" has exactly one meaning between startup quarantine and recovery.

Test results (real run on the pushed head):

$ venv/bin/python -m pytest tests/hermes_cli/test_session_recovery_lost_and_found.py \
    tests/hermes_cli/test_session_recovery.py tests/hermes_cli/test_sqlite3_cli_salvage_gate.py \
    tests/hermes_state/test_state_db_corrupt_quarantine.py tests/test_state_db_notadb_fail_closed.py -q
..................................................................s..... [ 84%]
.............                                                            [100%]
84 passed, 1 skipped in 19.95s

(The one skip is pre-existing on this interpreter: SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE needs Python 3.12+, in test_state_db_corrupt_quarantine.py:122, unrelated to this lane.)

mergeable_state is no longer dirty; it now reports blocked pending maintainer review.

[teknium1]

Landed on main in #108130 (04dd80a). Salvaged with authorship intact but the mechanism changed: header zeroing instead of the donor-header splice, which was verified wrong for direct opens (page count/freelist contradict the file -> malformed). Thank you @TaoMasterCoder.

[TaoMasterCoder]

Thanks for the salvage and the candid mechanism note, @teknium1.

Agreed on header zeroing being the right call — if the donor-header splice yields a file whose page count/freelist contradict the header on direct opens, that's malformed by definition, and zeroing lets the real recovery flow speak for itself. Noted for future work in this lane.