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

packages/engine/src/services/frameCapture.ts

@@ -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;
 }
 

packages/engine/src/services/streamingEncoder.ts

@@ -397,18 +397,30 @@ 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) => {
           stdin.end(() => resolve());
         });
       }
 
-      // Wait for FFmpeg to finish
       await exitPromise;
 
       const durationMs = Date.now() - startTime;

packages/producer/src/services/fileServer.test.ts

@@ -1,8 +1,12 @@
 import { describe, expect, it } from "bun:test";
+import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
+import path, { join } from "node:path";
+import { tmpdir } from "node:os";
 import {
   HF_BRIDGE_SCRIPT,
   HF_EARLY_STUB,
   injectScriptsAtHeadStart,
+  isPathInside,
   VIRTUAL_TIME_SHIM,
 } from "./fileServer.js";
 
@@ -40,6 +44,113 @@ describe("injectScriptsIntoHtml", () => {
   });
 });
 
+describe("isPathInside", () => {
+  it("returns true when the child equals the parent", () => {
+    expect(isPathInside("/tmp/project", "/tmp/project")).toBe(true);
+  });
+
+  it("returns true for direct children", () => {
+    expect(isPathInside("/tmp/project/index.html", "/tmp/project")).toBe(true);
+  });
+
+  it("returns true for deeply nested descendants", () => {
+    expect(isPathInside("/tmp/project/a/b/c/file.html", "/tmp/project")).toBe(true);
+  });
+
+  it("rejects siblings with a shared name prefix", () => {
+    // The classic prefix-bug: "/foo" should NOT contain "/foobar/x". A naive
+    // startsWith check without a trailing separator would incorrectly accept
+    // this as nested.
+    expect(isPathInside("/tmp/projectile/a", "/tmp/project")).toBe(false);
+    expect(isPathInside("/tmp/project-other/a", "/tmp/project")).toBe(false);
+  });
+
+  it("rejects paths outside the parent entirely", () => {
+    expect(isPathInside("/etc/passwd", "/tmp/project")).toBe(false);
+    expect(isPathInside("/tmp/other/file.html", "/tmp/project")).toBe(false);
+  });
+
+  it("rejects path-traversal attempts that escape the parent", () => {
+    // path.join("/tmp/project", "../etc/passwd") normalizes to "/tmp/etc/passwd"
+    // — outside the project root. The whole point of isPathInside is to catch
+    // exactly this after the join.
+    expect(isPathInside("/tmp/etc/passwd", "/tmp/project")).toBe(false);
+    expect(isPathInside("/tmp/project/../etc/passwd", "/tmp/project")).toBe(false);
+  });
+
+  it("accepts traversal that resolves back inside the parent", () => {
+    expect(isPathInside("/tmp/project/sub/../index.html", "/tmp/project")).toBe(true);
+  });
+
+  it("treats parents with and without trailing slashes the same", () => {
+    expect(isPathInside("/tmp/project/index.html", "/tmp/project/")).toBe(true);
+    expect(isPathInside("/tmp/project/index.html", "/tmp/project")).toBe(true);
+  });
+
+  it("resolves relative paths against the current working directory", () => {
+    // Both sides resolve against cwd, so a relative file under a relative dir
+    // should be considered nested. We don't assert the absolute path; we just
+    // check the containment relationship holds after resolution.
+    expect(isPathInside("a/b/c.html", "a/b")).toBe(true);
+    expect(isPathInside("a/b/../../c.html", "a/b")).toBe(false);
+  });
+
+  it("rejects symlink escapes when realpath enforcement is enabled", () => {
+    const rootDir = mkdtempSync(join(tmpdir(), "hf-file-server-root-"));
+    const outsideDir = mkdtempSync(join(tmpdir(), "hf-file-server-outside-"));
+    const outsideFile = join(outsideDir, "secret.txt");
+    const symlinkPath = join(rootDir, "escaped.txt");
+
+    try {
+      writeFileSync(outsideFile, "secret");
+      symlinkSync(outsideFile, symlinkPath);
+
+      expect(isPathInside(symlinkPath, rootDir)).toBe(true);
+      expect(isPathInside(symlinkPath, rootDir, { resolveSymlinks: true })).toBe(false);
+    } finally {
+      rmSync(rootDir, { recursive: true, force: true });
+      rmSync(outsideDir, { recursive: true, force: true });
+    }
+  });
+
+  describe("with path.win32 (cross-platform pinning tests)", () => {
+    // Pin Windows-path semantics on Linux/macOS CI by injecting the win32
+    // path module. Without this, accidental Unix-only assumptions (e.g. only
+    // splitting on "/") would silently regress for Windows users.
+    const win32 = { pathModule: path.win32 };
+
+    it("returns true when the child equals the parent", () => {
+      expect(isPathInside("C:\\foo", "C:\\foo", win32)).toBe(true);
+    });
+
+    it("returns true for direct children", () => {
+      expect(isPathInside("C:\\foo\\bar", "C:\\foo", win32)).toBe(true);
+    });
+
+    it("returns true for deeply nested descendants", () => {
+      expect(isPathInside("C:\\foo\\a\\b\\c.html", "C:\\foo", win32)).toBe(true);
+    });
+
+    it("rejects siblings with a shared name prefix", () => {
+      expect(isPathInside("C:\\foobar\\x", "C:\\foo", win32)).toBe(false);
+      expect(isPathInside("C:\\foo-other\\x", "C:\\foo", win32)).toBe(false);
+    });
+
+    it("rejects path-traversal attempts that escape the parent", () => {
+      expect(isPathInside("C:\\foo\\..\\etc\\passwd", "C:\\foo", win32)).toBe(false);
+    });
+
+    it("treats parents with and without trailing backslashes the same", () => {
+      expect(isPathInside("C:\\foo\\bar", "C:\\foo\\", win32)).toBe(true);
+      expect(isPathInside("C:\\foo\\bar", "C:\\foo", win32)).toBe(true);
+    });
+
+    it("rejects paths on a different drive letter", () => {
+      expect(isPathInside("D:\\foo\\bar", "C:\\foo", win32)).toBe(false);
+    });
+  });
+});
+
 describe("HF_EARLY_STUB + HF_BRIDGE_SCRIPT integration", () => {
   /**
    * Simulates the real injection order in a Puppeteer page:

packages/producer/src/services/fileServer.ts

@@ -10,10 +10,62 @@
 import { Hono } from "hono";
 import { serve } from "@hono/node-server";
 import type { IncomingMessage } from "node:http";
-import { readFileSync, existsSync, statSync } from "node:fs";
-import { join, extname } from "node:path";
+import { readFileSync, existsSync, realpathSync, statSync } from "node:fs";
+import { join, extname, resolve, sep } from "node:path";
 import { getVerifiedHyperframeRuntimeSource } from "./hyperframeRuntimeLoader.js";
 
+type PathModuleLike = {
+  resolve: (...segments: string[]) => string;
+  sep: string;
+};
+
+type IsPathInsideOptions = {
+  resolveSymlinks?: boolean;
+  /**
+   * Path module used for resolution and separator comparison. Defaults to
+   * `node:path` for the running platform. Tests inject `path.win32` /
+   * `path.posix` to exercise cross-platform behavior on a single OS.
+   */
+  pathModule?: PathModuleLike;
+};
+
+/**
+ * Returns true iff `child` is the same as, or nested inside, `parent` after
+ * symlink-free path normalization. Used to reject path-traversal attempts
+ * (e.g. GET `/../etc/passwd`) before opening any file.
+ *
+ * `path.join(root, "..")` normalizes traversal segments and can escape `root`
+ * entirely, so the join return value alone is not a safe guard. Callers must
+ * resolve both sides and compare prefixes with the platform separator
+ * appended to `parent` to avoid `/foo` matching `/foobar`.
+ *
+ * Exported for unit tests; not part of the public package surface.
+ */
+export function isPathInside(
+  child: string,
+  parent: string,
+  options: IsPathInsideOptions = {},
+): boolean {
+  const { resolveSymlinks = false, pathModule } = options;
+  const resolveFn = pathModule?.resolve ?? resolve;
+  const separator = pathModule?.sep ?? sep;
+  const resolvedChild = resolveFn(child);
+  const resolvedParent = resolveFn(parent);
+  const normalizedChild =
+    resolveSymlinks && existsSync(resolvedChild)
+      ? realpathSync.native(resolvedChild)
+      : resolvedChild;
+  const normalizedParent =
+    resolveSymlinks && existsSync(resolvedParent)
+      ? realpathSync.native(resolvedParent)
+      : resolvedParent;
+  if (normalizedChild === normalizedParent) return true;
+  const parentWithSep = normalizedParent.endsWith(separator)
+    ? normalizedParent
+    : normalizedParent + separator;
+  return normalizedChild.startsWith(parentWithSep);
+}
+
 const MIME_TYPES: Record<string, string> = {
   ".html": "text/html; charset=utf-8",
   ".css": "text/css; charset=utf-8",
@@ -479,13 +531,36 @@ export function createFileServer(options: FileServerOptions): Promise<FileServer
 
     // Remove leading slash
     const relativePath = requestPath.replace(/^\//, "");
-    const compiledPath = compiledDir ? join(compiledDir, relativePath) : null;
-    const hasCompiledFile = Boolean(
-      compiledPath && existsSync(compiledPath) && statSync(compiledPath).isFile(),
-    );
-    const filePath = hasCompiledFile ? (compiledPath as string) : join(projectDir, relativePath);
 
-    if (!existsSync(filePath) || !statSync(filePath).isFile()) {
+    // Resolve against compiledDir first (preferred — overrides project files
+    // for compositions emitted by the build), then projectDir as fallback.
+    // Each candidate is rejected if `..` segments push it outside the
+    // intended root: `path.join` normalizes traversal but does not enforce
+    // containment, so a request like `GET /../etc/passwd` would otherwise
+    // be served straight off the filesystem.
+    let filePath: string | null = null;
+    if (compiledDir) {
+      const candidate = join(compiledDir, relativePath);
+      if (
+        existsSync(candidate) &&
+        isPathInside(candidate, compiledDir, { resolveSymlinks: true }) &&
+        statSync(candidate).isFile()
+      ) {
+        filePath = candidate;
+      }
+    }
+    if (!filePath) {
+      const candidate = join(projectDir, relativePath);
+      if (
+        existsSync(candidate) &&
+        isPathInside(candidate, projectDir, { resolveSymlinks: true }) &&
+        statSync(candidate).isFile()
+      ) {
+        filePath = candidate;
+      }
+    }
+
+    if (!filePath) {
       if (!/favicon\.ico$/i.test(requestPath)) {
         console.warn(`[FileServer] 404 Not Found: ${requestPath}`);
       }