LAB-1765: turn cachekit compression on for /api/v2 cached bodies - #24
Conversation
The compression:false carve-out from LAB-768 was a ticket non-goal, not a
finding, and it stopped being defensible once the wasm32 cachekit-core
build was measured (LAB-431): the envelope's compress/decompress FFI
survives post-processing, and cachekit's Workers runtime imports the wasm
module statically, so it was already in the bundle. Verified: the deploy
is byte-identical at 373.44 KiB / 113.30 KiB gz with the flag either way.
Measured on production-shaped columnar /values bodies (600 series,
miniflare workerd, 10 rounds):
288 buckets, the default 24 h query 966 KiB JSON
stored 966 -> 350 KiB (2.76x) set 7.3 -> 17.3 ms get 1.0 -> 8.5 ms
500 buckets, 300000 rows = MAX_LIMIT ceiling 1653 KiB JSON
stored 1653 -> 606 KiB (2.73x) set 8.9 -> 25.4 ms get 1.2 -> 13.7 ms
Keep, and not because it is free: a hit costs ~8 ms more (~14 ms at the
ceiling) and that is CPU, not I/O. Cache API storage is free, so the
currency is retention -- entries are 0.3-1.6 MB, colo LRU evicts the big
ones first, and 2.7x more resident entries is a direct hit-rate lever
against a miss that costs a D1 aggregation over up to 300000 rows. Revert
trigger recorded in the code: wrangler tail showing hit-path CPU near the
limit.
compression is written explicitly rather than left to cachekit's default
ON, because upstream is adding a per-backend compressionDefault and the
Cache API backend will advertise FALSE -- relying on the default would
silently un-compress this cache on a future upgrade.
Fixes a pre-existing fail-open the expert panel found while reviewing the
flip: get<CachedResponse> is a blind cast, so any entry that decodes to
something else served `new Response(undefined)` -- an empty 200 stamped
`x-cache: HIT`, cached client-side for the whole TTL, nothing logged.
Reachable today, not hypothetically: the ByteStorage envelope is itself
valid positional MessagePack, so a compression-off reader decodes a k4
entry and hands back the 4-tuple as the value. isCachedResponse() turns
any foreign shape into a logged miss. Everything else in this layer fails
closed; this was the one path that did not.
KEY_VERSION k3 -> k4. The envelope reader cannot decode a k3 entry and
createCache.minimal sets degradation:false, so the get throws -- survivable
(handleApiCached catches, serves from D1, refills) but it would fire once
per live key across the deploy. The bump buys the same refill without the
error storm. A revert needs a bump too, for the misread reason above.
Tests: the 2 MiB 'x'.repeat round-trip is replaced with a ceiling-sized
production-shaped body compared whole (the old one would have passed on a
corrupted payload), plus a stored-size regression assertion per body shape
and two mismatch tests. Verified by breaking each one: disabling the shape
guard reds the foreign-entry test, reverting compression reds the pre-flip
test. The latency harness was run once for the numbers above and then
removed -- miniflare has no production baseline to regress against, so it
would have been 20 multi-MB round trips per CI run asserting nothing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe cache now uses k5 compressed storage with shared 64 MiB serializer limits. Cache reads validate decoded response shapes. Tests cover large production-shaped payloads, compression ratios, stale plain entries, and foreign cache values. ChangesCache format and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change enables compression for cached API responses while adding validation and cache-version handling; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Worker
participant cachekit
participant isCachedResponse
Worker->>cachekit: Read and decode cache entry
cachekit-->>Worker: Return decoded value
Worker->>isCachedResponse: Validate body and expires
isCachedResponse-->>Worker: Return valid response or miss
Worker->>cachekit: Store refilled response on miss
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
Kody review (PR #24): the shape guard used 'as CachedResponse' internally — an assertion inside the very function that exists to avoid one. TS 4.9 'in' narrowing checks property presence AND type with no assertion; runtime behaviour is identical (typeof undefined === 'undefined' made the old form safe by accident, the new form is safe by construction) and the dedicated foreign-entry test pins it. The spec's JSON.parse cast becomes a typed declaration — the expects that follow are the runtime check there.
This comment has been minimized.
This comment has been minimized.
|
@kody start-review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@worker/test/cache.spec.ts`:
- Around line 313-324: Remove the duplicate const hit declaration in the
ceiling-sized round-trip test, keeping a single declaration in the same scope so
the test compiles without block-scoped variable redeclaration errors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8d65942-cce0-4516-9f96-0faeefc6f876
📒 Files selected for processing (2)
worker/src/cache.tsworker/test/cache.spec.ts
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
# Conflicts: # worker/test/cache.spec.ts
|
Resolved merge conflict in |
|
@coderabbitai review |
|
# Conflicts: # worker/src/cache.ts
|
Resolved |
Ray asked whether the reported +8-14 ms lands in a user hot path. Checking rather than arguing turned up that the benchmark answered a question nobody asked: those numbers come from /api/v2/values, and public/app.js never fetches it. The dashboard fetches values/aggregate?group_by=fuel, /dispatch and /intensity -- all bucket-aggregated, all ~20 KiB. Measured end to end through worker.fetch at hours=24, compression off -> on: /values/aggregate?group_by=fuel 21 KiB hit 0.6 -> 1.1 ms /dispatch 20 KiB hit 0.5 -> 0.5 ms /intensity 17 KiB hit 0.4 -> 0.5 ms /values (API drill-down only) 501 KiB hit 1.6 -> 3.8 ms A page load pays ~+0.6 ms across all three fetches. The +8-14 ms figure is real but belongs to /values at the 300000-row ceiling, which is a scripted API consumer's request, not a rendered page. That also makes the keep argument a better one than the one committed earlier: the entries big enough for compression to matter are the same ones that pay the few milliseconds, and the same ones whose miss costs a D1 aggregation over up to 300000 rows. Cost and benefit land on the same endpoint instead of being traded across them. No behaviour change -- the comment was understating a cost on one endpoint and overstating it for every other, and it is the operational record for the next person deciding this.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Closes LAB-1765.
Turns cachekit's LZ4 ByteStorage envelope (wasm32 cachekit-core) on for the
/api/v2response cache, with the numbers that justify it and the fail-open path the review found on the way.Measured
Production-shaped columnar
/valuesbodies, 600 series, miniflare workerd, 10 rounds:MAX_LIMITceilingDecision: keep — and not because it's free. A hit costs ~8 ms more, ~14 ms at the ceiling, and that is CPU, not I/O. Cache API storage is free, so the currency is retention: entries are 0.3–1.6 MB, colo LRU evicts the big ones first, and 2.7x more resident entries is a direct hit-rate lever against a miss that costs a D1 aggregation over up to 300000 rows. The revert trigger is recorded in the code —
wrangler tailshowing hit-path CPU near the limit.Bundle cost is zero: cachekit's Workers runtime imports the wasm module statically, so it already shipped. Verified byte-identical at 373.44 KiB / 113.30 KiB gz with the flag either way.
compression: trueis written explicitly rather than left to cachekit's default ON — upstream is adding a per-backendcompressionDefaultand the Cache API backend will advertise FALSE, so relying on the default would silently un-compress this cache on a future upgrade.Fail-open fix (found by the expert panel, pre-existing)
get<CachedResponse>is a blind cast, so any entry that decoded to something else was served asnew Response(undefined)— an empty 200 stampedx-cache: HIT, cached client-side for the whole TTL, nothing logged. Not hypothetical: the ByteStorage envelope is itself valid positional MessagePack, so a compression-off reader decodes a k4 entry successfully and hands back the 4-tuple[compressed, checksum, size, 'msgpack']as the value (cachekit-ts finding, LAB-1388 — the tolerant read path is merged upstream but unreleased as of 0.1.5).isCachedResponse()turns any foreign shape into a logged miss. Everything else in this layer fails closed; this was the one path that didn't.KEY_VERSION k3 → k4
The envelope reader can't decode a k3 entry, and
createCache.minimalsetsdegradation: false, so the get throws — survivable (the handler catches, serves from D1, refills) but it would fire once per live key across the deploy. The bump buys the same refill without the error storm. A revert needs a bump too, for the misread reason above.Tests
152 green,
tscclean. The 2 MiB'x'.repeatround-trip is replaced with a ceiling-sized production-shaped body compared whole — the old one would have passed on a corrupted payload — plus a per-shape stored-size regression assertion and two mismatch tests.Both guards were verified by breaking them: disabling the shape guard reds the foreign-entry test; reverting compression reds the pre-flip test.
The latency harness ran once for the numbers above and was then removed — miniflare has no production baseline to regress against, so it would have been 20 multi-MB round trips per CI run asserting nothing.
Expert panel
Ran at high stakes (bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent). Applied: the fail-open CRIT (all four found it independently); a broken LCG in the body generator whose overflow collapsed the period to ~10k and inflated the measured ratio ~10% (numbers above are post-fix); a ratio assertion hoisted out of its loop so only one body shape was checked; a "measures the shipped config" claim the sizing test couldn't support (it measures the codec — the mismatch tests own the config); a 518400-row body the API can't emit; a duplicated serializer-cap literal; and a
toEqualthat would have diffed two multi-MB strings on failure.Rejected: replacing the LCG generator with something simpler —
'x'.repeatunder compression is a fake test. Security dismissed the decompression-bomb and CRIME/BREACH questions with a traced argument:.invalidis a reserved TLD so the cachekit key space can never collide with the zone's CDN entries, every envelope is written by this worker under the 64 MiB pre-compression cap, and the cached values are public AEMO data with no secret in the isolate to leak.Summary by CodeRabbit
Bug Fixes
Tests