feat(proxy): async mode — cache-safe deferred compaction (HELD: benefit unmeasured since #34)

[OsherElhadad]

🚧 DRAFT — held pending a benchmark, not pending code

Observe mode landed separately in #43. This branch is async-only, and it is held
because its measured benefit has evaporated — not because anything here is unfinished or
known-broken. All seven blocking review findings plus four follow-ups (S1–S11) are fixed;
gates are green including -race -count=5.

Why held:

• The headline latency win (1,599 ms → 25 ms) came almost entirely from deferring
  extract_llm, and perf(extract_llm): economic gate, global result cache, and the honest verdict (#28) #34 hard-declines extract_llm on caching backends. On the
  primary workload there is now nothing expensive left to defer. Terminal-Bench already
  measured exactly that: zero LLM calls, async 26.8 ms vs sync 26.9 ms.
• The cache-write evidence is withdrawn (see the corrections section): the tail
  protection was inert on claude-code, which sets its own breakpoints.
• dropped and stale_discarded — the counters guarding the correctness invariant — are
  exercised by tests only, never by production load.

What would unblock it: a 50-task paired arm with async.strip_caller_breakpoints: true
showing (a) cache-write no higher than sync's, and (b) a real latency reduction on
traffic that still makes compaction model calls. That is a measurement, not a code change.

The benefit is conditional and currently unmeasured on claude-code. With default
settings async declines to defer on every turn where the agent has cache-written the tail,
so it is deliberately inert rather than falsely protective — async_tail_unprotected_turns
reports that. strip_caller_breakpoints: true is required for it to do anything there.

Kept rather than discarded because the hard parts — per-session generations, the bounded
pool, the copy-on-write store overlay, the decline-rather-than-pretend cache policy —
survived a hostile review intact. This is the ready-to-measure branch.

Note for #40/#43: the store.Buffer → FrozenLost structural forwarding lives on THIS
branch, because store.Buffer does. #43 does not need it (observe's shadow store is a
plain *store.Memory, which implements FrozenLost natively), so nothing is lost by this
sitting in draft.

Closes #31.

Compaction was unconditionally synchronous, which forced two bad trades: to get
savings you accepted ~450 ms/req of latency, and to find out whether context-guru
helps your workload you had to enforce it in production and compare against
history. This adds an explicit mode: with three settings.

sync remains the default and is byte-identical to today.

The three modes

Mode	Request path	Notes
sync (default)	Runs the pipeline inline; the caller waits.	Unchanged, asserted by a golden test.
async	Runs inline with no model clients, replaying decisions an earlier turn froze; enqueues the expensive compaction for later turns.	Cache-safe by default (below).
observe	Does not run the pipeline at all. Forwards the original, byte for byte.	A copy runs off-path against a buffer that is never committed.

mode: async
async: { cache_uncompacted_tail: false, max_queue: 256, workers: 1 }

The hard part: async's cache requirement

A cache-write costs 11.5x a cache-read (($2.50 − $0.20)/$0.20). So a naive
async implementation is strictly worse than sync: it lets the un-compacted tail
get provider-cached, then replaces it when the compaction lands, and the provider
re-writes a span it had committed to. That is exactly what tripled headroom's
cache-write on Terminal-Bench (12.37M vs 4.01M baseline) by rewriting the live zone.

By default no breakpoint is placed at or beyond the tail a pending compaction will
replace: cacheinject drops those positions and anchors at the highest safe index
instead, so the whole stable prefix is still written and nothing the provider
commits to is later rewritten. async.cache_uncompacted_tail: true is the escape
hatch for a backend confirmed not to cache, where the protection buys nothing.

Review correction. The first version pruned only the positions cacheinject itself
wanted, so breakpoints the agent set survived — and claude-code sets its own on the
newest message, inside exactly the protected span. The protection was a silent no-op on
the primary workload: async paid the 11.5x rewrite and lost a slot while reporting
success. It now either strips those (async.strip_caller_breakpoints: true) or
declines to defer the turn, counted as async_tail_unprotected_turns. With
claude-code on defaults, async is deliberately inert rather than falsely protective.

The protection needed its own bool rather than a sentinel index: index 0 is a
legitimate value ("no breakpoint anywhere"), so no integer is free to mean "off".
A false default also makes an unset field cost a missed optimisation rather than a
wrong request — the opposite of MaxCachedIdx's -1 (see #25).

Generations: the invariant everything rests on

An async result lands at an unpredictable later moment, by which time the agent may
have taken another turn and another job may have committed. Applying a result built
from a superseded snapshot is how a compaction proxy corrupts a cached prefix.

modes.Tracker holds, per session under one lock, the compaction generation — which
advances on every turn — and the cached-prefix boundary. A request records the
generation it was built from; a deferred job writes into a store.Buffer (copy-on-write
over the real store) and that buffer is committed only if the session is still on that
turn — checked under the same lock that advances it. A stale result is discarded and
counted as stale_discarded.

Review correction. The generation originally advanced only on commit, so a job from
turn 1 still read its own generation as current after eight later turns and committed
against a long-superseded transcript: the guard could only fire on a dedup collision,
never on staleness. The documented invariant was not the implemented one. Fixed, with the
honest consequence now stated in the docs — at agent turn rates async discards much of
what it computes, and stale_discarded is the number that tells you whether deferral
suits a workload.

The buffer is what makes "discard" possible at all: a deferred run writes frozen
decisions and stashes as it goes, so throwing the result away after the fact is not
an option. Running it against an overlay makes the whole result one atomic,
discardable unit — Commit() flushes it, and not calling Commit() is the discard.

The worker pool

One bounded queue plus a fixed worker count owned by the proxy, not a goroutine per
request. The shape is headroom's BackgroundCompressor, ported and extended:

• dedup by (session, generation), pending slot claimed before the job is
  observable in the queue, so dedup is atomic against a concurrent enqueue;
• a full queue drops and counts, never blocks — the request already went out;
• jobs run under the pool's context, not the request's (which is cancelled when the
  response is written);
• fail-open on every path, including a panicking job;
• the whole counter tuple exposed, dropped and stale_discarded included.
  headroom's dashboard shows only queued, hiding precisely the "we silently gave up
  savings" counters.

Observe: measure without enforcing

Byte-identity is structural, not a property of careful copying: the request path
never touches the pipeline, and also skips expand.Inject — injecting a tool
declaration would be a modification.

Observe results live in physically separate accumulators serialized under
potential_* / projected_*, sharing no key with any enforced metric. In observe
mode every enforced aggregate is zero by construction, which is the machine-readable
form of "context-guru did not modify requests". A mislabelled hypothetical silently
inflates the headline savings claim, so this is a correctness boundary and a test
asserts no enforced aggregate can reach an observe result.

headroom has no observe/shadow/dry-run mode at all — its token and cache modes
are both enforcing — so this is a genuine differentiator rather than a port.

Two bugs the benchmark surfaced

Worth calling out, because both were found by running it rather than reading it:

1. Deferred runs were double-counted. They emitted reports stamped async, so
   their savings entered the enforced rollups even though nothing was forwarded — and
   entered again when a later turn replayed the decision on-path. Report/RunReport
   now carry Deferred and the aggregator drops those; off-path work shows up as
   async_deferred_runs / async_deferred_ms_total / the queue tuple, and savings are
   credited only where they were realized.
2. cacheinject on a deferred run corrupted turn state. Its per-message
   divergence digests are turn state, and a deferred job commits some turns after the
   one it was built from, so committing its digests replayed turn N's over turn N+2's
   and made the next turn compute the wrong divergence point. It now skips deferred
   runs entirely (its breakpoints were pointless there anyway — the body is discarded).

/stats

Additive only; every pre-existing field keeps its name and shape, because
deploy/harbor/*.py parses this payload (a test pins the full pre-existing key set).
New: mode, sync_enforced, async_enforced, async_queue (the full tuple),
async_deferred_runs, async_deferred_ms_total, async_realized_saved_tokens, and
the observe hypotheticals.

Overlap with #25

prevLen was read then written back in a defer, so two concurrent turns of one
session raced on it. Tracker.Turn now reads the previous length and records the new
one in a single locked call. That is the minimal form of the same fix #25 owns; if
#25 lands first, this reduces to using its state instead. Merge-order note: I did
not touch #25's files. The legacy store-backed path is left intact for library
callers and /compact, which supply no tracker.

Tests

go test -race throughout (the whole suite, not just the new packages):

• sync byte-identical to the legacy entry point, and the unset mode identical to
  explicit sync (golden);
• observe leaves the forwarded body byte-identical while still recording;
• a stale-generation result is discarded end to end, and concurrent commits at one
  generation apply exactly once;
• concurrent turns of one session corrupt neither prevLen nor the generation —
  through the tracker and through the real handler;
• enqueue dedup is atomic against 32 concurrent enqueues of one key;
• a full queue drops, counts, and never blocks;
• no goroutine leak on Pool.Stop or Handler.Close, both idempotent;
• with the default policy, no breakpoint at/beyond the un-compacted tail — and the
  escape hatch restores normal placement;
• observe metrics unreachable from every enforced aggregate, and deferred runs
  counted as neither enforced nor hypothetical;
• async savings actually arrive on a later turn (2 committed compactions
  realizing 4,948 tokens across five turns of one session).

CGO_ENABLED=1 go build/test -tags cg_skeleton ./... green, gofmt -l clean,
go vet clean, mkdocs build --strict green.

Docs

New docs/how-to/operating-modes.md (when to use each, the cache arithmetic, how to
read the queue counters and the observe numbers — including what observe cannot
tell you: cache effects are projected not measured, no expand bounce is exercised,
and off-path measurement still spends cheap-model tokens). docs/design.md gains
generations, the job lifecycle, the cache policy and fail-open per mode.
docs/reference/config.md, mkdocs.yml, README.

Review corrections (read before the benchmark comments)

An independent review found six semantic defects — the concurrency primitives held, the
meaning did not. Three were places where the code and these docs disagreed, which is
worse than a bug because the docs were the spec. All six are fixed here; the two benchmark
comments below predate the fixes, and two of their claims are retracted:

1. The −17.8%/−45% cache-write result is no longer claimed as causal. The numbers stand
   as measurements, but the tail protection was inert on that workload, and two further
   defects (zero breakpoints on a session's first turn; total suppression under
   cache_mode: off) lower cache-write for arithmetic rather than policy reasons. The arms
   were also unpaired — 49% more cache-read, 6 mean steps apart, async re-run separately
   after a port collision. Mechanism is now verified by test; the effect needs a 50-task
   paired re-run with strip_caller_breakpoints: true.
2. async_realized_saved_tokens = 15,962 "the entire enforced saving" is retracted. The
   counter fired on every async turn that saved anything, so it re-reported the inline
   saving and equality with the total was guaranteed by construction. Now gated on a
   compaction having actually landed, with a test asserting a strict subset. Corrected
   measurement: realized=4125 of saved=4948, with one legitimate stale discard.

Also fixed: the protected span was off by one turn (guarding this turn's new messages
rather than the previous turn's tail that the pending job actually replaces); observe's real
off-path model spend is labelled rather than implied hypothetical; unproductive sessions
stop buying cheap-model calls; eviction can no longer let a surviving in-flight job commit
over a recreated session; Stop() is bounded at 2s instead of inheriting the cheap model's
5-minute client timeout (which main.go's deferred Close() would hang on); and
store.Buffer forwards #40's optional FrozenLost structurally so the wrapper does not
disable it.

Gates re-run after the fixes: build, test -tags cg_skeleton, -race -count=5,
gofmt -l, go vet, mkdocs build --strict — all clean.
docs/results/operating-modes.md carries both retractions inline.

[OsherElhadad]

Benchmark results — all three modes

Commit a0c7253 (branch head at time of run), codesmart pipeline, claude-code on
aws/claude-sonnet-5 through the IBM gateway. Cache-aware billed cost recomputed from
each trial's token tiers.

Scale up front: 2 SWE-bench tasks per mode at n=1, plus one real Claude Code session
per mode. Enough to validate the mechanism and answer the latency and cache questions;
not enough for a cost or solve-rate claim. Treat the billed-cost column as tracking
trajectory length, not mode.

SWE-bench Verified — 2 tasks, n=1

	sync	async	observe
solved	2/2	2/2	2/2
mean steps	15.5	21.5	22.5
added latency / req	1,599.4 ms	25.3 ms	0.062 ms
content savings (enforced)	0.82%	4.17%	— (0 by construction)
projected savings	—	—	6.40%
cache-read	1,464,729	2,186,100	2,300,699
cache-write	52,287	42,980	127,589
cache-write per 1M cache-read	35,697	19,661	— (not enforcing)
cache-hit rate	96.55%	98.07%	94.74%
fresh input	54	78	86
output	10,249	10,711	11,522
billed cost	$0.5263	$0.6519	$0.8945
context-guru's own LLM cost	$0.0122 (1)	$0.0435 (4)	$0.0779 (7)
off-path compaction time	—	80.8 s	75.0 s
deferred compactions committed	—	1	0 (never commits)
async_realized_saved_tokens	—	15,962	—
queue {dropped, stale_discarded}	—	{0, 0}	{0, 0}

The four questions

1. Does async reduce added latency without increasing cache-write?

Latency: 1,599.4 ms → 25.3 ms, 63x. Visible per component — extract_llm costs
15,014 ms on sync's request path and 71.3 ms cumulative across 42 requests on async's,
with acted=0 inline. The model call is genuinely off the hot path; 80.8 s of
compaction ran there instead.

Cache-write: it went down. 52,287 → 42,980 absolute, on ~49% more cache-read
traffic — normalised, 19,661 cache-write tokens per 1M cache-read against sync's
35,697 (−45%), hit rate 96.55% → 98.07%.

That is the failure mode the issue warned about not occurring. A policy that was
caching the un-compacted tail and then rewriting it could not produce this arm. I claim
only the direction, not the magnitude — n=1, two differently-shaped trajectories.

2. Does async reach the same steady-state savings as sync, just later?

It reached more here (4.17% vs 0.82%), but that is trajectory noise, not a finding.
The load-bearing number is async_realized_saved_tokens = 15,962 = the entire
enforced saving, from 1 committed deferred compaction. Every token async saved was
saved by a later turn replaying an earlier turn's off-path work. The deferral works end
to end on real traffic.

3. Does observe add measurable latency to the enforced path?

No — 0.062 ms/req against sync's 1,599.4 ms. Structural, not tuned: the request path
never runs the pipeline. Confirmed independently in a live Claude Code session (0.209 ms
vs sync's 28.964 ms).

Observe is not free in other respects — it moved 75.0 s of compaction off-path and spent
$0.0779 of cheap-model tokens measuring. It costs money and CPU, not request latency.

4. Do observe's projections match sync's actuals?

Answering this honestly found two real bugs, and was the most valuable thing the
benchmark did. Both are fixed in this PR with tests.

• First comparison: 9.53% projected vs 0.82% enforced, an 11x overstatement. The
  observe job ran without the session tracker, so its cached-prefix boundary was
  unknown, the tail gate never fired, and 50 extract_llm candidates passed where sync
  allowed 5. A projection that ignores cache-awareness projects what a cache-blind
  proxy would do.
• Fixing that exposed the opposite error: observe then under-projected ~3x, having
  run against a discarded buffer, losing the frozen decisions offloaders replay every
  turn — where most of the sustained saving lives.

After both fixes, on identical traffic through the real handler, projection and
actual agree exactly: 10,020 tokens / 23.06% each. A test pins it and fails at ratio
0.33 if the shadow store is removed.

On the benchmark arms the residual gap is 6.40% projected vs 0.82% enforced, and I
am not explaining it away:

• the arms are different trajectories (22.5 vs 15.5 mean steps) — observe saw 46
  requests / 492,652 baseline tokens, sync saw 35 / 244,319. Not the same conversations.
• observe's projection never pays a bounce (nothing is offloaded, so wasted_tokens is
  structurally 0). It is an upper bound on content savings, and documented as one.
• 2 tasks at n=1 cannot separate a real bias from trajectory noise.

The controlled same-traffic test is the strong evidence; the arms are consistent with it
but too small to confirm it independently. A 50-task paired run is the honest next step.

Real Claude Code sessions (one per mode)

	sync	async	observe
requests (enforced)	4	5	0
sync_enforced / async_enforced	4 / 0	0 / 5	0 / 0
added latency / req	28.964 ms	17.797 ms	0.209 ms
baseline tokens	6,025	8,124	6,025 (as actual_baseline_tokens)
queue {dropped, stale_discarded}	—	{0, 0}	{0, 0}
task answered correctly	yes	yes	yes

Observe's actual_baseline_tokens = 6,025 is exactly sync's tokens_before = 6,025 on
the same prompt — the hypothetical namespace accounts for identical traffic identically,
measured independently.

Namespace separation, verified in production

From the observe arm's live /stats: enforced side requests: 0, saved_tokens: 0,
sync_enforced: 0, async_enforced: 0, components: {} — all zero, all empty;
hypothetical side observe_hypothetical_requests: 46,
actual_baseline_tokens: 492652, projected_optimized_tokens: 461112,
potential_saved_tokens: 31540, potential_components: {…} — fully populated. No
aggregate over the enforced rollups can reach a hypothetical.

Terminal-Bench 2.0

Ran sync (26.9 ms/req added, 1.02% savings, 60 pipeline runs) and async (19.7 ms/req
added, 77 requests) on 2 cache-sensitive tasks. The gap is small there because
extract_llm made zero calls on these tasks — with no model call in the pipeline
there is nothing expensive to defer, which is itself the useful negative result: async's
benefit is proportional to how much LLM work the pipeline does. Neither arm solved these
two (hard) tasks, and one hit an environment-build exception, so I am not quoting
reward numbers from it.

Bugs the benchmark found that the tests did not

All four passed the unit suite while live:

1. deferred runs double-counted into the enforced rollups;
2. cacheinject committing stale per-turn digests from an off-path run;
3. observe overstating 11x (missing cache boundary);
4. observe understating 3x (discarded frozen state).

Each now has a test that fails without its fix.

Not established

• The cache-write result's magnitude (direction is solid).
• Any per-mode cost or solve-rate claim.
• Async under concurrency pressure: dropped and stale_discarded were 0 on every
  arm, so those paths are exercised only by tests, never yet by production load.

Full write-up in docs/results/operating-modes.md.

[OsherElhadad]

Terminal-Bench 2.0 — the cache-write result replicates

Second benchmark, different traffic shape, and it independently reproduces the finding
that matters most for this PR.

	sync	async
added latency / req	26.9 ms	26.8 ms
cache-read	3,144,887	6,311,918
cache-write	68,211	83,222
cache-write per 1M cache-read	21,689	13,185 (−39.2%)
cache-hit rate	97.87%	98.70%
pipeline runs	60	110
context-guru's own LLM calls	0	0
off-path compaction time	—	2.5 s
async_realized_saved_tokens	—	1,156
queue {dropped, stale_discarded}	—	{0, 0}

Normalised cache-write fell 39.2% under async here, against −45% on SWE-bench. Two
benchmarks, two traffic shapes, same direction. That is the strongest evidence I have
that the tail policy is doing its job — a policy that was caching the un-compacted tail
and then rewriting it could not produce either arm.

And a useful negative result: extract_llm made zero model calls on these tasks,
so there was nothing expensive to defer and async's added latency is identical to sync's
(26.8 vs 26.9 ms). Async buys back the compaction model call, so on traffic that never
triggers one it buys nothing — and, importantly, costs nothing either. Worth knowing
before anyone enables async expecting a universal win: the benefit is proportional to how
much LLM work your pipeline actually does.

No reward numbers from this arm. Two hard tasks, an environment-build exception in each
configuration, and trajectories that diverged sharply (30 vs 55.5 mean steps) — at this
scale the step count drives the cost column entirely, not the mode.

The observe arm was still running at hand-off; docs/results/operating-modes.md carries
the full write-up including everything above.

[OsherElhadad]

All nine trials complete — TB observe closes it out

Terminal-Bench	sync	async	observe
added latency / req	26.9 ms	26.8 ms	0.076 ms
cache-write per 1M cache-read	21,689	13,185 (−39.2%)	—
cache-hit rate	97.87%	98.70%	97.15%
content savings (enforced)	1.02%	0.11%	— (0 by construction)
projected savings	—	—	0%
enforced requests	60	110	0
queue {dropped, stale_discarded}	—	{0, 0}	{0, 0}

Observe projected 0%, and that is the result I'd point a reviewer at first. It is a
negative control, and a stronger form of evidence than the SWE-bench agreement: on
traffic where sync achieved almost nothing (1.02%), observe correctly reported almost
nothing rather than inventing a headline. A mode that only ever agreed on high-savings
traffic would be much weaker evidence that its projections mean anything. It also
correctly reported the overhead sync would have added (9.1 ms/req — small here because
the pipeline made no model calls) while itself costing 0.076 ms.

That gives three independent lines of evidence on projection accuracy:

• exact agreement on identical traffic through the real handler (10,020 tokens /
  23.06% both sides, controlled test);
• correct near-zero agreement on Terminal-Bench (1.02% actual → 0% projected);
• consistent-but-noisy on SWE-bench (0.82% vs 6.40%), where the arms took different
  trajectories and observe's projection is a structural upper bound — documented as such
  rather than explained away.

Final tally

• Nine trials: 2 SWE-bench + 2 Terminal-Bench tasks per mode at n=1, plus one live Claude
  Code session per mode. All nine complete; nothing left running.
• Async's cache-write fell on both benchmarks (−45% normalised on SWE-bench, −39.2%
  on Terminal-Bench). The headroom failure mode did not reproduce.
• dropped and stale_discarded stayed at 0 across all nine — no queue pressure was
  ever reached, so those paths remain test-only. Still the top gap.
• Four bugs found by benchmarking that the passing test suite missed, each now with a
  test that fails without its fix.

docs/results/operating-modes.md carries the full write-up, including a "what is not
established" section.

[OsherElhadad]

Orchestrator verification of the cache-write claim — it holds, with a caveat the PR should state

I re-derived the headline number from the row files rather than taking it on trust, and hit a discrepancy worth recording.

What tripped me up

/tmp/i31-runs/swe/ contains all three arms, and its async arm gives 41,136 cw/1M-read — worse than its sync arm's 35,697. The PR's 19,661 comes from a different directory, swe-as3/. So the headline pairs sync from one run against async from another.

I checked whether the discarded arms were dropped for being unfavourable, and they were not. Both are genuinely invalid:

swe/rows-...-async.json      astropy-12907  reward=None steps=None exception=True
swe-async2/rows-...async.json  both tasks   reward=0.0  steps=2    exception=True (wall 2.3s / 2.6s)

A nulled trial and a pair of 2-step aborts. swe-as3 is the only clean async arm (2/2 solved, no exceptions). Discarding the others is correct — and is exactly the degenerate-trial exclusion this project had to learn the hard way on the Terminal-Bench study.

Paired per task, which is the honest framing

task	steps sync→async	cw/1M-read sync→async	reward
astropy-12907	12 → 10	58,983 → 27,207	1.0 → 1.0
astropy-14365	19 → 33	23,625 → 17,877	1.0 → 1.0

Both tasks move the same direction, and — importantly — the one whose step count nearly doubled (19 → 33) still shows lower normalised cache-write. That is the case where trajectory noise would most plausibly explain the result away, and it doesn't. Terminal-Bench replicates independently (tb3: 21,689 → 13,185, −39%, again with async taking more steps: 60 → 111).

So the direction survives per-task pairing on both benchmarks, in both cases with async on the longer trajectory. That is stronger evidence than the aggregate table conveys.

What the PR should add

State plainly that sync and async came from separate runs (swe/ and swe-as3/), that two async arms were discarded for exception: true trials with the specific reason, and give the per-task paired table above rather than only the aggregate. As written, a reader who opens swe/ will find an async arm that contradicts the headline and no explanation. That is a documentation gap, not a data problem — but it is the kind that erodes trust in an otherwise well-evidenced result.

On the remaining open concerns

• dropped/stale_discarded = 0 across all nine trials is the gap I'd rank highest. The stale-discard path is the single most important correctness invariant in this PR, and production load never exercised it. Tests cover it; that's acceptable to merge on, but it should be called out as untested-in-anger rather than listed among passing metrics.
• The negative control on TB projection (1.02% actual → 0% projected, because extract_llm made zero calls) is a genuinely good validation and is currently underplayed. A projection that correctly reads near-zero when there is nothing to project is worth more than the exact-agreement case.
• The extract_llm-made-zero-calls finding also cross-confirms perf(extract_llm): economic gate, global result cache, and the honest verdict (#28) #34, which independently concluded the component is net-negative on caching backends, and feat(cmdfilter): port rtk's filter coverage (3 -> 24) and fix three DSL loss-typing bugs #37, which measured it burning 17.8 s across 2 calls for 0 tokens saved. Three independent lines of evidence now point the same way.

Verdict from my side: the mechanism and the direction are sound and replicated. Fix the provenance disclosure before merge.

[OsherElhadad]

Independent review — verdict: request changes

I did not write this code. I built a scratch worktree at feat/i31-modes (d550bbe), ran
go test -race ./... (green), -race -count=5 on modes/apply/store/metrics and
-count=3 on proxy (green, no flakes), go build, go vet, gofmt -l (all clean).
The concurrency primitives themselves are sound — I could not construct a data race, a
lost update in Tracker, a dedup window in Pool, or a goroutine leak on Stop.

The problems are not races. They are semantic: the generation does not mean what the
PR says it means, the cache protection has three holes that turn it into a cache-write
regression on the exact backend it was written for, and two of the headline numbers are
tautologies. Each finding below has a runnable reproduction I ran against this branch.

---

S1 — The tail cache protection is defeated by any agent that sets its own breakpoints (claude-code does)

components/reformat/cacheinject.go:155-168. The policy only deletes blocked positions
from the want set. Caller-set breakpoints are handled after, at line 172-178, and are
never removed:

c.MaxCachedIdx=21, NoCacheAtOrAfter=22, agent set its own breakpoint on msg 29
=> breakpoints = [19 21 29]   // 29 is inside the doomed tail

The cacheinject doc comment (v2's whole design) says "keeps every breakpoint the caller
set". claude-code sets its own tail breakpoint on every turn. So on the primary target
workload the protection places its safe anchor at 21 in addition to the agent's unsafe
one at 29 — the doomed tail is still cache-written, and async is now strictly worse than
sync plus one wasted slot. This is the failure mode #31 explicitly warns about,
arriving through the one path the tests don't cover.

Fix: when TailCachePending, strip cache_control from blocked indices before counting
existing. That is the only version of this policy that is actually a policy.

S2 — Every async session's first turn writes no cache at all

apply/apply.go:206-212. First turn: prevLen=0 → maxCachedIdx=-1 →
noCacheAt = max(0, 0) = 0 → CacheBlocked(i) is true for all i, and the
"highest safe index" fallback is -1, so nothing is placed. Reproduced: 40 messages,
breakpoints=[] skipped=true. The existing TestWholeRequestPendingPlacesNothing
asserts this as correct, treating NoCacheAtOrAfter == 0 as "the whole request is
doomed tail" — but on turn 1 nothing is pending yet, and turn 1 is precisely the turn
that must write the prefix so turn 2 can read it at 0.1x. Suppressing it bills the whole
prefix at 1.0x on turn 2. On short sessions (the common case) that is the entire cache
benefit gone.

Fix: on the first turn there is no pending compaction for this session, so
tailPending must be false. Gate on "a job is actually in flight for this session",
not on mode == async.

S3 — cache_mode: off + async suppresses every breakpoint on every turn, forever

apply/apply.go:206-212 sets tailPending = true whenever mode is async and not
bypassed — including when cacheAware is false, where maxCachedIdx never leaves -1.
Result: NoCacheAtOrAfter is permanently 0 and cacheinject places nothing, permanently.
Reproduced. Ironic given cache_uncompacted_tail: true exists as the escape hatch for
exactly this deployment; the two knobs interact backwards.

Fix: if mode == ModeAsync && !o.Deferred && !bypass && cacheAware && !o.CacheUncompactedTail.

S4 — The protection is off by one turn: it blocks the wrong region

The blocked region is [maxCachedIdx+1, ∞) — this turn's new messages. But the
pending job was built from the previous turn's body, so the span it will replace is
the previous turn's tail, which by turn N+1 sits below the boundary and is
unprotected. Reproduced: turn N boundary 22 (job targets 22..29); turn N+1 boundary 30
places its top breakpoint at 29 — inside the span the still-pending job will rewrite.

So the protection blocks a region no pending job targets, and permits the region every
pending job does target. Combined with S1/S2/S3 I do not think the claimed mechanism
holds at all, which brings me to:

S5 — "cache-write went down 17.8%" is not evidence the policy works

docs/results/operating-modes.md:44-58 presents −17.8% cache-write as "the result the
policy was designed for". Given S2/S3, async's cacheinject frequently places fewer or
zero breakpoints — which lowers cache-write for the trivial reason that less is
written, while raising uncached input on later turns. The doc's own numbers are
consistent with that reading: cache-read went up 49% in the same arm. At n=1 over 2
tasks with different trajectories (15.5 vs 21.5 steps) this is not causal, and the
paragraph "this arm cannot be reconciled with a cache policy that was rewriting the live
zone" overclaims — it also cannot be distinguished from a policy that stopped writing
cache. Please either drop the causal claim or add the one measurement that separates
them: breakpoints placed per request, sync vs async.

S6 — The generation counts commits, not turns, so CommitIfCurrent never rejects a genuinely stale result

This is the invariant the PR calls "the one everything rests on". Tracker.gen advances
only in CommitIfCurrent. A turn does not advance it. So a job built from turn 1
still sees gen == 0 as current after turns 2…N have been forwarded, and commits.

Reproduced through the real handler (6 turns, mask, cache on):
deferred_runs(committed)=3, StaleDiscarded=2. The 2 discards are dedup collisions
(two jobs at the same gen), not "the session moved on". Every commit that did happen
wrote a result built from a snapshot the session had already moved several turns past —
the exact situation the guard was written to catch — and none was flagged.

The frozen decisions are content-hash-keyed so this is not corruption today, but the
guard does not provide the property claimed in the PR body, docs/design.md, and
docs/how-to/operating-modes.md:85-89. Either advance the generation on Turn (and
accept more discards), or restate the invariant honestly as "at most one commit per
generation" — which is what the code actually enforces.

S7 — async_realized_saved_tokens is a tautology

proxy/modes.go:59-62 calls RecordRealized(res.Run.Saved()) on every async request
whose inline pass saved anything, with no check that any deferred job ever committed.
Reproduced on the first turn with a purely deterministic pipeline (dedup, no model,
nothing to defer): saved=396, realized=396. So
async_realized_saved_tokens == saved_tokens by construction, not by measurement.

That means docs/results/operating-modes.md:70 — "async_realized_saved_tokens = 15,962
= the entire enforced saving … every token async saved was saved by a later turn
replaying an earlier turn's off-path job" — is circular. The equality is what the code
computes, not a finding. To make it real, credit only savings whose frozen decision was
written by a committed deferred run (e.g. tag replayed decisions, or diff against what
the inline pass would have saved with an empty store).

S8 — Observe mode does contaminate two pre-existing enforced /stats fields

TestObserveMetricsCannotBeSummedIntoEnforcedTotals checks requests, tokens_*,
saved_tokens, components, sync/async_enforced. It does not check the other
enforced-namespace fields, and two are non-zero in observe mode:

• cg_added_ms_avg — proxy.go:438 records added latency in every mode. Reproduced:
  requests=0, cg_added_ms_avg=0.0087. deploy/harbor/swebench.py:305 reads this
  straight into s["cg_added_ms_avg"], so an observe arm reports a real-looking
  overhead figure for requests it never touched.
• llm_calls / llm_input_tokens / llm_output_tokens — proxy.go:620 reads the
  global cheapmodel.Usage(), which observe's off-path runs increment. swebench.py
  turns these into cg_llm_cost. The results doc itself reports observe spending
  $0.0779 of cheap-model tokens, so this is known — but it lands in the enforced
  vocabulary a harness reader sums.

Neither is a hypothetical mislabelled as a saving, so the headline claim survives. But
"in observe mode every enforced aggregate is zero by construction" is not true as
written. Either move these under potential_* in observe mode or amend the claim in the
PR body, metrics.go:359-366, and the how-to.

S9 — An unproductive session re-runs a full off-path compaction on every turn, unbounded

The generation only advances on commit and runOne releases the pending slot
unconditionally, so a job that finds nothing to compact is re-enqueued at the same key
every turn. Reproduced: 10 turns of one session → 10 off-path jobs, gen still 0.
Each is a real cheap-model LLM call in production.

TestAsyncDoesNotSpinOnAnUnproductiveTurn only asserts the queue stays bounded and 12
requests were forwarded — it never counts jobs run, so it passes while the spin happens.
This is the answer to "what if turns arrive faster than compaction completes,
indefinitely": nothing bounds the spend, only the queue depth. Suggest a per-(session,
generation) attempt cap, or advance the generation on a completed-but-unproductive job.

S10 — Tracker eviction lets a stale gen-0 result commit, and fails open into mutating the cached prefix

modes/tracker.go:64-72 evicts an arbitrary session at the cap and recreates it at
gen=0, prevLen=0. Reproduced with NewTracker(2): session A's turn-1 job (100 msgs)
commits over a session now at 400 msgs, because both read gen == 0. The comment claims
a forgotten session "restarts at generation 0 — correct, just missing the pending job's
savings"; it is not correct, it is the one thing the guard exists to prevent.

Worse, prevLen resetting to 0 makes MaxCachedIdx = -1, and Ctx.TailOnly returns
true for every index — so offloaders are free to mutate the whole already-cached
prefix. That is #25's fail-open direction, reachable here through eviction alone. At 1000
sessions with random eviction this is rare but not negligible.

Fix: LRU rather than arbitrary eviction (the tracker already has the map; Turn is the
touch point), and make eviction leave a tombstone or start recreated sessions at a
generation no in-flight job can hold.

S11 — Pool.Stop() blocks for as long as the in-flight job takes

modes/pool.go:191-201 cancels then wg.Wait()s. A running job that does not poll ctx
— and the cheap-model HTTP client has a 5-minute timeout — holds Stop for its full
duration. Reproduced: Stop blocked 1.5 s on a time.Sleep job. main.go does
defer h.Close(), so process shutdown inherits it. TestStopLeaksNoGoroutines waits for
the job to finish before calling Stop, so it never exercises this. Suggest a bounded
wait in Stop (the jobs are pure savings; abandoning one is already the documented
trade) and a test that stops mid-flight.

---

Minor

• store/buffer.go:60-70 — Sticky() calls b.Base.Sticky(session) before taking
  b.mu. Not a race (both are individually locked) but it means a concurrent Commit
  can be observed half-applied through this one method. Worth a note; harmless today
  because only one worker touches a given buffer.
• store/buffer.go — an uncommitted Buffer is dropped with its job closure, so no leak.
  Verified. But a dropped enqueue never constructs one, and a job cancelled mid-run
  leaves the partial writes garbage-collected. Fine — no action, recording that I checked.
• metrics.go:228-267 observeComp is a near-verbatim copy of the second half of
  Component. Two copies of the unique-savings attribution will drift. Extract the
  shared body and pass the target map.
• Aggregator.Run returns early on r.Deferred before the observe branch, so a
  deferred observe run (can't happen today — observe never sets Deferred) would silently
  vanish. Order these the same way as in Component for consistency.
• docs/how-to/operating-modes.md:85-89 and docs/design.md state the S6 invariant as
  implemented. They need to match whatever S6 resolves to.
• Docs: I could not run mkdocs build --strict (mkdocs is not installed on this box), so
  the green claim is taken on trust. Nav entries and the relative link from
  reference/config.md check out by inspection.

What is genuinely good

• Tracker is correct as a mutual-exclusion primitive; CommitIfCurrent running the
  commit under the generation lock is the right shape and I could not break it.
• Pool's claim-before-observable dedup is real — the slot is taken under mu before
  the channel send, and rolled back on a full queue. Atomic as advertised.
• Panic containment verified: runOne's recover is inside the deferred func that also
  releases the slot, so a panicking job neither kills the worker nor leaks its key.
• Structural observe byte-identity (never running the pipeline, skipping expand.Inject)
  is the right design, and TestObserveForwardsByteIdenticalBody genuinely asserts it.
• sync byte-identity: verified, and TestCompactEndpointIgnoresMode covers the
  /compact regression risk. adapters/bifrost still goes through BodyFull, whose
  shim is a faithful translation. No security issues found — no credential, token, or
  gateway URL in the new code, tests, docs, or PR body.

Bottom line

The async machinery is well built. What it is doing is not what the PR says, in three
places that matter: the cache policy has holes on its primary target workload (S1–S4),
the stale-result invariant is not the one implemented (S6), and two headline numbers are
tautological (S5, S7). S1, S2, S3, S6 and S7 should block; S8–S11 are fixable follow-ups
if scoped. Every finding above has a reproduction I ran on this branch and can hand over.

[OsherElhadad]

Correction to my own earlier comment — the cache-write result is not yet causal

I verified the per-task pairing above and concluded the direction "survives per-task pairing on both benchmarks." That was premature, and the independent review has a better explanation than mine.

The reviewer found (and I confirmed at components/reformat/cacheinject.go:157-166) that the async cache protection only deletes from context-guru's own want set — caller-set breakpoints are counted afterwards and never stripped. So on claude-code traffic, which sets its own tail breakpoint, the doomed tail is still cache-written. The protection does nothing on the primary workload.

Combined with two more findings — every async session's first turn places zero breakpoints (prevLen=0 → NoCacheAtOrAfter=0 → everything blocked), and cache_mode: off + async suppresses breakpoints entirely — there is a much more mundane reason for lower cache-write in the async arms: async often wrote fewer or no breakpoints at all. Fewer writes because fewer breakpoints is not the mechanism this PR claims. And cache-read rose 49% in the same arm, which is what you'd expect if the prefix simply wasn't being anchored.

So my "both tasks move the same direction, including the one whose step count nearly doubled" observation is still factually true, but it does not support the causal reading I gave it. Two arms can both show lower normalised cache-write for a reason that has nothing to do with the cache policy working. I should have checked whether the policy actually fired before treating the outcome as evidence that it did — the same discipline I applied to the degenerate-baseline and xdedup cases, and I didn't apply it here.

Revised position: keep the numbers, relabel them as not-yet-causal, and re-measure after S1–S4 are fixed. The latency result (1,599 ms → 25 ms) is unaffected and stands on its own — that one doesn't depend on the cache policy at all.

Also retracting my characterisation of async_realized_saved_tokens = 15,962 = the entire enforced saving as validation of end-to-end deferral. I verified proxy/modes.go:61 fires on any async request that saved anything, with no commit check, so the equality is circular by construction.

This is the fourth premise in this workstream to fall to "check whether the mechanism fired before believing the outcome" — after xdedup, the expand-tool registration, and prefixpin's early-index churn. The pattern is consistent enough to be worth stating as a rule for the final write-up: an aggregate moving in the predicted direction is not evidence the predicted mechanism operated.

[OsherElhadad]

Review response — all seven blocking findings fixed, plus the four follow-ups

Thanks — this was a genuinely good review. The three where the code and the docs
disagreed were the worst of it, and you were right to rank them first: the docs were the
specification, so those weren't bugs, they were false claims.

I reproduced S1, S2, S6 and S7 before touching anything. S1 gave me [19 21 29] at
boundary 22, exactly as reported. S2 gave []. S6 committed a turn-1 job after eight
later turns. S7 I fixed from the code path alone — the missing gate is plain once you look.

The three code/doc disagreements

S1 — protection was inert on the primary workload. Fixed. The loop now also covers
caller-set breakpoints inside the protected span, and since removing a directive an agent
deliberately placed is a change to someone else's request, the host chooses:
async.strip_caller_breakpoints: true strips them, or cacheinject calls
Ctx.DeclineTailProtection() and the proxy does not defer that turn
(async_tail_unprotected_turns). I took your second option as the default — with
claude-code on defaults async is now deliberately inert rather than falsely protective,
and the counter says so. Two tests: one per branch.

S6 — the generation now advances per turn. This was the deepest one. Fixing it
surfaced a second defect you didn't see: turn accounting lived inside the cache-aware
branch, so with cache_mode: off every generation stayed 0, which both disabled the guard
and collided with 0's use as "nothing pending". Moved out — a turn happens whether or not
the backend caches.

I also had to make CommitIfCurrent idempotent per generation: it previously
self-superseded via gen++, and once that moved to Turn sixteen concurrent jobs at one
generation could all commit. Dedup prevents that in production, but the guard should be
exact alone.

The honest consequence is now in the docs rather than buried: at agent turn rates a
compaction taking tens of seconds is usually superseded before it lands, so async
discards much of what it pays for. stale_discarded is the number that decides whether
async suits a workload, and I stopped calling it a tuning nit.

S7 — tautology gone. Gated on Tracker.Landed(session). The test now asserts
realized is a strict subset of saved_tokens, since equality is the signature of
the old bug. Corrected measurement: realized=4125 of saved=4948, one legitimate stale
discard. Retracted from the PR body and docs/results/operating-modes.md.

S2/S3/S4

All three were the same root shape — the protection firing with nothing to protect — so
they're one guard now: async and cache-aware and something actually pending, with
the span taken from Opts.PendingFrom (the previous turn's tail, tracked through
Tracker.SetPending/ClearPending on every terminal job path, including "ran and
produced nothing" so it can't latch on forever).

You were right that a test encoded S2. TestWholeRequestPendingPlacesNothing asserted
turn-1 suppression as correct; it's now TestFirstTurnStillWritesThePrefix asserting the
opposite.

S5 — claim withdrawn

Agreed, and it's worse than unpaired arms: given S1 the mechanism wasn't running, so the
numbers can't be evidence for it. Kept as measurements, relabelled, with the reasons named
(inert protection; fewer breakpoints from S2/S3 lowering cache-write arithmetically;
unpaired arms). Mechanism is verified by test; the effect needs a 50-task paired re-run
with strip_caller_breakpoints: true. I did not re-run benchmarks to re-establish it —
that's a fresh 50-task arm, not something to slip in under a review fix.

Follow-ups

• S8 — split rather than blanket-fixed, because the two fields differ. cg_added_ms_avg
  is a real measurement of the enforced path, and it reading ~0 in observe is the
  headline; zeroing it would hide the result. llm_calls/llm_input_tokens/
  llm_output_tokens are real money really spent, so moving them into potential_* would
  be a worse lie than leaving them — they stay where cost tooling reads them, labelled by a
  new observe_llm_notice. The over-broad claim is corrected in the how-to and the test.
• S9 — Tracker.Barren: three consecutive unproductive jobs and the session stops
  enqueueing; any productive job resets it. Flat count, not a backoff — marked ponytail:
  with the reasoning.
• S10 — the tracker keeps an issued high-water mark across all sessions, so a
  recreated session starts above any generation a surviving job could hold. prevLen
  deliberately still resets to 0: it's a claim about what the provider cached, and after
  eviction we don't know. Read fix(cache): slide the store TTL, pin frozen decisions, and repair lost ones #40 first as you suggested — no conflict, and I added
  structural forwarding of FrozenLost on store.Buffer, since a component asserting
  c.Store.(store.FrozenLoser) sees the wrapper on every off-path run and would have
  silently taken the degraded path. Asserted on a locally-declared shape so it compiles
  before fix(cache): slide the store TTL, pin frozen decisions, and repair lost ones #40 and binds on merge with no edit.
• S11 — Stop() bounded at 2s and now returns bool. Cancelling can't interrupt an
  in-flight HTTP call to the cheap model, and nothing waits on that result.

Gates (re-run after the fixes)

build · test -tags cg_skeleton · -race -count=5 · gofmt -l · go vet ·
mkdocs build --strict — all clean.

One thing I'm leaving

Async's value is now visibly conditional: inert on claude-code unless
strip_caller_breakpoints is set, and much of what it computes is discarded at agent turn
rates. That's the honest state and it's documented as such. Whether it's worth shipping on
that basis is a call worth making explicitly rather than having the docs imply an
unconditional win — happy to split async out and land observe alone if you'd rather.