feat(cache): add prefixpin — pin an early message the agent rewrites every turn - #24
feat(cache): add prefixpin — pin an early message the agent rewrites every turn#24OsherElhadad wants to merge 1 commit into
Conversation
…every turn Providers hash the request prefix cumulatively, so a single changed character at an early message index makes every token above it unmatchable. cacheinject optimises WHERE the boundary goes; this handles the case where no boundary can help, because the mutation is below all of them. Measured across 1,955 real Bob requests on SWE-bench: 98.0% of turns were append-only and cached fine; the other 2% cost 5,796,220 tokens — 71.8% of ALL uncached input — because one early message mutated each turn. On one task the agent re-emitted a running <scratchpad> at index 1, changing 152 characters out of 6,024 (98.5% identical) ~1,374 tokens into a 181k-token prefix: only 0.76% of the prefix survived and the hit rate fell from 98% to 5.7%. A cache read costs 0.1x and uncached input 1.0x, so a mutation below the boundary makes every token above it ten times more expensive — worth ~31% of Bob's input cost, where every placement change measured ~0%. Remembers the first text for an early (index, role) slot and re-sends it when the slot is rewritten in place. Deliberately an Offload, not a Reformat: the model sees the pinned text, so a counter reads stale. Guarded on index, similarity, churn count and size; never the newest message, never tool results; the original is stashed so expand can recover it. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
OsherElhadad
left a comment
There was a problem hiding this comment.
Request changes — three correctness bugs I reproduced, plus the premise does not hold on claude-code traffic
I ran the suite (CGO_ENABLED=1 go test -tags cg_skeleton ./components/... and -race: all green, no races) and then wrote throwaway tests against feat/prefixpin to probe the guards. Four of five probes failed. Details below with the reproductions.
BLOCKER 1 — the pipeline reverts prefixpin whenever the pinned text is longer than the current text (components/offload/prefixpin.go:140)
components/pipeline.go:113 is a token-count never-worse guard: after > rep.TokensBefore → revert the whole component. prefixpin's job is byte stability, not token reduction, and re-sending the FIRST rendering grows the request whenever the first rendering was longer. A counter going "9" → "10" is exactly that direction, and it is at least as common as "THIRTY-SECOND" → "THIRTY-THIRD".
Reproduced (first rendering long, later renderings short):
turn 0: reverted=false skipped=true
turn 1: reverted=false skipped=true
turn 2: reverted=true skipped=false before=1608 after=1608
The failure is not benign, it is a permanent loop:
- the revert restores the agent's current text, so the prefix flips — the exact cache-write this component exists to prevent;
st.Churn++andsaveSlotat lines 130-131 happen before the pin, so the state advances anyway. Every subsequent turn re-pins and re-reverts forever, burning full-price input on every turn and loggingprefixpin.revertedin/stats.
Fix: prefixpin must not depend on shrinking. Either (a) exempt it explicitly (it is a cache-stability component like cacheinject, which mutates and saves ~0), or (b) tryMark-style: refuse to pin when TextTokens(st.First) > TextTokens(cur) so the guard can never fire. (b) is smaller and needs no pipeline change. Please add the "long first rendering" case to the test file — the current fixture scratchpad() only ever shrinks, which is why this is invisible today.
BLOCKER 2 — the withheld text is unreachable, so this is irreversible, not "reversible" (prefixpin.go:138-141)
CLAUDE.md:21-22: "Every lossy Offload must be reversible (a <<cg:HASH>> marker + the stashed original in the Store)." prefixpin stashes but emits no marker and never calls recordOwner. Reproduced:
pinned; markers in outgoing text = [] ; HasPlaceholder=false
key="cg:pin:s1:0" OwnsKey(session)=false
So: the model has no marker to reference, expand cannot be called for it, and GET /expand returns 404 (proxy/proxy.go:565, OwnsKey gate). Nothing can ever recover it.
I accept that prefixpin cannot emit a marker — a marker changes the bytes and destroys the byte-identity that is the entire point. That is fine as a design, but then the honest encoding is rep.Irreversible = true (see components/offload/marker.go:78, the existing mechanism for a deliberate unrecoverable drop) and the docs must stop claiming reversibility. Right now docs/components/prefixpin.md ("Offload — lossy, reversible … The original is stashed, so expand can recover it") and the PR body both describe behaviour the code does not have. Per the repo's docs rule this has to be corrected before merge, not after.
TestStashesOriginalForExpand (line 422) passes only because it calls c.Store.Get(key) directly. A test that asserts reachability — expand.ParseMarkers on the outgoing text, or OwnsKey — would have caught this. Please make it assert the real path.
Note also PR #33's finding that restoration demand is near zero in practice. That raises my concern rather than lowering it: it means the stale text is what the model gets, permanently, with no realistic recovery path.
BLOCKER 3 — similarity is asymmetric containment, so a deletion is scored 1.00 and the deleted content is resurrected (prefixpin.go:222-237)
hit/total iterates over b's lines only. If b is a strict subset of a, every line hits and the score is exactly 1.0 no matter how much a contains that b dropped. So the guard is blind in the one direction that is genuinely dangerous.
Reproduced — agent finishes a plan step and removes it from its scratchpad:
similarity(full, trimmed) = 1.000
turn 2: len sent=9455 (agent actually sent 4657)
4,798 characters of deliberately removed content were put back. This is the "actively misleading, not merely stale" case the design claims the guards close: a completed step reappears as pending, a resolved error reappears as live, a deleted file path reappears as present. An agent maintaining a running plan/scratchpad at an early index is precisely the population this component targets, and deleting finished items is precisely what such an agent does. The doc's framing ("a counter reads stale") understates this by a wide margin.
Fix: make it symmetric — score min(hit_a_in_b/|a|, hit_b_in_a/|b|), i.e. Jaccard-style rather than containment. Two extra lines. Add the deletion case as a test.
MAJOR 4 — the <4 lines fallback is a pure length ratio and scores unrelated content 1.00 (prefixpin.go:208-214)
if len(al) < 4 || len(bl) < 4 {
...
return lb / la // length ratio only
}Reproduced: similarity(strings.Repeat("aaaa ",400), strings.Repeat("zzzz ",400)) = 1.000. Two completely unrelated blocks of the same length pass the 0.80 gate and get pinned. The comment says this path "is only used to reject wildly different content" — it does the opposite: it accepts wildly different content of similar length. min_tokens: 200 does not save you; a 200-token single-paragraph instruction or a prose system prompt has few or no newlines. Either fall back to a real character-shingle score, or refuse to pin at all on this path (safer and shorter — a block with under four lines is not the scratchpad case anyway).
MAJOR 5 — max_pin_index: 4 is not "structural" on claude-code; indices 0-1 are the task instruction and the system prompt
I measured the readable captures directly (/tmp/cg-runs/capture-tb.jsonl, 73 requests; /tmp/cg-runs/capture-swebench.jsonl, 1,795 requests; grouped per session by body.metadata):
| index | role | mutations turn-over-turn (swebench) | (terminal-bench) |
|---|---|---|---|
| 0 | user (the task instruction) |
0 / 1637 | 0 / 70 |
| 1 | system (the system prompt) |
0 / 1587 | 0 / 69 |
| 2 | assistant |
0 / 1587 | 0 / 69 |
| 3 | user |
0 / 1537 | 0 / 68 |
Two conclusions:
- The premise does not hold on claude-code traffic at all. Zero early-index mutations in 1,868 requests across 53 sessions.
Enabled()returns true for every provider, so on the repo's primary workload this component is pure dead weight on the hot path — it runs, tokenises and hashes up to four large early messages per request, and can never act. The doc's "the failure mode is provider-independent" is an assertion with no supporting measurement; every number in the PR comes from Bob. Please either scope it (Enabledgated, or absent from every preset — it currently is in none, which is good) and document it as a Bob/Gemini-specific component, or bring evidence from non-Bob traffic. - What sits at indices 0-3 is the task instruction and the system prompt, not an inert structural header. If any agent does mutate there, pinning it means feeding a stale task instruction. That is the highest-consequence content in the request.
max_pin_index: 4being safe rests entirely on "these never change", which is only true for the agents where the component also never fires. Recommend defaulting to0(off) with the churning-agent case documented as opt-in.
MINOR
repeat_threshold: 2does not bound the risk you think. Two unrelated in-place edits within one session (a re-worded instruction, then a follow-up) trip it. And once tripped, the pin persists for the session's lifetime — nothing ever re-evaluates whether the slot went stable again. Consider decayingChurnon a turn wherest.First == cur.- Store eviction re-baselines to the new text and restarts the counter (
prefixpin.go:115-118). The defaultMemorystore is 1000 entries LRU (store/store.go:45); under concurrent sessions a slot key is evictable. On eviction the component sends the agent's current text (prefix flip → full cache write), then needsrepeat_thresholdmore turns at full price to recover — and the pin re-anchors to a different baseline than before, so the prefix moves twice. This is the same class of failure as issue #25 and prefixpin inherits it.Puton each churn turn does refresh the TTL, so the 1800s expiry is less of a problem than eviction — but a graceful behaviour on miss (skip rather than re-baseline) is worth having. - No freeze-and-replay. Only
maskandfailed_runusecomponents/offload/state.go:47-81. prefixpin rolls its own per-slot state instead, which is defensible (its decision is keyed by index, not content hash) — but it means the cache-stability invariant that freeze/replay exists to enforce is re-implemented here without the same guarantees. Worth a comment saying why freeze was not reused, so the next reader does not "fix" it. Ctx.TailOnly/MaxCachedIdxare not consulted at all. That is deliberately correct here — prefixpin mutates cached-prefix content on purpose in order to restore byte-identity — but it is the only component that does so, and nothing in the code says why. One comment referencingcomponents/component.go:128-135would stop this reading as an oversight.- Doc/impl mismatch. The doc's config table and the code comment at
prefixpin.go:64-65both describemin_similarityas "character-level overlap". The implementation is line-shingle containment with a length-ratio fallback — a materially different metric with different failure modes (see 3 and 4). Fix the description. docs/components/prefixpin.mdis not reflected inREADME.md:83or the component table atREADME.md:168, unlike every other Offload. Either add it or note that it is intentionally omitted from the index.- Test coverage gaps in
prefixpin_test.go: no test for turn 4+ (stability past the first pin), no test for a store miss mid-session, no test for the pipeline-level interaction (every test callsp.Offloaddirectly, which is exactly why BLOCKER 1 is invisible), no test for tool-result rejection despite the guard at line 104 being claimed in the doc.TestNeverPinsNewestMessage(line 377) passes a single-message request, so it exercises thelen(req.Input) < 2early-return rather than the newest-message guard. /stats: activity is visible asprefixpin.mutated/.revertedviametrics/metrics.go:141-149, no field renames, so harness parsing indeploy/harbor/*.pyis unaffected. Good. Noteactedwill always read 0 sinceSaved() > 0never holds — worth a line in the doc so nobody reads that as "never ran".- Security: clean. No credential, token, or gateway URL in the diff.
Minimum validation bar before I would merge this
The code fixes above are necessary but not sufficient. Given the two-benchmark finding that reward is what matters and that this component can resurrect deleted content:
- A reward-neutrality run on an arm where the component actually fires. Bob/SWE-bench, n≥50, prefixpin on vs off, reporting solve rate and input cost. Cost savings with a reward regression is not a win. If there is no Harbor arm where prefixpin fires, that is itself the answer: it should not be enabled by default anywhere.
- A no-op proof on claude-code. codesmart + prefixpin vs codesmart on SWE-bench, showing
prefixpin.mutated == 0and identical reward/cost. Cheap to run, and it converts my measurement above into a repo-owned result. - A replay against
/tmp/cg-runs/capture-swebench.jsonlasserting zero pins, checked in as a test or a script underscripts/. That is the regression guard that keeps this from silently starting to fire on the primary workload after some future default change. - Tests for each of the four reproductions above.
Happy to re-review once the blockers are addressed. The diagnosis in this PR (cumulative prefix hashing, no breakpoint can exclude an earlier block) is correct and worth solving, and I would like to see it land — the guards just do not currently hold.
Independently corroborated — and one correction to my own checkI ran my own measurement of the premise before acting on the review, and it took three attempts to get right. Recording the process because the failure mode is instructive. Attempt 1 (session = hash of truncated That was wrong. Inspecting the diffs showed similarity ratios of 0.004 and 0.458 with entirely different Attempt 3, tracking lineages by append-only prefix match (a turn continues a lineage only if the earlier turn's messages are a prefix of it):
So the reviewer's finding stands: early-index messages effectively never mutate in place on claude-code traffic. One event in ~6,500 comparisons is not a phenomenon worth a lossy component. I'm flagging my own error deliberately: this is the third premise in this workstream that came from a session-attribution or cumulative-counting artifact (see also the retracted Position on the four blockersI accept all four; each was reproduced by the reviewer with a concrete failure, and BLOCKER 1 and BLOCKER 3 are the serious ones:
DecisionNot merging. The diagnosis is sound and the Bob evidence is strong, but the mechanism does not hold as written, and it is dead weight — with a real downside tail — on this repo's primary workload. Blocking on the reviewer's minimum bar: fixes plus tests for all four reproductions, a reward-neutral run on an arm where it actually fires, a no-op proof on claude-code ( The |
Problem
Providers hash the request prefix cumulatively. A single changed character at an early message index makes every token above it unmatchable — there is no breakpoint position whose hash excludes an earlier block.
cacheinjectoptimises where the cache boundary goes. This component handles the case where no boundary can help, because the mutation sits below all of them.Evidence
Across 1,955 real Bob requests on SWE-bench:
On one task the agent re-emitted a running
<scratchpad>/<state_snapshot>at message index 1, re-rendering an iteration counter in ~20 places ("THIRTY-SECOND"->"THIRTY-THIRD","32"->"33"): 152 changed characters out of 6,024 — 98.5% identical content — sitting ~1,374 tokens into a 181k-token prefix. Only 0.76% of the prefix survived; the cache-hit rate collapsed from 98% to 5.7%.The economics are lopsided. A cache read costs 0.1x base input and uncached input costs 1.0x, so a mutation below the boundary makes every token above it ten times more expensive. Placement tuning only ever moves tokens between read (0.1x) and cache-write (1.25x) — which is why this is worth ~31% of Bob's input cost while every placement change measured ~0%.
Approach
Remember the first text seen for an early
(index, role)slot in this session. On a later turn, if that slot's text changed but is still recognisably the same content, re-send the first rendering so the prefix stays byte-identical.Lossiness — deliberately an Offload, not a Reformat
The model sees the pinned (older) text rather than what the agent just wrote, so information is withheld: a counter reads stale. That is a real behavioural change, and the reason for the guards. The original is stashed, so
expandcan recover it.Guards
Each closes a specific way this could do harm:
< max_pin_index(default 4) — an early, structural slot, never the working tail the agent is actively reasoning about;>= min_similarity(0.80) — a genuinely rewritten-in-place block, not a different message occupying the slot;repeat_threshold(2) times — a one-off edit is never pinned, only a per-turn churn pattern;min_tokens(200) skips slots too small to be worth the behavioural risk;Enabled for every provider: the failure mode is provider-independent, and it bites hardest on implicit-cache backends (Gemini/Bob, OpenAI) where there is no
cache_controlto place and prefix stability is the only available lever.Config
Testing
components/offload/prefixpin_test.go— 263 lines covering: the real scratchpad-churn case, each guard rejecting on its own axis (index, similarity, churn count, size), no pin on the newest message or on tool results, marker + stash reversibility, and session isolation.CGO_ENABLED=1 go build -tags cg_skeleton ./...andgo test -tags cg_skeleton ./...both green.Docs
New
docs/components/prefixpin.md(problem, evidence table, lossiness, every guard, config reference, when to enable) and a nav entry under Offload.Note on scope
An earlier draft of this branch also re-added a system-prefix reorder for the
x-anthropic-billing-headerblock. That was dropped: PR #22's own doc records the retraction — the header's suffix tracks the claude-code build, not the session (49 distinct values across 74 conversations, one value appearing in 24 of them), and a sequential Terminal-Bench run measured 92.8% cross-session cache hit with the header present. There was no poisoning to repair.