investigate(cache): compression-rotation cache-scope gap -- needs maintainer design decision - #79032
Closed
JoaoMarcos44 wants to merge 6 commits into
Closed
Conversation
…ucket sharing _content_cache_key() hashed only the static prefix (instructions + tools), so unrelated sessions sharing a system prompt collapsed onto the same prompt_cache_key routing bucket (NousResearch#78941). Scope the hash by session_id via _cache_scope_from_session_id(), which passes normal session_id through unchanged for isolation, but strips the per-fire timestamp off cron ids (cron_<job>_<YYYYMMDD_HHMMSS>) so repeat fires of the same job still share a stable warm key (preserves NousResearch#51395/NousResearch#52295). Single _content_cache_key/_cache_scope_from_session_id implementation in codex.py, reused by chat_completions.py for both transports.
The session_id/x-client-request-id HTTP headers sent for the Codex backend (cache routing/affinity, confirmed via NousResearch#47335 history and inline comments — not conversation identity) still used the raw session_id, reintroducing the same NousResearch#78941 bug for that header: cron timestamps kept the header cold, and unrelated sessions with the same static content could collide in the same header scope. Reuse the existing _cache_scope_from_session_id() helper (already used for the body's prompt_cache_key) instead of duplicating the cron normalization logic.
…ing key Nous Portal and OpenRouter provider profiles pin every turn of a session to the same upstream endpoint (body["session_id"]) so Anthropic/Vertex/Bedrock cache_control breakpoints stay warm. That key came straight from get_conversation_context() or session_id with no normalization, so cron re-fires (cron_<job_id>_<timestamp>, no parent_session_id to walk) got a fresh key every run and never pinned to the same endpoint -- the same NousResearch#51395/NousResearch#52295 class of bug the original prompt_cache_key fix addressed, just on a route NousResearch#78959 didn't touch. Reuses _cache_scope_from_session_id() (no new logic) and leaves the Portal's conversation= analytics tag and the xAI x-grok-conv-id header untouched, since those need per-fire identity, not cache affinity.
Fixes NousResearch#79012. compression/flush_memories/MoA/session_search calls went through _CodexCompletionsAdapter, which derived prompt_cache_key from instructions+tools only -- no session scope -- reproducing the NousResearch#78941 bucket-sharing bug on this second code path even after the main transport fix in this PR. set_runtime_main() now threads session_id through (turn_context.py passes agent.session_id); the adapter reads it back via _runtime_main_value("session_id") and scopes the key the same way the main transport does (_cache_scope_from_session_id).
…des, pin Grok cron affinity Fixes NousResearch#79013, NousResearch#79014, NousResearch#79015. - session_id header now carries the raw physical session id (NousResearch#57012 contract); x-client-request-id mirrors the body's effective prompt_cache_key instead of both diverging to a bare scope string. - extra_body.prompt_cache_key for xAI Responses now reads back a caller's top-level request_overrides={"prompt_cache_key": ...} instead of always using the auto-derived hash, so an explicit override actually governs the field xAI reads. - x-grok-conv-id (native xAI Responses transport and Grok-via-OpenRouter profile) is now scoped through _cache_scope_from_session_id(), so cron re-fires of the same job pin to the same backend instead of a new one every fire. - Fallback cache_key (when instructions/tools are empty) now falls back to the normalized scope instead of the raw session_id, same class of fix.
…arch#79017) Not a fix -- a demonstration for maintainer review. See NousResearch#79017 for the design discussion on why this needs a logical cache-scope concept distinct from the physical session_id, not a one-line patch.
Contributor
Author
|
Closing to recreate as a standalone PR against main (this one was pulling in #78959's unmerged diff, making it unreadable). New one incoming, self-contained. |
JoaoMarcos44
deleted the
investigate/79017-compression-rotation-cache-scope
branch
August 5, 2026 02:24
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.
Summary
Relates to #79017. Not a fix -- a reproduction, opened for maintainer input on whether/how to pursue it.
Stacked on #78959 -- this diff will look large until #78959 merges (it currently includes those commits because this branch builds on top of them;
_cache_scope_from_session_iddoesn't exist without them). Once #78959 merges, this diff will shrink to the one test file it actually adds._cache_scope_from_session_id()(#78959, closing #78941) scopesprompt_cache_keyby the physicalsession_id. Correct for isolating unrelated sessions and for cron re-fires of the same job. But context-compression rotation mints a new physicalsession_idmid-conversation to segment the transcript, so the same logical conversation goes cache-cold at every rotation boundary.What this PR adds
One
xfail(strict=True)test (tests/agent/transports/test_codex_cache_scope_compression_rotation_gap.py) that fails loudly if the behavior is ever silently "fixed" without anyone noticing, and documents the gap in runnable form instead of prose alone.Why no fix is proposed here
A real fix needs a new concept -- a logical cache-scope (conversation identity) distinct from both the physical session id and provider sticky-routing keys:
/new, branch, compression rotation/new, branch, independent subagent -- not rotationsession_id, xAIx-grok-conv-id)Threading that scope through the compression/rotation/branch code paths (and NOT reusing
gateway_session_keyor a generic conversation-root walk, both of which outlive/newor cross branch boundaries incorrectly) is a design task, not a diff this PR wants to force through unreviewed.Ask
Maintainer call on:
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A[Conversation Turn 1<br/>session-root] -->|Compression Threshold Hit| B[Rotation Event] B --> C[Conversation Turn 2<br/>session-rotated] A -->|scope = session-root| D[Cache Bucket: pck_root_hash] C -->|scope = session-rotated| E[Cache Bucket: pck_rotated_hash] D -. Same Logical Conversation .-> E D -->|Rotation Boundary| F[Cold Cache: Warm Prefix Lost] E --> F style F fill:#8b0000,stroke:#ff0038,color:#ffccd5 style D fill:#3a0000,stroke:#ff0038,color:#ffccd5 style E fill:#3a0000,stroke:#ff0038,color:#ffccd5Test plan
pytest tests/agent/transports/test_codex_cache_scope_compression_rotation_gap.py -v-> 1 xfailed (expected; strict, will error if it starts passing without a matching design)Relates to #79017 (design decision, not code fix).