Skip to content

fix(state): use PASSIVE WAL checkpoint in close() and pre-VACUUM paths (#45383) - #72549

Closed
TurgutKural wants to merge 2 commits into
NousResearch:mainfrom
TurgutKural:fix/close-vacuum-passive-checkpoint-45383
Closed

TurgutKural wants to merge 2 commits into
NousResearch:mainfrom
TurgutKural:fix/close-vacuum-passive-checkpoint-45383

Conversation

@TurgutKural

Copy link
Copy Markdown
Contributor

Summary

PR #64607 switched the periodic _try_wal_checkpoint() to PASSIVE to fix #45383, but left TRUNCATE in close() 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.timer fired at 07:40, systemctl restart → SIGTERM to the coder gateway.
  • SIGTERM handler (gateway.py:6750, sys.exit(128+signum)) → normal Python exit → SessionDB.close()PRAGMA wal_checkpoint(TRUNCATE).
  • Gateway was mid-write (active delegation writes + a gh pr checks subprocess). The interrupted TRUNCATE checkpoint tore b-tree pages and truncated the WAL to 0 bytes.
  • Result: coder state.db (648 MB / 165,969 pages) corrupted (btreeInitPage() error 11, trees 5/8), WAL empty, target session's messages unrecoverable.

The periodic PASSIVE checkpoint was not involved — the corruption came directly from close()'s TRUNCATE.

Changes

  • hermes_state.py:
    • close() (:2554) → PASSIVE (was TRUNCATE).
    • pre-VACUUM checkpoint before optimize_fts VACUUM (:3121) → PASSIVE.
    • pre-VACUUM checkpoint (:10646) → PASSIVE.
    • Zero wal_checkpoint(TRUNCATE) calls remain in the module.
  • tests/test_wal_checkpoint_strategy.py: TestCloseUsesTruncate renamed to TestCloseUsesPassive; asserts close() issues exactly one PASSIVE and zero TRUNCATE checkpoints.

Why PASSIVE is safe at close/vacuum

PASSIVE replays 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.py6 passed.
  • python -m py_compile hermes_state.py → OK.
  • grep wal_checkpoint(TRUNCATE) hermes_state.py → none.

This PR was prepared on a clean worktree branched directly from upstream/main (single commit, no carry-stack contamination).

Fixes #45383 (follow-up to PR #64607, which fixed the periodic path only).

@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 area/sessions Session lifecycle, resume, persistence, history P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 27, 2026
@TurgutKural
TurgutKural force-pushed the fix/close-vacuum-passive-checkpoint-45383 branch 3 times, most recently from 40df3c5 to de422dc Compare July 30, 2026 03:36
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for following up on the remaining close/vacuum checkpoint paths. The two hermes_state.py call sites are still present on current main (hermes_state.py:2542 and hermes_state.py:8322), so the change addresses live behavior.

Problems

  • The stated “all remaining” scope is incomplete after the SessionDB mixin split: optimize_fts_storage() still runs PRAGMA wal_checkpoint(TRUNCATE) after its VACUUM at hermes_state_search.py:562. gh pr diff 72549 changes only hermes_state.py and tests/test_wal_checkpoint_strategy.py.
  • The revised test covers close() but does not exercise SessionDB.vacuum() or optimize_fts_storage(vacuum=True), leaving both maintenance paths unguarded.

Suggested changes

  • Update the remaining hermes_state_search.py:562 checkpoint if PASSIVE is intended for every SessionDB close/maintenance checkpoint.
  • Add path-level tests for vacuum() and optimize_fts_storage(vacuum=True) that verify their emitted checkpoint mode.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 30, 2026
@TurgutKural
TurgutKural force-pushed the fix/close-vacuum-passive-checkpoint-45383 branch from 784cb8f to 9816d66 Compare July 30, 2026 15:44
@TurgutKural

Copy link
Copy Markdown
Contributor Author

Rebased onto current upstream/main (7965462). Confirmed the two hermes_state.py call sites are still present on main. The CI failure was a pre-existing vercel sandbox test issue now fixed upstream — not related to this PR's checkpoint changes.

@TurgutKural
TurgutKural force-pushed the fix/close-vacuum-passive-checkpoint-45383 branch 3 times, most recently from cc51d55 to 273d6f1 Compare August 2, 2026 03:35
PR #64607 switched the periodic _try_wal_checkpoint() to PASSIVE to fix
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 #45383
@TurgutKural
TurgutKural force-pushed the fix/close-vacuum-passive-checkpoint-45383 branch from 273d6f1 to 0cb8f19 Compare August 3, 2026 05:32
@TurgutKural

Copy link
Copy Markdown
Contributor Author

Upstream ile çeliştiği için kapatıyorum (upstream-absorbed / design conflict).

Kanıt — CI kırmızısı gerçek bir çelişki, flaky değil:

  • tests/test_hermes_state.py::TestConnectionLifecycle::test_writable_close_retains_truncate_checkpoint — writable close'ın TRUNCATE checkpoint yapmasını zorunlu kılıyor.
  • test_read_only_close_never_requests_wal_checkpoint — read-only close'ın hiç checkpoint yapmamasını zorunlu kılıyor.

Bu iki test upstream/main'de 57197cd48d (fix(dashboard): preserve maintenance writes on read polling, 2026-08-02) ile eklendi ve upstream/main'de geçiyor. PR'ın close()'ta PRAGMA wal_checkpoint(PASSIVE) dayatması birinci testi, read-only guard'ı kaldıran değişiklik ikinci testi kırıyor (CI: Python tests slice 2/8, 2 failed / 160 passed).

Upstream'in bilinçli tasarımı:

  • close() → TRUNCATE (WAL dosyasını küçültmek için, test ile garantiye alınmış)
  • periyodik checkpoint → PASSIVE (her N yazmada, _try_wal_checkpoint)
  • read-only close → hiç checkpoint yok

Çözüm zaten mevcut (#74645 ile):
Bu PR'ın istediği "PASSIVE'e geçme yeteneği" artık plugin sistemiyle karşılanıyor: #74645'in pre_db_checkpoint hook'u close/vacuum öncesi checkpoint modunu allow-list (PASSIVE/FULL/RESTART/TRUNCATE) içinde override etmeye izin veriyor — default TRUNCATE korunuyor, PASSIVE isteyen ortamlar (örn. SIGTERM race senaryosu #45383) plugin ile seçebiliyor. Hardcode PASSIVE hem upstream testleriyle çelişiyor hem de artık gereksiz.

Kapatma kararı yorumdaki kanıtlara dayanıyor; sorunun kendisi (#45383) plugin yoluyla çözülebilir durumda.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

state.db recurring B-tree corruption from WAL TRUNCATE checkpoint on large FTS5 databases

3 participants