From 8355b39ffcd67e6bf0040ac8e9360bb9b945585c Mon Sep 17 00:00:00 2001 From: James Date: Thu, 7 May 2026 04:55:24 +0000 Subject: [PATCH] fix(engine): skip caching oversized frames + add eviction telemetry --- .../src/services/videoFrameInjector.test.ts | 57 ++++++++++++++++++- .../engine/src/services/videoFrameInjector.ts | 41 ++++++++++++- 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/packages/engine/src/services/videoFrameInjector.test.ts b/packages/engine/src/services/videoFrameInjector.test.ts index ad3e1f812..28c813641 100644 --- a/packages/engine/src/services/videoFrameInjector.test.ts +++ b/packages/engine/src/services/videoFrameInjector.test.ts @@ -4,9 +4,12 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { __testing } from "./videoFrameInjector.js"; +import { DEFAULT_CONFIG } from "../config.js"; const { createFrameSourceCache } = __testing; +const SHARED_STATS = { evictions: 0, oversizedRejections: 0 }; + describe("frame source cache eviction", () => { let dir: string; @@ -39,6 +42,16 @@ describe("frame source cache eviction", () => { await cache.get(c); expect(cache.stats().entries).toBe(2); + expect(cache.stats().evictions).toBe(1); + + // Verify the *oldest* entry (a) was the victim — the LRU contract. + // A later get(a) is a miss-then-insert, which would also evict whichever + // entry is now oldest. We instrument the eviction counter to detect it. + const evictionsBefore = cache.stats().evictions; + await cache.get(a); + expect(cache.stats().evictions).toBe(evictionsBefore + 1); + // After re-inserting `a`, `b` is the next oldest. `c` is now newest. + // Touch `b` (move-to-front) → next eviction would be `c`, not `b`. }); it("evicts oldest entry when byte budget is exceeded", async () => { @@ -70,7 +83,7 @@ describe("frame source cache eviction", () => { expect(await cache.get(file)).toBe("/served/frame.png"); // Cache stays empty because the resolver short-circuits the read. - expect(cache.stats()).toEqual({ entries: 0, bytes: 0 }); + expect(cache.stats()).toMatchObject({ entries: 0, bytes: 0 }); served = null; const dataUri = await cache.get(file); @@ -87,4 +100,46 @@ describe("frame source cache eviction", () => { expect(second).toBe(first); expect(cache.stats().entries).toBe(1); }); + + it("skips caching an entry that alone exceeds the byte budget (no self-eviction)", async () => { + // 64 KB raw → ~88 KB base64 + prefix. Budget of 32 KB rejects this entry. + // The contract: caller still gets the data URI; cache stays empty so + // future inserts aren't blocked by the rejected entry's bookkeeping. + const cache = createFrameSourceCache(100, 32 * 1024); + const big = writeFrame("big.png", 64 * 1024); + + const dataUri = await cache.get(big); + expect(dataUri.startsWith("data:image/png;base64,")).toBe(true); + expect(cache.stats().entries).toBe(0); + expect(cache.stats().bytes).toBe(0); + expect(cache.stats().oversizedRejections).toBe(1); + expect(cache.stats().evictions).toBe(0); + + // A subsequent normal-sized entry must cache cleanly — the rejection + // path didn't pollute internal state. + const small = writeFrame("small.png", 1024); + await cache.get(small); + expect(cache.stats().entries).toBe(1); + }); + + it("at the production default (1500 MB), 1080p frames stay cached", async () => { + // Regression for the post-PR-#662 default: previously the cache held up + // to 256 entries × ~8 MB ≈ 2 GB at 1080p. The new byte-budget default of + // 1500 MB caps it tighter (~187 entries at 1080p ≈ 6s @ 30fps). This + // test pins the math so a future tweak to the default is visible. + const oneEightyP_jpegSize = 8 * 1024 * 1024; // ~8 MB JPEG (data URI) + const defaultBytesLimit = DEFAULT_CONFIG.frameDataUriCacheBytesLimitMb * 1024 * 1024; + const expectedMaxEntries = Math.floor(defaultBytesLimit / oneEightyP_jpegSize); + expect(expectedMaxEntries).toBeGreaterThanOrEqual(180); + expect(expectedMaxEntries).toBeLessThanOrEqual(200); + // At 30fps that's at least 6 seconds of look-ahead. Sequential access is + // strictly cheaper, so the cache helps any seek-back ≤ 6s. + expect(expectedMaxEntries / 30).toBeGreaterThanOrEqual(6); + }); + + // Suppress unused-import warning when the SHARED_STATS sentinel is dropped. + it("stats() exposes counters used by telemetry", async () => { + const cache = createFrameSourceCache(1, Number.MAX_SAFE_INTEGER); + expect(cache.stats()).toMatchObject({ ...SHARED_STATS, entries: 0, bytes: 0 }); + }); }); diff --git a/packages/engine/src/services/videoFrameInjector.ts b/packages/engine/src/services/videoFrameInjector.ts index 41707237a..b928decf9 100644 --- a/packages/engine/src/services/videoFrameInjector.ts +++ b/packages/engine/src/services/videoFrameInjector.ts @@ -23,11 +23,18 @@ export interface VideoFrameInjectorOptions extends Partial< interface FrameSourceCacheStats { entries: number; bytes: number; + /** Total entries evicted since cache creation. A high count vs a small + * composition signals the byte budget is too tight (cache thrash). */ + evictions: number; + /** Total inserts rejected because the entry alone exceeds bytesLimit. + * Non-zero means a single frame is bigger than the configured budget — + * raise `frameDataUriCacheBytesLimitMb` if it recurs in production. */ + oversizedRejections: number; } interface FrameSourceCache { get: (framePath: string) => Promise; - /** Exposed for tests; reflects the current cache occupancy. */ + /** Exposed for tests + telemetry; reflects current cache occupancy. */ stats: () => FrameSourceCacheStats; } @@ -36,6 +43,14 @@ interface FrameSourceCache { * oldest entry — entry count protects against pathological many-tiny-frames * cases, and the byte budget keeps memory bounded when the per-frame data * URI grows (4K PNG frames are ~33 MB once base64-encoded). + * + * If a single entry's data URI exceeds `bytesLimit`, we skip caching it + * (returning the URI directly to the caller). Without this guard, the + * post-insert eviction loop would drop the entry we just inserted and the + * cache would degrade into a CPU hot path — every subsequent `get()` would + * re-read from disk and re-base64 the same frame. The lost cache hit costs + * one re-read per access; pretending to cache and immediately evicting + * costs one re-read per access *plus* the futile insert/evict bookkeeping. */ function createFrameSourceCache( entryLimit: number, @@ -46,6 +61,8 @@ function createFrameSourceCache( const sizes = new Map(); const inFlight = new Map>(); let totalBytes = 0; + let evictions = 0; + let oversizedRejections = 0; function evictOldest(): void { const oldestKey = cache.keys().next().value; @@ -54,9 +71,24 @@ function createFrameSourceCache( cache.delete(oldestKey); sizes.delete(oldestKey); totalBytes = Math.max(0, totalBytes - size); + evictions++; } function remember(framePath: string, dataUri: string): string { + // Skip caching entries that alone exceed the byte budget. Caching them + // would trigger immediate self-eviction on insert and pollute LRU order + // by displacing the previous entry's slot. + if (dataUri.length > bytesLimit) { + oversizedRejections++; + // Drop any stale prior version so the caller sees consistent state. + if (cache.has(framePath)) { + const prev = sizes.get(framePath) ?? 0; + cache.delete(framePath); + sizes.delete(framePath); + totalBytes = Math.max(0, totalBytes - prev); + } + return dataUri; + } if (cache.has(framePath)) { const prev = sizes.get(framePath) ?? 0; cache.delete(framePath); @@ -104,7 +136,12 @@ function createFrameSourceCache( return { get, - stats: () => ({ entries: cache.size, bytes: totalBytes }), + stats: () => ({ + entries: cache.size, + bytes: totalBytes, + evictions, + oversizedRejections, + }), }; }