fix(cache): honor host-declared session key and conversation epoch for prompt cache affinity - #98170
fix(cache): honor host-declared session key and conversation epoch for prompt cache affinity#98170StanleyStetson wants to merge 1 commit into
Conversation
Context & ArchitectureThis PR unifies and finalizes the solution for the conversation-affinity caching issue described in #96811, building on the initial work from #97158 and #97709. Problem & Prior Blockers
Implemented Solution
|
3d03901 to
e347fe5
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head e347fe5436fc8a33d19382217177af6e4fd4b0f1 against its exact parent/current main@4209d371aa1bb8840ce8447555bdd863a1a96c38. This is a materially stronger candidate than #97158/#97709: the added conversation_epoch directly answers the previously demonstrated /new lifetime bug, the fork/background-review exclusions remain intact, and the provider ContextVar propagation is covered end-to-end. Exact-head CI is also genuinely green: CI 33279919278, Docker 33279918736, and Nix 33279918716 all completed successfully; the CI run includes Python tests, e2e, Windows/macOS, blocking ruff, Windows footguns, attribution, and All required checks pass.
I found one remaining generation bug in the other reset path, plus two landing/topology gates.
P1 — idle/daily policy resets do not advance the conversation generation, and can ABA back to epoch 1
The new generation is advanced only in the explicit SessionStore.reset_session() path:
gateway/session.pyaddsSessionEntry.conversation_epoch = 1and persists it;- the
reset_session()hunk constructs the successor withold_entry.conversation_epoch + 1; declared_conversation_scope()then hashes(gateway_session_key, conversation_epoch).
That fixes the exact /new defect raised on #97158/#97709.
But SessionStore.get_or_create_session() has a second logical-conversation reset path. _should_reset() returns idle/daily; when that fires, get_or_create_session() ends/removes the predecessor and later constructs a fresh SessionEntry(... was_auto_reset=True, auto_reset_reason=..., prev_session_id=...). That successor construction does not carry conversation_epoch, so this PR's dataclass default silently assigns 1.
That produces two bad reachable histories under one stable gateway key K:
conversation A: epoch 1 -> idle/daily reset -> conversation B: epoch 1
same gwk(K), despite a fresh conversation
conversation A: epoch 1 -> /new -> conversation B: epoch 2
conversation B: epoch 2 -> idle/daily reset -> conversation C: epoch 1
reuses A's old generation (ABA)
This is not just a cache-performance preference. Main already treats the policy reset as a conversation boundary: tests/gateway/test_48031_model_switch_after_auto_reset.py explicitly covers the first message after an idle/daily/suspended auto-reset and requires conversation-scope cleanup, while #79017/#86733's accepted cache-scope contract is stable across continuation/compression but fresh across a new conversation.
Required repair: make the epoch a monotonic successor generation for every path that replaces one logical conversation with another, not a special field increment inside only /new. The policy-reset constructor should derive the successor from the predecessor's generation before the old entry is discarded. Please pin at least these negative controls on a real SessionStore:
- epoch 1 -> idle/daily reset -> epoch 2 and a different
gwk_scope; - epoch 1 ->
/new-> epoch 2 -> idle/daily reset -> epoch 3, never back to 1; - persistence/reload preserves the current epoch before the next reset, so restart cannot recreate an old affinity generation.
The class rule here is the same one the previous /new review exposed: a routing coordinate may stay stable, but conversation affinity must be fenced by a monotonic generation that cannot roll back or be reused.
Landing blocker — preserve the #97158/#97709 origin chain instead of flattening it
This PR is not an independent implementation. Its FILE-LIST contains all seven files from #97158 and #97709 (portal_tags.py, prompt_cache_scope.py, hermes_state.py, both provider plugins, run_agent.py, and the declared-scope test), then adds the epoch projection in four more files.
That history matters because #97709 deliberately preserved JoaoMarcos44's original authored implementation commit and kept kshitijk4poor's affinity_token cleanup repair as a separate authored commit. The #97709 thread then identified the missing per-conversation epoch; this PR implements exactly that next refinement. Current #98170 collapses the whole chain into one StanleyStetson-authored commit even though its discussion correctly says it builds on both prior PRs.
Please preserve that lineage in the landing object: source implementation from JoaoMarcos44 (#97158), cleanup repair from kshitijk4poor (#97709), then this epoch refinement from StanleyStetson. If history has to be recomposed on current main, the equivalent provenance must still be explicit in commit/PR attribution. #98170 is best understood as the broader superseding candidate once the generation bug above is fixed; #97158 is the source implementation and #97709 is the salvage/cleanup refinement, not disposable duplicate work.
Because all three collide on the same seven files, only one should land. Once the final object is correct, the other two can be closed as superseded with their contributions named rather than silently orphaned.
Structural gate — this adds new authority to three existing godfiles
The new behavior lands inside gateway/run.py (>29k lines), hermes_state.py (>13k), and run_agent.py (>9k). That violates the repository's 2K invariant for modified files. More importantly for this particular change, gateway/run.py now reads session_store._entries directly to project conversation_epoch into TurnContext, so the generation lookup itself is outside the store's owned abstraction/lock boundary.
The narrow shape I would land is a bounded session-affinity/session-generation seam that owns: successor generation, safe lookup, fork classification, and turn projection. gateway/run.py should consume a public value, not reach into the private _entries map; hermes_state.py should not grow another public classifier in place if the session-lineage seam can own it. That extraction also gives the idle/daily fix one authoritative place instead of duplicating generation behavior across reset call sites.
Interlock / merge order
- #96811 is the live P0 this candidate is intended to close.
- #79017 / merged #86733 define the existing cache-scope boundary: continuation/compression inherits; new conversations/forks isolate.
- #97158 is the original host-declared-affinity implementation by JoaoMarcos44.
- #97709 is the salvage by kshitijk4poor that preserves that authorship, fixes the cleanup regression, and explicitly identifies the per-conversation epoch as the remaining design requirement.
- #98170 is therefore a complementary refinement and the natural superseder, provided it closes the policy-reset generation path and lands with the origin chain intact.
The exact-head green receipts are strong, and the epoch direction is the right answer to the earlier /new blocker. The remaining runtime defect is narrow but important: generation must advance at every conversation replacement, otherwise the new proof object can roll backward and reuse a prior affinity identity. Fix that, preserve the contributor lineage, extract the owning seam, and this becomes a much cleaner closure for #96811. 🚀
e347fe5 to
4e48356
Compare
|
Thank you @andrexibiza for the thorough review and clear guidance! All three items have been implemented, verified, and pushed to the branch ( 1. P1: Monotonic Generation on Idle/Daily Auto-Resets (ABA Prevention)
2. SessionStore & Structural Seam (Godfile Invariant)
3. Provenance & Co-Authorship Lineage
Validation
|
…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
4e48356 to
9ab4db7
Compare
|
This is a serious attempt at exactly the design gap flagged in the #97158 review (declared key + epoch so the scope survives per-response ids but rotates on /new) — and the epoch direction is right. Closing this particular implementation on three verified defects rather than carrying it as an open P0:
The epoch concept itself is now part of the design discussion on #97158, where the lifecycle question is being settled; the eventual revision will land through that thread (with #97709 as the salvage vehicle). Thanks — the ABA-monotonicity tests here are a genuinely useful checklist for that revision. |
|
@kshitijk4poor Thank you for the detailed review and for highlighting On the authorship point: my goal was to contribute and Continuing the discussion in #97158 as suggested. |
Summary
Unifies and finalizes host-declared conversation affinity caching across all supported transports (OpenAI, Codex, OpenRouter, Nous, Grok), resolving the per-response session churn described in #96811 while building on the foundational work from #97158 and #97709:
un_agent.py, including explicit \None-shadowing on nested child turns.
Attribution & Lineage
This PR serves as the superseding candidate incorporating:
Validation
Type of Change