fix(sessions): archiving a session must retire its routing entry, not just flip a flag - #578
Merged
Merged
Conversation
… a flag `set_session_archived` issued exactly one statement — it set `archived` and nothing else, leaving `end_reason = NULL` / `ended_at = NULL`. Every path that can evict a `SessionStore._entries` key gates on the row being *ended*: * `_prune_stale_sessions_locked` (startup) reads `row["end_reason"]` * the routing-time self-heal (NousResearch#54878) calls `_is_session_ended_in_db` * `prune_old_entries` ages on `updated_at`, which every full persist re-touches So an archived session's `gateway_routing` key was unreachable by all three and was rewritten from the live in-memory index forever. `archive_stale_sessions` routes through the same function, so enabling `sessions.auto_archive` would manufacture these in bulk. Fix, inside the existing write transaction: * live rows in the archived lineage get `end_reason = 'archived'` + `ended_at`, via COALESCE so `'compression'` and every other explicit boundary survives — the lineage CTEs depend on those edges. This re-enables three already-tested eviction paths rather than adding a fourth. * the durable `gateway_routing` rows mapping to that lineage are deleted in the same transaction, keyed on the mapped `session_id` (not `(scope, session_key)`: two profiles sharing one state.db produce the same key in different scopes for different sessions). * unarchive reverses only what archive wrote (rows still carrying `end_reason = 'archived'`); a real `end_reason` survives, and no routing entry is resurrected. `'archived'` is deliberately NOT in the recoverable set of `find_latest_gateway_session_for_peer` (`agent_close` / `ws_orphan_reap`), so the next inbound message starts a fresh session instead of silently reopening an archived conversation. Removed the dead `delete_gateway_routing_entries` (shipped with NousResearch#59203, zero callers ever). It keys on `(scope, session_key)`, which the archiving layer does not have, and it cannot run in the same transaction as the flag flip — a crash between the two would recreate the orphan. Replaced with a comment explaining why, so the next reader doesn't re-add it. Honest scope: this makes the orphan IMPOSSIBLE at the DB level, but the in-memory index is a separate copy. With a gateway UP, the next full persist still re-writes the key from `_entries` — measured. What changed is that the routing-time self-heal now fires on the archived row, so the key that comes back points at a NEW session and the archived one is unreachable. With no live gateway (CLI/desktop archive, offline sweep) the row is gone immediately. Side effect, stated: a session archived while still live is now an *ended* row, so `sessions.auto_prune` (off by default, 90d) can eventually reap it. Rows archived by `archive_sessions` were already ended and already eligible. Verified: * repro on the pre-fix tree: archived key present after full persist AND after restart; post-fix: gone from both DB and memory * scripts/run_tests.sh tests/hermes_state tests/test_hermes_state.py -> 659 passed, 0 failed * scripts/run_tests.sh tests/gateway tests/hermes_cli tests/dashboard -> 10648 passed, 0 failed * scripts/run_tests.sh (full) -> 31413 passed, 4 failed; the same 4 (test_otlp_exporter, test_modal_snapshot_isolation) fail identically on unmodified 31bc5c7 — inherited * mutation proof: 7 mutants (no end_reason / no routing DELETE / reopenable end_reason / end_reason clobbered / unarchive over-clears / delete drops lineage ancestors / ended_at left NULL) -> 7/7 KILLED, control GREEN, tree restored byte-clean * denormalized `effective_last_active` unchanged across archive/unarchive and still matches the fresh-CTE oracle; probe falsified by injecting drift
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
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.
Root cause
SessionDB.set_session_archivedissued exactly one statement — it flippedarchivedand nothing else, leavingend_reason = NULLandended_at = NULL.Every path that can evict a
SessionStore._entrieskey gates on the DB row being ended:_prune_stale_sessions_locked(startup)row["end_reason"]_is_session_ended_in_dbprune_old_entries(max_age_days)updated_at < cutoffSo the archived session's
gateway_routingkey was unreachable by all three and was rewritten from the live in-memory index indefinitely.archive_stale_sessionsroutes through the same function, so enablingsessions.auto_archivewould manufacture these in bulk — that is the real blast radius, not the one row that exists today.Reproduced on the unmodified base (
31bc5c7) with the realSessionStore+SessionDB:Fix
Took the
end_reasonroute the card preferred — it reuses three already-tested eviction paths instead of adding a fourth mechanism. All of it inside the existing single write transaction:end_reason = 'archived'+ended_at, viaCOALESCEso'compression'and every other explicit boundary survives (the recursive lineage CTEs are built on those edges);gateway_routingrows mapping to that lineage are deleted in the same transaction, keyed on the mappedsession_idrather than(scope, session_key)— two profiles sharing onestate.dbproduce the same key in different scopes for different sessions;end_reason = 'archived'). A realend_reasonsurvives, and no routing entry is resurrected — the next message rebuilds it through the normal create path.'archived'is deliberately not in the recoverable set offind_latest_gateway_session_for_peer(agent_close/ws_orphan_reap), so the next inbound message starts a fresh session rather than silently reopening an archived conversation. Verified, not assumed.The dead API
Deleted
delete_gateway_routing_entries(shipped with NousResearch#59203, never gained a caller). It is the wrong tool twice over: it keys on(scope, session_key), which the archiving layer does not have, and being a separate_execute_writeit cannot run in the same transaction as the flag flip — a crash between the two recreates the orphan it exists to prevent. Replaced with a comment saying why, so the next reader doesn't re-add it.Honest scope — impossible or merely rare?
Impossible at the DB level. Not impossible in memory. The in-memory
_entriesdict is a separate copy, and with a gateway up the next full persist still re-writes the key from it. Measured:What changed is that the row is now ended, so the self-heal fires: the key that comes back points at a new session and the archived one is unreachable. With no live gateway (CLI/desktop archive, offline sweep) the row is gone immediately and stays gone across a restart.
Evicting the live in-memory key at archive time would need a gateway-side hook (state.db has no handle on the running
SessionStore); that is a separate change and I did not make it. Called out rather than papered over.Side effect, stated
A session archived while still live is now an ended row, so
sessions.auto_prune(off by default, 90d retention) can eventually reap it. Measured directly — pre-fix shape survives the sweep, post-fix shape is deleted alongside an already-ended archived row that was always eligible. Rows archived byarchive_sessionswere already ended, so nothing changes for them.Verification
scripts/run_tests.sh tests/hermes_state tests/test_hermes_state.py→ 659 passed, 0 failedscripts/run_tests.sh tests/gateway tests/hermes_cli tests/dashboard→ 10648 passed, 0 failedscripts/run_tests.sh(full) → 31413 passed, 4 failed. The same 4 (tests/monitoring/test_otlp_exporter.py×2,tests/tools/test_modal_snapshot_isolation.py×2) fail identically on unmodified31bc5c7in a detached worktree — inherited, not this diff.tests/hermes_state/test_archive_retires_routing.py, all behavioral: they drive the realSessionStoreand read the durablegateway_routingtable after a full persist and a restart, never the in-memory dict (an immediate re-read is the exact false green this bug already produced once).effective_last_activeis unchanged across archive/unarchive and still matches the fresh-CTE oracle; that probe was falsified by injecting drift on two different rows and correctly reported FAIL both times.Negative controls that hold: a live unarchived session's routing entry is never removed; the per-user sibling key for the same chat is untouched; a restart with no archive evicts nothing; one corrupt
entry_jsonrow does not fail the write; another scope mapping the same key to a different session is untouched.Harnesses:
~/.hermes/plans/2026-08-10-archive-retires-routing/(repro.py,live_gateway_probe.py,prune_probe.py,denorm_probe.py,mutation.sh).No production data touched — every probe runs against a fresh
mktempHERMES_HOME.