Skip to content

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

Merged
vanceingalls merged 5 commits intomainfrom
vance/resource-management
Apr 23, 2026
Merged

fix(producer): tighten resource lifecycle and harden file server#371
vanceingalls merged 5 commits intomainfrom
vance/resource-management

Conversation

@vanceingalls
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls commented Apr 21, 2026

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

  • bun run --filter @hyperframes/producer typecheck passes.
  • 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).
  • Manual: kill a render mid-flight with a non-abort error; no orphaned ffmpeg processes (5A).
  • Manual: two render jobs back-to-back; cache cleared between jobs (5B).
  • Manual: abort during a transition frame; stops promptly, not after scene B (5C).
  • Manual: GET /../../../etc/passwd against the local file server returns 403/404 (5E).

Stack

Chunk 5 of plans/hdr-followups.md.

Copy link
Copy Markdown
Collaborator Author

vanceingalls commented Apr 21, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Apr 21, 2026
@vanceingalls vanceingalls force-pushed the vance/resource-management branch 3 times, most recently from cdcd0fd to f198969 Compare April 21, 2026 20:54
@vanceingalls vanceingalls marked this pull request as ready for review April 21, 2026 20:57
@vanceingalls vanceingalls force-pushed the vance/chunk-encoder-hdr branch from 2e06ff0 to db58e11 Compare April 22, 2026 01:16
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from f198969 to 19698fd Compare April 22, 2026 01:16
@vanceingalls vanceingalls force-pushed the vance/chunk-encoder-hdr branch from db58e11 to efd8422 Compare April 22, 2026 02:03
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from 19698fd to c4b1a1c Compare April 22, 2026 02:03
*
* Exported for unit tests; not part of the public package surface.
*/
export function isPathInside(child: string, parent: string): boolean {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

resolve() only normalizes the lexical path; it does not resolve symlinks. Because the later statSync(candidate).isFile() follows symlinks, a symlink inside projectDir or compiledDir that points outside the allowed root will still pass this check and get served. I reproduced this locally with a symlink-to-outside file, so this guard still needs a realpath-based containment check before we can treat the traversal issue as fixed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed. isPathInside now takes a resolveSymlinks option that uses realpathSync.native() when both sides exist, and the two app.get call sites in fileServer.ts pass resolveSymlinks: true. Test coverage in fileServer.test.ts exercises the symlink-escape case (expect(isPathInside(symlinkPath, rootDir, { resolveSymlinks: true })).toBe(false)) and Windows path semantics via the pathModule: path.win32 injection so the cross-platform pin runs on Linux/macOS CI as well. Thanks for catching this — the original lexical-only check was insufficient.

@vanceingalls vanceingalls force-pushed the vance/resource-management branch from fd205f3 to 658ed49 Compare April 23, 2026 01:58
@vanceingalls vanceingalls force-pushed the vance/chunk-encoder-hdr branch from a31369c to 19e5afc Compare April 23, 2026 02:52
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from 658ed49 to 4fd6596 Compare April 23, 2026 02:52
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls force-pushed the vance/chunk-encoder-hdr branch 2 times, most recently from 2f1942d to 7b268a9 Compare April 23, 2026 03:15
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from 4fd6596 to 0947ad8 Compare April 23, 2026 03:15
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls changed the base branch from vance/chunk-encoder-hdr to graphite-base/371 April 23, 2026 03:36
… transfer

Chunk 3 of HDR follow-ups. Three independent fixes that share a common
thread: HDR config flowing correctly from the EngineConfig down through
the encoders.

3A. chunkEncoder respects options.hdr (BT.2020 + mastering metadata)
  Previously buildEncoderArgs hard-coded BT.709 color tags and the
  bt709 VUI block in -x265-params, even when callers passed an HDR
  EncoderOptions. Today this is harmless because renderOrchestrator
  routes native-HDR content to streamingEncoder and only feeds
  chunkEncoder sRGB Chrome screenshots — but the contract was a lie.

  Now: when options.hdr is set, the libx265 software path emits
  bt2020nc + the matching transfer (smpte2084 for PQ,
  arib-std-b67 for HLG) at the codec level *and* embeds
  master-display + max-cll SEI in -x265-params via
  getHdrEncoderColorParams. libx264 still tags BT.709 inside
  -x264-params (libx264 has no HDR support) but the codec-level color
  flags flip so the container describes pixels truthfully. GPU
  H.265 (nvenc/videotoolbox/qsv/vaapi) gets the BT.2020 tags but no
  -x265-params block, so static mastering metadata is omitted —
  acceptable for previews, not HDR-aware delivery.

3B. convertSdrToHdr accepts a target transfer
  videoFrameExtractor.convertSdrToHdr was hard-coded to
  transfer=arib-std-b67 (HLG) regardless of the surrounding
  composition's dominant transfer. extractAllVideoFrames now calls
  analyzeCompositionHdr first, then passes the dominant transfer
  ("pq" or "hlg") into convertSdrToHdr so an SDR clip mixed into a PQ
  timeline gets converted with smpte2084, not arib-std-b67.

3C. EngineConfig.hdr type matches its declared shape
  The IIFE for the hdr field returned undefined when
  PRODUCER_HDR_TRANSFER wasn't "hlg" or "pq", but the field is typed
  as { transfer: HdrTransfer } | false. Returning false matches the
  type and avoids a downstream undefined check.

Tests
  - chunkEncoder.test.ts: replaced the previous "HDR options ignored"
    assertions with 8 new specs covering BT.2020 + transfer tagging,
    master-display/max-cll embedding, libx264 fallback behavior, GPU
    H.265 + HDR (tags but no x265-params), and range conversion for
    both SDR and HDR CPU paths.
  - All 313 engine unit tests pass (5 new HDR specs).

Follow-ups (separate PRs):
  - Producer regression suite runs in CI; not exercising HDR-tagged
    chunkEncoder yet because no live caller sets options.hdr there.
…d-transfer caller error

PR #370 review feedback (jrusso1020):

- chunkEncoder: when codec=h264 and hdr is set, log a warning and strip
  hdr instead of emitting a half-HDR file (BT.2020 container tags +
  BT.709 VUI inside the bitstream). libx264 has no HDR support; the only
  honest output is SDR/BT.709. Caller is told to use codec=h265.

- videoFrameExtractor: comment at the convertSdrToHdr call site clarifying
  that dominantTransfer is majority-wins; mixing PQ and HLG sources in a
  single composition is caller-error and the minority transfer's videos
  will be converted with the wrong curve. Render two compositions if you
  need both transfers.

- docs/guides/hdr.mdx: limitations section now documents (a) H.264 + HDR
  is rejected at the encoder layer, and (b) GPU H.265 (nvenc, videotoolbox,
  qsv, vaapi) emits BT.2020 + transfer tags but does NOT embed master-display
  or max-cll SEI, since ffmpeg won't pass x265-params through hardware
  encoders. Acceptable for previews, not for HDR10 delivery.
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from 0947ad8 to b7b30c6 Compare April 23, 2026 03:37
@graphite-app graphite-app Bot changed the base branch from graphite-base/371 to main April 23, 2026 03:37
StreamingEncoder.close() and closeCaptureSession() are both invoked from
renderOrchestrator's HDR cleanup path with a tracked-flag pattern, but the
outer finally block may still re-call them if the inner cleanup raised
before the flag flipped. Both must therefore be idempotent.

streamingEncoder.close: add an inline comment explaining why each step
(clearTimeout, removeEventListener, stdin.end gated on !destroyed,
shared exitPromise) is safe to repeat.

closeCaptureSession: previously NOT truly idempotent under browser-pool
semantics — releaseBrowser decrements pooledBrowserRefCount, so calling
it twice for the same acquire could close a browser another session still
holds. Add per-session pageReleased/browserReleased flags to the
CaptureSession interface and gate page.close()/releaseBrowser() behind
them. Set the flag AFTER the await so a mid-cleanup throw still allows
the outer defensive call to retry the unreleased resource.
@vanceingalls vanceingalls force-pushed the vance/resource-management branch from b7b30c6 to 504e005 Compare April 23, 2026 03:37
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
@vanceingalls vanceingalls merged commit 6fd9910 into main Apr 23, 2026
49 of 53 checks passed
Copy link
Copy Markdown
Collaborator Author

Merge activity

vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
vanceingalls added a commit that referenced this pull request Apr 23, 2026
Address jrusso1020's review on PR #371: add a path.win32 test suite that
exercises isPathInside on Linux/macOS CI to catch accidental Unix-only
assumptions (e.g. only splitting on "/") that would silently regress for
Windows users.

- isPathInside now accepts an optional `pathModule` (defaults to node:path)
  so tests can inject path.win32 / path.posix without changing call sites.
- New describe block covers equality, direct/deep children, sibling-prefix
  rejection, traversal escapes, trailing-backslash normalization, and
  cross-drive rejection.
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.

3 participants