fix(cache): honor the host-declared conversation key on the affinity-key path - #97158
fix(cache): honor the host-declared conversation key on the affinity-key path#97158JoaoMarcos44 wants to merge 1 commit 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
|
Thanks for isolating a real Hermes-native reproduction for the affinity-key churn. The I reviewed exact PR head 1.
|
| Boundary | current main | PR head | current main + PR |
|---|---|---|---|
| Consecutive Responses requests keep all affinity values | no | yes | yes |
/new rotates all affinity values |
yes | no | no |
For consecutive Responses requests, the PR correctly kept all six values stable:
- Chat Completions
prompt_cache_key; - Responses API
prompt_cache_key; - OpenRouter sticky
session_id; - Nous sticky
session_id; x-grok-conv-id; and- the internal resolved scope.
That is a direct positive witness for the #96811 fix. However, the same six values also remained identical across /new, even though the physical session ID rotated. This conflicts with the cache-scope contract established in #79017 and implemented by #86733: continuity survives compression rotation, but the scope resets on /new, /branch, and other independent-conversation boundaries.
This also makes the blast radius broader than the PR description suggests. Every ordinary native gateway supplies this key, so normal messaging conversations would move from physical/lineage affinity to per-chat affinity—not only hosts that mint one ID per response.
There is related work around the /v1/responses conversation identity in #47294, #41209, and #16517. Those are not duplicates of #96811: they cover namespacing, ownership, and durable continuity rather than provider affinity, but they may contain a semantic owner closer to a logical conversation than the deliberately cross-transcript gateway key. Conversely, #71556/#71608 use gateway_session_key for Langfuse grouping, where per-channel observability is a different and intentionally broader contract.
This is not future hardening beyond #96811. It is a negative control on the lifetime of the identity carrier introduced by this PR. A safe contract would need to remain stable across per-response physical IDs while rotating at /new and preserving the existing branch/delegate isolation.
2. The current head has a reproducible cleanup regression
The hosted Python test job is red, and I reproduced the affected selection locally:
4 failed, 9 passed
All four failures reach the outer cleanup before affinity_token has been assigned. The variable is created only after durable-lease, Relay, and task-start operations that may return or raise, but the outer finally reads it unconditionally at run_agent.py:9092, producing:
UnboundLocalError: cannot access local variable 'affinity_token'
where it is not associated with a value
This is attributable to the candidate change rather than CI infrastructure.
For balance, the new focused suite passes locally:
tests/agent/test_declared_conversation_scope.py: 22 passed
The adjacent prompt-cache, Portal-tag, and API-server selections also pass:
141 passed
Those green results validate the added helper and provider wiring; they do not negate the early-exit regression.
Real-path reproduction package
The complete witness, exact revisions, summarized outputs, instructions, and integrity hash are here:
https://gist.github.com/cervantesh/1fa5099000d0163478d5fb6a6a325b47
It makes zero model-provider network calls. It uses the real Responses handler and real /new handler, replacing only model execution with a deterministic capture. The resulting identities are then passed through the repository's actual resolver, OpenAI-wire transports, and OpenRouter/Nous provider profiles. The result was repeated and remained the same after integrating the PR onto current main.
Assessment
My current assessment is REQUEST CHANGES, for two strict reasons:
- the selected identity demonstrably crosses
/new, violating an existing isolation invariant; and - the exact PR head has a reproducible cleanup regression.
The general direction—letting a host declare a semantic affinity owner instead of inferring identity from UUID syntax—still looks sound. Evidence that would change this assessment would be:
- a declaration or derivation whose lifecycle is explicitly per logical conversation rather than per channel;
- a negative control proving
/newreceives a distinct scope; - the real-path witness remaining GREEN for consecutive per-response requests; and
- green required checks on the resulting exact head.
Studio can remain a separately acknowledged integration step because it still needs to supply whichever per-conversation declaration is adopted.
…r prompt cache affinity (NousResearch#96811) Unifies and finalizes host-declared conversation affinity caching across all supported transports (OpenAI, Codex, OpenRouter, Nous, Grok), building upon initial work in NousResearch#97158 and NousResearch#97709: - Consumes host-declared gateway_session_key and mixes conversation_epoch into gwk_<sha256(key:epoch)[:24]> - Advances conversation_epoch on explicit /new (SessionStore.reset_session) - Advances conversation_epoch monotonically on idle/daily policy auto-resets in get_or_create_session (preventing ABA rollback) - Adds public get_conversation_epoch() helper to SessionStore abstraction - Preserves physical-id isolation for /branch children, delegate subagents, tool-spawned sessions, and background-review forks (_persist_disabled) - Ensures safe _affinity_scope ContextVar propagation and explicit None-shadowing for nested child turns with safe try/finally - Adds 21 unit and integration tests covering stability, epoch rotation, fork exclusions, and persistence reload Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com> Co-authored-by: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Refs NousResearch#96811, NousResearch#97158, NousResearch#97709
|
Following the review feedback on #98170 and the discussion on #97158 / #97709, I put together an exploratory revision branch built on top of this salvage (preserving @JoaoMarcos44's and @kshitijk4poor's commits via cherry-pick). Sharing here in case any of these pieces are helpful for the salvage effort:
The branch and commit are available here if you'd like to inspect, cherry-pick, or adapt anything from it: |
|
Closing this revision while keeping the bug (#96811) open — the mechanism is real and your reproduction package proved it, but the carrier as implemented has a verified lifecycle violation that can't be fixed by a follow-up commit on this branch. State of the review, consolidated:
Your diagnosis, the four-surface consumer map, the |
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. (cherry picked from commit 05a3c8a)
The declared key is a per-CHAT identifier and outlives the conversation it names: reset_session() mints a fresh physical id on /new but keeps the key, and the idle/daily/suspended policy resets do the same. Hashing the key alone therefore mapped the conversation before a reset and the one after it onto one gwk_ scope -- the lifecycle violation @cervantesh raised on NousResearch#97158 and @kshitijk4poor reproduced on NousResearch#97709. No counter is introduced. The generation that must rotate is already durable: every one of those boundaries closes the outgoing row with an _RESET_END_REASONS end_reason, so SessionDB.latest_conversation_boundary reads the most recent one and declared_conversation_scope hashes 'key|generation'. That makes the carrier stable across a host's per-response physical ids -- a host that never resets writes no boundary, so every reply hashes the same value -- while rotating on every conversation replacement, /new and the policy auto-resets alike. ended_at only moves forward, so a retired generation can never be reused: no ABA. It also cannot drift from the rest of the codebase's notion of a conversation boundary, because find_latest_gateway_session_for_peer fences on the same set. The read is on the memoized resolution path, not per API call, and both lookups fail closed: an unqualified key would span a /new, so a DB error degrades to the physical-id scope. A SessionDB without the lookup keeps the previous behaviour. Refs NousResearch#96811
The declared key is a per-CHAT identifier and outlives the conversation it names: reset_session() mints a fresh physical id on /new but keeps the key, and the idle/daily/suspended policy resets do the same. Hashing the key alone therefore mapped the conversation before a reset and the one after it onto one gwk_ scope -- the lifecycle violation @cervantesh raised on #97158 and @kshitijk4poor reproduced on #97709. No counter is introduced. The generation that must rotate is already durable: every one of those boundaries closes the outgoing row with an _RESET_END_REASONS end_reason, so SessionDB.latest_conversation_boundary reads the most recent one and declared_conversation_scope hashes 'key|generation'. That makes the carrier stable across a host's per-response physical ids -- a host that never resets writes no boundary, so every reply hashes the same value -- while rotating on every conversation replacement, /new and the policy auto-resets alike. ended_at only moves forward, so a retired generation can never be reused: no ABA. It also cannot drift from the rest of the codebase's notion of a conversation boundary, because find_latest_gateway_session_for_peer fences on the same set. The read is on the memoized resolution path, not per API call, and both lookups fail closed: an unqualified key would span a /new, so a DB error degrades to the physical-id scope. A SessionDB without the lookup keeps the previous behaviour. Refs #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. (cherry picked from commit 05a3c8a)
The declared key is a per-CHAT identifier and outlives the conversation it names: reset_session() mints a fresh physical id on /new but keeps the key, and the idle/daily/suspended policy resets do the same. Hashing the key alone therefore mapped the conversation before a reset and the one after it onto one gwk_ scope -- the lifecycle violation @cervantesh raised on NousResearch#97158 and @kshitijk4poor reproduced on NousResearch#97709. No counter is introduced. The generation that must rotate is already durable: every one of those boundaries closes the outgoing row with an _RESET_END_REASONS end_reason, so SessionDB.latest_conversation_boundary reads the most recent one and declared_conversation_scope hashes 'key|generation'. That makes the carrier stable across a host's per-response physical ids -- a host that never resets writes no boundary, so every reply hashes the same value -- while rotating on every conversation replacement, /new and the policy auto-resets alike. ended_at only moves forward, so a retired generation can never be reused: no ABA. It also cannot drift from the rest of the codebase's notion of a conversation boundary, because find_latest_gateway_session_for_peer fences on the same set. The read is on the memoized resolution path, not per API call, and both lookups fail closed: an unqualified key would span a /new, so a DB error degrades to the physical-id scope. A SessionDB without the lookup keeps the previous behaviour. Refs NousResearch#96811
What does this PR do?
Lets the host declare the logical conversation behind its physical session ids, and makes the four conversation-affinity surfaces consult that declaration. This is the fix for the key churn demonstrated in #96811 — the mechanism behind the cost half of #96570.
Root cause
Every affinity hint Hermes sends is derived from the physical session id:
agent/transports/codex.py,agent/transports/chat_completions.pyprompt_cache_keyresolve_prompt_cache_scope()→ compression-lineage root → physical idplugins/model-providers/openrouterbody.session_id(sticky routing)get_conversation_root()→ physical idplugins/model-providers/nousbody.session_id(sticky routing)plugins/model-providers/openrouterx-grok-conv-idheaderA host that mints one physical session per response therefore re-keys all four on every reply, and the conversation never lands back on the routing bucket it just warmed. Two hosts do exactly that:
groupRuntimeSessionId()mintsgc_run_<room>_<profile>_<name>_<uuid4hex>per reply and destroys the session afterwards.POST /v1/responseswith client-managed history —session_id = stored_session_id or str(uuid.uuid4())ingateway/platforms/api_server.py. Withoutprevious_response_idthat is a fresh id per request, even though the same handler parsedX-Hermes-Session-Keya few lines earlier and passes it straight to_create_agent(gateway_session_key=…).Case 2 closes the whole loop inside this repo: the stable identity is already in hand, already on the agent — the affinity path just never consulted it.
Why not normalize the id
Stripping a per-run-looking suffix is not identity-safe, and #96768 already carries both negative controls as tests:
POST /v1/sessionspreserves a client-supplied id verbatim (two independent conversations may differ only in a trailing 32-hex identity), and Studio truncates its semantic prefix to 96 characters before appending the token (two members of one room can end up distinguished by that token alone). Same failure class as #79017: syntactic inference truncates legitimate identities and collides distinct owners.The declaration is semantic instead, and the carrier already exists:
gateway_session_key, the "stable per-chat key" (agent:main:telegram:dm:123) built bygateway.session.build_session_key()fromX-Hermes-Session-Key. Its own documentation notes that branching keys offsession_id, not this slot — exactly the property a conversation scope needs.agent/prompt_cache_scope.pysimply did not read it. (AIAgent.__init__already accepts it, andrun_agent.py:741already treats it as the context engine'sconversation_id.)Changes
agent/prompt_cache_scope.py—declared_conversation_scope()resolves the key togwk_<sha256[:24]>and outranks the lineage walk: it is stable across compression rotation and across per-response ids, which the walk cannot see. Hashed because — unlike a session id — the key embeds platform/chat/user identifiers and leaves the process verbatim as OpenRouter's stickysession_idand asx-grok-conv-id; hashing also keeps it inside every consumer's length budget (28 chars).agent/portal_tags.py— a second ambient scope for routing, kept separate from the conversation id, which is an attribution value (conversation=<id>on Portal requests). The two agree for any host that keeps one id per conversation; they diverge only for a per-response host. It is published only when a host declared something, so unset ⇒ providers fall back to the attribution id exactly as before: delegate trees keep sharing their parent's sticky key and Portal attribution is untouched.hermes_state.py—is_explicit_fork_child(), the public view of_is_explicit_fork_child_row()'s marker rules, so the fork boundary is enforced without re-implementing it.run_agent.py— publishes the declared routing scope beside the existing conversation context (turn entry, and the out-of-turn compaction forwarder), with symmetric resets.get_affinity_scope() or get_conversation_context() or session_id.Isolation the declaration must not cross (#79161)
/branchchildren, delegate subagents and tool children share their parent's chat key but are separate conversations — the session row's fork markers keep them on their own scope. Background-review forks clone the live runtime (key included), so_persist_disabledexcludes them. A DB error during that check degrades to the physical-id scope rather than risking a merge.Blast radius
Zero wire change for any agent without a declared key: no key ⇒
declared_conversation_scope()returnsNoneand every path resolves exactly as onmain.POST /v1/responses+X-Hermes-Session-Keyis fixed with no host change. Studio group chat needs one kwarg on its side (AIAgent(..., gateway_session_key=<stable per-room-member key>)), since its bridge constructs the agent directly; its truncate-before-nonce ordering is a separate producer-side bug worth fixing there too.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A[Reply N - gc_run_room_member_uuidA] --> R{Scope Resolver} B[Reply N+1 - gc_run_room_member_uuidB] --> R R -->|Declared chat key present| K[Hashed Logical Scope - gwk_sha256] R -->|No declaration| P[Physical Id / Lineage Root - unchanged] R -->|Branch, Delegate, Tool child| F[Own Scope - fork isolation kept] R -->|Background review fork| F K --> S1[prompt_cache_key] K --> S2[OpenRouter sticky session_id] K --> S3[Nous Portal sticky key] K --> S4[x-grok-conv-id] S1 --> W[One Warm Routing Bucket across every reply] S2 --> W S3 --> W S4 --> WRelated Issue
Fixes #96811
fixes #96570
Type of Change
How to Test
Local results: 22 passed (new file), 433 passed / 1 failed across the adjacent suites, 200 passed (
tests/hermes_state). The single failure —tests/agent/test_portal_tags.py::test_compress_context_preserves_ambient_context— reproduces identically on unmodifiedmainin this environment (AttributeError: 'DaemonThreadPoolExecutor' object has no attribute '_initializer', a Python 3.14concurrent.futureschange surfacing intools/daemon_pool.py), so it is not caused by this branch.ruff checkclean on all changed files.New coverage in
tests/agent/test_declared_conversation_scope.py: two per-response ids resolving to one scope; distinct declarations staying isolated; the raw key never appearing in the scope; no-declaration behavior identical to the lineage path;/branch, delegate, tool and background-review forks ignoring the declaration; the fork-check DB failure degrading safely;prompt_cache_keystability on both transports; stickysession_idandx-grok-conv-idfollowing the declared scope while an undeclared turn still resolves through the conversation id.