fix(state): use PASSIVE WAL checkpoint in close() and pre-VACUUM paths (#45383) - #72546
Closed
TurgutKural wants to merge 7 commits into
Closed
fix(state): use PASSIVE WAL checkpoint in close() and pre-VACUUM paths (#45383)#72546TurgutKural wants to merge 7 commits into
TurgutKural wants to merge 7 commits into
Conversation
…dence
Address GottZ review (4 items):
1. Alias set: replaced hard-coded {nous, nous-research} with registered
aliases {nous, nous-portal, nousresearch} from the provider plugin.
2. Precedence: explicit delegation.api_key now wins over runtime auth —
an explicit key means 'use this direct endpoint with this key'.
Runtime auth only fires for Nous-family providers WITHOUT explicit key.
3. Docs: rewrote docstring with numbered precedence list matching the
actual code behavior.
4. Coverage: added tests for primary path (Nous + base_url, no api_key
→ runtime auth), explicit-key-wins path, and registered alias coverage.
Address GottZ review (4 items): 1. Fail-closed: if on_session_start raises during pre-compress rebind, compression is skipped (messages returned unchanged) instead of running under a stale binding. 2. boundary_reason='rebind' passed to on_session_start so conforming engines can distinguish a mid-session rebind (keep state) from a genuine new-session start (reset state). 3. Precedence: bound_session_id (public) → _session_id (legacy) → current_session_id (older fallback), matching the documented contract-respecting order. 4. Coverage: added test_rebind_failure_skips_compression_fail_closed and test_rebind_uses_bound_session_id_precedence.
PR NousResearch#64607 switched the periodic _try_wal_checkpoint() to PASSIVE to fix NousResearch#45383, but left TRUNCATE in close() and the pre-VACUUM paths with the rationale that shutdown has 'no concurrent writers'. Field evidence shows that assumption is false: a SIGTERM-driven shutdown (systemd timer, systemctl restart, Ctrl-C during a busy session) calls SessionDB.close() while writes may still be in flight. The interrupted TRUNCATE checkpoint tears b-tree pages and truncates the WAL, leaving a corrupt main DB with no rollback path. Reproduced 2026-07-27: hermes-gateway-restart.timer fired at 07:40, SIGTERM'd the coder gateway mid-write -> coder state.db (648 MB / 165K pages) corrupted (btreeInitPage error 11, trees 5/8), WAL 0 bytes, target session's messages unrecoverable. Change all remaining wal_checkpoint(TRUNCATE) calls to PASSIVE so no code path can corrupt the DB under a SIGTERM race. PASSIVE replays committed WAL frames without an exclusive lock and cannot tear pages; the WAL is settled on next open. Updates the checkpoint-strategy tests accordingly. Fixes NousResearch#45383
alt-glitch
added
type/bug
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
2 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
PR #64607 switched the periodic
_try_wal_checkpoint()toPASSIVEto fix #45383, but leftTRUNCATEinclose()and the pre-VACUUM paths, on the assumption that shutdown has "no concurrent writers." Field evidence shows that assumption is false.Root cause (field evidence, 2026-07-27)
hermes-gateway-restart.timerfired at 07:40,systemctl restart→ SIGTERM to the coder gateway.gateway.py:6750,sys.exit(128+signum)) → normal Python exit →SessionDB.close()→PRAGMA wal_checkpoint(TRUNCATE).gh pr checkssubprocess). The interruptedTRUNCATEcheckpoint tore b-tree pages and truncated the WAL to 0 bytes.state.db(648 MB / 165,969 pages) corrupted (btreeInitPage() error 11, trees 5/8), WAL empty, target session's messages unrecoverable.The periodic
PASSIVEcheckpoint was not involved — the corruption came directly fromclose()'sTRUNCATE.Changes
hermes_state.py:close()(:2554) →PASSIVE(wasTRUNCATE).optimize_ftsVACUUM (:3121) →PASSIVE.:10646) →PASSIVE.wal_checkpoint(TRUNCATE)calls remain in the module.tests/test_wal_checkpoint_strategy.py:TestCloseUsesTruncaterenamed toTestCloseUsesPassive; assertsclose()issues exactly onePASSIVEand zeroTRUNCATEcheckpoints.Why PASSIVE is safe at close/vacuum
PASSIVEreplays committed WAL frames into the main DB without an exclusive lock, so it cannot tear pages under a SIGTERM race. The WAL is settled on the next open via the normal recovery cadence; shrink-to-zero is not required at close because a fresh gateway reopens the same DB and continues from the high-water mark. VACUUM already manages the WAL independently.Validation
pytest tests/test_wal_checkpoint_strategy.py→ 6 passed.python -m py_compile hermes_state.py→ OK.grep wal_checkpoint(TRUNCATE) hermes_state.py→ none.Fixes #45383 (follow-up to PR #64607, which fixed the periodic path only).