fix(state): refuse SessionDB open and writes on a deleted WAL generation - #101081
Closed
astraltrekkin wants to merge 1 commit into
Closed
astraltrekkin wants to merge 1 commit into
astraltrekkin wants to merge 1 commit into
Conversation
A live writer can keep a deleted state.db-wal inode while a second opener mints a fresh WAL at the same path. Fail closed on writable open (before connect) and on the write-path sidecar identity check so the second generation is never created. Co-authored-by: Noa <rainbowgore@users.noreply.github.com>
alt-glitch
added
type/bug
Contributor
PR #101081 — fix(state): refuse SessionDB open and writes on a deleted WAL generation
Non-blocking:
Verdict: LGTM. Fail-closed split-brain fix for the intermittent |
kshitijk4poor
added a commit
to kshitijk4poor/hermes-agent
that referenced
this pull request
Sep 2, 2026
…r the deleted-WAL guard Follow-ups on the salvaged NousResearch#101081 guard: - A clean close() lets SQLite unlink the WAL sidecars legitimately; the guard treated that as a lost generation and permanently halted the handle, so the NousResearch#94736 late-write self-heal reopen dropped transcript tails (4 existing tests failed). close() now clears the recorded sidecar generation, and _wal_generation_was_lost() re-adopts the current sidecars after a clean /proc/self probe instead of relying on a stale snapshot. - Healthy writes no longer walk /proc/self/fd: once a sidecar generation is recorded, the stat-based inode check alone detects an unlink/replace. The fd probe only runs in the empty-identity state (fresh DB, post-close reopen). - DeletedWalGenerationError now subclasses StateDbReplacedError, so the gateway retry queue and run_agent flush divert transcripts to the JSONL fallback exactly as they do for a replaced store, instead of retrying forever against a halted handle. - __init__ refuses once (under the startup lock) instead of twice per open, halving the system-wide /proc scan; dropped the dead include_self parameter and the dead _IS_WINDOWS clause. - Test fixes: rstrip(' (deleted)') char-set bug -> removesuffix; the non-linux test now patches sys.platform (the real gate) instead of _IS_WINDOWS.
kshitijk4poor
added a commit
that referenced
this pull request
Sep 2, 2026
…d-WAL guard Follow-ups on the salvaged #101081 guard: - A clean close() lets SQLite unlink the WAL sidecars legitimately; the guard treated that as a lost generation and permanently halted the handle, so the #94736 late-write self-heal reopen dropped transcript tails (4 existing tests failed). close() now clears the recorded sidecar generation, and _wal_generation_was_lost() re-adopts the current sidecars after a clean /proc/self probe instead of relying on a stale snapshot. - Healthy writes no longer walk /proc/self/fd: once a sidecar generation is recorded, the stat-based inode check alone detects an unlink/replace. The fd probe only runs in the empty-identity state (fresh DB, post-close reopen). - DeletedWalGenerationError now subclasses StateDbReplacedError, so the gateway retry queue and run_agent flush divert transcripts to the JSONL fallback exactly as they do for a replaced store, instead of retrying forever against a halted handle. - __init__ refuses once (under the startup lock) instead of twice per open, halving the system-wide /proc scan; dropped the dead include_self parameter and the dead _IS_WINDOWS clause. - Test fixes: rstrip(' (deleted)') char-set bug -> removesuffix; the non-linux test now patches sys.platform (the real gate) instead of _IS_WINDOWS.
Contributor
|
Merged via #101221 — your commit was cherry-picked with authorship preserved ( Two adjustments were folded on top in the salvage (second commit
Closing this PR in favor of the merged salvage. Thanks again! |
nftpoetrist
added a commit
to nftpoetrist/hermes-agent
that referenced
this pull request
Sep 3, 2026
…lted states _get_read_conn()'s fresh-open guard (this branch's first commit) only checked self._db_corrupt. Since that commit's base, hermes_state.py grew two more halted states that _reopen_after_close_locked already refuses to reopen for on the write side: self._db_replaced (the path now resolves to a different file generation, NousResearch#89332) and self._db_wal_generation_lost (this handle's WAL/SHM generation was deleted out from under it, salvage of NousResearch#101081/NousResearch#101221). _get_read_conn() was unaware of both — it would still happily open a brand-new sqlite3 connection to the file in either state, the same "reopen a damaged/replaced image" hazard the first commit fixed for the corrupt case. Fixed by returning None (falling back to the locked path, same as the _db_corrupt check) when either flag is set. Deliberately checks only the already-set flags, not the proactive _db_file_was_replaced()/ _wal_generation_was_lost() detection probes _reopen_after_close_locked also runs — that keeps this hot-path check as cheap as the existing _db_corrupt check (a bare flag test), consistent with this function's established style. An out-of-band replace/WAL-loss not yet observed by this handle is still caught the next time a write or an explicit check touches it. Added two tests mirroring the existing test_get_read_conn_refuses_fresh_open_when_quarantined: test_get_read_conn_refuses_fresh_open_when_replaced and test_get_read_conn_refuses_fresh_open_when_wal_generation_lost, both forcing _wal_active=True on a live (not yet closed) handle and asserting _get_read_conn() returns None without ever calling the mocked _connect_tracked_db. Mutation-verified: reverting the hermes_state.py hunk alone makes both new tests fail with the mock connection object returned instead of None; restoring it makes them pass. Ran tests/hermes_state/ (11 passed, 1 pre-existing skip) and tests/state/ + tests/hermes_state/ together (417 passed, 7 skipped, all pre-existing) — no regressions. Fresh competitor check: no open or closed PR covers this specific gap. NousResearch#101042 ("set busy_timeout=5000 on writer and reader connections") also touches _get_read_conn() but only inside the try block after the connection is already opened (adds a busy_timeout PRAGMA) — no line overlap with this commit's checks near the top of the function.
melon-xf
added a commit
to melon-xf/hermes-agent
that referenced
this pull request
Sep 3, 2026
…r the deleted-WAL guard Follow-ups on the salvaged NousResearch#101081 guard: - A clean close() lets SQLite unlink the WAL sidecars legitimately; the guard treated that as a lost generation and permanently halted the handle, so the NousResearch#94736 late-write self-heal reopen dropped transcript tails (4 existing tests failed). close() now clears the recorded sidecar generation, and _wal_generation_was_lost() re-adopts the current sidecars after a clean /proc/self probe instead of relying on a stale snapshot. - Healthy writes no longer walk /proc/self/fd: once a sidecar generation is recorded, the stat-based inode check alone detects an unlink/replace. The fd probe only runs in the empty-identity state (fresh DB, post-close reopen). - DeletedWalGenerationError now subclasses StateDbReplacedError, so the gateway retry queue and run_agent flush divert transcripts to the JSONL fallback exactly as they do for a replaced store, instead of retrying forever against a halted handle. - __init__ refuses once (under the startup lock) instead of twice per open, halving the system-wide /proc scan; dropped the dead include_self parameter and the dead _IS_WINDOWS clause. - Test fixes: rstrip(' (deleted)') char-set bug -> removesuffix; the non-linux test now patches sys.platform (the real gate) instead of _IS_WINDOWS.
7 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
5 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.
What does this PR do?
A live writer can keep an open fd to an already-unlinked
state.db-wal(or-shm) while a laterSessionDBopen would callsqlite3.connectand mint a fresh WAL at the same path. The two generations cannot see each other, which is the intermittentdatabase disk image is malformed/disk I/O errorfield report.This PR fails closed on that generation split:
SessionDB.__init__scans for deleted WAL/SHM holders (Linux/proc/*/fd, including this process) beforesqlite3.connect, so a second opener cannot create the replacement WAL.(st_dev, st_ino)at open and halts if that identity is gone or this process still holds a(deleted)sidecar fd.database.journal_modestayswalby default.deleteremains operator containment, not a new default.The
N live SessionDB handleswarning and the repair-only deleted-holder work in #97330 are left alone.Related Issue
Fixes #101064
Type of Change
Changes Made
hermes_state.py: addDeletedWalGenerationError,iter_deleted_sqlite_sidecar_holders(), andrefuse_deleted_wal_generation(); call the refuse helper before writable connect; snapshot sidecar inodes in_record_db_file_identity; extend_raise_if_db_replaced/ reopen / FTS fail-open to halt on a lost WAL generation; re-record identity after VACUUMTRUNCATE; classify the new error as"replaced"so in-file repair is not attempted.tests/hermes_state/test_deleted_wal_generation_guard.py: open refuse (no new WAL inode), writer halt after unlink, DELETE-mode two writers still work, clean reopen still works.How to Test
SessionDBforced into WAL, create a session, unlinkstate.db-wal/state.db-shmwhile the first handle is still open, then construct a secondSessionDBon the same path. ExpectDeletedWalGenerationErrorand no new WAL file at that path.DeletedWalGenerationErrorand_db_wal_generation_lost is Trueon the next write as well.resolve_journal_mode()returningdelete. Two writers on the same path still append; no WAL sidecar appears.18 passed (8 new + 10 existing identity tests).
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
After the fix, a second
SessionDBopen while the first handle still holdsstate.db-wal (deleted)raises:The path does not grow a replacement WAL. The original writer’s next append raises the same error instead of committing on the orphan inode.