Skip to content

fix(engine): keep a render's dependent extraction-cache entry alive during long captures - #3913

Open
miga-heygen wants to merge 1 commit into
mainfrom
fix/extraction-cache-live-touch
Open

fix(engine): keep a render's dependent extraction-cache entry alive during long captures#3913
miga-heygen wants to merge 1 commit into
mainfrom
fix/extraction-cache-live-touch

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

hyperframes render's shared, machine-wide video-frame extraction cache (one directory per source-video/extraction-params key, content-hash keyed) can be garbage-collected out from under a render that still depends on it. A render creates a symlink from its own compiled dir into a cache entry once, up front, then reads frames from that symlink for the rest of its (potentially long) capture pass. Separately, the cache's GC sweep evicts entries whose LRU-clock sentinel hasn't been touched in over an hour, once the cache exceeds its size budget — but that clock was set exactly ONCE, at cache-hit lookup time, and never again. A render depending on an entry for more than an hour, on a machine where some OTHER render's GC sweep fires during that window, can lose the entry mid-render: its symlinked frame reads then throw a plain ENOENT with no retry.

Fix: every actively-read video frame during capture now renews its cache entry's LRU clock (throttled to once per 5 minutes per cache directory — comfortably under the 1-hour eviction floor), via a new touchCacheDir(dir) extracted from the existing touchCacheEntry. touchCacheDir renews both signals the GC sweep actually reads: a published (complete) entry's completion-sentinel mtime, and a still-open (unpublished) partial writer directory's own mtime — the GC's aged-partial-dir check reads the latter directly and doesn't consult any sentinel at all, so touching only the sentinel would have left that path just as vulnerable.

Test plan

  • New tests in extractionCache.test.ts: an entry backdated past the GC's min-age floor survives a sweep after touchCacheDir, for both a published (sentinel-bearing) entry and a still-open partial writer directory (the latter closing a gap an adversarial review caught in an earlier draft of this fix — see below).
  • New tests in videoFrameInjector.test.ts: the capture hook renews a cache entry's sentinel mtime on the first active frame, throttles repeated renewals within the window, and doesn't throw when the frame path lives outside any real cache directory (a harmless no-op, matching HYPERFRAMES_EXTRACT_CACHE_DIR=off).
  • Verified via true revert-and-restore for both the initial fix and the partial-dir follow-up: reverting each change independently made its own new test(s) fail with the expected assertion, restoring made them pass again.
  • Full packages/engine test suite before/after: 5 pre-existing failures in ffprobe.test.ts (missing ffprobe binary / HDR PNG fixture issues in this sandbox — confirmed identical with this diff fully reverted), otherwise all passing including the 4 new tests. bunx tsc --noEmit clean on packages/engine and packages/producer (consumer package). oxlint/oxfmt --check clean on all touched files.
  • Adversarial review of the first draft caught that touching only the completion sentinel left a still-open partial writer directory unprotected against the GC's separate aged-partial-dir check (which reads the directory's own mtime directly, never a sentinel) — fixed and covered by a dedicated test before this PR was opened.

Deliberately out of scope: this closes the fix via lease-renewal-on-read (one of the mechanisms identified during investigation); it does not add reference-counting or per-render cache scoping, which would be a larger structural change for the same root cause.

…uring long captures

The shared, size-capped video-frame extraction cache evicts entries whose
LRU-clock sentinel hasn't been touched in over an hour. That clock was set
once, at cache-hit lookup time, and never again — so a render holding a
compiled-dir symlink (or a still-open partial writer dir) into an entry for
longer than an hour could lose it to a concurrent render's GC sweep on the
same shared machine, leaving its frame reads a dangling ENOENT with no
retry.

Every active video's captured frame now renews its cache entry's clock,
throttled to once per 5 minutes per directory. touchCacheDir renews both
signals gcExtractionCache actually reads: the directory's own mtime (which
gates a still-open partial writer dir) and the completion sentinel's mtime
(which gates a published entry).

Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict — APPROVE

Head verified: 3964e36987955d152d03a3f603633396aad92dc5.

The retention actually retains — both GC read paths are covered

touchCacheDir(dir) (new, extracted from touchCacheEntry) writes to BOTH:

  • utimesSync(dir, now, now) — directory's own mtime
  • utimesSync(join(dir, COMPLETE_SENTINEL), now, now) — completion sentinel's mtime

The docstring names the reason precisely: collectGcEntry ages a .partial-* writer dir by the DIRECTORY's own mtime BEFORE the sentinel is even consulted; a published (complete) entry is read by its COMPLETE_SENTINEL mtime. Touching only the sentinel — the naïve fix — would silently fail to renew a still-open partial-writer directory the render happens to still be reading through, which is the adversarial-review case Miga caught and fixed pre-open. Both writes are try/catch-guarded, so touchCacheDir is best-effort and never throws upstream — correct for an LRU touch (a missed touch just falls back to whatever touchCacheEntry already provided).

Backward-compat: touchCacheEntry(entry) now delegates to touchCacheDir(entry.dir) — pure refactor at the old call sites.

No leak — retention IS the lease pattern

The retention lives inside the createVideoFrameInjector closure:

  • lastCacheTouchByDir = new Map<string, number>() — closure-local, one Map per injector instance
  • renewCacheLease(framePath) — closure over the Map + throttle constant
  • Called from within the BeforeCaptureHook for every active video every frame

When the render's capture pass ends, the injector closure goes out of scope, the Map is GC'd, no more touchCacheDir calls fire. The cache entry's LRU clock ages normally from the last touch and becomes evictable on the next GC sweep — no ref counting, no global pinning, no forever-lease. This is exactly the "no leak" discipline Home asked me to check: retention lifetime is the render's lifetime, not the process's.

Per-render scope — one map per injector, not module-global

lastCacheTouchByDir is instantiated in the createVideoFrameInjector factory (line 191 of the new file), which returns a fresh BeforeCaptureHook per invocation. Different renders → different injectors → different Maps. Two concurrent renders touching the same cache entry each carry their own throttle timestamp; either one renewing keeps the entry alive for both. No cross-render coupling.

Throttle margin — 12x under GC floor

  • CACHE_TOUCH_THROTTLE_MS = 5 * 60 * 1000 (5 min)
  • EXTRACT_CACHE_MIN_AGE_MS = 1 hour (per PR body; videoFrameExtractor.ts)
  • Worst-case delay from render's last touch to next: 4:59 elapsed → throttle skips → next renewCacheLease fires 5:00 later → max ~10 min silent window
  • Safe: 12x under the 1-hour floor. Comfortable margin against clock skew, GC-timing jitter, or a render pausing mid-capture.

The comment on CACHE_TOUCH_THROTTLE_MS correctly cross-references EXTRACT_CACHE_MIN_AGE_MS in videoFrameExtractor.ts so a future contributor can see both sides of the invariant without hunting.

Call-site correctness

renewCacheLease(payload.framePath) is called inside the activePayloads loop BEFORE the frame-index-changed guard:

for (const [videoId, payload] of activePayloads) {
  activeIds.add(videoId);
  renewCacheLease(payload.framePath);           // ← always renewed
  const lastFrameIndex = lastInjectedFrameByVideo.get(videoId);
  if (lastFrameIndex === payload.frameIndex) continue;  // may skip injection
  ...
}

The comment states the reason: "even videos holding a static frame need their entry kept alive." Correct — a long-static video is exactly the case where a naïve renew-on-injection would leave the entry vulnerable to GC while the render is still reading the same frame path.

dirname(framePath) — resilient to non-symlink cases

Test coverage explicitly pins the HYPERFRAMES_EXTRACT_CACHE_DIR=off case: "doesn't throw when the frame path lives outside any real cache directory (a harmless no-op)." Since touchCacheDir's two utimesSync calls are try/catch-wrapped, a non-cache directory just gets its mtime touched harmlessly or fails silently — no downstream effect. Right shape.

Test coverage

  • extractionCache.test.ts — entry backdated past the GC's min-age floor survives a sweep after touchCacheDir, for BOTH a published (sentinel-bearing) entry AND a still-open partial writer directory. The partial-dir test is the specific adversarial-catch gap.
  • videoFrameInjector.test.ts — capture hook renews sentinel mtime on first active frame, throttles repeated renewals within the window, no-op on frame paths outside real cache dirs. CACHE_TOUCH_THROTTLE_MS exported via __testing so the throttle boundary can be exercised deterministically.
  • Revert-and-restore per PR body: each of the two fixes (initial + partial-dir follow-up) reverted independently made its test(s) fail with the expected assertion, restored made them pass.

CI

All required checks SUCCESS on this head: Build, Typecheck, Lint, Format, Test, Test: runtime contract, Producer unit/integration, SDK unit+contract+smoke, Studio load smoke, CLI smoke (required), Smoke: global install, Render on windows-latest, Tests on windows-latest (studio-core + studio-engine-cli), all 9 regression-shards, preview-regression, Player perf, CodeQL, semantic PR title, file size check, fallow audit. Windows Preflight (lint+format) also green. Only SKIPPED are unrelated Skills workflows (no skills touched).

Small notes (non-blocking)

  • Refactor discipline: touchCacheDir extracted so a caller with only a frame path can renew without reconstructing a CacheEntry. Old touchCacheEntry(entry) preserved as thin delegator — no behaviour change at existing call sites.
  • utimesSync on symlinks: follows the symlink by default (as opposed to lutimesSync), so a render whose compiled dir symlinks INTO a cache entry gets the target's mtime touched, which is what the GC reads. Correct choice.
  • Scope decision: Miga's PR body calls out that this closes the fix via lease-renewal-on-read, not via reference counting or per-render cache scoping — larger structural change, same root cause. Right scope for a mid-capture-eviction hotfix; the structural rework can follow separately if the class recurs.

Clean, minimal, well-tested. Stamp.

— Review by tai (pr-review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants