fix(cache): mix conversation epoch into declared scope for /new rotation (#96811) - #98795
fix(cache): mix conversation epoch into declared scope for /new rotation (#96811)#98795StanleyStetson wants to merge 3 commits into
Conversation
…key path Every conversation-affinity hint Hermes sends is derived from the PHYSICAL session id: prompt_cache_key on both OpenAI-wire transports, OpenRouter's and Nous Portal's sticky session_id, and xAI's x-grok-conv-id. A host that mints one physical session per RESPONSE re-keys all four on every reply, so the conversation never lands back on the routing bucket it just warmed (NousResearch#96811). Two hosts do exactly that. Hermes Studio's group chat mints gc_run_<room>_<profile>_<name>_<uuid4hex> per reply and destroys it after, and POST /v1/responses with client-managed history mints str(uuid4()) per request -- while parsing X-Hermes-Session-Key one screen earlier and handing it to the agent. Hermes must not infer the logical conversation from the id's syntax: that rule merges independent client-supplied ids and Studio members truncated past its 96-character boundary (the NousResearch#79017 failure class). It does not have to. gateway_session_key is already the stable per-chat key built by gateway.session.build_session_key from that header, and branching deliberately does not key off it. The affinity path simply never consulted it. - agent/prompt_cache_scope.py: declared_conversation_scope() resolves the key into gwk_<sha256[:24]> and outranks the lineage walk (it is stable across rotation AND across per-response ids). Hashed because, unlike a session id, the key embeds platform/chat/user identifiers and leaves the process verbatim as a sticky id and as x-grok-conv-id. - agent/portal_tags.py: a separate ambient scope for ROUTING, published only when a host declared one. The providers read the attribution id when it is unset, so delegate trees keep sharing their parent's sticky key and every host that keeps one id per conversation is byte-identical to before. - hermes_state.py: is_explicit_fork_child() -- the public view of the marker rules that keep /branch children, delegate subagents and tool children off their parent's chat key. Background-review forks clone the live runtime, so _persist_disabled excludes them for the same reason (NousResearch#79161). Refs NousResearch#96570 Fixes NousResearch#96811
The turn-lease timeout/interrupt paths return from inside the try block before set_affinity_scope() runs; the finally then read an unassigned local -> UnboundLocalError. This was the cause of the 4 red cross-process lease tests on PR NousResearch#97158's CI.
…ion (NousResearch#96811) Unifies conversation epoch rotation on top of the host-declared conversation affinity scoping from NousResearch#97158 / NousResearch#97709, resolving the /new lifetime and memoization invariants without hot-path regressions: - Keeps declared_conversation_scope() strictly INSIDE the memoized path in resolve_prompt_cache_scope() so no SQLite queries occur on the hot path - Mixes conversation_epoch into the declared scope as gwk_<sha256(key:epoch)[:24]> (bare key for epoch=1 for backward-compatible stable hashes) - Includes epoch in the prompt cache scope memo key (sid, db_present, epoch) so /new invalidates the cached scope on the same session key - Increments conversation_epoch in SessionStore.reset_session() (/new) and on idle/daily auto-resets, preventing ABA scope rollback - Adds public get_conversation_epoch() helper to SessionStore - Threads conversation_epoch through TurnContext, agent_init, and run_agent - Adds 10 new unit/integration tests for epoch rotation, memo invalidation, ABA monotonicity, SessionStore persistence, and live turn nested scope isolation Refs NousResearch#96811, NousResearch#97158, NousResearch#97709
teknium1
left a comment
There was a problem hiding this comment.
Review of head 4a6db3cda665aff33d87e07912bf2d3fa15af375
Reviewed against current origin/main (d63f996). Premise verified: /v1/responses with client-managed history mints str(uuid4()) per request, api_server keys _last_resolved_model off gateway_session_key for exactly this reason (api_server.py:3069-3075), and on main resolve_prompt_cache_scope has no way to see a host-declared conversation — so all four affinity surfaces (prompt_cache_key on both wire transports, OpenRouter/Nous sticky session_id, x-grok-conv-id) churn per reply. The bug is real and this PR's core mechanism (declared gwk_<sha256(key[:epoch])[:24]> scope, contextvar publication in portal_tags, precedence over the lineage walk) is sound and well-commented.
Ran the PR's test files in an isolated worktree under a memory cap: tests/agent/test_declared_conversation_scope.py 31/31 passed, tests/agent/test_prompt_cache_scope.py 33/33 passed. Test quality is good — they assert real wire-layer outputs (build_kwargs()["prompt_cache_key"], build_extra_body()["session_id"], extra_headers["x-grok-conv-id"]), fork/_persist_disabled boundaries, memo invalidation, and a live AIAgent.run_conversation contextvar shadowing test — not restatements of the gate expression.
What holds up:
- Fork isolation —
declared_conversation_scope()(agent/prompt_cache_scope.py:139-177) correctly bails on_persist_disabled, onis_explicit_fork_child, and degrades to physical scope on DB error rather than risking a fork merging onto its parent's key.hermes_state.is_explicit_fork_childis a clean read-only wrapper over the existing row-marker rules. - Memoization —
declared_conversation_scope()runs only inside the miss path ofresolve_prompt_cache_scope(); memo key now(sid, db_present, epoch)so/newinvalidates. After the first resolution per segment there are zero SQLite queries per turn. Claim verified in code. - No-declaration hosts — CLI/desktop paths never set
_gateway_session_key;set_affinity_scope(None)is a published-None, and all three provider call sites fall back toget_conversation_context() or session_id— byte-identical to pre-PR behavior. Confirmed by testtest_without_a_declaration_the_conversation_id_still_wins. run_conversationearly-return safety —affinity_token = Noneinitialized alongsidetokenbefore the turn-lease early returns (run_agent.py, the #97158 UnboundLocalError class); compression path only sets the scope when unset.
Blockers
-
Epoch is not durable — retired-scope reuse (ABA) through prune/recovery/restart.
conversation_epochlives only onSessionEntryin sessions.json. Three paths roll it back to 1 and re-issue a retired affinity identity:SessionStore.prune_old_entries(gateway/session.py:~3394) pops the entry; the nextget_or_create_sessionfor that key creates a fresh entry withconversation_epoch=1, whose hash (epoch-1 omits the:epochsuffix) equals the ORIGINAL epoch-1 conversation'sgwk_scope — a scope retired at the first/new.- The recovery path (
_recover_session_for_peerreopeningagent_close/ws_orphan_reaprows) rebuilds entries from state.db, which stores no epoch — recovered entries come back at epoch 1. - Any sessions.json loss/corruption resets every chat's epoch while state.db transcripts survive.
The prior review rounds on #97158/#97709 demanded a generation that provably never reuses a retired value; this implementation cannot prove non-reuse.test_mixed_new_and_auto_reset_never_rolls_back_epoch_abaonly covers in-memory monotonicity within one live entry — none of the three paths above are tested. (Competing #98811 keeps the generation in a durableconversation_generationscounter in state.db advanced transactionally with each boundary, which is the correct shape for this.)
-
No rotation on the host the issue is about. The epoch plumbing (
TurnContext.conversation_epoch, gateway/run.py:5858/5912/29549) only reaches native gateway platforms — which keep one session_id per chat and were never churning.gateway/platforms/api_server.py(the/v1/responses+ Studio group-chat host, i.e. the actual #96811 reproducer) constructsAIAgentwithgateway_session_keybut no epoch source;_gateway_conversation_epochstays 1 forever. Result: per-response churn is fixed there (good), but a client starting a NEW conversation on the sameX-Hermes-Session-Keystays pinned warm to the previous conversation's affinity bucket indefinitely — violating the cold-across-conversation half of the contract precisely where the bug manifests. There is no api_server test in this PR. -
api_server identity fragmentation untouched. This PR stabilizes the affinity hash only; the per-request session rows themselves remain unlinked (no session_key bind, no reset-fenced reuse), so transcript/identity fragmentation on the api_server path persists. #98811 carries that repair (
_declared_conversation_session/_bind_declared_conversationwith reset-fenced recovery mirroringSessionStore._recover_session_for_peer); #98795 has no equivalent.
Nits
- gateway/run.py:29549-29558 inlines the epoch lookup (with a
_entriesreach-in fallback) into the ~29k-line turn-assembly path. If the epoch mechanism survives, this belongs onSessionStoreonly (get_conversation_epochalready exists — the private-dict fallback duplicates it) and adds authority to a god-file the competing design avoids entirely. - Deploy-time one-time scope flip: on upgrade, every in-flight native-gateway conversation moves from physical-id scope to
gwk_— one full prompt-cache invalidation per active conversation. Between conversations this is fine; worth stating in the PR body as an accepted cost. - Fork-child pre-row window:
test_declaration_applies_before_the_row_landsasserts the declared scope applies before_ensure_db_sessionpersists the row. For a fork child whose row hasn't landed yet,is_explicit_fork_childreturns False (missing row ≠ fork) and the parent's declared scope is memoized for the segment (root is not None→ memoized). Low likelihood today (branch/delegate rows are created eagerly, delegates don't carry the key), but nothing pins it.
Overlap with #98811
Same issue, same lineage (#97158 → #97709), directly conflicting hunks in agent/portal_tags.py, agent/prompt_cache_scope.py, hermes_state.py, both provider plugins, run_agent.py, and the same test filename — only one can land. #98811's mechanism (durable boundary counter in state.db + api_server-layer session rebinding) addresses blockers 1–3 above by construction, has been through four review rounds with blockers addressed, and confines its changes to prompt_cache_scope/api_server/hermes_state without adding plumbing to gateway/run.py, gateway/session.py, turn_context.py, or the init_agent signature. #98795's contextvar + run_conversation publication is the cleaner way to reach the aux-call sites, but its generation mechanism is the weaker of the two.
Verdict: REQUEST CHANGES. The declared-scope core is solid and the tests are real, but the epoch is not durable (blocker 1), never rotates on the api_server host the issue is about (blocker 2), and the identity repair is missing (blocker 3). As written, #98811 is the stronger landing vehicle for the generation scheme; if the maintainers prefer this PR's plumbing style, it needs a durable generation source and api_server coverage — at which point it converges on #98811's design.
|
Thank you to everyone who reviewed this PR. I've been following the discussion on #98811 closely, and after reading the maintainer review there, I'm convinced @JoaoMarcos44's approach is the stronger one to carry this fix to production. #98811 not only addresses the same issue (#96811) but resolves it from a place that better survives the real failure modes — a durable generation counter in the database rather than per-chat state in Since both PRs solve the same problem and overlap across the same files, only one of them should merge. I'm closing this one in favor of #98811. I'm grateful for the time and review feedback here — it helped me understand the tradeoffs much better. Let me know if I can help in any way with #98811. |
Summary
Salvage and revision of #97158 and #97709 by @JoaoMarcos44 and @kshitijk4poor onto current
main, preserving original authorship via cherry-pick.Resolves #96811: hosts that mint per-response session IDs (Hermes Studio group chat,
POST /v1/responseswith client-managed history) re-keyed all conversation affinity surfaces on every reply (prompt_cache_key, OpenRouter/Nous stickysession_id, andx-grok-conv-id).This PR implements the full key +
conversation_epochcontract requested in the reviews of #97158 and #97709:gwk_<sha256(key:epoch)[:24]>fromgateway_session_key(X-Hermes-Session-Key/build_session_key), keeping per-response replies on the warmed cache bucket./newRotation: Mixesconversation_epochinto the hash (advancing on explicit/newinSessionStore.reset_session()and monotonically on idle/daily policy auto-resets inget_or_create_session), ensuring that post-reset conversations rotate to fresh provider affinity buckets./branch, delegate subagents, tool-spawned sessions) and background review forks (_persist_disabled) bypass the declared key and retain their isolated physical scope.declared_conversation_scope()strictly inside the memoization miss path ofresolve_prompt_cache_scope(), ensuring zero SQLite queries on subsequent calls within the turn.gwk_<hash>scope on deployment.Changes
agent/portal_tags.py: Ambient affinity scopeContextVar, set per turn and compacted session.agent/prompt_cache_scope.py:declared_conversation_scope()mixingkey:epochintogwk_, memoized under(session_id, db_present, epoch).gateway/session.py:conversation_epochfield onSessionEntry,get_conversation_epoch()onSessionStore, incremented onreset_session()and monotonic auto-resets.gateway/turn_context.py: Addedconversation_epochtoTurnContext.agent/agent_init.py&run_agent.py: Plumbedgateway_conversation_epochthroughinit_agenttoagent._gateway_conversation_epoch.gateway/run.py: Populatedconversation_epochinTurnContextand passed toAIAgent.hermes_state.py: Publicis_explicit_fork_child(session_id)helper.tests/agent/test_declared_conversation_scope.py: 31 tests covering stability across per-response IDs, epoch advance on/new, memo invalidation, ABA monotonicity, and disk persistence reload.Validation
test_declared_conversation_scope.py(31 passed) andtest_prompt_cache_scope.py(33 passed).test_turn_lease.pyandtest_durable_turn_lease.py.SessionDB.get_session(0 additional queries on repeated calls).Closes #96811. Refs #96570, #97158, #97709.