fix: L1 TTL cap, loud size rejections, Cache API compression default, Node-free workers types (LAB-1388) - #98
Conversation
…he API compression default, Node-free workers types Four findings from the first production dogfooding of 0.1.5 on Workers (nem.api LAB-768): 1. L1 re-population on a plain get() used the cache defaultTtl, so an L1 copy could outlive the L2 entry it was read from. New optional Backend.getWithTtl capability surfaces the remaining TTL in the same storage round trip (Cache API: max-age minus Age; Redis: pipelined GET+TTL), and CacheImpl caps L1 re-population at that remaining lifetime. Backends without the capability keep the previous defaultTtl bound (documented). 2. A set() rejected for serializer.maxEncodedSize (1 MiB default) is invisible in production: degradation (on by default) swallows it, and consumer try/catch does too — the cache silently never stores its largest values. setEntry now reports a rate-limited, greppable '[cachekit] set rejected, value NOT cached' line through the library logger before the error continues, and the limit is called out in the minimal-intent docs and README. 3. The Cache API backend now advertises compressionDefault=false (Cloudflare stores Response bodies compressed at rest; the wasm LZ4 envelope compressed twice for little win). New optional Backend.compressionDefault feeds the cache-level default; an explicit compression option always wins. 4. types/cache.ts imported ioredis's nominal Redis type, dragging Node-typed declarations into the workers .d.ts closure — Workers consumers without @types/node failed tsc with 'Cannot find name Buffer' unless they set skipLibCheck. InvalidationConfig.redis is now the structural RedisPubSubLike (an ioredis client satisfies it as-is; compile-time-asserted in redis.ts). check-workers-bundle gained a type-closure guard that fails if ioredis/prom-client/@types/node declarations ever re-enter the workers type surface.
…undary mapping (LAB-1388) Panel CRIT (confirmed by execution): the ByteStorage envelope is itself valid positional MessagePack, so a compression-off cache doing a plain decode of an enveloped entry SUCCEEDS and serves the 4-tuple envelope as the cached value — silent corruption on the 0.1.5→0.1.6 Cache API upgrade path and in mixed-version fleets, invisible to degradation. getEntry now sniffs envelope-shaped bytes (fixarray(4) + bin marker) on the compression-off path and does a verified unpack (xxHash3 rejects false positives) via a lazily-created codec, falling back to plain decode. The pre-existing test claiming this mismatch degrades to null was vacuous — it closed the writer first, clearing the shared in-memory store — and is replaced by tests pinning both mismatch directions. Panel MAJ: Redis getWithTtl mapped TTL=0 (sub-second remainder) and -2 (expired between the pipelined GET and TTL) to null/unknown, handing a dying entry the full defaultTtl L1 lifetime — the exact bug the capability exists to fix. Now -1 → null, everything else clamps ≥ 0 so the caller's > 0 gate skips L1. A failing TTL pipeline leg is reported once through the library logger instead of silently reverting L1 bounding to defaultTtl. Also: Cache API get() delegates to getWithTtl (one read path); VALUE_TOO_LARGE_WARN_INTERVAL_MS is module-private (one consumer, not public API).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds TTL-aware backend reads, L1 freshness limits, compression-envelope compatibility, oversized-value handling, Redis type decoupling, Workers declaration checks, and native binding version validation. ChangesCache runtime and platform boundaries
Native binding validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CacheCore
participant Backend
participant L1Cache
CacheCore->>Backend: getWithTtl(key)
Backend-->>CacheCore: cached value and remaining TTL
CacheCore->>L1Cache: repopulate with capped lifetime
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
…arn, guarded ts import (LAB-1388) - Cache API: an explicit X-CacheKit-No-Expiry marker header now distinguishes genuine no-expiry entries (ttl <= 0) from a caller who legitimately set exactly one year — both write the same max-age, so max-age alone was ambiguous. Any max-age without the marker is a real TTL; 0.1.5-written sentinel entries report ~1 year remaining, which the caller's TTL cap bounds identically to null (no behavior change). - The LAB-1388 oversized-value warning now also fires on the interop encode path: encodeInteropValue throws synchronously outside the reliability executor (deliberate — degradation must not swallow model rejections), which also meant it bypassed warnValueTooLarge; a consumer's own try/catch would hide the rejection invisibly. - check-workers-bundle.mjs: guard the dynamic typescript import with a clear failure message instead of an unhandled rejection.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cachekit/src/cache-core.ts (1)
491-511: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSupport encrypted mixed-compression reads or document the limitation
When compression is enabled for the writer and disabled for the reader, the reader uses
compressed=falseAAD. Decryption fails beforetryUnwrapEnvelope()runs. Add an encrypted mixed-compression test and support the alternate AAD path, or document that encrypted caches require matching compression settings.🤖 Prompt for 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. In `@packages/cachekit/src/cache-core.ts` around lines 491 - 511, Update the decryption flow around this.encryption.decrypt so encrypted reads can retry with the alternate compression/envelope AAD when the reader’s useEnvelope setting differs from the writer’s. After successful fallback decryption, continue through tryUnwrapEnvelope for mixed-compression data, while preserving normal authentication failures and plain-read behavior. Add coverage for an encrypted compression-enabled write read with compression disabled.
🤖 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 `@packages/cachekit/README.md`:
- Around line 447-457: Update the envelope-tolerance sentence in the README so
the phrase describing entries stored by an earlier version is grammatically
complete by adding the appropriate preposition, while preserving the existing
meaning and surrounding documentation.
In `@packages/cachekit/src/cache-core.ts`:
- Around line 520-532: Update L1Cache.set so a zero TTL is represented as
non-expiring rather than an immediate expiration, and adjust the L1 population
logic around l1TtlSeconds to allow repopulation when remainingTtl is null and
the effective TTL is zero. Add coverage for both defaultTtl: 0 and explicit ttl:
0 while preserving positive-TTL expiration behavior.
---
Outside diff comments:
In `@packages/cachekit/src/cache-core.ts`:
- Around line 491-511: Update the decryption flow around this.encryption.decrypt
so encrypted reads can retry with the alternate compression/envelope AAD when
the reader’s useEnvelope setting differs from the writer’s. After successful
fallback decryption, continue through tryUnwrapEnvelope for mixed-compression
data, while preserving normal authentication failures and plain-read behavior.
Add coverage for an encrypted compression-enabled write read with compression
disabled.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 288528aa-68df-44b1-bc37-fb2725b5fe15
📒 Files selected for processing (14)
packages/cachekit/README.mdpackages/cachekit/scripts/check-workers-bundle.mjspackages/cachekit/src/backends/redis.tspackages/cachekit/src/backends/types.tspackages/cachekit/src/backends/workers-cache-api.test.tspackages/cachekit/src/backends/workers-cache-api.tspackages/cachekit/src/cache-core.tspackages/cachekit/src/cache.test.tspackages/cachekit/src/exports-common.tspackages/cachekit/src/intents-core.tspackages/cachekit/src/invalidation/redis-channel.tspackages/cachekit/src/types/cache.tspackages/cachekit/src/workers/index.tspackages/cachekit/test/integration/redis-backend.integration.test.ts
Fix a dangling preposition in the envelope-tolerance README paragraph. Fix the real defect: L1Cache.set stored expiresAt = now + ttl, so a zero-or-negative ttl (the ts-wide Backend contract's "no expiry" value, per redis.ts/workers-kv.ts/memcached.ts) expired the L1 entry on the very next millisecond instead of caching it forever. Root-caused in the shared L1Cache.set so both callers (direct writes and L1 repopulation on a plain get()) inherit the fix. The repopulation guard in cache-core.ts also had to stop treating a zero "no-expiry" cap the same as a literal zero-second cap, or Math.min collapsed a real remaining TTL from the backend down to zero and skipped repopulation entirely. CodeRabbit-Resolved: packages/cachekit/README.md:457:Fix a dangling phrase in the e CodeRabbit-Resolved: packages/cachekit/src/cache-core.ts:532:Represent zero TTL as non-expi
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Conflict in |
… (LAB-1768) Semantic port, not a textual merge: LAB-1388's three get/set-path behaviours re-composed onto LAB-238's shared decodeEntry/l1Payload/ setEntry structure in cache-core.ts. - Envelope tolerance (sniff 0x94+bin -> verified unpack -> plain-decode fallback) now lives in the shared decodeEntry, so it covers both L2 reads and secure-cache L1 hits. For encrypted caches the AAD useEnvelope flag (frozen v0x03 set, protocol#12) stays authoritative: a compression-mismatched entry fails AAD verification loudly, and we deliberately do not retry decrypt with the flipped flag. - getWithTtl L1 re-population cap (min(declared TTL, remaining), skip at <=0) composed with l1Payload so the capped L1 write stores ciphertext for encrypted caches. - Rate-limited size-rejection warn rewoven around setEntry's L1Write-returning serialize path, still firing before degradation can swallow ValueTooLargeError. Both suites intact and green: cache.test.ts (LAB-1388) and cache.encryption-l1.test.ts (LAB-238); 727 passed / 1 pre-existing manual skip.
- No-expiry L1 re-population handed L1 Infinity ms, and an Infinity originalTtl makes getWithSwr's freshness check compare Infinity > Infinity — permanently stale, arming a spurious background refresh (origin recompute + unconditional L2 rewrite) per marker window, forever, on exactly the entries configured to never expire. Clamp to L1's canonical no-expiry encoding (0) at the boundary; regression test proves the phantom-refresh loop is gone (verified failing pre-fix). - tryUnwrapEnvelope no longer conflates "not an envelope" with "codec construction failed": a broken NAPI/wasm binding now surfaces loudly through the reliability executor instead of silently serving raw envelope tuples. - Envelope-tolerance comment corrected: xxHash3 is keyless — it rejects accidental envelope look-alikes, not adversarially crafted ones; posture accepted eyes-open, blast radius bounded by maxDecodedSize/maxDepth. - Size-rejection warn drops the serializer-config remediation hint on the interop path, whose caps are protocol constants that serializer config does not govern.
1dcaec9
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cachekit/src/cache-core.ts (1)
811-818: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the interop envelope mode for encrypted L1 entries.
Line 818 always passes
falsetodecodeL1Entry. An encrypted interop entry was written withuseEnvelope(false), but a compression-enabled cache then verifies it withuseEnvelope(true). This drops a valid L1 entry and, when degradation is disabled, makesexists()throw. Store the envelope mode with the encrypted L1 payload, or bypass L1 decoding inexists()until the mode is available.🤖 Prompt for 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. In `@packages/cachekit/src/cache-core.ts` around lines 811 - 818, Update the L1 encrypted-entry flow around decodeL1Entry so exists() preserves the entry’s original envelope mode instead of always passing false. Store or retrieve the mode written with each encrypted L1 payload and pass it through consistently, ensuring compression-enabled interop entries validate without being dropped or causing exists() to throw.
🤖 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 `@packages/cachekit/src/cache-core.ts`:
- Line 410: Remove the raw key interpolation from the size-rejection log in
cache-core.ts, using no key or a non-reversible identifier while preserving the
error and hint details. Update packages/cachekit/README.md lines 179-182 to
document the resulting safe log format.
- Around line 303-306: Update the lazy byte-storage/envelope-reader creation
around createByteStorage and decodeEntry so a reader created after close() is
tracked as temporary and released when closed is already true. Ensure the
shutdown cleanup near the existing codec-freeing logic still handles readers
created before shutdown, while preventing any post-shutdown reader leak during
in-flight encrypted reads.
---
Outside diff comments:
In `@packages/cachekit/src/cache-core.ts`:
- Around line 811-818: Update the L1 encrypted-entry flow around decodeL1Entry
so exists() preserves the entry’s original envelope mode instead of always
passing false. Store or retrieve the mode written with each encrypted L1 payload
and pass it through consistently, ensuring compression-enabled interop entries
validate without being dropped or causing exists() to throw.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 830f2cc7-06b9-4f3d-aa0f-be321519291e
📒 Files selected for processing (6)
packages/cachekit-core-ts/index.jspackages/cachekit/README.mdpackages/cachekit/src/cache-core.tspackages/cachekit/src/cache.test.tspackages/cachekit/src/l1/lru-cache.test.tspackages/cachekit/src/l1/lru-cache.ts
…rom size-rejection log (LAB-1768) - tryUnwrapEnvelope: an in-flight read resuming after close() now uses a throwaway codec freed immediately, instead of resurrecting (and leaking) the cached envelopeReader close() has already freed. Wasm-relevant only; the NAPI binding is GC-managed. - warnValueTooLarge: log a non-reversible blake2b-128 digest (keyHash=) instead of the raw caller-controlled key, which may embed PII or credentials. README documents the new format and how to match a digest. - Tests: post-close read frees exactly one throwaway codec and stays correct; size-rejection log carries keyHash= and never the raw key.
…velope ops, shared blake2b16Hex (LAB-1768) - withEnvelopeCodec: the compression-ON pack/unpack sites had the same post-close hazard the CodeRabbit round fixed for envelopeReader — an in-flight read/write resuming after close() ran on the byteStorage codec close() had already freed (wasm use-after-free on Workers, silently degraded to a spurious miss). All three envelope codec sites now share one closed-aware throwaway pattern; regression test verified failing pre-fix on the default compression-on path. - close() nulls envelopeReader after freeing it — a freed-but-dangling wasm codec behind a non-null reference is an instant UAF for any future caller that forgets the closed check. - blake2b16Hex extracted to key-generator: the size-rejection keyHash and the File backend filename are the same protocol-locked formula (py _key_to_path parity); README notes a logged keyHash names the entry's cache file on the File backend.
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
|
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 `@packages/cachekit/src/cache.test.ts`:
- Around line 498-566: Extract the duplicated close-during-read setup from the
adjacent post-close tests into a shared helper returning { reader, pending,
counts }. Parameterize only the cache key and compression setting, while
preserving the gated backend, codec creation/free counters, close/release
sequencing, and each test’s distinct final assertions; update both tests to use
the helper.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 983e8015-b959-4142-b415-5d5fa6165bb5
📒 Files selected for processing (5)
packages/cachekit/README.mdpackages/cachekit/src/backends/file.tspackages/cachekit/src/cache-core.tspackages/cachekit/src/cache.test.tspackages/cachekit/src/serialization/key-generator.ts
CodeRabbit flagged the two throwaway-codec tests as duplicating the gated backend, the codec counters, and the close/release interleaving verbatim — only the key, the stored value, and the reader's compression mode differ. Kept as one fixture because the duplication is load-bearing, not cosmetic: if the two copies of the close/release ordering ever drifted, the compression-ON test would silently stop exercising the wasm use-after-free window it exists to pin, and nothing would fail. One fixture makes that interleaving impossible to change for only one of them. Mutation-verified: reverting either throwaway-codec guard in cache-core.ts still fails both tests through the shared fixture.
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:
|
|
@coderabbitai review |
|
1 participant
Summary
This PR bundles four dogfooding fixes (LAB-1388) that address silent failure modes and cross-process correctness issues discovered in production usage.
1. L1 cache TTL cap on cross-process reads
Adds an optional
Backend.getWithTtl()capability that returns a value alongside its remaining TTL in a single storage round trip. When an L2 hit re-populates a process's local L1 cache, the copy is now capped at the entry's actual remaining lifetime instead ofdefaultTtl.GET+TTL(one round trip); a failing TTL leg is reported once and falls back to thedefaultTtlbound.Cache-Control: max-ageminus the edge-reportedAgeheader.defaultTtlbehavior — now documented as a known limitation.Fixes the bug where a plain
get()at t=29s of a 30s entry would keep serving stale data in L1 for the fulldefaultTtlafter the L2 entry expired.2. Loud size-rejection warnings
set()rejections for values exceedingserializer.maxEncodedSize(1 MiB default) were effectively invisible — graceful degradation (on by default) swallows the failure, as do consumer try/catch blocks. The cache now emits a single, greppable, rate-limited (60s) warning through the pluggable logger ([cachekit] set rejected, value NOT cached (key=...)), even when the underlyingValueTooLargeErroris absorbed. README and intent docs now call out the 1 MiB default as a cache-off switch.3. Cache API compression default + envelope tolerance
Backends can now advertise a preferred
compressionDefault. The Workers Cache API backend advertises compression off (Cloudflare already stores response bodies compressed at rest), avoiding double-compression CPU cost. An explicitcompression:option always overrides.Reads are now envelope-tolerant: a compression-off cache detects, integrity-verifies (xxHash3), and unwraps entries written with the ByteStorage envelope. This fixes a silent-corruption bug where the envelope (itself valid MessagePack) would decode "successfully" and serve the raw envelope structure as the cached value — enabling safe upgrades and mixed-version fleet rollouts.
4. Node-free workers types
Replaces the nominal
import type { Redis } from 'ioredis'in the shared type surface (InvalidationConfig) with a structuralRedisPubSubLikeinterface. On 0.1.5, the nominal type dragged ioredis's Node-typed declarations into the Workers.d.tsclosure, forcing every Workers consumer without@types/nodeintoskipLibCheck. A new CI type-closure guard incheck-workers-bundle.mjsbuilds the workers declaration closure with no Node libs and fails if ioredis / prom-client /@types/nodeleak in.Test Coverage
getWithTtlheader math and advertised defaults.getWithTtl.Summary
This PR addresses several fixes identified during dogfooding (LAB-1388), focused on the Cloudflare Workers Cache API backend, size-rejection observability, and the workers bundle type guard.
Changes
1. Cache API: Unambiguous no-expiry marker
The Workers Cache API backend previously overloaded a one-year
max-ageas the "no expiry" sentinel. This was ambiguous: a caller could legitimately set a TTL of exactly one year, which produced the samemax-age.X-CacheKit-No-Expirymarker header, written only whenttl <= 0.getWithTtlnow reportsttlSeconds: null(no expiry) only when the marker header is present, instead of inferring it frommax-age.max-ageas a real ~1-year remainder, still bounded by the caller's own TTL cap — so behavior is unchanged for them.2. Loud size rejections on the interop encode path
ValueTooLargeErrorraised during interop value encoding happens outside the reliability executor, so degradation never hides it — but a consumer's owntry/catcharoundset()could silently swallow it.ValueTooLargeErrorand emits the greppable size-rejection warning before re-throwing.3. Workers bundle type-closure guard hardening
check-workers-bundle.mjsscript now wraps the TypeScript compiler import in atry/catch, exiting with a clear error message if thetypescriptdevDependency fails to load.Functional Impact
Summary
This PR delivers a set of dogfooding fixes for the
cachekitpackage under LAB-1388, primarily addressing incorrect TTL handling around the "no expiry" contract.Changes
L1 TTL "no expiry" handling
lru-cache.ts: Fixed the L1 cache so that attl <= 0value is now correctly treated as "no expiry" (matching the ts-wide Backend contract). Previously,now + 0caused entries to expire on the very next millisecond instead of being cached indefinitely. Entries withttl <= 0now useInfinityforexpiresAt.cache-core.ts: Fixed L1 repopulation so that when the configureddefaultTtl(or explicit TTL) is0/negative, it is treated as infinite. Previously, acapSecondsof0collapsed theMath.minTTL cap to0, which tripped thel1TtlSeconds > 0skip-guard and prevented L1 repopulation entirely for entries that should never expire. The fix still allows a real backend-reportedremainingTtlto cap the L1 TTL.Tests
lru-cache.test.ts: Added a test verifying that entries set withttl <= 0(both0and negative) never expire, even after simulating a full year of elapsed time.cache.test.ts: Added an integration test confirming that whendefaultTtlis0and an entry has no expiry, an L2 hit correctly repopulates L1 — verified by clearing the backing store and confirming a subsequent read is served from L1.Documentation
README.md: Minor grammar correction in the compression envelope documentation.Note
While the PR title references additional fixes (loud size rejections, Cache API compression default, Node-free workers types), the provided code changes only cover the L1 TTL "no expiry" handling, its tests, and a README grammar fix.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation