fix(llm): stop rolling cache breakpoints from forfeiting marker slots - #936
Merged
Merged
Conversation
Follow-up review fixes for #933 (#921). - Fill breakpoint slots newest-first and consume a slot only when a marker actually lands, so a turn that cannot carry one (empty content, trailing tool_use) falls back to an older completed turn instead of being dropped. Previously the budget was truncated before markability was known, so a long history could ship one marker or none. - Replace the unreachable budget arithmetic (hasSystemBlocks/hasTools bools could only ever yield 2) with a reserved-slot count plus a separate anthropicMessageCacheBudget helper, pinned per reservation level by test. - Collapse the two identical branches in anthropicCompletedTurnEndIndexes and drop the prevGroupStart variable that only chose between them. - Route all four cache_control literals through anthropicEphemeralCacheControl. - Correct the doc comment: the fallback marker sits one completed turn back, not two, and the in-flight exclusion is a conservative choice rather than a free one — agent.Loop re-sends the same slice, so a marker there would be read by the next iteration. - Add the CHANGELOG entries #933 never wrote.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Review follow-up to #933 (#921). Cleanup and one real logic gap; placement rules are unchanged.
tool_use) cost a slot outright and the request shipped with one breakpoint or none. Slots are now filled newest-first and consumed only when a marker actually lands, so an unmarkable turn falls through to an older completed turn.hasSystemBlocks/hasToolscould only ever yield 2 — all four combinations collapse through the cap — and thebudget <= 0guard was dead. Replaced by a singlereservedBreakpointscount plusanthropicMessageCacheBudget, whose branches are now reachable and pinned per reservation level.anthropicCompletedTurnEndIndexes, plus theprevGroupStartvariable that existed only to choose between them.cache_controlliteral, now behindanthropicEphemeralCacheControl()(returns a fresh map per call — callers store it into blocks they do not own).agent.Loopappends each tool exchange to the same slice and re-sends it, so a marker on a completed in-flighttool_resultwould be read by the next iteration. The comment now states this is a deliberately conservative choice and names the in-flight marker as the next step.Deliberately out of scope
Actually placing a marker inside the in-flight group, and the
Tools: nilfinal call atinternal/agent/loop.go:431(tools precede messages in the cached prefix, so that call sits in its own cache lineage and pays a 1.25x write). Both change token spend rather than clean up code, and #933's test plan left thecache_read_input_tokensmeasurement unchecked — they belong in a PR that measures first.Test plan
[3]vs[1,3])make test— full suite greenmake vet, gofmt cleanmake lint-diff— 0 issuesmake test-cover-diff— 95.7% (45/47 changed lines), threshold 80%🤖 Generated with Claude Code