Skip to content

fix(state): optional self-skip in deleted-WAL holder scan breaks post-halt self-deadlock - #109997

Closed
boomzikazita wants to merge 1 commit into
NousResearch:mainfrom
boomzikazita:fix/wal-guard-self-deadlock
Closed

boomzikazita wants to merge 1 commit into
NousResearch:mainfrom
boomzikazita:fix/wal-guard-self-deadlock

Conversation

@boomzikazita

Copy link
Copy Markdown

Problem

On Python < 3.12, a connection whose WAL generation was lost is retired unclosed by design (_prepare_connection_retirement has no setconfig — closing would checkpoint retired frames onto the newer generation). The orphaned -wal/-shm descriptors therefore linger in /proc/self/fd for the lifetime of the process.

iter_deleted_sqlite_sidecar_holders() counts this PID as a holder. After the process's first halt, refuse_deleted_wal_generation() — called before every fresh sqlite3.connect — scans /proc, finds those dead retired descriptors, and refuses the process's own new connections. In production this loops every 5–10 minutes; restarting only resets the cycle (the next halt leaks the next generation).

Repro layout: one state.db opened concurrently by gateway + dashboard + CLI sessions, all in WAL mode. Any process whose connections all close briefly unlinks the sidecar; every other process's still-open connections become orphaned-generation holders on their next write → halt → retire-unclosed leak → self-refusal.

Fix

HERMES_WAL_GUARD_SKIP_SELF=1 makes the holder scan skip this PID.

Why skipping self is safe here

  • The descriptors leaked by retire-unclosed are dead: no code path reads or writes through them again.
  • The WAL that the refuse path protects belongs to brand-new connections minted after the scan — no dual-WAL hazard.
  • A still-active old connection holding a deleted sidecar cannot smuggle frames onto the newer generation: its writes go through the halt path and are caught by _wal_generation_was_lost() (capture + raise).

Alternative considered

Tracking retired fd numbers in SessionDB and excluding exactly those in the scan (instead of the whole PID) — finer-grained, but requires threading retire state into the scan; the env-gated whole-PID skip is the minimal change and matches the existing fail-closed default (unset env = current behavior).

Test plan

  • tests/hermes_state/test_deleted_wal_generation_guard.py — 15 passed, 3 skipped
  • tests/hermes_state/test_retired_wal_generation_capture.py — included above
  • Production: env set on gateway + dashboard units, zero FATAL in the observation window (was every 5–10 min)

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 13, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: #109966 / #109687 (deleted-WAL generation refusal cluster). Complementary open fixes touching the same guard: #109766 (opt-in in-place self-heal for a lost generation) and #109758 (macOS holder enumeration via libproc). Different mechanisms, so not marked duplicate; flagging so reviewers see the whole set.

@boomzikazita

Copy link
Copy Markdown
Author

Production follow-up (same day): this patch fixes the self-refusal deadlock as designed, but soak testing exposed its boundary — it does not address inter-process WAL generation turnover.

What we observed after deploying it: with the gateway in WAL, the dashboard process (hermes serve) periodically closes its whole connection pool, which unlinks the -wal/-shm sidecars. The gateway's live connections then hit _wal_generation_was_lost() → halt → write refusals — a separate path from the self-refusal this patch targets, and not fixable by any holder-scan change.

Final production resolution: database.journal_mode: delete in config.yaml (all 7 Hermes DBs switched offline). With no sidecar files there are no generations to lose, so both the guard and the halt path never trigger. For multi-process deployments (gateway + dashboard + CLI on one state.db) this config appears to be the architecturally right answer — worth documenting, since WAL's "any process closing its last connection unlinks the sidecar" behavior makes generation turnover a steady state, and the new guard turns each turnover into a FATAL.

This PR's updated positioning: insurance for deployments that stay on WAL (prevents the post-halt self-deadlock); operators on multi-process layouts should prefer journal_mode: delete. Happy to add a docs note to that effect if you point me at the right file.

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Thanks @boomzikazita for the production report — closing this PR.

hermes_state_dbfile.py::iter_deleted_sqlite_sidecar_holders includes the current PID on purpose (docstring: "on the open/write refuse path the in-process writer holding the orphan inode must not mint a replacement WAL"); the safe self-skip already lives in _foreign_state_db_holders. An env-gated (HERMES_WAL_GUARD_SKIP_SELF) bypass of a deliberately fail-closed guard, with no tests, isn't something we can land — and per your own follow-up it didn't hold in production either. The generation-turnover trigger behind the loop was fixed at the lock layer by #110544 (75e155ab09, 274fd56dca, 3e43cee505); please retest on a build containing those and open an issue with the repro if it still recurs.

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 P1 High — major feature broken, no workaround 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.

3 participants