fix(agent): same-model review fork inherits the parent's resolved cache scope (#109964)
build_cache_parity_fork gives the same-model fork the parent's session_id, cached system prompt, tools[] and session_start — but with _persist_disabled=True and _session_db=None, BOTH cache-identity resolvers diverged from the parent on their own: declared_conversation_scope failed closed on _persist_disabled, and the lineage walk skipped on the missing DB. The fork's affinity header (set_affinity_scope) and body prompt_cache_key (cache_scope_id on the OpenAI-wire transports) therefore keyed a different bucket than the gateway parent, costing one cold ~full-context request per review. Not gateway-only: any parent whose lineage root != current physical id diverges too (teknium1's triage table). Fix, per the triage's suggested direction: on the not-routed branch only, the fork stamps _inherited_cache_scope = resolve_prompt_cache_scope_safe (parent) — the parent's ALREADY-RESOLVED scope, no DB access from the fork, persistence fully detached. Both declared_conversation_scope and resolve_prompt_cache_scope return the inherited scope first when set, so the header path and the body path are fixed together (fixing only one leaves the other divergent — Vivamisu's header/body split observation). Routed (different-model) forks, /branch children, delegate/tool children and fresh sessions set nothing; the fail-closed default stands untouched. /btw shares build_cache_parity_fork and gets the repair for free.
This commit is contained in:
@@ -16,6 +16,7 @@ from contextlib import contextmanager, suppress
|
||||
from dataclasses import dataclass, field
|
||||
from typing import Any, Dict, Iterator, List, Optional, Tuple
|
||||
|
||||
from agent.prompt_cache_scope import resolve_prompt_cache_scope_safe
|
||||
from agent.thread_scoped_output import thread_scoped_silence
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -957,6 +958,18 @@ def build_cache_parity_fork(
|
||||
if not _routed:
|
||||
review_agent._cached_system_prompt = agent._cached_system_prompt
|
||||
review_agent.session_start = agent.session_start
|
||||
# Cache-scope parity (#109964): the fork shares the parent's physical session_id and
|
||||
# byte-identical prefix, but is _persist_disabled (declared scope fails closed) and
|
||||
# _session_db=None (lineage walk skipped) — so BOTH cache-identity resolvers keyed it
|
||||
# into a different bucket than the gateway parent, costing one cold ~full-context
|
||||
# request per review. Inherit the parent's ALREADY-RESOLVED scope once, here: no DB
|
||||
# access from the fork, persistence stays fully detached, and both consumers (the
|
||||
# affinity header via set_affinity_scope and the body prompt_cache_key via
|
||||
# cache_scope_id) resolve the parent's bucket together. Routed (different-model)
|
||||
# forks do NOT inherit: their prefix is cache-cold anyway.
|
||||
inherited_scope = resolve_prompt_cache_scope_safe(agent)
|
||||
if inherited_scope:
|
||||
review_agent._inherited_cache_scope = inherited_scope
|
||||
_inherit_parent_tool_surface(review_agent, agent)
|
||||
_detach_fork_compression(review_agent)
|
||||
# Compaction bounds a single request; this bounds the WHOLE review (checked in
|
||||
|
||||
@@ -87,10 +87,23 @@ def declared_conversation_scope(agent: Any) -> Optional[str]:
|
||||
"""Host-declared logical conversation scope (``gwk_<sha256[:24]>``), or None.
|
||||
|
||||
Hashes ``(source, gateway_session_key, generation)``. None (fall back to the physical id)
|
||||
when no key is declared, for a background-review fork (``_persist_disabled``), for an
|
||||
explicit fork child, and on any DB error (fail closed rather than merge a fork onto its
|
||||
parent's key).
|
||||
when no key is declared, for an explicit fork child, and on any DB error (fail closed
|
||||
rather than merge a fork onto its parent's key).
|
||||
|
||||
A same-model background-review fork (``_persist_disabled=True``, ``_session_db=None``)
|
||||
resolves the parent's *already-resolved* scope via ``_inherited_cache_scope`` BEFORE the
|
||||
``_persist_disabled`` fail-closed branch (#109964): the fork's entire purpose is prefix
|
||||
parity with the parent, but the exclusion below plus the missing DB made both resolvers
|
||||
diverge — the header path (affinity/sticky session) and the body path
|
||||
(``prompt_cache_key``) keyed the fork into a different bucket, costing one cold
|
||||
~full-context request per review. The inherited value is stamped once at fork time by
|
||||
``build_cache_parity_fork`` (no DB access from the fork), so persistence stays fully
|
||||
detached. Nothing sets the attribute for routed (different-model) forks, ``/branch``
|
||||
children, delegate/tool children, or fresh sessions — the fail-closed default stands.
|
||||
"""
|
||||
inherited = getattr(agent, "_inherited_cache_scope", None)
|
||||
if inherited:
|
||||
return str(inherited)
|
||||
key = str(getattr(agent, "_gateway_session_key", "") or "").strip()
|
||||
if not key or getattr(agent, "_persist_disabled", False):
|
||||
return None
|
||||
@@ -131,7 +144,11 @@ def declared_conversation_scope(agent: Any) -> Optional[str]:
|
||||
|
||||
def resolve_prompt_cache_scope(agent: Any) -> str:
|
||||
"""Rotation-stable cache-scope id: declared scope, else the compression-lineage root of
|
||||
``agent.session_id`` (the physical id without ancestry/DB). Memoized on the agent."""
|
||||
``agent.session_id`` (the physical id without ancestry/DB). Memoized on the agent.
|
||||
|
||||
A same-model review fork's inherited scope (``_inherited_cache_scope``) resolves through
|
||||
:func:`declared_conversation_scope` above, which honors it first — fixing header and body
|
||||
paths together (#109964)."""
|
||||
sid = str(getattr(agent, "session_id", None) or "")
|
||||
if not sid:
|
||||
return ""
|
||||
|
||||
@@ -479,3 +479,94 @@ def test_review_effort_notice_only_for_same_model_review_forks():
|
||||
assert _warns({"reasoning_effort": "low"}, write_origin="side_question") == []
|
||||
with patch.object(bg_review, "_resolve_review_runtime", return_value=routed_runtime):
|
||||
assert _warns({"reasoning_effort": "low"}) == []
|
||||
|
||||
|
||||
def test_same_model_fork_inherits_parent_cache_scope_gateway_key(tmp_path):
|
||||
"""#109964 invariant 1 (gateway-key case): the same-model review fork must
|
||||
resolve the PARENT's cache scope, even though it is _persist_disabled and
|
||||
_session_db=None. Pre-fix, both resolvers diverged on their own — the header
|
||||
(affinity) and body (prompt_cache_key) keyed a different bucket, costing one
|
||||
cold ~full-context request per review."""
|
||||
import run_agent
|
||||
from agent.background_review import build_cache_parity_fork
|
||||
from agent.prompt_cache_scope import (
|
||||
declared_conversation_scope,
|
||||
resolve_prompt_cache_scope,
|
||||
)
|
||||
from hermes_state import SessionDB
|
||||
|
||||
db = SessionDB(db_path=tmp_path / "state.db")
|
||||
try:
|
||||
agent = _make_agent_stub(run_agent.AIAgent)
|
||||
# Gateway parent shape: declared key, real DB row behind it.
|
||||
agent._gateway_session_key = "gw-key-1"
|
||||
agent._session_db = db
|
||||
db.create_session("sess-123", source="test")
|
||||
|
||||
with patch.object(run_agent, "AIAgent", _make_recorder_class()):
|
||||
fork, _rt, routed = build_cache_parity_fork(agent, max_iterations=5)
|
||||
|
||||
assert not routed
|
||||
parent_scope = resolve_prompt_cache_scope(agent)
|
||||
assert parent_scope.startswith("gwk_"), parent_scope
|
||||
# The fork stamps the parent's resolved scope; both resolvers honor it.
|
||||
assert getattr(fork, "_inherited_cache_scope", None) == parent_scope
|
||||
assert declared_conversation_scope(fork) == parent_scope
|
||||
assert resolve_prompt_cache_scope(fork) == parent_scope
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
|
||||
def test_same_model_fork_inherits_parent_cache_scope_rotated_lineage(tmp_path):
|
||||
"""#109964 invariant 1 (rotated-lineage case): a CLI parent whose lineage
|
||||
root != current physical id must also pass its scope to the fork. Pre-fix
|
||||
the parent resolved 'root-sid' while the fork fell to the physical id."""
|
||||
import run_agent
|
||||
from agent.background_review import build_cache_parity_fork
|
||||
from agent.prompt_cache_scope import resolve_prompt_cache_scope
|
||||
from hermes_state import SessionDB
|
||||
|
||||
db = SessionDB(db_path=tmp_path / "state.db")
|
||||
try:
|
||||
agent = _make_agent_stub(run_agent.AIAgent)
|
||||
agent._session_db = db
|
||||
# Legacy compression rotation: parent row ends, child inherits its lineage.
|
||||
db.create_session("root-sid", source="test")
|
||||
db.end_session("root-sid", "compression")
|
||||
db.create_session("sess-123", source="test", parent_session_id="root-sid")
|
||||
agent.session_id = "sess-123"
|
||||
|
||||
with patch.object(run_agent, "AIAgent", _make_recorder_class()):
|
||||
fork, _rt, routed = build_cache_parity_fork(agent, max_iterations=5)
|
||||
|
||||
assert not routed
|
||||
assert resolve_prompt_cache_scope(agent) == "root-sid"
|
||||
assert getattr(fork, "_inherited_cache_scope", None) == "root-sid"
|
||||
assert resolve_prompt_cache_scope(fork) == "root-sid"
|
||||
finally:
|
||||
db.close()
|
||||
|
||||
|
||||
def test_routed_fork_does_not_inherit_cache_scope():
|
||||
"""#109964 invariant 2: a routed (different-model) fork is cache-cold on
|
||||
that model anyway — it must NOT inherit the parent's scope. Nor may fresh
|
||||
agents (no attribute set) be affected: the fail-closed default stands."""
|
||||
import run_agent
|
||||
from agent.background_review import build_cache_parity_fork
|
||||
|
||||
agent = _make_agent_stub(run_agent.AIAgent)
|
||||
agent._gateway_session_key = "gw-key-1"
|
||||
agent._prompt_cache_scope_memo = (("sess-123", True), "gwk_parentscope0000000000abc")
|
||||
|
||||
_RoutedRecorder = _make_recorder_class()
|
||||
|
||||
with patch.object(run_agent, "AIAgent", _RoutedRecorder), \
|
||||
patch("agent.background_review._resolve_review_runtime",
|
||||
lambda *a, **k: {"routed": True, "model": "other-model"}):
|
||||
fork, _rt, routed = build_cache_parity_fork(agent, max_iterations=5)
|
||||
|
||||
assert routed
|
||||
assert not getattr(fork, "_inherited_cache_scope", None), (
|
||||
"Routed fork must not inherit the parent's cache scope — its prefix "
|
||||
"is cache-cold on the different model regardless."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user