fix(producer): tighten resource lifecycle and harden file server (#371)

## Summary

Five resource-management fixes in `renderOrchestrator.ts` and `fileServer.ts`: HDR encoder cleanup on non-abort errors, `frameDirMaxIndexCache` eviction, mid-transition abort responsiveness, pre-allocated transition buffers, and a path-traversal guard for the local file server.

## Why

`Chunk 5` of `plans/hdr-followups.md`. These are independent leaks/hangs/security issues that had each been called out in prior PR reviews and never landed.

## What changed

**5A — HDR encoder + `domSession` cleanup.** The HDR streaming encoder and `domSession` were spawned outside any outer `try/finally`, so a non-abort error between encoder spawn and the inner cleanup leaked the FFmpeg process and held the browser page open. Wrapped the entire HDR (and SDR streaming) capture path in a `try/finally` with explicit `*Closed` flags, and defensively close both in the outer `finally` if they haven't been closed already. `StreamingEncoder.close()` and `closeCaptureSession()` are both idempotent, so double-close is safe.

**5B — `frameDirMaxIndexCache` + `hdrFrameDirs` eviction.** `frameDirMaxIndexCache` is module-scoped and grew monotonically: every render added entries that were never removed. Lifted `hdrFrameDirs` to the outer scope, drop the matching cache entry in the per-video `rmSync` block, and sweep any survivors in the outer `finally`. The on-disk frames themselves were already torn down with `workDir`; this just stops the in-process Map from leaking entries across renders.

**5C — Abort signal between scene A and scene B.** During a shader transition the orchestrator captures scene A and scene B back-to-back inside a single outer frame iteration. An abort that arrived while scene A was capturing wouldn't be noticed until the next outer frame — after scene B had already been fully composited and discarded. Added `assertNotAborted()` at the top of the inner `[transBufferA, transBufferB]` loop so abort is observed before the second scene's DOM seek + screenshot.

**5D — Pre-allocated transition buffers (already addressed).** The transition buffers (`transBufferA`, `transBufferB`, `transOutput`, `normalCanvas`) are pre-allocated outside the per-frame loop. The remaining `Buffer.from` copies sit in HDR transfer conversion (Chunk 8B territory) and image preload, neither of which is the per-frame hot path.

**5E — `fileServer` path-traversal guard.** `fileServer.ts` joined `compiledDir` / `projectDir` with the request path and only checked `existsSync` + `isFile`. `path.join` normalizes `..` segments, so `GET /../etc/passwd` would resolve to `/etc/passwd` and be served straight off disk if the file existed. Added an `isPathInside(child, parent)` helper that resolves both sides and compares prefixes with the platform separator appended (so `/foo` doesn't match `/foobar`), and rejects any candidate that lands outside its intended root.

## Test plan

- [x] `bun run --filter @hyperframes/producer typecheck` passes.
- [x] `fileServer.test.ts` 13/13 pass (4 existing + 9 new `isPathInside` cases covering same-path, nested, prefix-only siblings, escaping traversal, traversal that resolves back inside, trailing-slash handling, and relative-path resolution).
- [x] Manual: kill a render mid-flight with a non-abort error; no orphaned `ffmpeg` processes (5A).
- [x] Manual: two render jobs back-to-back; cache cleared between jobs (5B).
- [x] Manual: abort during a transition frame; stops promptly, not after scene B (5C).
- [x] Manual: `GET /../../../etc/passwd` against the local file server returns 403/404 (5E).

## Stack

Chunk 5 of `plans/hdr-followups.md`.
This commit is contained in:
Vance Ingalls
2026-04-22 21:45:44 -07:00
committed by GitHub
parent aea85af044
commit 6fd99109c9
5 changed files with 1185 additions and 870 deletions
+28 -2
View File
@@ -42,6 +42,11 @@ export interface CaptureSession {
outputDir: string;
onBeforeCapture: BeforeCaptureHook | null;
isInitialized: boolean;
// Tracks whether the page/browser handles have already been released by
// closeCaptureSession. Used to make closeCaptureSession idempotent under
// browser-pool semantics (see the function body for the full invariant).
pageReleased?: boolean;
browserReleased?: boolean;
browserConsoleBuffer: string[];
capturePerf: {
frames: number;
@@ -532,8 +537,29 @@ export async function captureFrameToBuffer(
}
export async function closeCaptureSession(session: CaptureSession): Promise<void> {
if (session.page) await session.page.close().catch(() => {});
if (session.browser) await releaseBrowser(session.browser, session.config);
// INVARIANT: closeCaptureSession is idempotent. The renderOrchestrator HDR
// cleanup path tracks a `domSessionClosed` flag and may still re-call this
// in the outer finally if the inner cleanup raised before the flag flipped.
//
// Naive idempotency would be unsafe under pool semantics: releaseBrowser
// decrements pooledBrowserRefCount, so calling it twice for the same
// acquire could close a browser that another session still holds. We make
// it safe by gating each release behind a per-session "released" flag —
// the second call sees the flag already set and skips the release.
//
// We set the flag AFTER (not before) the await so that if a release throws
// midway, the unreleased resource is retried by the outer defensive call.
// Example: page release succeeds, browser release throws → pageReleased=true
// but browserReleased=false → second call no-ops on page and retries browser.
// This matches the orchestrator's intent for HDR cleanup.
if (!session.pageReleased && session.page) {
await session.page.close().catch(() => {});
session.pageReleased = true;
}
if (!session.browserReleased && session.browser) {
await releaseBrowser(session.browser, session.config);
session.browserReleased = true;
}
session.isInitialized = false;
}
@@ -397,10 +397,23 @@ export async function spawnStreamingEncoder(
},
close: async (): Promise<StreamingEncoderResult> => {
// INVARIANT: close() is idempotent. The renderOrchestrator HDR cleanup
// path tracks an `encoderClosed` flag and may still re-call close() in
// the outer finally if the inner cleanup raised before the flag flipped.
// Each step here must be safe to repeat:
// - clearTimeout: safe to call on an already-cleared/fired timer
// - removeEventListener: no-op if the listener was already removed
// (and {once: true} would have removed it on the first abort anyway)
// - stdin.end gated on !destroyed: skipped on the second call
// - exitPromise: a single shared Promise; awaiting an already-resolved
// Promise resolves immediately with the same captured exitCode
// The returned StreamingEncoderResult is therefore consistent across
// repeated calls. If you change this method, preserve idempotency or
// a regression here will silently double-close ffmpeg and produce
// harder-to-trace errors at the orchestrator layer.
clearTimeout(timer);
if (signal) signal.removeEventListener("abort", onAbort);
// Close stdin to signal end of input
const stdin = ffmpeg.stdin;
if (stdin && !stdin.destroyed) {
await new Promise<void>((resolve) => {
@@ -408,7 +421,6 @@ export async function spawnStreamingEncoder(
});
}
// Wait for FFmpeg to finish
await exitPromise;
const durationMs = Date.now() - startTime;