fix(ai): step cache breakpoints down instead of disabling caching outright - #3947
Conversation
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
b10c5c2 to
9f8cff9
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Terminal review — fix(ai): step cache breakpoints down instead of disabling caching outright
Reviewer: maintainer red-team (emergency AI-subsystem lane)
Head reviewed: 9f8cff9fabc99dce4c05e0e3b2569b0482d02298
Base declared: dev @ 3765d8422
Verdict: NOT_MERGE_READY — rebase + re-run required (hold, do not merge at this head)
1. Diff vs. merged #3943 and current dev
#3943 (merged as a884843b, now in dev) introduced a binary kill-switch: on a A maximum of 4 blocks with cache_control may be provided rejection it set generatedCachingDisabled = true and retried once with all generated caching off, sticky for the provider session.
This PR replaces that with a graded monotonic stepdown:
type GeneratedCacheBudget = 2 | 1 | 0;
- budget
2→ normal explicit mode (conversation-prefix anchor + current-turn refresh point). - on overflow rejection → step to
1(keep the higher-value prefix anchor, drop the current-turn marker). - on a second overflow rejection → step to
0(== #3943's behavior,mode: "none"). - session-sticky in both directions;
close()resets to2.
The state shape change (generatedCachingDisabled: boolean → generatedCacheBudget: 2|1|0) is threaded through every callsite: getCacheControl, buildParams, applyPromptCaching, applyExplicitPromptCaching, the stream retry block. No stale references to the old field remain (grepped the whole tree). Rename is clean.
Red-team — is the stepdown safe?
- Termination: each rejection strictly lowers budget (
2→1→0); thebudget === 0arm ingetCacheControlreturnsmode: "none", andapplyExplicitPromptCachingearly-returns onbudget === 0. At most two self-induced retries per turn, then the classifier stops claiming. No infinite loop, bounded exactly like #3943 plus one. ✅ - Value ordering is correct: the prefix anchor (last completed assistant turn) is spent first, the cheap current-turn refresh second. An endpoint that frees exactly one slot keeps caching the reusable prefix — strictly better than #3943's all-or-nothing. ✅
- First-turn edge case is handled: on a turn with no prior assistant message, the prefix-anchor loop places nothing and
remainingstays at budget, so the reduced budget still reaches the current-turn marker instead of emitting zero markers. The new testgives up generated caching only after the reduced budget is also rejectedplus the rewritten single-turn proxy-SSE test pin this. ✅ - Automatic mode is budget-correct: automatic emits exactly one top-level marker; any non-zero budget covers it, and
budget === 0returns early before thecache_controlassignment. The added comment is accurate. ✅ - No widening of the claim: the overflow guard is unchanged (
!fallbackManaged && firstTokenTime === undefined && isAnthropicCacheBreakpointOverflowError(...)), so unrelated 400s / non-400 / our own pre-flight failure still surface immediately. Only the recovery action changed. ✅ - Docs (
docs/models.md) match the new semantics. ✅
This is a strict improvement over #3943 with no regression surface I could find. The behavior delta is: "endpoint with one free slot now keeps prefix caching" instead of "loses all generated caching on first rejection."
2. Adversarial test / CI assessment (run locally against the diff applied to current dev)
| Check | Result |
|---|---|
packages/ai/test/anthropic-stream-envelope.test.ts + anthropic-cache.test.ts |
47 pass / 0 fail (incl. both new stepdown cases + rewritten overflow tests). Baseline dev = 46 pass; PR adds +1 net. |
Broader matrix: anthropic-cache, anthropic-stream-envelope, anthropic-retry, anthropic-thinking-repair-retry, auth-gateway-anthropic-caching, auth-gateway-anthropic-to-codex-caching |
69 pass / 0 fail (2 e2e correctly skipped: no E2E env). |
bun --cwd=packages/ai run check:types (after regenerating the missing docs-index.generated, which is a pre-existing local-install artifact, identical failure on clean baseline dev) |
pass (0 new type errors from this PR). |
grep -rn generatedCachingDisabled (whole tree) |
0 hits — rename complete. |
Exact-head CI on GitHub (head 9f8cff9fa): all PR-relevant jobs pass — check:@gajae-code/ai, test:packages/ai/test/anthropic-stream-envelope.test.ts, native-build, root-check, cli-smoke, ts-build (stats + coding-agent), 5/6 cargo-build, all gjc-state-gates, Local public surfaces, Telegram daemon generation guard. One job still pending: cargo-build: pi-natives (Rust build of a crate this PR does not touch — expected to pass, but not green at the moment of review).
3. Why NOT_MERGE_READY at this head — concrete blockers
Blocker A — branch is CONFLICTING / mergeStateStatus=DIRTY vs current dev. This is the deciding factor; GitHub will not produce a clean merge of the exact head you asked me to review.
Root cause: dev advanced after the force-push at 12:30:39Z with the 0.12.15 release back-merge (473eab944 merge: bring main's corrected 0.12.15 release back into dev), which re-landed #3943 (0c836b216) and relocated its changelog entry. A real test-merge (git merge 9f8cff9fa into latest dev) conflicts in exactly two files — both bookkeeping, neither the actual code:
packages/ai/CHANGELOG.md— the PR rewords the cache-breakpoint entry, but that entry now lives in the released## [0.12.15] - 2026-08-06section (the[Unreleased]slot the PR targets is empty on current dev). Resolving this is not a blind "take theirs": per repo policy (AGENTS.md: "add entries under## [Unreleased], never edit released sections") the contributor must rebase and place the new stepdown wording under[Unreleased], leaving the released0.12.15entry describing #3943's behavior untouched. This needs the contributor, not a merge-bot.artifacts/issue-3670-anthropic-cache-eval.json—providerSourceBlobOid/providerSourceSha256pins diverged (dev =e4dcc79e1a…, PR =9e400ada3f…). This is a generated eval hash; it must be regenerated against the mergedanthropic.tsviaWRITE_ISSUE_3670_EVAL=1 bun test packages/ai/test/anthropic-cache-eval.integration.test.ts. Not mechanically mergeable.
The substantive files — packages/ai/src/providers/anthropic.ts and packages/ai/test/anthropic-stream-envelope.test.ts — merge cleanly. There is no semantic conflict.
Blocker B — exact-head CI not fully green. cargo-build: pi-natives is still pending at review time. It will almost certainly pass (unrelated crate), but the condition "exact head CI green" is not literally met yet.
4. Overlap check
No functional overlap blocker. Searched all open PRs targeting dev: only #3927 (ClinePass / Command Code GOAT, feat(providers)) shares a file (docs/models.md), and it does not touch anthropic.ts, the cache code, the test, or the cache-eval artifact. Its docs/models.md edit is in a different section. No conflict path overlaps #3947's code.
5. Required remediation (hold for contributor)
- Rebase onto current
dev(473eab944+) to clear the race. packages/ai/CHANGELOG.md: add the stepdown wording as a new entry under## [Unreleased]; do not edit the released## [0.12.15]block (that entry correctly describes the shipped #3943 behavior).- Regenerate
artifacts/issue-3670-anthropic-cache-eval.jsonwithWRITE_ISSUE_3670_EVAL=1 bun test packages/ai/test/anthropic-cache-eval.integration.test.tsagainst the rebasedanthropic.ts. - Push and let exact-head CI go green (it already passes on every job that touches this PR's files).
Once rebased conflict-free with green exact-head CI, this is MERGE_READY — the code is verified correct and is a strict improvement over merged #3943. Not merging at the current head because the two conditions above (conflict-free exact head, fully green exact-head CI) are not satisfied.
Signed: maintainer red-team, emergency AI-subsystem lane. Rebased re-review offered on next force-push.
…right The first cut of this recovery treated the overflow rejection as a kill switch: one 400 and generated caching went off for the whole session. That overcorrects. The rejection says "too many", not "none allowed", and explicit mode emits two markers -- a conversation-prefix anchor on the last assistant turn plus a refresh point on the current turn. A gateway leaving one free slot would have kept caching its prefix, but got nothing. Recovery is now graded: 2 -> 1 -> 0. The first retry drops the current-turn refresh point and keeps the prefix anchor, which is the marker that actually covers the reusable conversation. Only a second rejection gives caching up entirely. The reduced budget is sticky per provider session so later turns neither re-trigger the 400 nor lose more caching than the endpoint requires. The anchor consumes budget only when a marker is actually placed, so a first turn with no assistant history still emits its single current-turn marker under a reduced budget rather than silently emitting nothing. Lore-id: 28f83611 Constraint: an endpoint with one free slot must keep caching the conversation prefix Constraint: reduced budget must never emit zero markers while a slot remains usable Rejected: disable all generated caching on first rejection | silent cost regression on gateways with a free slot Rejected: count breakpoints before sending | gateway-added markers are invisible in our params Confidence: high Scope-risk: narrow Reversibility: easy Tested: graded step-down, full give-up after two rejections, statusless proxy SSE form, unrelated 400 leaves budget intact; all three fail on the parent commit Not-tested: live traffic against the reporting gateway
9f8cff9 to
bceb809
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Post-rebase re-review — graded cache-breakpoint stepdown
Reviewer: maintainer red-team (emergency AI-subsystem lane)
Rebased head: bceb8091e (was 9f8cff9fa)
Base: dev @ 473eab944
Verdict: MERGE_READY
This re-review follows the owner-directed rebase of this PR onto current dev. The original technical findings (adversarial code review of the generatedCacheBudget: 2|1|0 stepdown vs. merged #3943's binary kill-switch) are unchanged and still hold — see the prior review comment for the full analysis. This comment records only the post-rebase state.
Rebase resolution (owned conflicts)
The rebase from the stale base 3765d8422 onto current dev@473eab944 resolved exactly the two owned bookkeeping conflicts identified in the prior review — no code conflict:
packages/ai/CHANGELOG.md— the stepdown wording now lives under## [Unreleased](its correct home), and the released## [0.12.15]section is left untouched describing the shipped #3943 behavior, per repo policy ("add entries under## [Unreleased], never edit released sections").artifacts/issue-3670-anthropic-cache-eval.json— regenerated against the rebasedanthropic.tsviaWRITE_ISSUE_3670_EVAL=1 bun test packages/ai/test/anthropic-cache-eval.integration.test.ts. Verified:providerSourceBlobOid=git rev-parse HEAD:packages/ai/src/providers/anthropic.ts=git hash-object anthropic.ts=9e400ada3f74e4ff88652fe069aa2bd5766f9807. Consistent.
packages/ai/src/providers/anthropic.ts and the test merged cleanly (no conflict). Authorship preserved (probe <re2rar@gmail.com>). Exactly one commit on dev. git diff origin/dev..head = 5 files, +153/-44.
Local verification on rebased head
| Check | Result |
|---|---|
Test merge into dev |
clean, zero conflicts |
anthropic-stream-envelope + anthropic-cache + anthropic-retry + anthropic-thinking-repair-retry + anthropic-cache-eval.integration |
71 pass / 0 fail |
@gajae-code/ai typecheck |
clean |
Exact-head CI (new head bceb8091e)
Fully green: 19× success, 6× skipped (platform-specific), 0× in progress, 0× failure. Includes check:@gajae-code/ai, test:packages/ai/test/anthropic-stream-envelope.test.ts, native-build, all 6 cargo-build, both ts-build, all gjc-state-gates, root-check, cli-smoke.
Conditions for merge (owner directive)
- Terminal verdict
MERGE_READY - Exact-head CI green
- No overlap blocker (no open PR touches the cache-breakpoint code; #3927 shares only an unrelated
docs/models.mdsection)
All three conditions satisfied.
Signed: maintainer red-team, emergency AI-subsystem lane. Rebased re-review complete.
Remediation complete — all four items addressedHead is now 1. Rebase onto current
|
| Check | Result |
|---|---|
anthropic-cache + anthropic-stream-envelope + anthropic-cache-eval.integration |
49 pass / 0 fail |
bun --cwd=packages/ai run check |
pass |
packages/ai full suite |
2207 pass / 337 skip / 1 fail (pre-existing) |
The one failure is no-strict-env.test.ts > keeps strict mode on when neither name is set. It reproduces identically on unmodified origin/dev (verified via detached checkout in the same shell): the test spawns a probe without clearing an inherited PI_NO_STRICT=1 from the environment. Unrelated to this PR.
Thanks for the precise remediation list — especially catching that the changelog entry needed to move rather than be force-resolved. Both conditions you named (conflict-free exact head, fully green exact-head CI) are now satisfied.
gajae.pr-review-verdict.v1 needs-human sha256:bceb8091e4de09d8edf298c8ae7d2d726580f1d1 reviewer:human evidence:local-bun-test-packages-ai + exact-head-CI-23-success
Re-review welcome.
What
Follow-up to #3943 (merged as
a884843b3). That fix stops the cache-breakpoint-overflow 400 from killing the turn, but it recovers with a kill switch: one rejection and generated caching goes off for the whole provider session. This makes the recovery graded instead.Why
The rejection says "too many", not "none allowed". Turning caching off entirely on the first 400 discards more than the endpoint actually asked for.
applyExplicitPromptCachingemits two markers: an anchor on the last assistant turn, which covers the reusable conversation prefix, and a refresh point on the current turn. A gateway that leaves one free slot can accept the anchor — the marker that carries essentially all of the caching value — but under the merged behavior it gets nothing and pays full input cost on every subsequent turn.That is the same silent-cost-regression failure mode the review of the closed #3935 flagged when rejecting a static
nonedefault. #3943 avoided it in the default path and then reintroduced it in the recovery path. This closes that gap: caching is abandoned only when the endpoint proves it has no room at all.from/toare now logged on the step-down so the degradation is visible rather than silent.Edge case
The prefix anchor consumes budget only when a marker is actually placed. On a first turn there is no assistant history, so a reduced budget still emits its single current-turn marker instead of silently emitting nothing.
Testing
Four wire-level regressions:
All four fail on the parent commit (
a884843b3) and pass here — verified by stashing onlyanthropic.ts.artifacts/issue-3670-anthropic-cache-eval.jsonregenerated viaWRITE_ISSUE_3670_EVAL=1; it pins the provider source blob oid/sha256 by design, so it is re-derived on this base rather than carried forward.One pre-existing failure is unrelated and reproduces on unmodified
dev:no-strict-env.test.ts > keeps strict mode on when neither name is setspawns a probe without clearing an inheritedPI_NO_STRICT=1from the shell environment.Not tested: live traffic against the reporting gateway.
GJC verdict
No independent architect/critic review was run.
devbun checkpasses —bun --cwd=packages/ai run checkpasses; fullbun checkblocked by an unrelated pre-existingcheck:sdk-closureself-test failure ondev