Skip to content

feat(cache): add prefixpin — pin an early message the agent rewrites every turn - #24

Draft
OsherElhadad wants to merge 1 commit into
mainfrom
feat/prefixpin
Draft

feat(cache): add prefixpin — pin an early message the agent rewrites every turn#24
OsherElhadad wants to merge 1 commit into
mainfrom
feat/prefixpin

Conversation

@OsherElhadad

Copy link
Copy Markdown
Collaborator

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.

cacheinject optimises 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:

requests share of uncached input
append-only turns (cached fine) 98.0% 28.2%
one early message mutated 2.0% 71.8% — 5,796,220 tokens

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 expand can recover it.

Guards

Each closes a specific way this could do harm:

  • only messages at index < max_pin_index (default 4) — an early, structural slot, never the working tail the agent is actively reasoning about;
  • only when similarity >= min_similarity (0.80) — a genuinely rewritten-in-place block, not a different message occupying the slot;
  • only after the slot has churned 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;
  • never the newest message, never tool results.

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_control to place and prefix stability is the only available lever.

Config

pipeline: [prefixpin, cacheinject]
components:
  prefixpin:
    max_pin_index: 4        # 0 disables
    min_similarity: 0.80
    repeat_threshold: 2
    min_tokens: 200

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 ./... and go 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-header block. 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.

…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 OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++ and saveSlot at 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 logging prefixpin.reverted in /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:

  1. 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 (Enabled gated, 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.
  2. 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: 4 being safe rests entirely on "these never change", which is only true for the agents where the component also never fires. Recommend defaulting to 0 (off) with the churning-agent case documented as opt-in.

MINOR

  • repeat_threshold: 2 does 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 decaying Churn on a turn where st.First == cur.
  • Store eviction re-baselines to the new text and restarts the counter (prefixpin.go:115-118). The default Memory store 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 needs repeat_threshold more 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. Put on 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 mask and failed_run use components/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 / MaxCachedIdx are 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 referencing components/component.go:128-135 would stop this reading as an oversight.
  • Doc/impl mismatch. The doc's config table and the code comment at prefixpin.go:64-65 both describe min_similarity as "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.md is not reflected in README.md:83 or the component table at README.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 calls p.Offload directly, 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 the len(req.Input) < 2 early-return rather than the newest-message guard.
  • /stats: activity is visible as prefixpin.mutated / .reverted via metrics/metrics.go:141-149, no field renames, so harness parsing in deploy/harbor/*.py is unaffected. Good. Note acted will always read 0 since Saved() > 0 never 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:

  1. 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.
  2. A no-op proof on claude-code. codesmart + prefixpin vs codesmart on SWE-bench, showing prefixpin.mutated == 0 and identical reward/cost. Cheap to run, and it converts my measurement above into a repo-owned result.
  3. A replay against /tmp/cg-runs/capture-swebench.jsonl asserting zero pins, checked in as a test or a script under scripts/. That is the regression guard that keeps this from silently starting to fire on the primary workload after some future default change.
  4. 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.

@OsherElhadad

Copy link
Copy Markdown
Collaborator Author

Independently corroborated — and one correction to my own check

I 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 messages[0]) reported 0 mutations at idx0/idx1 but 38/72 and 37/71 at idx2/idx3 on the Terminal-Bench capture — i.e. ~52% churn, which would have supported this PR's premise on TB.

That was wrong. Inspecting the diffs showed similarity ratios of 0.004 and 0.458 with entirely different toolu_ IDs — not in-place rewrites at all. The TB capture runs concurrent agent sessions that share a byte-identical first message (the message-count sequence interleaves 59,61,63,… with 2,4,6,8,…), so every fingerprint I tried collapsed them into one lineage and I was diffing unrelated conversations against each other.

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):

capture lineages idx0 idx1 idx2 idx3
Terminal-Bench 3 0/69 0/69 0/69 1/69
SWE-bench 50 0/1677 0/1627 0/1627 0/1577

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 xdedup §C1 and the refuted "expand tool never registered"). The common cause is trusting a derived grouping over the raw append-only structure of the request stream.

Position on the four blockers

I accept all four; each was reproduced by the reviewer with a concrete failure, and BLOCKER 1 and BLOCKER 3 are the serious ones:

  • BLOCKER 1 (pipeline reverts on token growth while Churn++/saveSlot have already advanced) is a genuine perpetual flip-flop — the component would cause the cache-writes it exists to prevent. The state mutation must move after the pin succeeds, and byte-stability needs an exemption from the token-count never-worse guard or it can never work.
  • BLOCKER 3 (asymmetric containment scores deletions at 1.00) is the "actively misleading rather than merely stale" case: 4,798 deleted characters resurrected, a completed step reappearing as pending. Symmetric/Jaccard scoring.
  • BLOCKER 2 is a documentation-vs-code mismatch with a real consequence: the design cannot emit a marker without breaking byte-identity, so the honest encoding is Irreversible = true and the doc must stop promising expand recovery. This matters more now that fix(expand): stop buffering every SSE response on a marker check that always matched #33 established restoration demand is near zero anyway — the fresh text would rarely come back even if it were reachable.
  • MAJOR 5: given the numbers above, max_pin_index: 0 (off) as the default, documented as Bob/Gemini-specific.

Decision

Not 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 (prefixpin.mutated == 0), and a checked-in capture replay asserting zero pins as a regression guard.

The idx0 = task instruction, idx1 = system prompt observation is worth keeping in the docs regardless — it makes "index < 4 is structural" indefensible as a default, whatever the provider.

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

Labels

None yet

Projects

Status: New/ToDo

Development

Successfully merging this pull request may close these issues.

2 participants