fix(gateway): give the routing index one store instead of the ambient one - #99559
fix(gateway): give the routing index one store instead of the ambient one#99559caya8205-2 wants to merge 5 commits into
Conversation
…ient scope NousResearch#88734 made SessionStore._db follow the ambient HERMES_HOME so a multiplexed profile's rows reach its own state.db. That is correct for the inbound message path, which installs the scope via _profile_runtime_scope. Nothing else does. _session_expiry_watcher (gateway/run.py) walks the single process-wide _entries dict — every profile's keys — and finalizes expired sessions with no scope installed, so _db resolved the ROOT store for rows that live under profiles/<name>/state.db. The scoped inbound path and the unscoped background path then maintained two copies of the same logical session whose end_reason drifted apart independently. Once they disagreed, the NousResearch#54878 stale-routing guard read one copy while the routing index pointed at the other, and a live conversation was dropped and recreated — silently, since that branch only sets was_auto_reset when a reset policy also fired. Field evidence from a live two-profile install: session 20260814_234313 was end_reason=None in the root store but agent_close in the profile store, while 20260822_225807 was inverted. Both directions, which rules out a single mis-scoped writer. The owning profile is already encoded in the session key, so derive the store from it: _profile_home_for_key / _db_for_key, plus _db_for_session_id for the entry points addressed by session id. 40 self._db uses across 14 methods now resolve that way. No signature changed and no existing test was modified. _profile_home_for_key returns None when multiplexing is off, when the key carries the legacy agent:main namespace, or when the profile has no live directory, so single-profile installs resolve exactly where they always did. The explicit-path branch still goes through SessionDB.__init__ -> _ensure_test_isolation, keeping the live-DB guard over per-profile paths. Part of NousResearch#66887. The routing-index half — _routing_scope() and the sessions.json mirror still pinned to one frozen sessions_dir while the handle moves — is left for a follow-up rather than mixed in here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on the memo introduced with _profile_home_for_key. The problem is sharper than "no invalidation on profile deletion": caching the miss pinned a profile that appears AFTER the gateway started to the ambient store for the life of the process, which is the exact failure this helper exists to prevent. That is not hypothetical — an enrollment bridge can provision profiles/<name>/ at runtime, so a key is legitimately seen before its directory exists. Memoize hits only. A miss costs one profile_exists() stat and recurs only for profiles that genuinely do not exist, so the hot path for real profiles is still a dict hit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review P1. _profile_home_for_key() returned the same None for three different states — multiplexing off / legacy agent:main namespace, a named profile whose directory does not exist yet, and a resolution error — and _db_for_key() collapsed all of them to the ambient store. That recreated the very split this change removes. The enrollment bridge provisions profiles/<name>/ at runtime, so a key such as agent:fitness:telegram:dm:1 can legitimately be seen first: the first lookup landed in root state.db, and the next one, after provisioning, in profiles/fitness/state.db. One qualified session identity, two physical stores. The resolver-exception path fell open the same way. Ownership is now tri-state: - no named owner -> ambient DB (single-profile behavior intact) - named owner + home -> that profile's DB - named owner, unresolvable -> None, and a warning; never root Callers already treat a missing DB as "skip the mutation", which is the defer-don't-misroute behavior wanted here. _append_transcript_message is the one path reached with an id the entry-point guard did not check (the compression-child id), so it now raises explicitly and lets the caller's retry queue hold the row instead of relying on an AttributeError. Tests exercise the effect boundary rather than cache state: a named key before its profile exists leaves root untouched and lands only in the profile store once provisioned, and a resolver exception fails closed too. Both fail against the previous two-state behavior by returning a live SessionDB where None is required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review P1 NousResearch#2. _append_to_transcript_serialized() writes the compression continuation to child_id BEFORE publishing either _transcript_reroutes or the _entries update — that ordering is load-bearing for backlog order, so it must not move. At that moment nothing in the routing index points at the child, so _db_for_session_id(child_id) missed its scan and fell through to _db_for_key(None), i.e. the ambient store. The fail-closed guard did not fire because root is a live handle. The row therefore targeted root rather than the already-proven parent owner. With no child row there the append is rejected by the FOREIGN KEY constraint, the pending queue never drains and the reroute cannot advance; against a split-brain root the message would instead be written cross-profile. Record ownership before the mutation instead of moving the publication: a private _session_owner_hints map carries session_id -> owning key for ids whose owner is proven but not yet published, consulted by the new _owner_key_for_session_id() after the index scan misses, and dropped as soon as routing publishes. Signatures are unchanged, so the existing suites that stub _append_transcript_message keep working untouched; the map is read through getattr for stores built via object.__new__. The regression is physical rather than mocked: an ended compression parent and a live child that exist only in profiles/fitness/state.db, no active profile scope, append to the parent, then assert all four effects — the row lands on the child in the profile store, the pending queue drains, the reroute and the routing entry advance, and root state.db stays untouched. Without the hint it fails exactly as the review predicted, on "FOREIGN KEY constraint failed" against root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… one Second half of NousResearch#66887. _entries is a single flat dict holding every profile's keys, so the index it persists to has to be a single file — but it was read and written through _db, which resolves whichever profile scope is active. A whole-index rewrite during one profile's turn copied every other profile's routing rows into that profile's store, and startup, which runs unscoped, then loaded a different copy than the last writer produced. That is why the startup recovery pass never sees a secondary profile's crash marker, which is the half this issue's title names. mark_turn_active() persists through the single-entry fast path (state.db only, no sessions.json mirror), so a marker written during a profile's turn landed in that profile's store and _recover_unclean_sessions(), running with no scope, read a store that had never heard of it. The turn was silently never promoted to resume_pending. Capture the gateway's own home at construction — the store is built at startup before any profile scope exists — and route the index through it: _ensure_loaded_locked, _reconcile_recovered_routing_locked, _persist_routing_data and _save_entry now use _routing_db. A pinned handle still wins, so suites that install a fake or disable the DB are unaffected. _prune_stale_sessions_locked is the mixed case and is split accordingly: it now asks _db_for_key(key) whether each session ended, because that is a per-session question, while the index write stays on the single store. One ambient handle previously answered it for every profile at once, which could prune a live secondary-profile route on the strength of the root store's copy of that session. Regression as requested on the issue: mark a turn active under a secondary profile's scope, then build a fresh store with no scope and run recover_interrupted_turns(). It promotes exactly one turn to resume_pending here and promotes zero against the previous behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 96d12240b741cece2d75039b8b75216a63ca1d99, including the one-commit child delta over #97309 (373bb3e227b3478d4150738827ad16a8b73e6d9a → 96d12240…), the #66887 incident/recovery history, current main, exact-head workflows, the historical #69042 shape objection, merged #88734 / #88632 lineage, and the still-open #96038 corruption-classification interlock.
The architectural split is right. #88734 established active-scope per-profile DB handles; #97309 derives per-session row ownership from the qualified agent:<profile>:… key for unscoped/background work; this PR correctly recognizes that _entries is a different kind of object — one process-wide routing index — and gives that index one durable owner. Moving _ensure_loaded_locked, reconciliation, full persistence and the single-entry fast path onto _routing_db, while keeping _prune_stale_sessions_locked's session-liveness check on _db_for_key(key), is exactly the correct owner boundary. The new restart regression also proves the forward-state failure cleanly.
I found two P1 boundaries before this can be used as the recovery half of #66887.
P1 — the regression proves post-fix writes, but the first upgrade boot still loses markers already stranded by the bug
The deployed failure this PR repairs has already produced split routing copies. Before this change, mark_turn_active() persisted through _save_entry(), and that fast path wrote the routing row to the ambient/profile state.db without refreshing sessions.json. So an interrupted secondary-profile turn can already exist in this shape at upgrade time:
- root routing / root
sessions.json: structural entry, with no currentactive_turn_token; profiles/<name>/state.db.gateway_routing: newer copy of the same key carrying the active-turn marker.
The new test_crash_marker_from_a_secondary_profile_survives_restart starts by constructing a store with this PR's _routing_db behavior already active, then writes the marker. That proves future markers land centrally, but it never seeds the pre-fix layout.
On an actual upgrade, _ensure_loaded_locked() now reads the one central _routing_db, then uses root sessions.json only as a missing-key legacy import. It never inspects or reconciles the profile-local gateway_routing rows that the old ambient writer created. If root already has the key, the stale central copy wins and the newer profile-local marker is orphaned. The first restart after upgrading therefore still fails to promote that already-interrupted turn — exactly the recovery loss named in #66887. The same applies to newer fast-path routing metadata stranded in a profile-local copy.
Please add a deterministic one-time reconciliation/migration for legacy profile-local routing rows before startup recovery, with explicit conflict precedence, or an equivalent mechanism that consumes those old copies into the canonical routing index. The regression needs to seed the old layout directly: root structural route/mirror plus a newer profile-local row carrying an active-turn token; instantiate the new store unscoped; prove recover_interrupted_turns() promotes exactly once; and prove the canonical routing index now contains the reconciled state. Forward-state coverage is good, but a recovery fix has to recover the state the old code already emitted.
P1 — failure to capture the routing owner silently re-enables ambient routing writes
__init__ catches every exception around get_hermes_home() and sets _routing_home = None. _routing_db then interprets that as:
if home is None:
return self._dbThat fallback is the exact authority collapse this PR is eliminating. If routing-home capture fails during construction but the ordinary DB path later succeeds, any routing write performed under a named profile scope again follows ambient _db and lands in that profile's state.db; startup remains unscoped and reads a different copy.
None needs to mean “canonical routing owner unavailable”, not “ambient is acceptable”. Either derive the routing owner from another deterministic launch-owned coordinate (for example the already-fixed gateway sessions_dir, if that invariant is made explicit), or fail closed to the non-SQLite routing fallback. Do not route a process-global index through a profile-local ambient handle.
Please add a regression that forces routing-home capture/resolution failure, enters a named profile scope, persists a routing mutation, and proves the profile DB's gateway_routing table remains untouched. The system may degrade, but it must not widen ownership.
Interlocks / merge order / credit
This is complementary work, not a duplicate:
- #88734 is the merged predecessor that salvaged #88632 and preserved Jack Lau's authorship; it fixed active-scope handle resolution and lifecycle.
- #97309 is this PR's direct prerequisite and the per-session half of #66887. This child should remain stacked after it; its unique code delta is only
gateway/session.pyplus the multiplex regression file. - #99559 is the process-wide routing-index half. Keeping
_entriesflat is the right answer to the older #69042 objection; #69042 is historical/incomplete overlap, not work to erase or relabel. - #96038 remains complementary and still open. The parent #97309 widens real background writes across named-profile
SessionDBhandles, while #96038 supplies the fail-closed canonical-vs-FTS corruption classification that prevents structural corruption from entering live FTS recovery. That interaction should remain explicit in landing order/evidence rather than disappearing when this child is merged.
One non-blocking artifact-truth nit: _save_sessions_json() still writes a _README saying all sessions live in ~/.hermes/state.db. After the #88734/#97309 architecture, the routing index is centralized there but named-profile session/message rows are not. Please update that sentinel while this ownership distinction is fresh; otherwise the fallback artifact teaches operators the opposite storage model from the code.
Exact-object acceptance
The author-reported focused result (21 passed) and wider 126-pass sweep are useful development evidence. They are not the landing receipt for this head. Exact-head CI 33413934488, Docker 33413933856, and Nix 33413933801 are all action_required; CI has zero jobs. The PR's recorded base is also behind live main@d16622cd14a41574c0a2cf237279f8d71f7e1274, so after the two recovery/ownership fixes and after #97309 is in its final form, recompose the child on the actual landing base and rerun the focused storage/restart suites plus exact-head hosted CI/Docker/Nix.
This is a strong decomposition of a nasty multi-profile data-loss seam, and the mixed-owner split in _prune_stale_sessions_locked is especially well judged. The remaining work is at the two places where historical or unavailable ownership can still fall outside the new canonical route. Once those are closed, the routing half is the right shape. 🚀
alt-glitch
added
type/bug
|
Independent verification of this PR against current Verdict: complete for the recovery half of #66887; applies cleanly to current mainThe triage note on #66887 asked for the fix to cover (a) the startup recovery pass, (b) the unscoped expiry watcher, and (c) the routing-index rewrite ( Live repro: isolated Mechanics checked
Notes for the merge decision (Teknium's call)
No salvage PR opened — the original is merge-ready as-is; opening a duplicate would only strip the contributor of the direct merge. |
|
Thanks @caya8205-2 — this landed on main via #99770, which carried all five of your commits with authorship preserved (fork-branch CI couldn't dispatch here, so the identical series ran on an internal branch), plus two follow-up commits repinning sibling tests to the new single-store contract. Your root-cause and mechanism were exactly right — the routing index now has one home, and the startup recovery pass no longer writes secondary-profile sessions into the ambient store. Closing as superseded by the merged salvage; #66887's recovery half is done. Excellent work. |
4 participants
What does this PR do?
Gives the gateway routing index a single store instead of whichever profile scope happens to be active. This is the second half of #66887 — the recovery half the issue title names — and it is stacked on #97309, so it should land after that one.
_entriesis one flat dict holding every profile's keys, so the index it persists to has to be one file. It was read and written through_db, which resolves the ambientHERMES_HOME. Two consequences:That is exactly why the startup recovery pass never sees a secondary profile's crash marker.
mark_turn_active()persists through the single-entry fast path —state.dbonly, nosessions.jsonmirror — so a marker written during a profile's turn landed in that profile's store, and_recover_unclean_sessions()read a store that had never heard of it. The interrupted turn was silently never promoted toresume_pending.Related Issue
Part of #66887, and the direct answer to the request on that issue to cover the startup recovery pass.
Worth stating explicitly, since it came up there: migrating
recover_interrupted_turns/suspend_recently_active/discard_active_turn_markersto_db_for_key()would have been a no-op. None of them touches a per-session store — each readsentry.active_turn_tokenout of_entriesand persists throughself._save(), andhermes_state.pyhas noactive_turncolumn at all. The marker is a routing-index fact, so this is the patch that governs it.Type of Change
Bug fix — data loss (an interrupted turn on a secondary profile is never resumed).
Changes Made
gateway/session.py_routing_homecaptured in__init__. The store is constructed at startup, before any profile scope exists, which is what makes the index deterministic._routing_dbproperty — the one store that owns the index, whatever scope is active. A pinned handle still wins, so suites that install a fake or disable the DB are unaffected._ensure_loaded_locked,_reconcile_recovered_routing_locked,_persist_routing_dataand_save_entrynow go through it._prune_stale_sessions_lockedis the mixed case and is split accordingly: it asks_db_for_key(key)whether each session ended — a per-session question — while the index write stays on the single store. One ambient handle previously answered it for every profile at once, so a live secondary-profile route could be pruned on the strength of the root store's copy of that session.tests/gateway/test_multiplex_session_db_profile_scope.py— the regression asked for on the issue.How to Test
test_crash_marker_from_a_secondary_profile_survives_restartmarks a turn active under a secondary profile's scope, then builds a fresh store with no scope installed and runsrecover_interrupted_turns().resume_pending=True,resume_reason="restart_interrupted", marker cleared.assert promoted == 1fails with0— nothing is promoted, because the marker is in a store startup never reads.Wider:
test_session,test_session_store_stale_prune,test_session_store_lock_io,test_multiplex_phase0,test_session_store_runtime_stale_guard,test_session_store_expiry_finalized— 126 passed.ruffandcheck-windows-footgunsclean.Tested on: Debian 13 container (Python 3.11,
uv sync --locked --extra all --extra dev), Docker on Windows 10 Pro 19045.As on #97309:
tests/gateway/test_session_store_prune.py::test_session_store_default_db_uses_runtime_hermes_homefails in my environment on pristinemaintoo (the conftest re-pointsDEFAULT_DB_PATH, which wins over the runtimeHERMES_HOMEthe test sets). Unrelated to this change — verified by revertinggateway/session.pyand re-running. The exact-head CI runs still sit ataction_requiredand I cannot clear that from here.What this does not change
_entriesstays a single flat dict — keys are already profile-namespaced, and the #69042 review was right that splitting it is the wrong shape. What changes is only which file the index is persisted to and loaded from. Session rows keep the per-owner targeting from #97309.