diff --git a/packages/engine/src/services/extractionCache.test.ts b/packages/engine/src/services/extractionCache.test.ts index 860f3df2e9..997e5852f8 100644 --- a/packages/engine/src/services/extractionCache.test.ts +++ b/packages/engine/src/services/extractionCache.test.ts @@ -26,6 +26,7 @@ import { publishCacheEntry, readKeyStat, rehydrateCacheEntry, + touchCacheDir, type CacheKeyInput, } from "./extractionCache.js"; @@ -441,6 +442,23 @@ describe("gcExtractionCache", () => { expect(existsSync(freshPartial)).toBe(true); }); + // A render still writing into its own (unpublished) partial dir depends on + // it exactly like a symlinked complete entry does. The aged-partial check + // reads the DIRECTORY's own mtime, not any sentinel — touchCacheDir must + // renew that too, or a long-lived partial dir is just as vulnerable to a + // concurrent GC sweep as an untouched complete entry. + it("touchCacheDir renews a partial directory's own mtime so a live writer survives the aged-partial sweep", () => { + const dependedOnPartial = join(tmpRoot, `${SCHEMA_PREFIX}ghi.partial-1234-cafef00d`); + mkdirSync(dependedOnPartial, { recursive: true }); + const old = new Date(Date.now() - 120_000); + utimesSync(dependedOnPartial, old, old); + + touchCacheDir(dependedOnPartial); + gcExtractionCache(tmpRoot, { maxBytes: 1_000_000, minAgeMs: 60_000 }); + + expect(existsSync(dependedOnPartial)).toBe(true); + }); + it("ignores non-cache-prefix directories under the same root", () => { const animatedGif = join(tmpRoot, "animated-gif"); mkdirSync(animatedGif, { recursive: true }); @@ -458,4 +476,17 @@ describe("gcExtractionCache", () => { gcExtractionCache(join(tmpRoot, "missing"), { maxBytes: 1, minAgeMs: 60_000 }), ).not.toThrow(); }); + + // A render can keep reading an entry long after the one-time touch its + // cache-hit lookup performed. touchCacheDir is how a live reader proves the + // entry is still in use — without it, an in-use entry idle past minAge is + // indistinguishable from an abandoned one and gets swept like `oldest` above. + it("touchCacheDir renews an entry's LRU clock so a live dependent survives the sweep", () => { + const dependedOnDir = makeEntry("depended-on", 60, 120_000); + + touchCacheDir(dependedOnDir); + gcExtractionCache(tmpRoot, { maxBytes: 1, minAgeMs: 60_000 }); + + expect(existsSync(dependedOnDir)).toBe(true); + }); }); diff --git a/packages/engine/src/services/extractionCache.ts b/packages/engine/src/services/extractionCache.ts index fac2406153..46a3528e8c 100644 --- a/packages/engine/src/services/extractionCache.ts +++ b/packages/engine/src/services/extractionCache.ts @@ -281,16 +281,35 @@ export function publishCacheEntry(entry: CacheEntry, partialDir: string): CacheP } /** - * Update the LRU clock for a complete cache entry. Misses and filesystem - * races are harmless: the caller can still use the entry it already found. + * Update the LRU clock for the cache entry directory at `dir`. Misses and + * filesystem races are harmless: the caller can still use the entry it already + * found. Takes a directory rather than a `CacheEntry` so a reader holding only + * a frame path can renew the clock with `dirname(framePath)`. + * + * Touches both signals `gcExtractionCache` reads, since which one is + * authoritative depends on the entry's state: `collectGcEntry` ages out a + * `.partial-*` writer dir by the DIRECTORY's own mtime before the sentinel is + * even considered, while a published (complete) entry is read by its + * `COMPLETE_SENTINEL` mtime. Touching only the sentinel would silently fail + * to renew a still-open partial dir a render depends on. */ -export function touchCacheEntry(entry: CacheEntry): void { +export function touchCacheDir(dir: string): void { + const now = new Date(); try { - const now = new Date(); - utimesSync(join(entry.dir, COMPLETE_SENTINEL), now, now); + utimesSync(dir, now, now); } catch { // Best effort LRU touch. } + try { + utimesSync(join(dir, COMPLETE_SENTINEL), now, now); + } catch { + // Best effort LRU touch. + } +} + +/** Update the LRU clock for a complete cache entry. See `touchCacheDir`. */ +export function touchCacheEntry(entry: CacheEntry): void { + touchCacheDir(entry.dir); } /** diff --git a/packages/engine/src/services/videoFrameInjector.test.ts b/packages/engine/src/services/videoFrameInjector.test.ts index eb2098ac4b..f49caf4e7b 100644 --- a/packages/engine/src/services/videoFrameInjector.test.ts +++ b/packages/engine/src/services/videoFrameInjector.test.ts @@ -1,9 +1,10 @@ // @vitest-environment node import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, rmSync, statSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { type Page } from "puppeteer-core"; +import { COMPLETE_SENTINEL } from "./extractionCache.js"; // Hoist mocks before importing the module under test so the mock factory wins. // The cache-hygiene block exercises createVideoFrameInjector against stubbed @@ -25,12 +26,18 @@ vi.mock("./screenshotService.js", () => ({ import { __testing, createVideoFrameInjector } from "./videoFrameInjector.js"; import { type FrameLookupTable } from "./videoFrameExtractor.js"; +import { type BeforeCaptureHook } from "./frameCapture.js"; import { DEFAULT_CONFIG } from "../config.js"; -const { createFrameSourceCache } = __testing; +const { createFrameSourceCache, CACHE_TOUCH_THROTTLE_MS } = __testing; const SHARED_STATS = { evictions: 0, oversizedRejections: 0 }; +// Bypass the on-disk frame cache by handing back a synthetic data URI. +function inlineResolver(framePath: string): string { + return `data:image/png;base64,fake-${framePath}`; +} + describe("frame source cache eviction", () => { let dir: string; @@ -178,11 +185,6 @@ describe("createVideoFrameInjector cache hygiene against page-side skips", () => } as unknown as FrameLookupTable; } - // Bypass the on-disk frame cache by handing back a synthetic data URI. - function inlineResolver(framePath: string): string { - return `data:image/png;base64,fake-${framePath}`; - } - function makeGpuInjector() { const evaluate = vi.fn(async () => undefined); const page = { evaluate } as unknown as Page; @@ -318,3 +320,85 @@ describe("createVideoFrameInjector cache hygiene against page-side skips", () => expect(evaluate.mock.calls.some((call) => call[1] === 1.5)).toBe(false); }); }); + +describe("createVideoFrameInjector extraction-cache lease renewal", () => { + // Regression: a render can hold a compiled-dir symlink into a shared + // extraction-cache entry far longer than the entry's one-time cache-hit + // touch keeps it alive, so another render's GC sweep can evict a directory + // this render still reads from. Every active frame must renew the entry's + // LRU clock, throttled. + // + // Fake timers drive both halves of each assertion: they advance `Date.now()` + // for the injector's throttle AND the `new Date()` touchCacheDir stamps onto + // the sentinel, so renewal is observable without depending on real + // wall-clock resolution between two same-millisecond touches. + const fakePage = { evaluate: async () => undefined } as unknown as Page; + let cacheDir: string; + let sentinelPath: string; + + function makeHook(framePath: string): BeforeCaptureHook { + const table = { + getActiveFramePayloads: () => new Map([["v", { framePath, frameIndex: 0 }]]), + } as unknown as FrameLookupTable; + const hook = createVideoFrameInjector(table, { frameSrcResolver: inlineResolver }); + if (!hook) throw new Error("expected an injector hook for a non-null lookup table"); + return hook; + } + + function sentinelMtimeMs(): number { + return statSync(sentinelPath).mtimeMs; + } + + beforeEach(() => { + injectVideoFramesBatchMock.mockReset(); + syncVideoFrameVisibilityMock.mockReset(); + syncVideoFrameVisibilityMock.mockResolvedValue(undefined); + injectVideoFramesBatchMock.mockResolvedValue(["v"]); + + cacheDir = mkdtempSync(join(tmpdir(), "hf-cache-entry-")); + sentinelPath = join(cacheDir, COMPLETE_SENTINEL); + writeFileSync(sentinelPath, "", "utf-8"); + const hourAgo = new Date(Date.now() - 60 * 60 * 1000); + utimesSync(sentinelPath, hourAgo, hourAgo); + + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + rmSync(cacheDir, { recursive: true, force: true }); + }); + + it("renews the cache entry's sentinel mtime on the first active frame", async () => { + const before = sentinelMtimeMs(); + const hook = makeHook(join(cacheDir, "frame_00001.jpg")); + + await hook(fakePage, 0); + + expect(sentinelMtimeMs()).toBeGreaterThan(before); + }); + + it("throttles renewal so repeated frames don't hammer the filesystem clock", async () => { + const hook = makeHook(join(cacheDir, "frame_00001.jpg")); + + await hook(fakePage, 0); + const firstTouch = sentinelMtimeMs(); + + vi.advanceTimersByTime(CACHE_TOUCH_THROTTLE_MS / 2); + await hook(fakePage, 1); + expect(sentinelMtimeMs()).toBe(firstTouch); + + vi.advanceTimersByTime(CACHE_TOUCH_THROTTLE_MS); + await hook(fakePage, 2); + expect(sentinelMtimeMs()).toBeGreaterThan(firstTouch); + }); + + it("does not throw when the frame path is outside any cache directory", async () => { + // The resolver bypasses the frame read, but renewal still runs against + // dirname(framePath) — a non-cache dir has no sentinel to touch, and + // touchCacheDir's best-effort contract must swallow that. + const hook = makeHook("/no/such/cache/dir/frame_00001.jpg"); + + await expect(hook(fakePage, 0)).resolves.toBeUndefined(); + }); +}); diff --git a/packages/engine/src/services/videoFrameInjector.ts b/packages/engine/src/services/videoFrameInjector.ts index 8677ffc0d7..f04bb1e5bc 100644 --- a/packages/engine/src/services/videoFrameInjector.ts +++ b/packages/engine/src/services/videoFrameInjector.ts @@ -9,7 +9,9 @@ import { type Page } from "puppeteer-core"; import { promises as fs } from "fs"; +import { dirname } from "node:path"; import { type FrameLookupTable } from "./videoFrameExtractor.js"; +import { touchCacheDir } from "./extractionCache.js"; import { injectVideoFramesBatch, syncVideoFrameVisibility } from "./screenshotService.js"; import { type BeforeCaptureHook } from "./frameCapture.js"; import { DEFAULT_CONFIG, type EngineConfig } from "../config.js"; @@ -151,7 +153,19 @@ function createFrameSourceCache( }; } -export const __testing = { createFrameSourceCache }; +/** + * How often a running render re-touches a shared extraction-cache entry it is + * still reading from. The entry's LRU clock is set once, when the extractor's + * lookup hits it, and never again — so a render holding a compiled-dir symlink + * into that entry for hours looks abandoned to a concurrent GC sweep, which + * can evict the directory out from under it. Re-touching on read turns each + * captured frame into a lease renewal. Far under the 1-hour GC floor + * (`EXTRACT_CACHE_MIN_AGE_MS` in videoFrameExtractor.ts) while still keeping + * the `utimesSync` rare. + */ +const CACHE_TOUCH_THROTTLE_MS = 5 * 60 * 1000; + +export const __testing = { createFrameSourceCache, CACHE_TOUCH_THROTTLE_MS }; /** * Creates a BeforeCaptureHook that injects pre-extracted video frames @@ -174,6 +188,22 @@ export function createVideoFrameInjector( const bytesLimit = bytesLimitMb * 1024 * 1024; const frameCache = createFrameSourceCache(entryLimit, bytesLimit, config?.frameSrcResolver); const lastInjectedFrameByVideo = new Map(); + const lastCacheTouchByDir = new Map(); + + /** + * Renew this render's lease on the extraction-cache entry `framePath` lives + * in. Called for every active video on every frame — including one whose + * frame index hasn't moved, since a long-held-static frame needs its entry + * kept alive just as much as a changing one — so it throttles per directory. + */ + function renewCacheLease(framePath: string): void { + const cacheDir = dirname(framePath); + const now = Date.now(); + const lastTouch = lastCacheTouchByDir.get(cacheDir); + if (lastTouch !== undefined && now - lastTouch < CACHE_TOUCH_THROTTLE_MS) return; + touchCacheDir(cacheDir); + lastCacheTouchByDir.set(cacheDir, now); + } // fallow-ignore-next-line complexity return async (page: Page, time: number) => { @@ -186,6 +216,7 @@ export function createVideoFrameInjector( []; for (const [videoId, payload] of activePayloads) { activeIds.add(videoId); + renewCacheLease(payload.framePath); const lastFrameIndex = lastInjectedFrameByVideo.get(videoId); if (lastFrameIndex === payload.frameIndex) continue; pendingReads.push(