DUX-8760: Add prompt-cache breakpoints to the BedrockInvokeModel protocol#18
Conversation
Auto-inject system + tail cache_control breakpoints (config-gated, default on) so InvokeModel requests get native Bedrock prompt caching. Translate Converse-format Content::Raw blocks (persisted by the consumer app) into Anthropic Messages format at every entry point, including cachePoint -> cache_control with ttl passthrough. Capture cache_read_input_tokens / cache_creation_input_tokens on the streaming message_start path, matching the non-streaming chat.rb behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Sam Boland <sam.boland@appfolio.com>
|
Jira Issue: https://appfolio.atlassian.net/browse/DUX-8760 |
|
🤖 Supernova Code Review — View trace |
|
Code review please. |
Reviewed the code at the PR head. Findings:
Both are worth addressing before merge, especially #1 since silent mutation of persisted message data is the kind of bug that's hard to trace back. Want me to queue a fix for the mutation issue (dup the block before attaching |
| return unless messages | ||
|
|
||
| block = messages.reverse_each.lazy.filter_map { |m| last_cacheable_block(m[:content]) }.first | ||
| block[:cache_control] = { type: 'ephemeral' } if block |
There was a problem hiding this comment.
Tail injection clobbers translated breakpoints. This assignment unconditionally overwrites block[:cache_control]. If the final message's last cacheable block already carries a breakpoint from a translated cachePoint — including one with ttl: '1h' — it is replaced with a plain { type: 'ephemeral' }, silently downgrading a caller-requested 1-hour TTL to 5 minutes. That contradicts the comment on lines 88-89 ("Never removes a breakpoint that arrived via a translated cachePoint"). The budget count at line 96 already counted that block, so the overwrite isn't needed to stay within budget either.
Recommend: skip injection when the found block already has cache_control (either key form) — the tail is already marked. E.g. in last_cacheable_block or guard before the assignment:
block[:cache_control] = { type: 'ephemeral' } if block && !(block[:cache_control] || block['cache_control'])There was a problem hiding this comment.
Queued — the fix will guard inject_tail_cache_breakpoint against overwriting an existing cache_control (checking both key forms) and add a spec covering the preserved-ttl case.
|
|
||
| def translate_raw_block(block, result) | ||
| return result << block unless block.is_a?(Hash) | ||
| return result << block if block.key?(:type) || block.key?('type') |
There was a problem hiding this comment.
In-place mutation of caller-owned hashes. Pass-through blocks returned here are the caller's own hash objects, and attach_cache_point (line 283), inject_system_cache_breakpoint? (line 122), and inject_tail_cache_breakpoint (line 130) all write :cache_control directly into whatever block they land on. For Content::Raw messages, the consumer app wraps the ActiveRecord content_raw attribute itself — so an injected breakpoint mutates persisted-model data in memory: it survives into any retry or later round-trip that reuses the same message objects, and if the record is saved again for any reason the injected cache_control is written back to the database.
Recommend: result << block.dup on both pass-through returns (lines 264-265), and have attach_cache_point operate on the duped copy — the rendered payload should never alias caller-owned hashes. (This confirms the earlier bot review's mutation finding; consider this the actionable version of that thread.)
There was a problem hiding this comment.
Queued as a follow-up to the tail-injection fix — it'll dup pass-through blocks in translate_raw_block and make attach_cache_point work on the copy, plus add a spec asserting the caller's original hashes stay untouched.
| end | ||
|
|
||
| def count_blocks_with_cache_control(blocks) | ||
| (blocks || []).count { |b| b.is_a?(Hash) && b[:cache_control] } |
There was a problem hiding this comment.
Symbol-only key checks defeat the budget guard and dedup for string-keyed blocks. translate_raw_block deliberately tolerates string keys, but the three helpers downstream are symbol-only: this counter checks b[:cache_control], the system dedup at line 120 checks b[:cache_control], and last_cacheable_block at line 135 checks block[:type]. A string-keyed Anthropic-format block from a DB round-trip ({'type' => 'text', 'cache_control' => {...}}) passes through translation unchanged, and then:
- It isn't counted toward the 4-breakpoint budget — a request can end up with more than 4 breakpoints and be rejected by Bedrock.
- The system dedup misses it and sets a symbol
:cache_controlon a hash that already has'cache_control', which serializes to duplicate"cache_control"keys in the JSON body. - The tail walker skips string-keyed cacheable blocks entirely, so the breakpoint drifts to an earlier message than intended.
Recommend: make all three helpers key-indifferent the same way translate_raw_block already is — b[:cache_control] || b['cache_control'] here and at line 120, block[:type] || block['type'] at line 135.
There was a problem hiding this comment.
Queued — this will make count_blocks_with_cache_control, inject_system_cache_breakpoint?, and last_cacheable_block check both symbol and string keys, plus add specs for string-keyed blocks across the budget-count, dedup, and tail-selection paths.
- inject_tail_cache_breakpoint no longer overwrites a pre-existing cache_control on the tail block (e.g. one carrying a translated ttl) - translate_raw_block/attach_cache_point now dup blocks before writing cache_control, so injection never mutates the caller's original Content::Raw hash objects (e.g. an ActiveRecord-persisted array) - cache_control/type lookups are key-indifferent (symbol or string) across budget counting, system dedup, and tail block selection, so string-keyed blocks from a DB round-trip are handled consistently
Summary
Add automatic prompt-cache breakpoint injection to the
BedrockInvokeModelprotocol, enabling native Bedrock/Anthropic caching and eliminating unnecessary full-price input billing.Changes
Configuration
bedrock_invoke_model_prompt_cachingconfiguration option (defaultstrue)Bedrockprovider configurationCore Implementation
MAX_CACHE_BREAKPOINTS = 4constant (Bedrock limit)CACHEABLE_BLOCK_TYPESto identify cache-eligible blocksinject_cache_control_breakpointshelper to intelligently place breakpoints:render_payloadwhen config is enabledCache Metrics
cache_read_input_tokens,cache_creation_input_tokens) from message_start events and include inChunkTesting
Impact
Enables production agents_app deployment through BedrockInvokeModel with native caching, reducing input token costs by ~75% on cached requests (measured in consumer app). Fully backward-compatible via config toggle (defaults on).
Notes
Agent session: https://staging.supernova.dx.appf.io/coders/c0d5406c-9d91-4c90-a0b1-a7cfa60b0237