Skip to content

investigate(cache): compression-rotation cache-scope gap -- needs maintainer design decision - #79036

Closed
JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:investigate/79017-standalone
Closed

JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:investigate/79017-standalone

Conversation

@JoaoMarcos44

Copy link
Copy Markdown
Contributor

Summary

Relates to #79017. Not a fix -- a standalone, self-contained reproduction for maintainer input on whether/how to pursue it. One file, no dependency on #78959's merge state (that PR introduces the actual scoping code this test is about; this file inlines a minimal reference copy purely so it's reviewable on main today).

_cache_scope_from_session_id() (proposed in #78959, closing #78941) scopes prompt_cache_key by the physical session_id. Correct for isolating unrelated sessions and for cron re-fires of the same job. But context-compression rotation mints a new physical session_id mid-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/design/test_cache_scope_compression_rotation_gap.py) that documents the gap in runnable form and fails loudly if anyone tries to close it without a matching design.

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:

Identity Role Changes on
Physical session/transcript id One execution/segment /new, branch, compression rotation
Logical cache-scope (proposed) Stable prompt-cache bucket /new, branch, independent subagent -- not rotation
Provider sticky-routing (OpenRouter session_id, xAI x-grok-conv-id) Pins requests to the warm backend Per-provider contract

Threading that scope through the compression/rotation/branch code paths (and NOT reusing gateway_session_key or a generic conversation-root walk, both of which outlive /new or cross branch boundaries incorrectly) is a design task, not a diff this PR wants to force through unreviewed.

Ask

Maintainer call on:

  1. Is this worth fixing, given it only costs one cold cache-prefix per rotation (relatively rare)?
  2. If yes, does the logical-cache-scope shape above look right before someone implements it?
%%{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:#ffccd5
Loading

Test plan

  • pytest tests/design/test_cache_scope_compression_rotation_gap.py -v -> 1 xfailed (expected; strict, will error if it starts passing without a matching design)
  • Zero dependency on other open PRs or unmerged code -- applies cleanly to main as-is

Relates to #79017 (design decision, not code fix).

…cope gap

Not a fix -- a self-contained demonstration for NousResearch#79017, reviewable
directly on main with zero dependency on NousResearch#78959's merge state.
@JoaoMarcos44
JoaoMarcos44 marked this pull request as ready for review August 5, 2026 02:25
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/compression Context compression and continuation sessions P0 Critical — data loss, security, crash loop needs-decision Awaiting maintainer decision before any implementation sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) labels Aug 5, 2026
@kshitijk4poor

Copy link
Copy Markdown
Contributor

Thanks for the thorough follow-through on the #78941#78959#79017 chain — the design write-up here (physical vs logical identity vs provider sticky-routing) is genuinely useful. Closing this one, though, for three reasons:

  1. The premise went stale this morning. fix(cache): scope prompt_cache_key by session to stop cross-session bucket sharing #78959 merged via fix(cache): scope prompt_cache_key by session to stop cross-session bucket sharing #79161, so _cache_scope_from_session_id / _CRON_SESSION_ID_RE now live on main at agent/transports/codex.py — "there is nothing in main to import" no longer holds.

  2. The strict-xfail guard is structurally inert. The test asserts equality of two unrelated hardcoded ids through a function that passes non-cron ids through unchanged, and it exercises an inlined copy rather than production code — so no change anywhere in the repo can ever flip it to XPASS. strict=True only fires on XPASS, which is unreachable here; the "fails loudly if anyone closes the gap without a design" property doesn't hold. The repo's existing strict-xfail convention (e.g. tests/gateway/test_qqbot_scope_paths.py) is self-cleaning precisely because those tests run against production code and flip when the companion fix lands; this would be the first permanently-dead one.

  3. The design discussion already has a home in prompt_cache_key loses continuity across context-compression session rotation (needs a logical cache-scope concept) #79017, which is the right venue for the maintainer call you're asking for. A reproduction snippet in the issue body carries the same information without adding a perpetual XFAIL line to every CI run.

One substantive note carried over to #79017: main already has a logical-conversation identity — run_agent._conversation_root_id() (session-lineage root walk, resets on /new) published via set_conversation_context() — and the Nous/OpenRouter sticky-routing keys already resolve through it, so they survive rotation today. The gap is narrower than the table in this PR suggests: it's the codex prompt_cache_key derivation and the auxiliary-client path still using the raw physical id, and it only bites installs that opt out of the default compression.in_place: true (default compaction never rotates the session_id at all). That reframing is now on the issue.

Appreciate the care in making this reviewable standalone — the analysis lives on in #79017 even though the test artifact doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/compression Context compression and continuation sessions comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation P0 Critical — data loss, security, crash loop sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants