Skip to content

refactor: consolidate and harden Anthropic prompt-cache checkpoint handling#3712

Merged
Sayt-0 merged 8 commits into
docker:mainfrom
dgageot:worktree-board-3de226c403f5c4b5
Jul 17, 2026
Merged

refactor: consolidate and harden Anthropic prompt-cache checkpoint handling#3712
Sayt-0 merged 8 commits into
docker:mainfrom
dgageot:worktree-board-3de226c403f5c4b5

Conversation

@dgageot

@dgageot dgageot commented Jul 17, 2026

Copy link
Copy Markdown
Member

chat.Message.CacheControl (Anthropic prompt-cache checkpoints) was handled in seven places with implicit cross-layer contracts, which had already produced two production bugs: markers surviving compaction and exceeding Anthropic's 4-breakpoint limit, and a panic in extractSystemBlocks on empty flagged messages. This PR consolidates the handling to three logical owners and makes the known failure modes structurally impossible.

On the deduplication side, the two hand-rolled 5-case cache-control switches are replaced with the SDK's GetCacheControl union accessor (verified to cover every block kind the converters produce, on both standard and Beta unions — this test already caught a regression when anthropic-sdk-go v1.57→v1.58 landed on main mid-branch). The shared message-breakpoint budget helper is extracted, and the entire mechanism — budget ledger, system-block marking, message-tail breakpoints, system-block extraction — is consolidated into pkg/model/provider/anthropic/cache_control.go.

On the robustness side, system cache breakpoints are now capped so a request can never exceed Anthropic's 4-breakpoint limit regardless of upstream marks; excess marks are dropped and debug-logged, and the worst case is pinned by test. session.CompactionInput clears Cost/CacheControl on its own copies, removing the compactor's mirrored clearing loops. session.AddMessage strips CacheControl at ingestion, enforcing the documented contract that transcripts never carry marks — guarding against API clients that echo marks back. chat.Message.CacheControl now documents its full contract, including why the field must stay JSON-serialized (marks must survive before_llm_call hook rewrite round-trips; out-of-band boundary indices would be silently misplaced by hook message inserts/deletes).

Behavior changes only affect inputs that previously caused API rejections or panics. All other paths are byte-identical, confirmed by e2e cassette replay.

dgageot added 8 commits July 17, 2026 11:03
…points

The SDK's ContentBlockParamUnion.GetCacheControl (and its Beta twin)
returns a pointer to the variant's CacheControl field for exactly the
block kinds we handled by hand (text, tool_use, tool_result, image,
document), so both hand-rolled switches collapse to a single assignment.
…nverters

Both the standard and Beta message converters duplicated the same
comment and 4-breakpoint budgeting logic; hoist it into
messageCacheBreakpoints.
…trol.go

Pure move: messageCacheBreakpoints, applyMessageCacheControl, and
applyBetaMessageCacheControl relocate unchanged so the prompt-caching
breakpoint logic lives in one file.
… the limit

The 4-breakpoint budget was an implicit cross-layer contract: the
session marks up to two system messages and the provider spends the
rest, but nothing enforced the global invariant \u2014 a third upstream
mark (e.g. from a hook-rewritten message list) would produce a request
Anthropic rejects outright (the failure mode of docker#2236).

Own the whole budget in cache_control.go: document the ledger and cap
honored system marks at maxSystemCacheBreakpoints, dropping the excess
instead of forwarding it. Behavior is unchanged for compliant input.
Cost and CacheControl clearing lived in two mirrored copies (the
compactor's CompactionInput wrapper and the compaction benchmark),
each a forget-to-clear hazard for the next consumer \u2014 exactly how
where the message copies are made: every consumer now gets clean input
by construction, and the compactor and benchmark no longer handle
cache control at all.
…control.go

Pure move of extractSystemBlocks and extractBetaSystemBlocks so the
entire checkpoint mechanism (budget ledger, system-block marking with
cap, message-tail breakpoints) lives in one file, and document the
CacheControl contract on chat.Message: request-assembly state, set by
the session on assembled copies, JSON-serialized only because marks
must survive the before_llm_call hook rewrite round-trip.

Note: deleting the field in favor of out-of-band boundary indices was
considered and rejected \u2014 hooks.Input.Messages / updated_messages
round-trip assembled messages through external hook processes as JSON,
and per-message marks are the only representation that survives
hook-driven message-list rewrites (index-based boundaries would be
silently misplaced by an insert/delete).
… accessor contract

Two hardening follow-ups to the breakpoint-budget work:
- log (debug) when an over-budget system mark is dropped, so upstream
  misbehavior is diagnosable instead of silent
- pin GetCacheControl coverage for every block kind the converters
  produce, on both the standard and Beta unions, so an SDK bump that
  changes accessor coverage fails tests instead of silently changing
  which requests get cached
…marks

The contract on chat.Message.CacheControl said stored transcript items
never carry marks, but nothing enforced it \u2014 an API client echoing a
mark back could persist it and have it resurface on every future
prompt assembly. Strip the flag at AddMessage ingestion (alongside the
existing capToolResultContent normalization) and reword the
CompactionInput clearing rationale in session terms rather than
compactor internals.
@dgageot
dgageot requested a review from a team as a code owner July 17, 2026 09:49

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

This refactor cleanly consolidates the Anthropic prompt-cache checkpoint handling. The budget arithmetic is correct (tool + system + message ≤ 4 in all combinations), the AddMessage ingestion strip and CompactionInput clearing are correctly implemented, and the GetCacheControl() accessor swap is sound (verified by the SDK coverage test that pins all block kinds). One low-confidence edge case in extractSystemBlocks is noted inline.

Comment on lines +62 to +79
if len(msg.MultiContent) > 0 {
for _, part := range msg.MultiContent {
if part.Type == chat.MessagePartTypeText {
if txt := strings.TrimSpace(part.Text); txt != "" {
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt})
}
}
}
} else if txt := strings.TrimSpace(msg.Content); txt != "" {
// Trim system-message content: YAML literal blocks (instruction: |) always
// append a trailing newline, and we don't want that in API payloads.
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{
Text: txt,
})
}

if msg.CacheControl && len(systemBlocks) > 0 {
marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[low] extractSystemBlocks: CacheControl=true system message with all-whitespace MultiContent silently mismarks the previous message's last block

When a system message has CacheControl=true and MultiContent composed entirely of whitespace-only text parts, it contributes zero blocks to systemBlocks. The guard len(systemBlocks) > 0 is satisfied by earlier messages, so markSystemBlockCacheControl fires on the previous message's last block instead of the intended boundary — silently attributing the cache checkpoint to the wrong location.

// Problematic path: msg contributes no blocks but still triggers mark
if msg.CacheControl && len(systemBlocks) > 0 {
    marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked)
}

A minimal guard that checks whether this message contributed any blocks would prevent the misattribution:

Suggested change
if len(msg.MultiContent) > 0 {
for _, part := range msg.MultiContent {
if part.Type == chat.MessagePartTypeText {
if txt := strings.TrimSpace(part.Text); txt != "" {
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt})
}
}
}
} else if txt := strings.TrimSpace(msg.Content); txt != "" {
// Trim system-message content: YAML literal blocks (instruction: |) always
// append a trailing newline, and we don't want that in API payloads.
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{
Text: txt,
})
}
if msg.CacheControl && len(systemBlocks) > 0 {
marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked)
blocksBeforeThisMsg := len(systemBlocks)
if len(msg.MultiContent) > 0 {
for _, part := range msg.MultiContent {
if part.Type == chat.MessagePartTypeText {
if txt := strings.TrimSpace(part.Text); txt != "" {
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{Text: txt})
}
}
}
} else if txt := strings.TrimSpace(msg.Content); txt != "" {
// Trim system-message content: YAML literal blocks (instruction: |) always
// append a trailing newline, and we don't want that in API payloads.
systemBlocks = append(systemBlocks, anthropic.TextBlockParam{
Text: txt,
})
}
if msg.CacheControl && len(systemBlocks) > blocksBeforeThisMsg {
marked = markSystemBlockCacheControl(&systemBlocks[len(systemBlocks)-1], marked)
}

In practice this combination (CacheControl=true + fully whitespace MultiContent) is unlikely because such a system message carries no useful content. The checkpoint would still be consumed (burning one of the 2 system budget slots) on a block that has nothing to do with the intended message boundary.

Confidence Score
🟡 moderate 62/100

@Sayt-0
Sayt-0 merged commit ad9f237 into docker:main Jul 17, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants