Skip to content

fix(state): PASSIVE not TRUNCATE for all state.db checkpoints (#45383) - #84277

Open
lkz-de wants to merge 1 commit into
NousResearch:mainfrom
lkz-de:fix/wal-checkpoint-passive-45383
Open

fix(state): PASSIVE not TRUNCATE for all state.db checkpoints (#45383)#84277
lkz-de wants to merge 1 commit into
NousResearch:mainfrom
lkz-de:fix/wal-checkpoint-passive-45383

Conversation

@lkz-de

@lkz-de lkz-de commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

fix(state): PASSIVE not TRUNCATE for all state.db checkpoints (#45383)

Problem

SessionDB.close() ran PRAGMA wal_checkpoint(TRUNCATE). Every cron run_agent
opens and closes its own transient SessionDB, so on a busy fleet TRUNCATE — a
full WAL reset — fired many times an hour, racing the gateway's long-lived writer
on a large (multi-GB, ~500k-page) WAL database and tearing hot B-tree pages.

This is structurally the corruption this module's own periodic checkpoint was
already switched to PASSIVE to avoid (issue #45383 — see the _try_wal_checkpoint
docstring). But close() and two manual-maintenance paths kept using TRUNCATE.

Evidence

Diagnosed as the root cause of three state.db B-tree corruptions in Aug 2026:

  • Damage localized to the hottest-written pagesgateway_routing and the
    sessions indexes — across all incidents. Random storage faults would not
    repeat the same victim set.
  • Corrupt pages were whole zero-filled, still live, and off the freelist
    (secure_delete=OFF, so app deletes never zero pages) — the signature of
    zero-filled WAL-frame reads over hot page slots during a checkpoint/reset
    race, not disk or application SQL.
  • Corruptions occurred with no crash/OOM and only after WAL was enabled —
    impossible for a plain SQL bug, and inconsistent with fsync/host faults (with
    no crash, the page cache returns correct data regardless of fsync honesty).

Fix

Route every checkpoint on the shared state.db through PASSIVE:

  • close() (hermes_state.py)
  • pre-VACUUM in vacuum() (hermes_state.py)
  • post-optimize-storage (hermes_state_search.py)

PASSIVE never resets/truncates the WAL and never takes the exclusive checkpoint
lock, so a transient closer cannot lose a race with the live writer. The WAL is
instead bounded by journal_size_limit and the writer's natural
post-checkpoint reset.

TRUNCATE legitimately belongs on a sole-opener/quiescent connection (offline
maintenance); this PR does not try to detect that — PASSIVE is the safe
default. A future refinement could gate TRUNCATE on has_live_connection().

Relationship to existing PRs / issues

Also addresses #80255.

Companion config (not in this diff)

Because close() no longer truncates, deployments should bound the WAL: set
database.journal_size_limit (e.g. 64 MB). The plumbing already exists in
apply_database_pragmas.

Testing

  • uv run --with pytest --with pytest-asyncio python -m pytest tests/test_wal_checkpoint_strategy.py tests/test_hermes_state.py -q -o 'addopts='226 passed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 12, 2026
…search#45383)

SessionDB.close() ran `PRAGMA wal_checkpoint(TRUNCATE)`. Every cron
run_agent opens and closes its own transient SessionDB, so on a busy
fleet this fired a full WAL reset many times an hour, racing the
gateway's long-lived writer on a large WAL database and tearing hot
B-tree pages -- structurally the same corruption this module's own
periodic checkpoint was already switched to PASSIVE to avoid (NousResearch#45383).
Only close() and two manual-maintenance paths still used TRUNCATE.

Route every checkpoint on the shared state.db through PASSIVE:
  - close()                    (hermes_state.py)
  - pre-VACUUM in vacuum()     (hermes_state.py)
  - post-optimize-storage      (hermes_state_search.py)

PASSIVE never resets/truncates the WAL and never takes the exclusive
checkpoint lock, so it cannot lose a transient closer's race with the
live writer. The WAL is instead bounded by `journal_size_limit` and the
writer's natural post-checkpoint reset. TRUNCATE belongs only on a
sole-opener/quiescent connection (e.g. offline maintenance); this change
does not try to detect that -- PASSIVE is the safe default.

Diagnosed as the root cause of three state.db B-tree corruptions in
2026-08: damage localized to the hottest-written pages (gateway_routing
and the sessions indexes), with whole zero-filled pages still live and
off the freelist -- the checkpoint/reset-race signature, not disk or
application SQL.

Tests: tests/test_wal_checkpoint_strategy.py now asserts PASSIVE at
close(), before vacuum(), and after optimize_fts_storage() VACUUM;
tests/test_hermes_state.py asserts close() likewise. Focused run:
226 passed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@webtecnica

Copy link
Copy Markdown
Contributor

Thanks @lkz-de for the superset — our #80346 covered close + pre-VACUUM but missed the post-optimize-storage site, and your production corruption evidence is the decisive argument for PASSIVE-everywhere. Closed #80346 in favor of this one; the full TRUNCATE→PASSIVE conversion + WAL-race protection is exactly what #80255 needed. Happy to help with the merge or any follow-up.

teknium1 pushed a commit that referenced this pull request Aug 13, 2026
Follow-up to the state.db PASSIVE checkpoint salvage (PR #84277,
#45383/#80255/#44795): the kanban dispatcher's periodic explicit
checkpoint still used TRUNCATE on the shared kanban.db. The dispatch
flock only serializes dispatchers — CLI kanban commands in other
processes write to the same board without it, so the TRUNCATE races
live writers exactly like the state.db close() path did.

Switch it to PASSIVE and bound the -wal file with
journal_size_limit=8MiB set at connection init (SQLite trims the file
on the writer's natural post-checkpoint reset), since PASSIVE never
truncates.

tests/hermes_cli/test_kanban_db_repair.py updated to assert PASSIVE
and reject TRUNCATE. Remaining TRUNCATE call sites are test fixtures
operating on private temp DBs (sole opener), which is the legitimate
use.
teknium1 pushed a commit that referenced this pull request Aug 13, 2026
…try transient EIO on journal-mode probe

Salvaged remainder of PR #82280 (state.db hardening rollup):

- Runtime connection corruption: a sibling process replacing/truncating
  the backing file breaks the live write connection — every subsequent
  write raises 'file is not a database' and the gateway wedges
  permanently (messages pile up in memory). Add a bounded one-shot
  reconnect on the write path: close the broken connection, reopen the
  DB file (re-running WAL activation + schema reconciliation), retry
  the failed write once.
- _on_disk_journal_mode: retry transient 'disk i/o error' (virtualized
  block devices) a few times before returning None, so a one-shot EIO
  doesn't push callers onto the fail-closed unknown-mode branch.

The rollup's write-lock machinery, checkpoint-strategy changes, and
repair serialization are intentionally NOT included — superseded by
PRs #84277 and #69609, or wrong-direction per the POSIX
lock-cancellation findings (#71724 lineage).
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(state): PASSIVE not TRUNCATE for all state.db checkpoints (#45383)

The corruption rationale is convincing and the TRUNCATE-everywhere removal is consistent. Observations:

  1. WAL bound depends on config — the updated docstrings state the WAL "is bounded by journal_size_limit", but that PRAGMA is only applied when the operator sets database.journal_size_limit in config (see _apply_database_pragmas, default None → not applied; SQLite's default limit is effectively unbounded). With TRUNCATE gone from every path, a long-lived gateway's WAL now stays at its high-water mark until a natural writer reset occurs (which requires no other attached readers). Consider either defaulting journal_size_limit in DEFAULT_CONFIG, or explicitly documenting in the PR/changelog that operators who care about WAL file size must set it.
  2. close() going PASSIVE means the transient per-cron-run connections no longer shrink the WAL at all — an accepted tradeoff given the race, but a real disk-footprint regression for cron-heavy setups. A changelog note for operators watching state.db-wal size would be worth it.
  3. Tests are thorough: the TrackingConnection verifies PASSIVE-before-VACUUM ordering, PASSIVE-after-optimize-VACUUM ordering, and that close() never TRUNCATEs; the existing read-only no-checkpoint guard still holds. The doc-comment in tests/test_wal_checkpoint_strategy.py was updated consistently with the new policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants