fix(engine): skip caching oversized frames + add eviction telemetry

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 });
+  });
 });

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<string>;
-  /** 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<string, number>();
   const inFlight = new Map<string, Promise<string>>();
   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,
+    }),
   };
 }