From a9a68db289a8006d89256a65d915adcb734bc630 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Tue, 4 Aug 2026 01:02:12 +1000 Subject: [PATCH 1/8] =?UTF-8?q?fix:=20LAB-1388=20dogfooding=20fixes=20?= =?UTF-8?q?=E2=80=94=20L1=20TTL=20cap,=20size-rejection=20warn,=20Cache=20?= =?UTF-8?q?API=20compression=20default,=20Node-free=20workers=20types?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cachekit/README.md | 67 ++++- .../cachekit/scripts/check-workers-bundle.mjs | 77 ++++++ packages/cachekit/src/backends/redis.ts | 42 +++- packages/cachekit/src/backends/types.ts | 43 ++++ .../src/backends/workers-cache-api.test.ts | 126 ++++++++++ .../src/backends/workers-cache-api.ts | 44 +++- packages/cachekit/src/cache-core.ts | 75 +++++- packages/cachekit/src/cache.test.ts | 229 ++++++++++++++++++ packages/cachekit/src/constants.ts | 8 + packages/cachekit/src/exports-common.ts | 2 + packages/cachekit/src/intents-core.ts | 7 + .../src/invalidation/redis-channel.ts | 21 +- packages/cachekit/src/types/cache.ts | 36 ++- packages/cachekit/src/workers/index.ts | 8 +- .../redis-backend.integration.test.ts | 31 +++ 15 files changed, 790 insertions(+), 26 deletions(-) create mode 100644 packages/cachekit/src/backends/workers-cache-api.test.ts diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index 947270d..622642a 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -131,9 +131,53 @@ const cache = createCache({ baseDelay: 100, }, }, + + // Serializer DoS-protection limits — see "Value size limits" below. + serializer: { + maxEncodedSize: 1024 * 1024, // 1 MiB default + maxDecodedSize: 10 * 1024 * 1024, // 10 MiB default + }, + + // ByteStorage envelope (LZ4 + integrity). Defaults to true, unless the + // backend advertises compression off because its store already + // compresses at rest (the Workers Cache API backend does). + compression: true, +}); +``` + +### Value size limits — the 1 MiB default is a cache-off switch, not a suggestion + +`set()` (and every `wrap()` store) rejects values whose encoded size exceeds +`serializer.maxEncodedSize` — **1 MiB by default** — with `ValueTooLargeError`. +Two things make this rejection easy to miss in production: + +- **Graceful degradation is on by default**, and it treats a failed `set()` + as "skip silently" — the call resolves normally. +- Consumers that guard `set()` with try/catch (correctly — cache failures + shouldn't fail requests) absorb the error the same way. + +Either way the result is a cache that quietly never stores exactly its +largest — often hottest — values, while small values keep caching fine. If +your responses can exceed 1 MiB (large API payloads, reports, aggregates), +raise both limits up front: + +```typescript +const cache = createCache.minimal({ + backend: workersCacheAPI(), + serializer: { + maxEncodedSize: 64 * 1024 * 1024, + maxDecodedSize: 64 * 1024 * 1024, // must cover reads of what you write + }, }); ``` +The SDK also reports every size rejection through its +[pluggable logger](#observability) as a rate-limited, greppable +`[cachekit] set rejected, value NOT cached (key=...)` line — watch for it +after deploying a new cache. (Backends have their own hard ceilings too: +Workers KV values cap at 25 MiB, Memcached items at 1 MiB server-side, +CachekitIO per plan.) + ## Stampede Protection A cold cache key hit by N concurrent callers would normally execute the wrapped @@ -191,6 +235,17 @@ Four backends implement the same `Backend` interface (raw bytes in/out) and plug The Memcached and File backends are **Node-runtime only** and live behind subpath exports, so browser/edge bundles that import the package root never pull in `memjs` or `node:fs`. +**L1 freshness across processes.** When a plain `get()` hits L2, the value is +re-populated into this process's L1. On backends that surface the entry's +remaining TTL in the same read (`Backend.getWithTtl` — Redis via a pipelined +`GET`+`TTL`, the Workers Cache API via response freshness headers), that L1 +copy is capped at the entry's **remaining lifetime**, so it can never outlive +the L2 entry it came from. On backends without the capability (KV, CachekitIO, +Memcached, File, custom backends that only implement `get`), the L1 copy is +bounded by `defaultTtl` — if you rely on TTLs for correctness across processes +there, set a small `defaultTtl`, disable L1 (`l1: { enabled: false }`), or +implement `getWithTtl` on your custom backend. + ### Memcached ```typescript @@ -389,9 +444,13 @@ export default { Beyond CachekitIO, two Cloudflare-native backends keep cache state in the edge itself — no round-trip to api.cachekit.io. Both store the same opaque -ByteStorage payloads as every other backend, so encryption and the wire -envelope are unchanged (secure caches store only ciphertext), and both plug -into `createCache` or any intent via `backend:`: +payloads as every other backend, so encryption is unchanged (secure caches +store only ciphertext), and both plug into `createCache` or any intent via +`backend:`. One default differs: the Cache API backend advertises the +ByteStorage compression envelope **off** — Cloudflare already stores +`Response` bodies compressed at rest, so the wasm envelope would spend +isolate CPU compressing twice. Pass `compression: true` to re-enable it +(e.g. to shrink bodies below a size limit before storage): ```typescript import { createCache, workersKV, workersCacheAPI } from '@cachekit-io/cachekit/workers'; @@ -420,6 +479,8 @@ TTL and consistency semantics differ from Redis/CachekitIO — pick by workload: | TTL | Native `expirationTtl`; **60s minimum** — shorter TTLs are clamped up, never down | `Cache-Control: max-age`, honored to the second, no floor | | `ttl <= 0` ("no expiry") | Stored without expiration | Capped at 1-year max-age (the Cache API has no unbounded storage) | | Eviction | Durable until expiry | Best-effort — entries may be dropped under cache pressure | +| Compression default | On (ByteStorage envelope) | **Off** — Cloudflare stores bodies compressed at rest | +| L1 freshness on read | Bounded by `defaultTtl` (KV reads don't surface remaining TTL) | Capped at the entry's **remaining TTL** (from `max-age` − `Age`) | | Best for | Shared config, sessions, rarely-written hot reads | Request-local acceleration in front of a shared source | The Cache API is request-keyed under the hood; the backend maps each cache diff --git a/packages/cachekit/scripts/check-workers-bundle.mjs b/packages/cachekit/scripts/check-workers-bundle.mjs index f2cb7d7..fe49d3a 100644 --- a/packages/cachekit/scripts/check-workers-bundle.mjs +++ b/packages/cachekit/scripts/check-workers-bundle.mjs @@ -69,3 +69,80 @@ if (violations.length > 0) { console.log( 'bundle guard OK: workers entry graph is free of node:*, .node, NAPI, ioredis, prom-client' ); + +// ── Type-closure guard (LAB-1388) ────────────────────────────────────────── +// Same invariant at the type level: build a program over the workers entry's +// published declaration closure and fail if any Node-typed declarations get +// pulled in (ioredis, prom-client, @types/node). On 0.1.5, a nominal +// `import type { Redis } from 'ioredis'` in the shared type module forced +// every Workers consumer without @types/node into skipLibCheck over dozens +// of `Cannot find name 'Buffer'` errors inside ioredis/built/**. +// +// The detection is a file scan of the program's resolved sources, NOT just +// diagnostics: in THIS repo @types/node is installed, so a leaked ioredis +// reference typechecks clean here while still breaking consumers that don't +// have it. Diagnostics are checked too (with skipLibCheck off) so the +// closure is also proven self-consistent against ES+DOM libs alone. +const ts = (await import('typescript')).default; + +const typesEntry = join(pkgDir, 'dist', 'workers', 'index.d.ts'); +const compilerOptions = { + noEmit: true, + strict: true, + skipLibCheck: false, + types: [], + // ES lib only, plus DOM for the fetch/Response/caches types the Workers + // backends reference structurally. Deliberately NO Node lib/types. + lib: ['lib.es2022.d.ts', 'lib.dom.d.ts', 'lib.dom.iterable.d.ts'], + module: ts.ModuleKind.NodeNext, + moduleResolution: ts.ModuleResolutionKind.NodeNext, +}; + +const program = ts.createProgram([typesEntry], compilerOptions); + +const FORBIDDEN_TYPE_SOURCES = [ + { name: 'ioredis declarations', filter: /node_modules\/ioredis\// }, + { name: 'prom-client declarations', filter: /node_modules\/prom-client\// }, + { name: '@types/node', filter: /node_modules\/@types\/node\// }, +]; + +const typeViolations = []; +for (const sourceFile of program.getSourceFiles()) { + const fileName = sourceFile.fileName.replace(/\\/g, '/'); + for (const { name, filter } of FORBIDDEN_TYPE_SOURCES) { + if (filter.test(fileName)) { + typeViolations.push(`${name}: ${fileName}`); + } + } +} + +if (typeViolations.length > 0) { + console.error('type-closure guard: Node-typed declarations reached the workers .d.ts closure:'); + for (const violation of typeViolations.slice(0, 10)) { + console.error(` - ${violation}`); + } + if (typeViolations.length > 10) { + console.error(` ... and ${typeViolations.length - 10} more`); + } + process.exit(1); +} + +const diagnostics = ts.getPreEmitDiagnostics(program); +if (diagnostics.length > 0) { + console.error('type-closure guard: workers .d.ts closure fails without Node types:'); + console.error( + ts.formatDiagnosticsWithColorAndContext(diagnostics.slice(0, 20), { + getCanonicalFileName: (f) => f, + getCurrentDirectory: () => pkgDir, + getNewLine: () => '\n', + }) + ); + if (diagnostics.length > 20) { + console.error(` ... and ${diagnostics.length - 20} more`); + } + process.exit(1); +} + +console.log( + 'type-closure guard OK: workers .d.ts closure is free of ioredis / prom-client / @types/node' +); diff --git a/packages/cachekit/src/backends/redis.ts b/packages/cachekit/src/backends/redis.ts index 7596c29..2dd4168 100644 --- a/packages/cachekit/src/backends/redis.ts +++ b/packages/cachekit/src/backends/redis.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'node:crypto'; import { Redis as IoRedis, type RedisOptions } from 'ioredis'; -import { LockableBackend, RedisBackendConfig, TTLBackend } from './types.js'; +import { GetWithTtlResult, LockableBackend, RedisBackendConfig, TTLBackend } from './types.js'; +import type { RedisPubSubLike } from '../types/cache.js'; import { BackendError, TimeoutError } from '../errors.js'; import { logError } from '../logger.js'; import { @@ -23,6 +24,18 @@ else return 0 end`; +// Compile-time proof that a real ioredis client satisfies the structural +// RedisPubSubLike that replaced the nominal ioredis type in +// InvalidationConfig (LAB-1388). Anchored here because this is the one +// type-checked src module that already imports ioredis (tests are excluded +// from `tsc --noEmit`; this module is Node-closure-only, so the ioredis +// types never reach the workers type surface). Type-only — erased at +// runtime; if the structural type ever drifts incompatible, `pnpm +// type-check` fails here instead of in every consumer's build. +type AssertPubSubCompatible = T; +// eslint-disable-next-line @typescript-eslint/no-unused-vars +type _IoRedisIsPubSubCompatible = AssertPubSubCompatible; + /** * Redis backend implementation using ioredis. * @@ -115,6 +128,33 @@ export class RedisBackend implements LockableBackend, TTLBackend { } } + /** + * See {@link Backend.getWithTtl}: GET + TTL pipelined onto one round trip + * (LAB-1388), so CacheImpl can cap L1 re-population at the entry's + * remaining lifetime without a second network hop. TTL's -2 (missing) / + * -1 (no expiry) collapse to null, matching {@link TTLBackend.getTTL}; + * a TTL-command failure downgrades to "unknown" rather than failing a + * read whose GET succeeded. + */ + async getWithTtl(key: string): Promise { + this.ensureNotClosed(); + + try { + const results = await this.client.pipeline().getBuffer(key).ttl(key).exec(); + if (!results || results.length !== 2) { + throw new Error('pipeline returned no results'); + } + const [getErr, buf] = results[0] as [Error | null, Buffer | null]; + if (getErr) throw getErr; + if (buf === null) return null; + const [ttlErr, ttl] = results[1] as [Error | null, number]; + const ttlSeconds = !ttlErr && typeof ttl === 'number' && ttl > 0 ? ttl : null; + return { value: new Uint8Array(buf), ttlSeconds }; + } catch (error) { + throw this.wrapError('get', error); + } + } + async set(key: string, value: Uint8Array, ttl?: number): Promise { this.ensureNotClosed(); diff --git a/packages/cachekit/src/backends/types.ts b/packages/cachekit/src/backends/types.ts index 2655dc4..9da9c61 100644 --- a/packages/cachekit/src/backends/types.ts +++ b/packages/cachekit/src/backends/types.ts @@ -26,6 +26,26 @@ export interface Backend { */ get(key: string): Promise; + /** + * Retrieve a value together with its remaining TTL — in the SAME storage + * round trip as a plain get (LAB-1388). Optional capability: implement it + * only when the store surfaces the expiry on read for free (Cache API + * response headers, Redis GET+TTL pipeline). Do NOT implement it as + * get + a second TTL request — CacheImpl calls this on every L2 hit when + * L1 is enabled, and a second round trip there doubles read latency (and + * on metered backends, cost). + * + * CacheImpl uses the returned `ttlSeconds` to cap L1 re-population at the + * entry's remaining lifetime, so an L1 copy never outlives the L2 entry it + * was read from. Backends without this capability fall back to `get()`, + * where L1 re-population is bounded only by the cache's default TTL. + * + * @returns The stored bytes plus remaining TTL in seconds (`null` TTL = + * unknown or no expiry), or `null` when the key is missing + * @throws {BackendError} if the operation fails + */ + getWithTtl?(key: string): Promise; + /** * Store a value with optional TTL. * @@ -98,6 +118,29 @@ export interface Backend { * Like `keyPrefix`, the value MUST be constant from construction onward. */ readonly transformsKeys?: boolean; + + /** + * The backend's preferred default for the ByteStorage compression + * envelope (LAB-1388). Left unset, the cache-level default stays `true`. + * Backends whose store already compresses values at rest (the Cloudflare + * Cache API stores `Response` bodies compressed) advertise `false` here so + * the default configuration doesn't spend CPU compressing twice. An + * explicit `compression:` option on the cache always wins. + * + * Like `keyPrefix`, the value MUST be constant from construction onward. + */ + readonly compressionDefault?: boolean; +} + +/** Result of {@link Backend.getWithTtl}: the bytes plus remaining lifetime. */ +export interface GetWithTtlResult { + /** The stored bytes. */ + value: Uint8Array; + /** + * Remaining TTL in seconds, or `null` when unknown / no expiry (matches + * {@link TTLBackend.getTTL}'s collapse of Redis's -1). + */ + ttlSeconds: number | null; } /** diff --git a/packages/cachekit/src/backends/workers-cache-api.test.ts b/packages/cachekit/src/backends/workers-cache-api.test.ts new file mode 100644 index 0000000..eff4a45 --- /dev/null +++ b/packages/cachekit/src/backends/workers-cache-api.test.ts @@ -0,0 +1,126 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { CacheAPIBackend, workersCacheAPI, type CacheLike } from './workers-cache-api.js'; + +/** + * Node-lane unit tests for the Cache API backend's header math and + * advertised defaults (LAB-1388). The real-workerd behavior (actual + * caches.default storage) is covered by test/workers/edge-backends — + * these tests pin the freshness arithmetic getWithTtl derives from the + * stored response's Cache-Control/Age headers, which workerd's local + * cache emulation cannot exercise (it never reports Age). + */ +class FakeCache implements CacheLike { + readonly store = new Map(); + + async match(url: string): Promise { + // Clone so repeated matches can each consume their body, like the real + // Cache API. + return this.store.get(url)?.clone(); + } + + async put(url: string, response: Response): Promise { + this.store.set(url, response); + } + + async delete(url: string): Promise { + return this.store.delete(url); + } +} + +describe('CacheAPIBackend (unit, mocked caches global)', () => { + let fake: FakeCache; + + beforeEach(() => { + fake = new FakeCache(); + (globalThis as { caches?: unknown }).caches = { default: fake }; + }); + + afterEach(() => { + delete (globalThis as { caches?: unknown }).caches; + }); + + const value = new Uint8Array([1, 2, 3, 4]); + + it('advertises compression off (Cloudflare compresses response bodies at rest)', () => { + expect(new CacheAPIBackend().compressionDefault).toBe(false); + }); + + it('getWithTtl returns null on miss', async () => { + expect(await workersCacheAPI().getWithTtl('ns:missing')).toBeNull(); + }); + + it('getWithTtl reports the stored max-age when the edge reports no Age', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + + const result = await backend.getWithTtl('ns:key'); + expect(result).not.toBeNull(); + expect(Array.from(result!.value)).toEqual([1, 2, 3, 4]); + expect(result!.ttlSeconds).toBe(30); + }); + + it('getWithTtl subtracts the Age header from max-age', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + + // Simulate the Cloudflare edge reporting the entry's age on match. + const [url, stored] = [...fake.store.entries()][0]; + const headers = new Headers(stored.headers); + headers.set('Age', '29'); + fake.store.set(url, new Response(value, { headers })); + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBe(1); + }); + + it('getWithTtl floors remaining freshness at zero when Age >= max-age', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + + const [url, stored] = [...fake.store.entries()][0]; + const headers = new Headers(stored.headers); + headers.set('Age', '45'); + fake.store.set(url, new Response(value, { headers })); + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBe(0); + }); + + it('getWithTtl reports null (no expiry) for the no-expiry sentinel', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 0); // ttl <= 0 → 1-year sentinel max-age + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBeNull(); + }); + + it('getWithTtl reports null when freshness headers are absent', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + + const [url] = [...fake.store.keys()]; + fake.store.set(url, new Response(value)); // no Cache-Control at all + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBeNull(); + }); + + it('getWithTtl ignores a malformed Age header', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + + const [url, stored] = [...fake.store.entries()][0]; + const headers = new Headers(stored.headers); + headers.set('Age', 'bogus'); + fake.store.set(url, new Response(value, { headers })); + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBe(30); + }); + + it('get() round-trips unchanged (regression)', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 30); + expect(Array.from((await backend.get('ns:key'))!)).toEqual([1, 2, 3, 4]); + }); +}); diff --git a/packages/cachekit/src/backends/workers-cache-api.ts b/packages/cachekit/src/backends/workers-cache-api.ts index a045767..5bc612d 100644 --- a/packages/cachekit/src/backends/workers-cache-api.ts +++ b/packages/cachekit/src/backends/workers-cache-api.ts @@ -1,4 +1,4 @@ -import { Backend } from './types.js'; +import { Backend, GetWithTtlResult } from './types.js'; import { BackendError, ConfigurationError } from '../errors.js'; import { classifyWorkersRuntimeError } from './error-classifier.js'; import { DEFAULT_TTL_SECONDS } from '../constants.js'; @@ -97,6 +97,13 @@ export class CacheAPIBackend implements Backend { * See Backend.transformsKeys. */ readonly transformsKeys = true; + /** + * Cloudflare stores Cache API `Response` bodies compressed at rest, so the + * ByteStorage LZ4 envelope would spend isolate CPU compressing twice for + * little win — advertise compression off by default (LAB-1388). An + * explicit `compression: true` on the cache still enables it. + */ + readonly compressionDefault = false; private readonly cacheName?: string; private readonly defaultTtl: number; private cachePromise: Promise | null = null; @@ -118,6 +125,25 @@ export class CacheAPIBackend implements Backend { } } + /** + * See {@link Backend.getWithTtl}: same single `match()` round trip as + * get(); the remaining lifetime comes from the stored response's own + * freshness headers (the `Cache-Control: max-age` this backend wrote, + * minus the `Age` the Cloudflare edge reports). Lets CacheImpl cap L1 + * re-population at the entry's remaining lifetime (LAB-1388). + */ + async getWithTtl(key: string): Promise { + this.ensureNotClosed(); + try { + const response = await (await this.cache()).match(keyUrl(key)); + if (response === undefined) return null; + const ttlSeconds = remainingTtlSeconds(response); + return { value: new Uint8Array(await response.arrayBuffer()), ttlSeconds }; + } catch (error) { + throw this.wrapError('get', error); + } + } + async set(key: string, value: Uint8Array, ttl?: number): Promise { this.ensureNotClosed(); const effectiveTtl = ttl ?? this.defaultTtl; @@ -218,6 +244,22 @@ function keyUrl(key: string): string { return SYNTHETIC_KEY_BASE + encodeURIComponent(key); } +/** + * Remaining lifetime of a matched response: the `max-age` set() wrote minus + * the `Age` header the edge reports. Null (unknown / no expiry) when the + * entry carries the no-expiry sentinel or the headers are absent — e.g. + * test harnesses that don't emit `Age` report the full max-age, which the + * caller's own TTL cap still bounds; never a negative freshness. + */ +function remainingTtlSeconds(response: Response): number | null { + const maxAgeMatch = /(?:^|[,\s])max-age=(\d+)/.exec(response.headers.get('Cache-Control') ?? ''); + if (!maxAgeMatch) return null; + const maxAge = Number(maxAgeMatch[1]); + if (maxAge >= CACHE_API_NO_EXPIRY_MAX_AGE) return null; + const age = Number(response.headers.get('Age')); + return Math.max(0, maxAge - (Number.isFinite(age) && age > 0 ? Math.floor(age) : 0)); +} + /** * Create a Cache API backend (per-data-center read-through tier). * diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index 71bdbe5..c10d413 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -28,12 +28,13 @@ import { decodeInteropValue, } from './serialization/interop.js'; import { createInvalidationEvent } from './invalidation/event.js'; -import { BackendError, ConfigurationError } from './errors.js'; +import { BackendError, ConfigurationError, ValueTooLargeError } from './errors.js'; import { DEFAULT_TTL_SECONDS, DEFAULT_LOCK_TIMEOUT_MS, DEFAULT_LOCK_WAIT_MS, DEFAULT_LOCK_POLL_MS, + VALUE_TOO_LARGE_WARN_INTERVAL_MS, } from './constants.js'; /** @@ -259,8 +260,12 @@ export class CacheImpl implements SecureCache { // Initialize encryption this.encryption = options.encryption ? runtime.createEncryption(options.encryption) : null; - // Initialize ByteStorage (LZ4 compression + xxHash3-64 integrity) - this.byteStorage = (options.compression ?? true) ? runtime.createByteStorage() : null; + // Initialize ByteStorage (LZ4 compression + xxHash3-64 integrity). The + // default honors the backend's advertised preference (LAB-1388): stores + // that already compress values at rest (the Cache API) advertise false so + // the default config doesn't compress twice. An explicit option wins. + const compressionEnabled = options.compression ?? this.backend.compressionDefault ?? true; + this.byteStorage = compressionEnabled ? runtime.createByteStorage() : null; // Initialize serializer this.serializer = new MessagePackSerializer(options.serializer); @@ -327,6 +332,24 @@ export class CacheImpl implements SecureCache { void this.metrics.recordError(error instanceof Error ? error.constructor.name : 'Unknown'); } + /** Timestamp of the last oversized-value warning (rate limiting). */ + private lastSizeWarnAt = 0; + + /** + * One-line, greppable, rate-limited report of a set() rejected for size + * (LAB-1388) — the only reliable signal of the rejection when degradation + * or a consumer catch-block absorbs the ValueTooLargeError itself. + */ + private warnValueTooLarge(key: string, error: ValueTooLargeError): void { + const now = Date.now(); + if (now - this.lastSizeWarnAt < VALUE_TOO_LARGE_WARN_INTERVAL_MS) return; + this.lastSizeWarnAt = now; + logError( + `[cachekit] set rejected, value NOT cached (key=${key}): ${error.message}. ` + + 'Raise serializer.maxEncodedSize / maxDecodedSize if values this large are expected.' + ); + } + private publishL1Stats(): void { if (!this.l1) return; const stats = this.l1.stats; @@ -386,6 +409,10 @@ export class CacheImpl implements SecureCache { * cache-level `compression` option. `ttlSeconds` (when known, i.e. from * wrap()) bounds the L1 repopulation lifetime so an entry never outlives * its declared TTL in L1 long after L2 and the other SDKs expired it. + * On backends that surface the remaining TTL on read (getWithTtl), the + * bound tightens to the entry's actual remaining lifetime — a plain get() + * at t=29s of a 30s entry re-populates L1 for 1s, not defaultTtl + * (LAB-1388). */ private async getEntry(key: string, interop: boolean, ttlSeconds?: number): Promise { this.ensureNotClosed(); @@ -403,7 +430,18 @@ export class CacheImpl implements SecureCache { // Fetch from L2 (backend) return this.run('get', null, async (): Promise => { - const data = await this.backend.get(key); + // When L1 will be re-populated, prefer the TTL-carrying read (same + // storage round trip — see Backend.getWithTtl) so the L1 copy can be + // capped at the entry's remaining lifetime below. + let data: Uint8Array | null; + let remainingTtl: number | null = null; + if (this.l1 && this.backend.getWithTtl) { + const result = await this.backend.getWithTtl(key); + data = result?.value ?? null; + remainingTtl = result?.ttlSeconds ?? null; + } else { + data = await this.backend.get(key); + } if (data === null) { this.recordMiss(); return null; @@ -427,11 +465,19 @@ export class CacheImpl implements SecureCache { // Populate L1. Interop keys are {namespace}:{operation}:{hash} — group // under the user-facing namespace segment so namespace-level - // invalidation matches entries written through wrap(). + // invalidation matches entries written through wrap(). The lifetime is + // the declared TTL (or defaultTtl on a plain get), capped at the L2 + // entry's remaining TTL when the backend surfaced it — so the L1 copy + // never outlives the entry it was read from (LAB-1388). if (this.l1) { const namespace = interop ? key.slice(0, key.indexOf(':')) : extractNamespace(key); - this.l1.set(key, value, (ttlSeconds ?? this.defaultTtl) * 1000, namespace); - this.publishL1Stats(); + const capSeconds = ttlSeconds ?? this.defaultTtl; + const l1TtlSeconds = + remainingTtl !== null ? Math.min(capSeconds, remainingTtl) : capSeconds; + if (l1TtlSeconds > 0) { + this.l1.set(key, value, l1TtlSeconds * 1000, namespace); + this.publishL1Stats(); + } } this.recordHit('l2'); @@ -476,8 +522,19 @@ export class CacheImpl implements SecureCache { const interopSerialized = interop ? encodeInteropValue(value) : null; return this.run('set', undefined, async (): Promise => { - // Serialize - const serialized = interopSerialized ?? this.serializer.encode(value); + // Serialize. A size rejection here is invisible in production configs + // — degradation (on by default) swallows set() failures, and careful + // consumers try/catch set() anyway — so a cache whose hottest values + // exceed maxEncodedSize silently never stores them (LAB-1388). Emit + // one greppable, rate-limited warning before the error continues into + // the reliability stack. + let serialized: Uint8Array; + try { + serialized = interopSerialized ?? this.serializer.encode(value); + } catch (error) { + if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error); + throw error; + } // Compress with ByteStorage (before encryption) let data: Uint8Array = useEnvelope ? this.byteStorage!.pack(serialized) : serialized; diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 9351003..9ae6855 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -1,6 +1,9 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { createCache } from './cache.js'; import { generateKey } from './serialization/key-generator.js'; +import { setLogger } from './logger.js'; +import { ValueTooLargeError } from './errors.js'; +import { MessagePackSerializer } from './serialization/serializer.js'; import type { SecureCache } from './types/cache.js'; import type { Backend } from './backends/types.js'; @@ -502,4 +505,230 @@ describe('Cache Integration', () => { await swrCache.close(); }); }); + + // ── LAB-1388 dogfooding fixes ───────────────────────────────────────── + + describe('L1 re-population TTL cap (LAB-1388)', () => { + /** In-memory backend that tracks expiry and surfaces remaining TTL on + * read — the getWithTtl capability (Cache API / Redis shape). */ + class TtlAwareBackend implements Backend { + readonly store = new Map(); + + private live(key: string) { + const entry = this.store.get(key); + if (!entry) return null; + if (entry.expiresAt !== null && entry.expiresAt <= Date.now()) { + this.store.delete(key); + return null; + } + return entry; + } + + async get(key: string): Promise { + return this.live(key)?.value ?? null; + } + + async getWithTtl(key: string) { + const entry = this.live(key); + if (!entry) return null; + const ttlSeconds = + entry.expiresAt === null ? null : Math.max(0, (entry.expiresAt - Date.now()) / 1000); + return { value: entry.value, ttlSeconds }; + } + + async set(key: string, value: Uint8Array, ttl: number): Promise { + this.store.set(key, { value, expiresAt: ttl > 0 ? Date.now() + ttl * 1000 : null }); + } + + async delete(key: string): Promise { + return this.store.delete(key); + } + + async exists(key: string): Promise { + return this.live(key) !== null; + } + + async close(): Promise { + this.store.clear(); + } + } + + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('caps a plain get() L1 re-population at the entry remaining lifetime', async () => { + const shared = new TtlAwareBackend(); + const writer = createCache({ backend: shared, defaultTtl: 300 }); + // Second cache over the same backend = another isolate/process with + // its own (empty) L1. + const reader = createCache({ backend: shared, defaultTtl: 300 }); + + await writer.set('ns:entry', 'v1', { ttl: 30 }); + + // t=29s: the reader's plain get() is an L2 hit — pre-fix its L1 copy + // got defaultTtl (300s) and served 'v1' long past the entry's expiry. + vi.advanceTimersByTime(29_000); + expect(await reader.get('ns:entry')).toBe('v1'); + + // t=31s: entry expired in L2; the reader's L1 copy must be gone too. + vi.advanceTimersByTime(2_000); + expect(await reader.get('ns:entry')).toBeNull(); + + await writer.close(); + await reader.close(); + }); + + it('keeps serving from L1 within the remaining lifetime', async () => { + const shared = new TtlAwareBackend(); + const writer = createCache({ backend: shared, defaultTtl: 300 }); + const reader = createCache({ backend: shared, defaultTtl: 300 }); + + await writer.set('ns:entry', 'v1', { ttl: 30 }); + vi.advanceTimersByTime(10_000); + expect(await reader.get('ns:entry')).toBe('v1'); // L2 hit, L1 capped at ~20s + + // Still fresh at t=25s — and served from the reader's L1 (delete the + // L2 entry to prove the read never goes back to the backend). + shared.store.clear(); + vi.advanceTimersByTime(15_000); + expect(await reader.get('ns:entry')).toBe('v1'); + + await writer.close(); + await reader.close(); + }); + + it('falls back to defaultTtl on backends without getWithTtl (documented limitation)', async () => { + const shared = new InMemoryBackend(); // no expiry tracking, no getWithTtl + const writer = createCache({ backend: shared, defaultTtl: 300 }); + const reader = createCache({ backend: shared, defaultTtl: 300 }); + + await writer.set('ns:entry', 'v1', { ttl: 30 }); + vi.advanceTimersByTime(29_000); + expect(await reader.get('ns:entry')).toBe('v1'); + + // The backend itself never expires entries and reports no TTL, so the + // reader's L1 copy legitimately lives out defaultTtl — unchanged + // pre-existing behavior for TTL-blind backends. + vi.advanceTimersByTime(2_000); + expect(await reader.get('ns:entry')).toBe('v1'); + + await writer.close(); + await reader.close(); + }); + }); + + describe('oversized-value set() warning (LAB-1388)', () => { + afterEach(() => { + setLogger(null); + vi.useRealTimers(); + }); + + // ~2 MiB of unique-ish content — over the 1 MiB default maxEncodedSize. + const oversized = () => 'x'.repeat(2 * 1024 * 1024); + + it('reports a rate-limited warning even when degradation swallows the error', async () => { + vi.useFakeTimers(); + const logs: string[] = []; + setLogger((message) => logs.push(message)); + + const c = createCache({ backend: new InMemoryBackend() }); + + // Degradation is on by default: set() resolves silently… + await expect(c.set('ns:big', oversized())).resolves.toBeUndefined(); + // …but the rejection is reported, once, greppably. + expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(1); + expect(logs[0]).toContain('maxEncodedSize'); + + // Rate-limited: a hot oversized key cannot flood the sink. + await c.set('ns:big', oversized()); + expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(1); + + // A fresh interval reports again. + vi.advanceTimersByTime(61_000); + await c.set('ns:big', oversized()); + expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(2); + + await c.close(); + }); + + it('still throws ValueTooLargeError when degradation is disabled', async () => { + const logs: string[] = []; + setLogger((message) => logs.push(message)); + + const c = createCache({ + backend: new InMemoryBackend(), + reliability: { degradation: false }, + }); + + await expect(c.set('ns:big', oversized())).rejects.toThrow(ValueTooLargeError); + expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(1); + + await c.close(); + }); + }); + + describe('backend-advertised compression default (LAB-1388)', () => { + /** Plain MessagePack view of raw backend bytes ('decode failed' when the + * envelope bytes aren't even valid MessagePack). */ + const plainDecode = (bytes: Uint8Array): unknown => { + try { + return new MessagePackSerializer().decode(bytes); + } catch { + return 'decode failed'; + } + }; + + class NoCompressionPreferenceBackend extends InMemoryBackend { + readonly compressionDefault = false; + readonly raw = new Map(); + + override async set(key: string, value: Uint8Array, ttl: number): Promise { + this.raw.set(key, value); + await super.set(key, value, ttl); + } + } + + it('honors compressionDefault=false: stored bytes are plain MessagePack', async () => { + const b = new NoCompressionPreferenceBackend(); + const c = createCache({ backend: b, l1: { enabled: false } }); + + await c.set('ns:k', { hello: 'world' }); + // No ByteStorage envelope: the raw backend bytes decode directly. + const stored = [...b.raw.values()][0]; + expect(new MessagePackSerializer().decode(stored)).toEqual({ hello: 'world' }); + expect(await c.get('ns:k')).toEqual({ hello: 'world' }); + + await c.close(); + }); + + it('explicit compression: true overrides the backend preference', async () => { + const b = new NoCompressionPreferenceBackend(); + const c = createCache({ backend: b, compression: true, l1: { enabled: false } }); + + await c.set('ns:k', { hello: 'world' }); + // Enveloped: plain MessagePack decode of the raw bytes must not yield + // the original value (the envelope wraps it). + expect(plainDecode([...b.raw.values()][0])).not.toEqual({ hello: 'world' }); + expect(await c.get('ns:k')).toEqual({ hello: 'world' }); + + await c.close(); + }); + + it('backends without a preference keep the compressed default', async () => { + const b = new NoCompressionPreferenceBackend(); + // Erase the preference to model a legacy/custom backend. + Object.defineProperty(b, 'compressionDefault', { value: undefined }); + const c = createCache({ backend: b, l1: { enabled: false } }); + + await c.set('ns:k', { hello: 'world' }); + expect(plainDecode([...b.raw.values()][0])).not.toEqual({ hello: 'world' }); + + await c.close(); + }); + }); }); diff --git a/packages/cachekit/src/constants.ts b/packages/cachekit/src/constants.ts index 9e73358..d4cb31d 100644 --- a/packages/cachekit/src/constants.ts +++ b/packages/cachekit/src/constants.ts @@ -47,6 +47,14 @@ export const DEFAULT_MAX_DEPTH = 100; /** Maximum collection size for Maps, Sets, Arrays, Objects */ export const DEFAULT_MAX_COLLECTION_SIZE = 10000; +/** + * Minimum interval between "set rejected: value too large" warnings + * (LAB-1388). The rejection itself is often invisible (degradation swallows + * set failures; consumers try/catch set), so the SDK reports it through the + * logger — rate-limited so a hot oversized key can't flood the sink. + */ +export const VALUE_TOO_LARGE_WARN_INTERVAL_MS = 60_000; + /** Maximum size for key generation (64KB) */ export const KEY_GEN_MAX_SIZE = 64 * 1024; diff --git a/packages/cachekit/src/exports-common.ts b/packages/cachekit/src/exports-common.ts index bc70819..35b141f 100644 --- a/packages/cachekit/src/exports-common.ts +++ b/packages/cachekit/src/exports-common.ts @@ -17,10 +17,12 @@ export type { EncryptionConfig, ReliabilityConfig, StampedeConfig, + RedisPubSubLike, } from './types/cache.js'; export type { Backend, + GetWithTtlResult, CachekitIOBackendConfig, LockableBackend, TTLBackend, diff --git a/packages/cachekit/src/intents-core.ts b/packages/cachekit/src/intents-core.ts index ec6d2fa..e964099 100644 --- a/packages/cachekit/src/intents-core.ts +++ b/packages/cachekit/src/intents-core.ts @@ -63,6 +63,13 @@ type IntentBackendOptions = * * Disables circuit breaker, retry, and degradation for maximum throughput. * Use for read-heavy, non-critical caching (product catalogs, public APIs). + * + * Read-heavy public APIs routinely serve multi-MB responses — mind the + * serializer's **1 MiB default `maxEncodedSize`** (LAB-1388): values above + * it are rejected with ValueTooLargeError and NEVER cached, and the + * rejection is easy to absorb silently (consumer try/catch around set). + * If your payloads can exceed 1 MiB, raise `serializer.maxEncodedSize` + * (and `maxDecodedSize`) explicitly — see the README's "Value size limits". */ export type MinimalOptions = BaseIntentOptions & IntentBackendOptions; diff --git a/packages/cachekit/src/invalidation/redis-channel.ts b/packages/cachekit/src/invalidation/redis-channel.ts index f5081b5..1f78f62 100644 --- a/packages/cachekit/src/invalidation/redis-channel.ts +++ b/packages/cachekit/src/invalidation/redis-channel.ts @@ -1,10 +1,21 @@ -import type { Redis } from 'ioredis'; +// Deliberately NOT `import type { Redis } from 'ioredis'`: this module's +// types flow into InvalidationConfig, which sits in the shared type closure +// re-exported by the workers entry — nominal ioredis types would drag +// @types/node requirements onto every Workers consumer (LAB-1388). The +// structural RedisPubSubLike covers exactly the Pub/Sub surface used here, +// and a real ioredis client satisfies it as-is. +import type { RedisPubSubLike } from '../types/cache.js'; import type { InvalidationEvent, InvalidationCallback } from '../l1/types.js'; import { logError } from '../logger.js'; import { serializeEvent, deserializeEvent } from './event.js'; const DEFAULT_CHANNEL = 'cachekit:invalidate'; +/** Channel names are UTF-8; decode explicitly — the structural + * RedisPubSubLike types messageBuffer args as Uint8Array, whose own + * toString() is NOT utf-8. */ +const utf8 = new TextDecoder(); + /** * Configuration for RedisInvalidationChannel. */ @@ -35,13 +46,13 @@ export interface RedisInvalidationChannelConfig { * ``` */ export class RedisInvalidationChannel { - private readonly redis: Redis; + private readonly redis: RedisPubSubLike; private readonly channelName: string; - private subscriber: Redis | null = null; + private subscriber: RedisPubSubLike | null = null; private callbacks: InvalidationCallback[] = []; private running = false; - constructor(redis: Redis, config: RedisInvalidationChannelConfig = {}) { + constructor(redis: RedisPubSubLike, config: RedisInvalidationChannelConfig = {}) { this.redis = redis; this.channelName = config.channelName ?? DEFAULT_CHANNEL; } @@ -85,7 +96,7 @@ export class RedisInvalidationChannel { // Handle incoming messages this.subscriber.on('messageBuffer', (channel, message) => { - if (channel.toString() !== this.channelName) return; + if (utf8.decode(channel) !== this.channelName) return; try { const event = deserializeEvent(new Uint8Array(message)); diff --git a/packages/cachekit/src/types/cache.ts b/packages/cachekit/src/types/cache.ts index 125344a..2cd1326 100644 --- a/packages/cachekit/src/types/cache.ts +++ b/packages/cachekit/src/types/cache.ts @@ -4,14 +4,36 @@ import type { MetricsConfig } from '../metrics/prometheus.js'; import type { CircuitBreakerConfig } from '../reliability/circuit-breaker.js'; import type { RetryConfig } from '../reliability/retry.js'; import type { SerializerConfig } from '../serialization/serializer.js'; -import type { Redis } from 'ioredis'; + +/** + * Structural view of the Redis Pub/Sub surface the invalidation channel + * drives — an `ioredis` `Redis` instance satisfies it as-is. + * + * Structural on purpose (LAB-1388): a nominal `import type { Redis } from + * 'ioredis'` here would pull ioredis's Node-typed declarations into the + * shared type closure, forcing every Workers consumer without @types/node + * into `skipLibCheck` over dozens of `Cannot find name 'Buffer'` errors. + * This module is re-exported by the workers entry, so its type closure MUST + * stay Node-free (`Uint8Array` here, never `Buffer` — Buffers satisfy it). + */ +export interface RedisPubSubLike { + /** Publish a message to a channel (ioredis: `publish`). */ + publish(channel: string, message: string | Uint8Array): Promise; + /** Create a dedicated connection for subscribing (ioredis: `duplicate`). */ + duplicate(): RedisPubSubLike; + /** Binary-safe message events (ioredis: `messageBuffer`). */ + on(event: 'messageBuffer', listener: (channel: Uint8Array, message: Uint8Array) => void): unknown; + subscribe(channel: string): Promise; + unsubscribe(channel: string): Promise; + quit(): Promise; +} /** * Configuration for cross-instance cache invalidation via Redis Pub/Sub. */ export interface InvalidationConfig { - /** Redis client for Pub/Sub (will be duplicated for subscriber) */ - redis: Redis; + /** Redis client for Pub/Sub (will be duplicated for subscriber) — pass an ioredis `Redis` instance */ + redis: RedisPubSubLike; /** Channel name for invalidation messages (default: "cachekit:invalidate") */ channelName?: string; } @@ -193,7 +215,13 @@ export interface CacheOptions { /** Serializer configuration */ serializer?: Partial; - /** Enable ByteStorage wire format (LZ4 compression + xxHash3-64 integrity). Default: true */ + /** + * Enable ByteStorage wire format (LZ4 compression + xxHash3-64 integrity). + * Default: true, unless the backend advertises `compressionDefault: false` + * because its store already compresses at rest (the Workers Cache API + * backend does — see Backend.compressionDefault). An explicit value here + * always wins. + */ compression?: boolean; /** diff --git a/packages/cachekit/src/workers/index.ts b/packages/cachekit/src/workers/index.ts index ca6ab56..ae57803 100644 --- a/packages/cachekit/src/workers/index.ts +++ b/packages/cachekit/src/workers/index.ts @@ -3,9 +3,11 @@ * `workerd` condition on the root export). * * Workers-safe surface: no node:* builtins (no nodejs_compat required), no - * ioredis, no NAPI addon, no prom-client. Crypto and the ByteStorage wire - * envelope run on the wasm32 build of cachekit-core - * (@cachekit-io/cachekit-core-wasm, ~55 KB gzipped). + * ioredis, no NAPI addon, no prom-client — in the runtime module graph AND + * the published .d.ts closure (both CI-guarded by check-workers-bundle; + * LAB-1388), so consumers typecheck without @types/node and without + * skipLibCheck. Crypto and the ByteStorage wire envelope run on the wasm32 + * build of cachekit-core (@cachekit-io/cachekit-core-wasm, ~55 KB gzipped). * * Deltas vs the Node entrypoint: * - Backends: CachekitIO (`createCache.io` / `backend: { apiKey }`), the diff --git a/packages/cachekit/test/integration/redis-backend.integration.test.ts b/packages/cachekit/test/integration/redis-backend.integration.test.ts index 0268dfe..a8062a5 100644 --- a/packages/cachekit/test/integration/redis-backend.integration.test.ts +++ b/packages/cachekit/test/integration/redis-backend.integration.test.ts @@ -121,6 +121,37 @@ describe.skipIf(!dockerAvailable)('RedisBackend Integration (Testcontainers)', ( } }); + describe('getWithTtl (LAB-1388)', () => { + it('returns the value and remaining TTL in one round trip', async () => { + await backend.set('gwt-key', new Uint8Array([7, 8]), 60); + const result = await backend.getWithTtl('gwt-key'); + expect(result).not.toBeNull(); + expect(result!.value).toEqual(new Uint8Array([7, 8])); + expect(result!.ttlSeconds).toBeGreaterThan(0); + expect(result!.ttlSeconds).toBeLessThanOrEqual(60); + }); + + it('returns null for a missing key', async () => { + expect(await backend.getWithTtl('gwt-missing')).toBeNull(); + }); + + it('returns null TTL for a key without expiry', async () => { + await client.set(`${testPrefix}gwt-persistent`, 'v'); + const result = await backend.getWithTtl('gwt-persistent'); + expect(result).not.toBeNull(); + expect(result!.ttlSeconds).toBeNull(); + }); + + it('respects the keyPrefix (pipeline commands are prefixed like get/set)', async () => { + await backend.set('gwt-prefixed', new Uint8Array([9]), 60); + // The raw client sees the prefixed key; getWithTtl reads it back + // through the same prefixing. + expect(await client.exists(`${testPrefix}gwt-prefixed`)).toBe(1); + const result = await backend.getWithTtl('gwt-prefixed'); + expect(result!.value).toEqual(new Uint8Array([9])); + }); + }); + describe('TTLBackend', () => { it('getTTL returns remaining seconds for a key with expiry', async () => { await backend.set('ttl-key', new Uint8Array([1]), 60); From 1eb92593d377dc16803f897846f2bb4659e4aa09 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Tue, 4 Aug 2026 01:19:48 +1000 Subject: [PATCH 2/8] =?UTF-8?q?fix:=20expert-panel=20remediation=20?= =?UTF-8?q?=E2=80=94=20envelope-tolerant=20reads,=20Redis=20TTL=20boundary?= =?UTF-8?q?=20mapping=20(LAB-1388)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/cachekit/README.md | 6 +- packages/cachekit/src/backends/redis.ts | 34 +++++++++-- .../src/backends/workers-cache-api.ts | 11 +--- packages/cachekit/src/cache-core.ts | 57 ++++++++++++++++++- packages/cachekit/src/cache.test.ts | 45 ++++++++++++--- packages/cachekit/src/constants.ts | 8 --- 6 files changed, 129 insertions(+), 32 deletions(-) diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index 622642a..cfbe3f3 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -450,7 +450,11 @@ store only ciphertext), and both plug into `createCache` or any intent via ByteStorage compression envelope **off** — Cloudflare already stores `Response` bodies compressed at rest, so the wasm envelope would spend isolate CPU compressing twice. Pass `compression: true` to re-enable it -(e.g. to shrink bodies below a size limit before storage): +(e.g. to shrink bodies below a size limit before storage). Reads are +envelope-tolerant either way: a compression-off cache detects, verifies, +and unwraps entries an earlier version (or a compression-on peer in a +mixed fleet) stored with the envelope, so upgrades and gradual rollouts +never serve envelope bytes as values: ```typescript import { createCache, workersKV, workersCacheAPI } from '@cachekit-io/cachekit/workers'; diff --git a/packages/cachekit/src/backends/redis.ts b/packages/cachekit/src/backends/redis.ts index 2dd4168..f83a1ed 100644 --- a/packages/cachekit/src/backends/redis.ts +++ b/packages/cachekit/src/backends/redis.ts @@ -128,13 +128,24 @@ export class RedisBackend implements LockableBackend, TTLBackend { } } + /** One-shot latch for reporting a failing TTL pipeline leg (LAB-1388). */ + private ttlLegFailureLogged = false; + /** * See {@link Backend.getWithTtl}: GET + TTL pipelined onto one round trip * (LAB-1388), so CacheImpl can cap L1 re-population at the entry's - * remaining lifetime without a second network hop. TTL's -2 (missing) / - * -1 (no expiry) collapse to null, matching {@link TTLBackend.getTTL}; - * a TTL-command failure downgrades to "unknown" rather than failing a - * read whose GET succeeded. + * remaining lifetime without a second network hop. + * + * TTL result mapping: -1 (no expiry) → null; 0 (sub-second remainder) and + * -2 (expired between the pipelined GET and TTL — pipelines aren't atomic) + * → 0, which the caller's `> 0` gate turns into "don't re-populate L1". + * Collapsing those to null instead would hand a dying entry the full + * default-TTL L1 lifetime — the exact bug this capability fixes. + * + * A TTL-command failure downgrades to "unknown" rather than failing a + * read whose GET succeeded — but it is reported once through the library + * logger, because a persistently failing TTL leg (e.g. a Redis-compatible + * proxy without TTL) silently reverts L1 bounding to defaultTtl. */ async getWithTtl(key: string): Promise { this.ensureNotClosed(); @@ -148,7 +159,20 @@ export class RedisBackend implements LockableBackend, TTLBackend { if (getErr) throw getErr; if (buf === null) return null; const [ttlErr, ttl] = results[1] as [Error | null, number]; - const ttlSeconds = !ttlErr && typeof ttl === 'number' && ttl > 0 ? ttl : null; + let ttlSeconds: number | null; + if (ttlErr || typeof ttl !== 'number') { + ttlSeconds = null; + if (!this.ttlLegFailureLogged) { + this.ttlLegFailureLogged = true; + logError( + '[cachekit] Redis getWithTtl: TTL pipeline leg failed — L1 re-population ' + + 'falls back to the defaultTtl bound (reported once)', + ttlErr ?? undefined + ); + } + } else { + ttlSeconds = ttl === -1 ? null : Math.max(0, ttl); + } return { value: new Uint8Array(buf), ttlSeconds }; } catch (error) { throw this.wrapError('get', error); diff --git a/packages/cachekit/src/backends/workers-cache-api.ts b/packages/cachekit/src/backends/workers-cache-api.ts index 5bc612d..6486280 100644 --- a/packages/cachekit/src/backends/workers-cache-api.ts +++ b/packages/cachekit/src/backends/workers-cache-api.ts @@ -115,14 +115,9 @@ export class CacheAPIBackend implements Backend { } async get(key: string): Promise { - this.ensureNotClosed(); - try { - const response = await (await this.cache()).match(keyUrl(key)); - if (response === undefined) return null; - return new Uint8Array(await response.arrayBuffer()); - } catch (error) { - throw this.wrapError('get', error); - } + // Same single match() as getWithTtl — one read path to keep correct + // (exists() already re-learned that lesson with its body-cancel fix). + return (await this.getWithTtl(key))?.value ?? null; } /** diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index c10d413..fcc0a13 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -34,9 +34,17 @@ import { DEFAULT_LOCK_TIMEOUT_MS, DEFAULT_LOCK_WAIT_MS, DEFAULT_LOCK_POLL_MS, - VALUE_TOO_LARGE_WARN_INTERVAL_MS, } from './constants.js'; +/** + * Minimum interval between "set rejected: value too large" warnings + * (LAB-1388). The rejection itself is often invisible (degradation swallows + * set failures; consumers try/catch set), so the SDK reports it through the + * logger — rate-limited so a hot oversized key can't flood the sink. + * Module-private on purpose: one consumer, not a tuning knob. + */ +const VALUE_TOO_LARGE_WARN_INTERVAL_MS = 60_000; + /** * Sentinel for "the lock path did not resolve the miss — compute without * it". Distinct from null: the wrapped function may legitimately resolve @@ -46,6 +54,17 @@ const LOCK_FALLTHROUGH = Symbol('cachekit.lock-fallthrough'); const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * Cheap structural sniff for the ByteStorage envelope: a positional msgpack + * 4-tuple whose first element is binary — fixarray(4) marker followed by a + * bin8/bin16/bin32 marker. User values matching this shape are possible but + * the verified unpack (xxHash3-64 over the payload) disambiguates; the sniff + * only exists so ordinary reads never pay an unpack attempt. + */ +function looksLikeEnvelope(bytes: Uint8Array): boolean { + return bytes.length > 2 && bytes[0] === 0x94 && bytes[1] >= 0xc4 && bytes[1] <= 0xc6; +} + /** * Metrics fallback when the runtime supplies no collector (Workers, or * metrics disabled). Duplicates NoopMetrics from metrics/prometheus.js @@ -170,6 +189,10 @@ export class CacheImpl implements SecureCache { private readonly backgroundRefresh: BackgroundRefreshManager; private readonly encryption: EncryptionLike | null; private readonly byteStorage: ByteStorageLike | null; + private readonly createByteStorage: () => ByteStorageLike; + /** Lazily-created codec for envelope-tolerant reads on compression-off + * caches (LAB-1388) — see getEntry. */ + private envelopeReader: ByteStorageLike | null = null; private readonly serializer: MessagePackSerializer; private readonly defaultTtl: number; private readonly invalidationChannel: InvalidationChannelLike | null = null; @@ -266,6 +289,9 @@ export class CacheImpl implements SecureCache { // the default config doesn't compress twice. An explicit option wins. const compressionEnabled = options.compression ?? this.backend.compressionDefault ?? true; this.byteStorage = compressionEnabled ? runtime.createByteStorage() : null; + // Kept for lazy envelope-tolerant reads (see getEntry): a compression-off + // cache still needs a codec the first time it meets an enveloped entry. + this.createByteStorage = () => runtime.createByteStorage(); // Initialize serializer this.serializer = new MessagePackSerializer(options.serializer); @@ -335,6 +361,21 @@ export class CacheImpl implements SecureCache { /** Timestamp of the last oversized-value warning (rate limiting). */ private lastSizeWarnAt = 0; + /** + * Verified unpack of a suspected legacy/foreign ByteStorage envelope on a + * compression-off cache. Returns null when the bytes aren't actually an + * envelope (checksum/shape mismatch) — the caller then treats them as + * plain serialized data. The codec is created lazily and only once. + */ + private tryUnwrapEnvelope(bytes: Uint8Array): Uint8Array | null { + try { + this.envelopeReader ??= this.createByteStorage(); + return this.envelopeReader.unpack(bytes); + } catch { + return null; + } + } + /** * One-line, greppable, rate-limited report of a set() rejected for size * (LAB-1388) — the only reliable signal of the rejection when degradation @@ -456,6 +497,17 @@ export class CacheImpl implements SecureCache { // Decompress with ByteStorage (after decryption) if (useEnvelope) { plaintext = this.byteStorage!.unpack(plaintext); + } else if (!interop && looksLikeEnvelope(plaintext)) { + // Envelope tolerance (LAB-1388): a compression-off cache can read + // entries a compression-on writer stored — same store, older SDK + // default, or a mixed-version fleet mid-rollout. This is NOT + // optional hygiene: the envelope is itself valid MessagePack (a + // positional 4-tuple), so a plain decode would "succeed" and serve + // the envelope structure as the cached value — silent corruption, + // invisible to degradation. The unpack's xxHash3 check makes sniff + // false-positives vanishingly unlikely, and any unpack failure + // falls back to treating the bytes as plain-serialized. + plaintext = this.tryUnwrapEnvelope(plaintext) ?? plaintext; } // Deserialize @@ -1016,9 +1068,10 @@ export class CacheImpl implements SecureCache { // Dispose encryption (zeroizes key material) attempt(() => this.encryption?.dispose()); - // Release the envelope codec (zeroizes/frees wasm resources on Workers; + // Release the envelope codecs (zeroizes/frees wasm resources on Workers; // no-op for the GC-managed NAPI binding) attempt(() => this.byteStorage?.free?.()); + attempt(() => this.envelopeReader?.free?.()); // Clear L1 (this also clears L1's internal refreshingKeys) attempt(() => this.l1?.clear()); diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 9ae6855..680700d 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -363,31 +363,60 @@ describe('Cache Integration', () => { await cache.close(); }); - it('should return null when reading compressed data with compression disabled (mismatch)', async () => { + // The pre-LAB-1388 version of this test closed the writer BEFORE the + // read — InMemoryBackend.close() clears the shared store, so it + // "verified" a degradation-to-null that never actually happened. The + // envelope is itself valid MessagePack, so without envelope tolerance a + // plain decode would have SUCCEEDED and served the raw 4-tuple envelope + // as the cached value. Tolerance makes the mismatch read correct. + it('reads enveloped entries correctly even with compression disabled (envelope tolerance)', async () => { const sharedBackend = new InMemoryBackend(); - // Write with compression enabled + // Write with compression enabled (0.1.5 default, or a mixed fleet) const writer = createCache({ backend: sharedBackend, compression: true, l1: { enabled: false }, }); await writer.set('test:mismatch', { data: 'compressed' }); - await writer.close(); - // Read with compression disabled — ByteStorage envelope hits serializer.decode() - // which fails (envelope structure doesn't match expected types), caught by - // ReliabilityExecutor → degrades to null (cache miss). - // KNOWN LIMITATION: compression config must be consistent within a deployment. + // Read with compression disabled — the envelope is detected, verified + // (xxHash3), and unwrapped; the caller gets the original value, never + // the envelope structure. const reader = createCache({ backend: sharedBackend, compression: false, l1: { enabled: false }, }); const result = await reader.get('test:mismatch'); - expect(result).toBeNull(); + expect(result).toEqual({ data: 'compressed' }); + + await writer.close(); await reader.close(); }); + + it('degrades to null when an envelope-on cache reads raw-serialized bytes (reverse mismatch)', async () => { + const sharedBackend = new InMemoryBackend(); + + const rawWriter = createCache({ + backend: sharedBackend, + compression: false, + l1: { enabled: false }, + }); + await rawWriter.set('test:reverse', { data: 'raw' }); + + // Envelope-on reader: unpack fails integrity, ReliabilityExecutor + // degrades to a miss (pre-existing behavior, now actually pinned). + const envelopeReader = createCache({ + backend: sharedBackend, + compression: true, + l1: { enabled: false }, + }); + expect(await envelopeReader.get('test:reverse')).toBeNull(); + + await rawWriter.close(); + await envelopeReader.close(); + }); }); describe('Error Handling', () => { diff --git a/packages/cachekit/src/constants.ts b/packages/cachekit/src/constants.ts index d4cb31d..9e73358 100644 --- a/packages/cachekit/src/constants.ts +++ b/packages/cachekit/src/constants.ts @@ -47,14 +47,6 @@ export const DEFAULT_MAX_DEPTH = 100; /** Maximum collection size for Maps, Sets, Arrays, Objects */ export const DEFAULT_MAX_COLLECTION_SIZE = 10000; -/** - * Minimum interval between "set rejected: value too large" warnings - * (LAB-1388). The rejection itself is often invisible (degradation swallows - * set failures; consumers try/catch set), so the SDK reports it through the - * logger — rate-limited so a hot oversized key can't flood the sink. - */ -export const VALUE_TOO_LARGE_WARN_INTERVAL_MS = 60_000; - /** Maximum size for key generation (64KB) */ export const KEY_GEN_MAX_SIZE = 64 * 1024; From 2ac98dbf15061dcfade75a7484424faed148a4d5 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Tue, 4 Aug 2026 02:11:08 +1000 Subject: [PATCH 3/8] =?UTF-8?q?fix:=20Kody=20review=20round=20=E2=80=94=20?= =?UTF-8?q?no-expiry=20marker=20header,=20interop-path=20size=20warn,=20gu?= =?UTF-8?q?arded=20ts=20import=20(LAB-1388)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../cachekit/scripts/check-workers-bundle.mjs | 9 +++- .../src/backends/workers-cache-api.test.ts | 12 ++++- .../src/backends/workers-cache-api.ts | 44 ++++++++++++------- packages/cachekit/src/cache-core.ts | 14 +++++- packages/cachekit/src/cache.test.ts | 21 +++++++++ 5 files changed, 79 insertions(+), 21 deletions(-) diff --git a/packages/cachekit/scripts/check-workers-bundle.mjs b/packages/cachekit/scripts/check-workers-bundle.mjs index fe49d3a..b780441 100644 --- a/packages/cachekit/scripts/check-workers-bundle.mjs +++ b/packages/cachekit/scripts/check-workers-bundle.mjs @@ -83,7 +83,14 @@ console.log( // reference typechecks clean here while still breaking consumers that don't // have it. Diagnostics are checked too (with skipLibCheck off) so the // closure is also proven self-consistent against ES+DOM libs alone. -const ts = (await import('typescript')).default; +let ts; +try { + ts = (await import('typescript')).default; +} catch (error) { + console.error('type-closure guard: failed to load the typescript compiler (devDependency)'); + console.error(error?.message ?? error); + process.exit(1); +} const typesEntry = join(pkgDir, 'dist', 'workers', 'index.d.ts'); const compilerOptions = { diff --git a/packages/cachekit/src/backends/workers-cache-api.test.ts b/packages/cachekit/src/backends/workers-cache-api.test.ts index eff4a45..250701f 100644 --- a/packages/cachekit/src/backends/workers-cache-api.test.ts +++ b/packages/cachekit/src/backends/workers-cache-api.test.ts @@ -86,14 +86,22 @@ describe('CacheAPIBackend (unit, mocked caches global)', () => { expect(result!.ttlSeconds).toBe(0); }); - it('getWithTtl reports null (no expiry) for the no-expiry sentinel', async () => { + it('getWithTtl reports null (no expiry) for ttl <= 0 via the marker header', async () => { const backend = workersCacheAPI(); - await backend.set('ns:key', value, 0); // ttl <= 0 → 1-year sentinel max-age + await backend.set('ns:key', value, 0); // ttl <= 0 → marker header + 1-year max-age const result = await backend.getWithTtl('ns:key'); expect(result!.ttlSeconds).toBeNull(); }); + it('getWithTtl treats a legitimate exactly-one-year TTL as a real TTL, not the sentinel', async () => { + const backend = workersCacheAPI(); + await backend.set('ns:key', value, 31_536_000); // same max-age as the sentinel, no marker + + const result = await backend.getWithTtl('ns:key'); + expect(result!.ttlSeconds).toBe(31_536_000); + }); + it('getWithTtl reports null when freshness headers are absent', async () => { const backend = workersCacheAPI(); await backend.set('ns:key', value, 30); diff --git a/packages/cachekit/src/backends/workers-cache-api.ts b/packages/cachekit/src/backends/workers-cache-api.ts index 6486280..aa772bb 100644 --- a/packages/cachekit/src/backends/workers-cache-api.ts +++ b/packages/cachekit/src/backends/workers-cache-api.ts @@ -10,6 +10,14 @@ import { DEFAULT_TTL_SECONDS } from '../constants.js'; */ const CACHE_API_NO_EXPIRY_MAX_AGE = 31_536_000; +/** + * Marker header distinguishing a genuine no-expiry entry (`ttl <= 0`) from + * a caller who legitimately set a TTL of exactly one year — both write the + * same `max-age`, so max-age alone is ambiguous. getWithTtl reports + * `ttlSeconds: null` (no expiry) only when this header is present. + */ +const NO_EXPIRY_HEADER = 'X-CacheKit-No-Expiry'; + /** * Synthetic URL base for cache keys. The Cache API is request-keyed, so each * cache key maps to a URL under a deliberately non-routable host — it can @@ -142,19 +150,19 @@ export class CacheAPIBackend implements Backend { async set(key: string, value: Uint8Array, ttl?: number): Promise { this.ensureNotClosed(); const effectiveTtl = ttl ?? this.defaultTtl; - const maxAge = effectiveTtl > 0 ? Math.ceil(effectiveTtl) : CACHE_API_NO_EXPIRY_MAX_AGE; + const noExpiry = effectiveTtl <= 0; + const maxAge = noExpiry ? CACHE_API_NO_EXPIRY_MAX_AGE : Math.ceil(effectiveTtl); try { - await ( - await this.cache() - ).put( - keyUrl(key), - new Response(value, { - headers: { - 'Cache-Control': `max-age=${maxAge}`, - 'Content-Type': 'application/octet-stream', - }, - }) - ); + const headers: Record = { + 'Cache-Control': `max-age=${maxAge}`, + 'Content-Type': 'application/octet-stream', + }; + // Explicit marker instead of overloading max-age as the no-expiry + // signal: a caller may legitimately set ttl to exactly one year, + // which writes the same max-age as the sentinel. getWithTtl reads + // this header to report "no expiry" (null) unambiguously. + if (noExpiry) headers[NO_EXPIRY_HEADER] = '1'; + await (await this.cache()).put(keyUrl(key), new Response(value, { headers })); } catch (error) { throw this.wrapError('set', error); } @@ -242,15 +250,19 @@ function keyUrl(key: string): string { /** * Remaining lifetime of a matched response: the `max-age` set() wrote minus * the `Age` header the edge reports. Null (unknown / no expiry) when the - * entry carries the no-expiry sentinel or the headers are absent — e.g. - * test harnesses that don't emit `Age` report the full max-age, which the - * caller's own TTL cap still bounds; never a negative freshness. + * entry carries the explicit no-expiry marker header or the freshness + * headers are absent. Any max-age without the marker is treated as a real + * TTL — including exactly one year, which a caller may legitimately set. + * (Entries written by 0.1.5, before the marker existed, report their + * sentinel max-age as a real ~1-year remainder; the caller's own TTL cap + * bounds it, so behavior is unchanged for them.) Test harnesses that don't + * emit `Age` report the full max-age — never a negative freshness. */ function remainingTtlSeconds(response: Response): number | null { + if (response.headers.get(NO_EXPIRY_HEADER) !== null) return null; const maxAgeMatch = /(?:^|[,\s])max-age=(\d+)/.exec(response.headers.get('Cache-Control') ?? ''); if (!maxAgeMatch) return null; const maxAge = Number(maxAgeMatch[1]); - if (maxAge >= CACHE_API_NO_EXPIRY_MAX_AGE) return null; const age = Number(response.headers.get('Age')); return Math.max(0, maxAge - (Number.isFinite(age) && age > 0 ? Math.floor(age) : 0)); } diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index fcc0a13..6145590 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -570,8 +570,18 @@ export class CacheImpl implements SecureCache { // never reaches the reliability executor, where degradation would // silently swallow it and retry/circuit-breaker would count it as a // backend failure. Auto-mode encoding stays inside the executor - // (existing degrade semantics unchanged). - const interopSerialized = interop ? encodeInteropValue(value) : null; + // (existing degrade semantics unchanged). A size rejection still routes + // through the LAB-1388 warning: degradation never hides this path, but + // a consumer's own try/catch around set() does. + let interopSerialized: Uint8Array | null = null; + if (interop) { + try { + interopSerialized = encodeInteropValue(value); + } catch (error) { + if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error); + throw error; + } + } return this.run('set', undefined, async (): Promise => { // Serialize. A size rejection here is invisible in production configs diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 680700d..dd9e229 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -699,6 +699,27 @@ describe('Cache Integration', () => { await c.close(); }); + + it('warns on the interop encode path too (rejection throws to the caller but hides behind consumer try/catch)', async () => { + const logs: string[] = []; + setLogger((message) => logs.push(message)); + + const c = createCache({ backend: new InMemoryBackend() }); + const big = c.wrap(async () => oversized(), { + namespace: 'ns', + ttl: 60, + interop: 'bigop', + interopArity: 0, + }); + + // Interop model/size rejection is a deterministic caller error — + // degradation never swallows it — but it must still emit the + // greppable warning for consumers whose own try/catch absorbs it. + await expect(big()).rejects.toThrow(ValueTooLargeError); + expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(1); + + await c.close(); + }); }); describe('backend-advertised compression default (LAB-1388)', () => { From 5b182cc98474fb266e528de4d25c63e95eb1391c Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Tue, 4 Aug 2026 02:46:25 +1000 Subject: [PATCH 4/8] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20README=20preposition=20+=20L1=20zero-TTL=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/cachekit/README.md | 2 +- packages/cachekit/src/cache-core.ts | 8 +++++++- packages/cachekit/src/cache.test.ts | 18 ++++++++++++++++++ packages/cachekit/src/l1/lru-cache.test.ts | 10 ++++++++++ packages/cachekit/src/l1/lru-cache.ts | 5 ++++- 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index cfbe3f3..8da5d0f 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -452,7 +452,7 @@ ByteStorage compression envelope **off** — Cloudflare already stores isolate CPU compressing twice. Pass `compression: true` to re-enable it (e.g. to shrink bodies below a size limit before storage). Reads are envelope-tolerant either way: a compression-off cache detects, verifies, -and unwraps entries an earlier version (or a compression-on peer in a +and unwraps entries that an earlier version (or a compression-on peer in a mixed fleet) stored with the envelope, so upgrades and gradual rollouts never serve envelope bytes as values: diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index 6145590..2932799 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -524,8 +524,14 @@ export class CacheImpl implements SecureCache { if (this.l1) { const namespace = interop ? key.slice(0, key.indexOf(':')) : extractNamespace(key); const capSeconds = ttlSeconds ?? this.defaultTtl; + // ttl <= 0 means "no expiry" (ts-wide Backend contract) — treat it + // as infinite here so Math.min still caps to a real remainingTtl + // when the backend reports one, instead of collapsing to 0 and + // tripping the skip-guard below for an entry that should never + // expire in L1 (LAB-1388). + const capOrForever = capSeconds > 0 ? capSeconds : Infinity; const l1TtlSeconds = - remainingTtl !== null ? Math.min(capSeconds, remainingTtl) : capSeconds; + remainingTtl !== null ? Math.min(capOrForever, remainingTtl) : capOrForever; if (l1TtlSeconds > 0) { this.l1.set(key, value, l1TtlSeconds * 1000, namespace); this.publishL1Stats(); diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index dd9e229..a1553a4 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -649,6 +649,24 @@ describe('Cache Integration', () => { await writer.close(); await reader.close(); }); + + it('repopulates L1 forever when defaultTtl is 0 and the entry has no expiry (LAB-1388)', async () => { + const shared = new TtlAwareBackend(); + const writer = createCache({ backend: shared, defaultTtl: 0 }); + const reader = createCache({ backend: shared, defaultTtl: 0 }); + + await writer.set('ns:entry', 'v1'); // ttl<=0 -> no expiry, per Backend contract + + // Pre-fix, capSeconds (0) collapsed the Math.min cap to 0 and the + // `l1TtlSeconds > 0` guard skipped L1 repopulation entirely. + expect(await reader.get('ns:entry')).toBe('v1'); // L2 hit, should populate L1 + + shared.store.clear(); // prove the next read comes from L1, not L2 + expect(await reader.get('ns:entry')).toBe('v1'); + + await writer.close(); + await reader.close(); + }); }); describe('oversized-value set() warning (LAB-1388)', () => { diff --git a/packages/cachekit/src/l1/lru-cache.test.ts b/packages/cachekit/src/l1/lru-cache.test.ts index 4d2cd92..32cc330 100644 --- a/packages/cachekit/src/l1/lru-cache.test.ts +++ b/packages/cachekit/src/l1/lru-cache.test.ts @@ -35,6 +35,16 @@ describe('L1Cache', () => { vi.useRealTimers(); }); + it('ttl <= 0 never expires (LAB-1388: matches the ts-wide "no expiry" contract)', () => { + vi.useFakeTimers(); + cache.set('zero', 'value', 0, 'test'); + cache.set('negative', 'value', -1, 'test'); + vi.advanceTimersByTime(1000 * 60 * 60 * 24 * 365); // 1 year + expect(cache.get('zero')).toBe('value'); + expect(cache.get('negative')).toBe('value'); + vi.useRealTimers(); + }); + it('deletes key and returns true', () => { cache.set('key', 'value', 10000, 'test'); expect(cache.delete('key')).toBe(true); diff --git a/packages/cachekit/src/l1/lru-cache.ts b/packages/cachekit/src/l1/lru-cache.ts index d665288..316ee53 100644 --- a/packages/cachekit/src/l1/lru-cache.ts +++ b/packages/cachekit/src/l1/lru-cache.ts @@ -227,7 +227,10 @@ export class L1Cache { const now = Date.now(); const entry: CacheEntry = { value, - expiresAt: now + ttl, + // ttl <= 0 means "no expiry" (ts-wide Backend contract, LAB-1388) — + // without this guard `now + 0` expires the entry on the very next + // millisecond instead of caching it forever. + expiresAt: ttl > 0 ? now + ttl : Infinity, originalTtl: ttl, size, namespace, From 1dcaec995cea6b85634ef5616d87d624c0da2281 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sun, 9 Aug 2026 10:12:44 +1000 Subject: [PATCH 5/8] fix: expert-panel remediation on the LAB-238 composition (LAB-1768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- packages/cachekit/src/cache-core.ts | 43 +++++++++++++++++++++-------- packages/cachekit/src/cache.test.ts | 40 +++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index 9261969..ac44955 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -380,8 +380,12 @@ export class CacheImpl implements SecureCache { * plain serialized data. The codec is created lazily and only once. */ private tryUnwrapEnvelope(bytes: Uint8Array): Uint8Array | null { + // Codec construction stays OUTSIDE the try: a broken binding must fail + // loudly (through the reliability executor), not be conflated with "not + // an envelope" — that would silently serve raw envelope tuples, the + // exact corruption this path exists to prevent (expert panel, LAB-1768). + this.envelopeReader ??= this.createByteStorage(); try { - this.envelopeReader ??= this.createByteStorage(); return this.envelopeReader.unpack(bytes); } catch { return null; @@ -393,14 +397,17 @@ export class CacheImpl implements SecureCache { * (LAB-1388) — the only reliable signal of the rejection when degradation * or a consumer catch-block absorbs the ValueTooLargeError itself. */ - private warnValueTooLarge(key: string, error: ValueTooLargeError): void { + private warnValueTooLarge(key: string, error: ValueTooLargeError, interop: boolean): void { const now = Date.now(); if (now - this.lastSizeWarnAt < VALUE_TOO_LARGE_WARN_INTERVAL_MS) return; this.lastSizeWarnAt = now; - logError( - `[cachekit] set rejected, value NOT cached (key=${key}): ${error.message}. ` + - 'Raise serializer.maxEncodedSize / maxDecodedSize if values this large are expected.' - ); + // Interop caps are protocol constants serializer config does not govern + // — the remediation hint only holds for the serializer path (expert + // panel, LAB-1768). + const hint = interop + ? '' + : ' Raise serializer.maxEncodedSize / maxDecodedSize if values this large are expected.'; + logError(`[cachekit] set rejected, value NOT cached (key=${key}): ${error.message}.${hint}`); } private publishL1Stats(): void { @@ -515,9 +522,13 @@ export class CacheImpl implements SecureCache { // hygiene: the envelope is itself valid MessagePack (a positional // 4-tuple), so a plain decode would "succeed" and serve the envelope // structure as the cached value — silent corruption, invisible to - // degradation. The unpack's xxHash3 check makes sniff false-positives - // vanishingly unlikely, and any unpack failure falls back to treating - // the bytes as plain-serialized. + // degradation. The unpack's xxHash3 check rejects ACCIDENTAL + // look-alikes; it is keyless, so it is not a defense against an + // adversarial writer deliberately crafting a valid envelope as its + // cached value (accepted eyes-open in LAB-1388/LAB-1768 — blast + // radius bounded by maxDecodedSize/maxDepth on the unpacked bytes). + // Any unpack failure falls back to treating the bytes as + // plain-serialized. // // Encrypted caches never reach this branch for a genuinely mismatched // entry: the AAD binds useEnvelope (frozen v0x03 set, protocol#12), so @@ -651,7 +662,13 @@ export class CacheImpl implements SecureCache { const l1TtlSeconds = remainingTtl !== null ? Math.min(capOrForever, remainingTtl) : capOrForever; if (l1TtlSeconds > 0) { - this.l1.set(key, this.l1Payload(value, data), l1TtlSeconds * 1000, namespace); + // Hand L1 its own canonical no-expiry encoding (ttl <= 0), never + // Infinity ms: an Infinity originalTtl turns getWithSwr's + // freshness check into `Infinity > Infinity` — permanently stale, + // arming a spurious background refresh per marker window, forever + // (expert panel, LAB-1768). + const l1TtlMs = Number.isFinite(l1TtlSeconds) ? l1TtlSeconds * 1000 : 0; + this.l1.set(key, this.l1Payload(value, data), l1TtlMs, namespace); this.publishL1Stats(); } } @@ -718,7 +735,7 @@ export class CacheImpl implements SecureCache { try { interopSerialized = encodeInteropValue(value); } catch (error) { - if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error); + if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error, true); throw error; } } @@ -734,7 +751,9 @@ export class CacheImpl implements SecureCache { try { serialized = interopSerialized ?? this.serializer.encode(value); } catch (error) { - if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error); + // Only serializer.encode throws here — a non-null interopSerialized + // already survived encodeInteropValue above. + if (error instanceof ValueTooLargeError) this.warnValueTooLarge(key, error, false); throw error; } diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index a1553a4..31fc151 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -667,6 +667,46 @@ describe('Cache Integration', () => { await writer.close(); await reader.close(); }); + + it('keeps a no-expiry L1 re-population SWR-fresh (no phantom refresh loop, LAB-1768)', async () => { + const shared = new TtlAwareBackend(); + const writer = createCache({ backend: shared, defaultTtl: 0 }); + const reader = createCache({ + backend: shared, + defaultTtl: 0, + l1: { swrEnabled: true, swrThresholdRatio: 2 }, + }); + + let computes = 0; + const compute = async () => { + computes++; + return 'v1'; + }; + + // Seed L2 (no expiry) through the writer so the reader's first wrap() + // read is an L2 hit that re-populates its L1 through the getWithTtl + // path with ttlSeconds: null. + const seed = writer.wrap(compute, { namespace: 'noexp', ttl: 0 }); + expect(await seed()).toBe('v1'); + expect(computes).toBe(1); + + const read = reader.wrap(compute, { namespace: 'noexp', ttl: 0 }); + expect(await read()).toBe('v1'); // L2 hit → L1 re-populate + expect(computes).toBe(1); + + // Pre-fix, the L1 copy carried originalTtl = Infinity, so getWithSwr's + // freshness check compared Infinity > Infinity — permanently stale — + // and every read here armed a background refresh that re-ran compute + // and rewrote L2, forever, on exactly the entries configured to never + // expire. + expect(await read()).toBe('v1'); + expect(await read()).toBe('v1'); + await Promise.resolve(); // let any (wrongly) scheduled refresh start + expect(computes).toBe(1); + + await writer.close(); + await reader.close(); + }); }); describe('oversized-value set() warning (LAB-1388)', () => { From f4ac78743d04fc7a8dec0cf084c1b8855f08b35e Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sun, 9 Aug 2026 11:12:03 +1000 Subject: [PATCH 6/8] =?UTF-8?q?fix:=20CodeRabbit=20round=20=E2=80=94=20fre?= =?UTF-8?q?e=20post-close=20envelope=20codec,=20redact=20keys=20from=20siz?= =?UTF-8?q?e-rejection=20log=20(LAB-1768)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- packages/cachekit/README.md | 7 ++- packages/cachekit/src/cache-core.ts | 36 +++++++++++-- packages/cachekit/src/cache.test.ts | 81 +++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 6 deletions(-) diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index e638345..7b87986 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -178,8 +178,11 @@ const cache = createCache.minimal({ The SDK also reports every size rejection through its [pluggable logger](#observability) as a rate-limited, greppable -`[cachekit] set rejected, value NOT cached (key=...)` line — watch for it -after deploying a new cache. (Backends have their own hard ceilings too: +`[cachekit] set rejected, value NOT cached (keyHash=...)` line — watch for it +after deploying a new cache. The line carries a non-reversible blake2b digest +of the cache key rather than the key itself (keys are caller-controlled and +may embed sensitive data); to match a digest to a suspect key, hash the key +with blake2b (16-byte output, hex). (Backends have their own hard ceilings too: Workers KV values cap at 25 MiB, Memcached items at 1 MiB server-side, CachekitIO per plan.) diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index ac44955..f14f1af 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -11,6 +11,8 @@ import type { import type { Backend, L1Metrics, LockableBackend } from './backends/types.js'; import type { InvalidationEvent } from './l1/types.js'; import type { MetricsCollector, MetricsConfig } from './metrics/prometheus.js'; +import { blake2b } from '@noble/hashes/blake2.js'; +import { bytesToHex, utf8ToBytes } from '@noble/hashes/utils.js'; import { logError } from './logger.js'; import { L1Cache } from './l1/lru-cache.js'; import { ReliabilityExecutor } from './reliability/executor.js'; @@ -377,18 +379,37 @@ export class CacheImpl implements SecureCache { * Verified unpack of a suspected legacy/foreign ByteStorage envelope on a * compression-off cache. Returns null when the bytes aren't actually an * envelope (checksum/shape mismatch) — the caller then treats them as - * plain serialized data. The codec is created lazily and only once. + * plain serialized data. The codec is created lazily and cached — except + * after close(), when a throwaway codec is used and freed immediately. */ private tryUnwrapEnvelope(bytes: Uint8Array): Uint8Array | null { // Codec construction stays OUTSIDE the try: a broken binding must fail // loudly (through the reliability executor), not be conflated with "not // an envelope" — that would silently serve raw envelope tuples, the // exact corruption this path exists to prevent (expert panel, LAB-1768). - this.envelopeReader ??= this.createByteStorage(); + // + // After close() the cached reader has already been freed — an in-flight + // read resuming post-shutdown must not resurrect the cache (close() will + // never free it again), so it gets a throwaway codec freed right here. + const reader = this.closed + ? this.createByteStorage() + : (this.envelopeReader ??= this.createByteStorage()); try { - return this.envelopeReader.unpack(bytes); + return reader.unpack(bytes); } catch { return null; + } finally { + if (reader !== this.envelopeReader) { + try { + reader.free?.(); + } catch (error) { + // Never mask the unpack result with a cleanup failure — report it + // through the logger instead. + logError( + `[cachekit] failed to free post-close envelope codec: ${error instanceof Error ? error.message : String(error)}` + ); + } + } } } @@ -407,7 +428,14 @@ export class CacheImpl implements SecureCache { const hint = interop ? '' : ' Raise serializer.maxEncodedSize / maxDecodedSize if values this large are expected.'; - logError(`[cachekit] set rejected, value NOT cached (key=${key}): ${error.message}.${hint}`); + // Keys are caller-controlled and may embed PII/credentials — log a + // non-reversible digest, not the key itself. Same key → same digest, so + // repeated rejections still correlate, and holders of a suspect key can + // recompute the digest to match it. + const keyHash = bytesToHex(blake2b(utf8ToBytes(key), { dkLen: 16 })); + logError( + `[cachekit] set rejected, value NOT cached (keyHash=${keyHash}): ${error.message}.${hint}` + ); } private publishL1Stats(): void { diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 31fc151..6b091fb 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -6,6 +6,7 @@ import { ValueTooLargeError } from './errors.js'; import { MessagePackSerializer } from './serialization/serializer.js'; import type { SecureCache } from './types/cache.js'; import type { Backend } from './backends/types.js'; +import type { ByteStorageLike } from './cache-core.js'; /** * Simple in-memory backend for testing cache integration. @@ -417,6 +418,82 @@ describe('Cache Integration', () => { await rawWriter.close(); await envelopeReader.close(); }); + + it('frees a throwaway envelope codec when an in-flight read resumes after close()', async () => { + const sharedBackend = new InMemoryBackend(); + + const writer = createCache({ + backend: sharedBackend, + compression: true, + l1: { enabled: false }, + }); + await writer.set('test:postclose', { data: 'compressed' }); + + // Capture the enveloped bytes now — closing the caches clears the + // shared in-memory store. + const stored = await sharedBackend.get('test:postclose'); + expect(stored).not.toBeNull(); + + // Backend whose get() blocks until released, so the read can be + // suspended across close(). + let releaseGet!: () => void; + const gateOpened = new Promise((resolve) => (releaseGet = resolve)); + let getEntered!: () => void; + const getStarted = new Promise((resolve) => (getEntered = resolve)); + const gatedBackend = new Proxy(sharedBackend, { + get(target, prop, receiver) { + if (prop === 'get') { + return async () => { + getEntered(); + await gateOpened; + return stored; + }; + } + return Reflect.get(target, prop, receiver); + }, + }); + + const reader = createCache({ + backend: gatedBackend, + compression: false, + l1: { enabled: false }, + }); + + // Track every codec the reader creates for envelope tolerance and + // every free() on them. + let created = 0; + let freed = 0; + const impl = reader as unknown as { createByteStorage: () => ByteStorageLike }; + const realFactory = impl.createByteStorage; + impl.createByteStorage = () => { + created++; + const codec = realFactory(); + return { + pack: (data) => codec.pack(data), + unpack: (packed) => codec.unpack(packed), + free: () => { + freed++; + codec.free?.(); + }, + }; + }; + + // Start the read (passes the closed guard), then close while it is + // suspended inside the backend. + const pending = reader.get('test:postclose'); + await getStarted; + await reader.close(); + releaseGet(); + + // The read still completes correctly — via a throwaway codec that was + // freed immediately, never cached on the closed instance. + expect(await pending).toEqual({ data: 'compressed' }); + expect(created).toBe(1); + expect(freed).toBe(1); + expect((reader as unknown as { envelopeReader: unknown }).envelopeReader).toBeNull(); + + await writer.close(); + }); }); describe('Error Handling', () => { @@ -730,6 +807,10 @@ describe('Cache Integration', () => { // …but the rejection is reported, once, greppably. expect(logs.filter((m) => m.includes('set rejected'))).toHaveLength(1); expect(logs[0]).toContain('maxEncodedSize'); + // Keys are caller-controlled and may embed PII — the line carries a + // non-reversible digest, never the raw key. + expect(logs[0]).toContain('keyHash='); + expect(logs[0]).not.toContain('ns:big'); // Rate-limited: a hot oversized key cannot flood the sink. await c.set('ns:big', oversized()); From 761d70b474817b93c9caea49ee5630008816585c Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sun, 9 Aug 2026 11:23:13 +1000 Subject: [PATCH 7/8] =?UTF-8?q?fix:=20expert-panel=20remediation=20?= =?UTF-8?q?=E2=80=94=20throwaway=20codec=20for=20ALL=20post-close=20envelo?= =?UTF-8?q?pe=20ops,=20shared=20blake2b16Hex=20(LAB-1768)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- packages/cachekit/README.md | 4 +- packages/cachekit/src/backends/file.ts | 6 +- packages/cachekit/src/cache-core.ts | 61 +++++++++++----- packages/cachekit/src/cache.test.ts | 70 +++++++++++++++++++ .../src/serialization/key-generator.ts | 13 +++- 5 files changed, 131 insertions(+), 23 deletions(-) diff --git a/packages/cachekit/README.md b/packages/cachekit/README.md index 7b87986..175c28f 100644 --- a/packages/cachekit/README.md +++ b/packages/cachekit/README.md @@ -182,7 +182,9 @@ The SDK also reports every size rejection through its after deploying a new cache. The line carries a non-reversible blake2b digest of the cache key rather than the key itself (keys are caller-controlled and may embed sensitive data); to match a digest to a suspect key, hash the key -with blake2b (16-byte output, hex). (Backends have their own hard ceilings too: +with blake2b (16-byte output, hex). This is the same digest the File backend +uses as its on-disk filename, so on that backend a logged `keyHash` names the +entry's cache file directly. (Backends have their own hard ceilings too: Workers KV values cap at 25 MiB, Memcached items at 1 MiB server-side, CachekitIO per plan.) diff --git a/packages/cachekit/src/backends/file.ts b/packages/cachekit/src/backends/file.ts index d566b8e..5014eed 100644 --- a/packages/cachekit/src/backends/file.ts +++ b/packages/cachekit/src/backends/file.ts @@ -2,8 +2,7 @@ import { constants as fsConstants } from 'node:fs'; import fs, { type FileHandle } from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; -import { blake2b } from '@noble/hashes/blake2.js'; -import { bytesToHex } from '@noble/hashes/utils.js'; +import { blake2b16Hex } from '../serialization/key-generator.js'; import { Backend, FileBackendConfig, TTLBackend } from './types.js'; import { BackendError } from '../errors.js'; @@ -262,8 +261,7 @@ export class FileBackend implements Backend, TTLBackend { * unset: key identity is preserved, nothing is prefixed on the wire). */ private keyToPath(key: string): string { - const hash = bytesToHex(blake2b(new TextEncoder().encode(key), { dkLen: 16 })); - return path.join(this.config.cacheDir, hash); + return path.join(this.config.cacheDir, blake2b16Hex(key)); } /** diff --git a/packages/cachekit/src/cache-core.ts b/packages/cachekit/src/cache-core.ts index f14f1af..42437f3 100644 --- a/packages/cachekit/src/cache-core.ts +++ b/packages/cachekit/src/cache-core.ts @@ -11,8 +11,6 @@ import type { import type { Backend, L1Metrics, LockableBackend } from './backends/types.js'; import type { InvalidationEvent } from './l1/types.js'; import type { MetricsCollector, MetricsConfig } from './metrics/prometheus.js'; -import { blake2b } from '@noble/hashes/blake2.js'; -import { bytesToHex, utf8ToBytes } from '@noble/hashes/utils.js'; import { logError } from './logger.js'; import { L1Cache } from './l1/lru-cache.js'; import { ReliabilityExecutor } from './reliability/executor.js'; @@ -26,6 +24,7 @@ import { generateKey, generateParamsHash, extractNamespace, + blake2b16Hex, } from './serialization/key-generator.js'; import { generateInteropKey, @@ -399,17 +398,35 @@ export class CacheImpl implements SecureCache { } catch { return null; } finally { - if (reader !== this.envelopeReader) { - try { - reader.free?.(); - } catch (error) { - // Never mask the unpack result with a cleanup failure — report it - // through the logger instead. - logError( - `[cachekit] failed to free post-close envelope codec: ${error instanceof Error ? error.message : String(error)}` - ); - } - } + if (reader !== this.envelopeReader) this.freeThrowawayCodec(reader); + } + } + + /** + * Pack/unpack through the compression-on codec (`byteStorage`) — or, after + * close() has freed it, through a throwaway codec freed right here. Same + * post-shutdown hazard tryUnwrapEnvelope guards: an in-flight operation + * resuming after close() must neither touch freed wasm memory (a + * use-after-free on Workers) nor cache a codec nothing will ever free. + * Callers must have established useEnvelope (byteStorage non-null). + */ + private withEnvelopeCodec(use: (codec: ByteStorageLike) => T): T { + const codec = this.closed ? this.createByteStorage() : this.byteStorage!; + try { + return use(codec); + } finally { + if (codec !== this.byteStorage) this.freeThrowawayCodec(codec); + } + } + + /** Free a post-close throwaway codec without masking the caller's result. */ + private freeThrowawayCodec(codec: ByteStorageLike): void { + try { + codec.free?.(); + } catch (error) { + logError( + `[cachekit] failed to free post-close envelope codec: ${error instanceof Error ? error.message : String(error)}` + ); } } @@ -432,7 +449,7 @@ export class CacheImpl implements SecureCache { // non-reversible digest, not the key itself. Same key → same digest, so // repeated rejections still correlate, and holders of a suspect key can // recompute the digest to match it. - const keyHash = bytesToHex(blake2b(utf8ToBytes(key), { dkLen: 16 })); + const keyHash = blake2b16Hex(key); logError( `[cachekit] set rejected, value NOT cached (keyHash=${keyHash}): ${error.message}.${hint}` ); @@ -542,7 +559,7 @@ export class CacheImpl implements SecureCache { plaintext = await this.encryption.decrypt(plaintext, key, useEnvelope); } if (useEnvelope) { - plaintext = this.byteStorage!.unpack(plaintext); + plaintext = this.withEnvelopeCodec((codec) => codec.unpack(plaintext)); } else if (!interop && looksLikeEnvelope(plaintext)) { // Envelope tolerance (LAB-1388): a compression-off cache can read // entries a compression-on writer stored — same store, older SDK @@ -786,7 +803,9 @@ export class CacheImpl implements SecureCache { } // Compress with ByteStorage (before encryption) - let data: Uint8Array = useEnvelope ? this.byteStorage!.pack(serialized) : serialized; + let data: Uint8Array = useEnvelope + ? this.withEnvelopeCodec((codec) => codec.pack(serialized)) + : serialized; // Encrypt if encryption enabled if (this.encryption) { @@ -1292,8 +1311,16 @@ export class CacheImpl implements SecureCache { // Release the envelope codecs (zeroizes/frees wasm resources on Workers; // no-op for the GC-managed NAPI binding) + // envelopeReader is also nulled: a freed-but-dangling wasm codec behind a + // non-null reference is an instant use-after-free for any future caller + // that forgets the `closed` check. byteStorage is readonly and cannot be + // nulled — its callers route through withEnvelopeCodec, which checks + // `closed` before touching it. attempt(() => this.byteStorage?.free?.()); - attempt(() => this.envelopeReader?.free?.()); + attempt(() => { + this.envelopeReader?.free?.(); + this.envelopeReader = null; + }); // Clear L1 (this also clears L1's internal refreshingKeys) attempt(() => this.l1?.clear()); diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 6b091fb..21b2b35 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -494,6 +494,76 @@ describe('Cache Integration', () => { await writer.close(); }); + + it('uses a freed-immediately throwaway codec for post-close reads on compression-ON caches too', async () => { + // Same in-flight-across-close() interleaving as above, but on the + // DEFAULT compression-on path: close() frees this.byteStorage, so the + // resumed read must not unpack with the freed codec (a use-after-free + // on the wasm binding) — it gets a throwaway instead (expert panel, + // LAB-1768). + const sharedBackend = new InMemoryBackend(); + + const writer = createCache({ + backend: sharedBackend, + compression: true, + l1: { enabled: false }, + }); + await writer.set('test:postclose-on', { data: 'enveloped' }); + + const stored = await sharedBackend.get('test:postclose-on'); + expect(stored).not.toBeNull(); + + let releaseGet!: () => void; + const gateOpened = new Promise((resolve) => (releaseGet = resolve)); + let getEntered!: () => void; + const getStarted = new Promise((resolve) => (getEntered = resolve)); + const gatedBackend = new Proxy(sharedBackend, { + get(target, prop, receiver) { + if (prop === 'get') { + return async () => { + getEntered(); + await gateOpened; + return stored; + }; + } + return Reflect.get(target, prop, receiver); + }, + }); + + const reader = createCache({ + backend: gatedBackend, + compression: true, + l1: { enabled: false }, + }); + + let created = 0; + let freed = 0; + const impl = reader as unknown as { createByteStorage: () => ByteStorageLike }; + const realFactory = impl.createByteStorage; + impl.createByteStorage = () => { + created++; + const codec = realFactory(); + return { + pack: (data) => codec.pack(data), + unpack: (packed) => codec.unpack(packed), + free: () => { + freed++; + codec.free?.(); + }, + }; + }; + + const pending = reader.get('test:postclose-on'); + await getStarted; + await reader.close(); + releaseGet(); + + expect(await pending).toEqual({ data: 'enveloped' }); + expect(created).toBe(1); + expect(freed).toBe(1); + + await writer.close(); + }); }); describe('Error Handling', () => { diff --git a/packages/cachekit/src/serialization/key-generator.ts b/packages/cachekit/src/serialization/key-generator.ts index b5fba15..7e23e6b 100644 --- a/packages/cachekit/src/serialization/key-generator.ts +++ b/packages/cachekit/src/serialization/key-generator.ts @@ -1,5 +1,5 @@ import { blake2b } from '@noble/hashes/blake2.js'; -import { bytesToHex } from '@noble/hashes/utils.js'; +import { bytesToHex, utf8ToBytes } from '@noble/hashes/utils.js'; import { MessagePackSerializer } from './serializer.js'; import { KEY_GEN_MAX_SIZE, KEY_GEN_MAX_DEPTH, CACHE_KEY_HASH_LENGTH } from '../constants.js'; @@ -60,6 +60,17 @@ export function generateParamsHash(args: unknown[]): string { return bytesToHex(hash); } +/** + * `blake2b(utf8(key), digestSize=16)` hex — 32 chars. The SDK's canonical + * short key digest, byte-identical to cachekit-py's `_key_to_path` stem: the + * File backend's flat filename and the redacted `keyHash=` in size-rejection + * logs are the same value, so a logged rejection can be matched to its cache + * file (or to a suspect key by recomputing) without ever logging the key. + */ +export function blake2b16Hex(key: string): string { + return bytesToHex(blake2b(utf8ToBytes(key), { dkLen: 16 })); +} + /** * Extract namespace from a full cache key. * From bf25263bec7893aefc02ac666867bb83936a33cb Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sun, 9 Aug 2026 12:05:18 +1000 Subject: [PATCH 8/8] test: extract shared post-close read fixture (LAB-1768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cachekit/src/cache.test.ts | 218 ++++++++++++---------------- 1 file changed, 96 insertions(+), 122 deletions(-) diff --git a/packages/cachekit/src/cache.test.ts b/packages/cachekit/src/cache.test.ts index 21b2b35..73a9f29 100644 --- a/packages/cachekit/src/cache.test.ts +++ b/packages/cachekit/src/cache.test.ts @@ -340,6 +340,90 @@ describe('Cache Integration', () => { }); describe('Compression Error Handling', () => { + /** + * Drives the post-close in-flight read interleaving shared by the throwaway-codec + * tests: start a get(), suspend it inside the backend, close() the cache, then + * release it. Returns before the read settles so each caller asserts its own outcome. + * + * Only the key, the stored value, and the reader's compression mode vary. The gated + * backend, the codec accounting, and the close/release ordering are deliberately a + * single fixture — if those drifted between the two callers, one of them would stop + * exercising the use-after-free window it exists to pin. + */ + async function startPostCloseRead(opts: { + key: string; + value: { data: string }; + readerCompression: boolean; + }) { + const { key, value, readerCompression } = opts; + const sharedBackend = new InMemoryBackend(); + + // The writer always envelopes; the reader's compression mode is what varies. + const writer = createCache({ + backend: sharedBackend, + compression: true, + l1: { enabled: false }, + }); + await writer.set(key, value); + + // Capture the enveloped bytes now — closing the caches clears the + // shared in-memory store. + const stored = await sharedBackend.get(key); + expect(stored).not.toBeNull(); + + // Backend whose get() blocks until released, so the read can be + // suspended across close(). + let releaseGet!: () => void; + const gateOpened = new Promise((resolve) => (releaseGet = resolve)); + let getEntered!: () => void; + const getStarted = new Promise((resolve) => (getEntered = resolve)); + const gatedBackend = new Proxy(sharedBackend, { + get(target, prop, receiver) { + if (prop === 'get') { + return async () => { + getEntered(); + await gateOpened; + return stored; + }; + } + return Reflect.get(target, prop, receiver); + }, + }); + + const reader = createCache({ + backend: gatedBackend, + compression: readerCompression, + l1: { enabled: false }, + }); + + // Track every codec the reader creates for envelope tolerance and + // every free() on them. + const counts = { created: 0, freed: 0 }; + const impl = reader as unknown as { createByteStorage: () => ByteStorageLike }; + const realFactory = impl.createByteStorage; + impl.createByteStorage = () => { + counts.created++; + const codec = realFactory(); + return { + pack: (data) => codec.pack(data), + unpack: (packed) => codec.unpack(packed), + free: () => { + counts.freed++; + codec.free?.(); + }, + }; + }; + + // Start the read (passes the closed guard), then close while it is + // suspended inside the backend. + const pending = reader.get(key); + await getStarted; + await reader.close(); + releaseGet(); + + return { reader, pending, counts, writer }; + } + it('should return null when backend returns corrupted compressed data (graceful degradation)', async () => { const corruptBackend = new InMemoryBackend(); const cache = createCache({ @@ -420,76 +504,17 @@ describe('Cache Integration', () => { }); it('frees a throwaway envelope codec when an in-flight read resumes after close()', async () => { - const sharedBackend = new InMemoryBackend(); - - const writer = createCache({ - backend: sharedBackend, - compression: true, - l1: { enabled: false }, + const { reader, pending, counts, writer } = await startPostCloseRead({ + key: 'test:postclose', + value: { data: 'compressed' }, + readerCompression: false, }); - await writer.set('test:postclose', { data: 'compressed' }); - - // Capture the enveloped bytes now — closing the caches clears the - // shared in-memory store. - const stored = await sharedBackend.get('test:postclose'); - expect(stored).not.toBeNull(); - - // Backend whose get() blocks until released, so the read can be - // suspended across close(). - let releaseGet!: () => void; - const gateOpened = new Promise((resolve) => (releaseGet = resolve)); - let getEntered!: () => void; - const getStarted = new Promise((resolve) => (getEntered = resolve)); - const gatedBackend = new Proxy(sharedBackend, { - get(target, prop, receiver) { - if (prop === 'get') { - return async () => { - getEntered(); - await gateOpened; - return stored; - }; - } - return Reflect.get(target, prop, receiver); - }, - }); - - const reader = createCache({ - backend: gatedBackend, - compression: false, - l1: { enabled: false }, - }); - - // Track every codec the reader creates for envelope tolerance and - // every free() on them. - let created = 0; - let freed = 0; - const impl = reader as unknown as { createByteStorage: () => ByteStorageLike }; - const realFactory = impl.createByteStorage; - impl.createByteStorage = () => { - created++; - const codec = realFactory(); - return { - pack: (data) => codec.pack(data), - unpack: (packed) => codec.unpack(packed), - free: () => { - freed++; - codec.free?.(); - }, - }; - }; - - // Start the read (passes the closed guard), then close while it is - // suspended inside the backend. - const pending = reader.get('test:postclose'); - await getStarted; - await reader.close(); - releaseGet(); // The read still completes correctly — via a throwaway codec that was // freed immediately, never cached on the closed instance. expect(await pending).toEqual({ data: 'compressed' }); - expect(created).toBe(1); - expect(freed).toBe(1); + expect(counts.created).toBe(1); + expect(counts.freed).toBe(1); expect((reader as unknown as { envelopeReader: unknown }).envelopeReader).toBeNull(); await writer.close(); @@ -501,66 +526,15 @@ describe('Cache Integration', () => { // resumed read must not unpack with the freed codec (a use-after-free // on the wasm binding) — it gets a throwaway instead (expert panel, // LAB-1768). - const sharedBackend = new InMemoryBackend(); - - const writer = createCache({ - backend: sharedBackend, - compression: true, - l1: { enabled: false }, + const { pending, counts, writer } = await startPostCloseRead({ + key: 'test:postclose-on', + value: { data: 'enveloped' }, + readerCompression: true, }); - await writer.set('test:postclose-on', { data: 'enveloped' }); - - const stored = await sharedBackend.get('test:postclose-on'); - expect(stored).not.toBeNull(); - - let releaseGet!: () => void; - const gateOpened = new Promise((resolve) => (releaseGet = resolve)); - let getEntered!: () => void; - const getStarted = new Promise((resolve) => (getEntered = resolve)); - const gatedBackend = new Proxy(sharedBackend, { - get(target, prop, receiver) { - if (prop === 'get') { - return async () => { - getEntered(); - await gateOpened; - return stored; - }; - } - return Reflect.get(target, prop, receiver); - }, - }); - - const reader = createCache({ - backend: gatedBackend, - compression: true, - l1: { enabled: false }, - }); - - let created = 0; - let freed = 0; - const impl = reader as unknown as { createByteStorage: () => ByteStorageLike }; - const realFactory = impl.createByteStorage; - impl.createByteStorage = () => { - created++; - const codec = realFactory(); - return { - pack: (data) => codec.pack(data), - unpack: (packed) => codec.unpack(packed), - free: () => { - freed++; - codec.free?.(); - }, - }; - }; - - const pending = reader.get('test:postclose-on'); - await getStarted; - await reader.close(); - releaseGet(); expect(await pending).toEqual({ data: 'enveloped' }); - expect(created).toBe(1); - expect(freed).toBe(1); + expect(counts.created).toBe(1); + expect(counts.freed).toBe(1); await writer.close(); });