fix: address PR #596 review issues (#597)

## Summary

Fixes three issues identified in the [post-merge review](https://github.com/heygen-com/hyperframes/pull/596#pullrequestreview-4214283515) of PR #596:

- **P1 (cache bypass):** When `extractCacheDir` is set, extracted frames live outside `compiledDir`, so `createCompiledFrameSrcResolver` rejects them and every frame falls back to base64 data URIs. Fix: symlink cached frame directories into `compiledDir/__hyperframes_video_frames/` after extraction and remap `framePaths` so the served-frame fast path works.

- **P2 (pooled browser stale state):** `closeCaptureSession` force-killed the Chrome process on timeout via raw `SIGKILL` without clearing `pooledBrowser` / `pooledBrowserRefCount`, leaving other sessions with a dead browser reference. Fix: add `forceReleaseBrowser()` in `browserManager` that atomically clears pool state before killing the process.

- **P3 (reserved chars in URLs):** `createCompiledFrameSrcResolver` encodes path segments with `encodeURIComponent`, but the file server used `c.req.path` (which only applies `decodeURI`) to look up files on disk. Video IDs containing `#`, `?`, or `%` produced 404s. Fix: apply `decodeURIComponent` per path segment in the file server's catch-all route.

## Test plan

- [x] `createCompiledFrameSrcResolver` tests: symlinked cache paths resolve to served URLs; cache-external paths return null; reserved characters encode correctly
- [x] `forceReleaseBrowser` tests: kills process + disconnects; tolerates already-killed process
- [x] `createFileServer` test: `video%231/frame.jpg` serves file from `video#1/frame.jpg` on disk
- [x] Typecheck: engine + producer pass
- [x] Lint + format: 0 warnings, 0 errors

packages/engine/src/services/browserManager.test.ts

@@ -1,6 +1,6 @@
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
-import { buildChromeArgs } from "./browserManager.js";
+import { buildChromeArgs, forceReleaseBrowser } from "./browserManager.js";
 
 describe("buildChromeArgs browser GPU mode", () => {
   const base = { width: 1920, height: 1080 };
@@ -46,3 +46,33 @@ describe("buildChromeArgs browser GPU mode", () => {
     expect(args).not.toContain("--use-angle=metal");
   });
 });
+
+describe("forceReleaseBrowser", () => {
+  it("kills the browser process and disconnects", () => {
+    const killFn = vi.fn(() => true);
+    const disconnectFn = vi.fn();
+    const mockBrowser = {
+      process: () => ({ kill: killFn, killed: false }),
+      disconnect: disconnectFn,
+    } as any;
+
+    forceReleaseBrowser(mockBrowser);
+
+    expect(killFn).toHaveBeenCalledWith("SIGKILL");
+    expect(disconnectFn).toHaveBeenCalled();
+  });
+
+  it("tolerates an already-killed process", () => {
+    const killFn = vi.fn();
+    const disconnectFn = vi.fn();
+    const mockBrowser = {
+      process: () => ({ kill: killFn, killed: true }),
+      disconnect: disconnectFn,
+    } as any;
+
+    forceReleaseBrowser(mockBrowser);
+
+    expect(killFn).not.toHaveBeenCalled();
+    expect(disconnectFn).toHaveBeenCalled();
+  });
+});

packages/engine/src/services/browserManager.ts

@@ -234,6 +234,30 @@ export async function releaseBrowser(
   await browser.close().catch(() => {});
 }
 
+export function forceReleaseBrowser(browser: Browser): void {
+  if (pooledBrowser && pooledBrowser === browser) {
+    pooledBrowserRefCount = 0;
+    pooledBrowser = null;
+  }
+  const proc = (
+    browser as unknown as {
+      process?: () => { kill: (signal?: NodeJS.Signals) => boolean; killed?: boolean } | null;
+    }
+  ).process?.();
+  if (proc && !proc.killed) {
+    try {
+      proc.kill("SIGKILL");
+    } catch {
+      // Best-effort cleanup.
+    }
+  }
+  try {
+    browser.disconnect();
+  } catch {
+    // Best-effort cleanup.
+  }
+}
+
 export interface BuildChromeArgsOptions {
   width: number;
   height: number;

packages/engine/src/services/frameCapture.ts

@@ -16,6 +16,7 @@ import { quantizeTimeToFrame } from "@hyperframes/core";
 import {
   acquireBrowser,
   releaseBrowser,
+  forceReleaseBrowser,
   buildChromeArgs,
   resolveHeadlessShellPath,
   type CaptureMode,
@@ -95,27 +96,6 @@ async function waitForCloseWithTimeout(promise: Promise<unknown>): Promise<boole
   return !timedOut;
 }
 
-function forceKillBrowserProcess(browser: Browser): void {
-  const browserProcess = (
-    browser as unknown as {
-      process?: () => { kill: (signal?: NodeJS.Signals) => boolean; killed?: boolean } | null;
-    }
-  ).process?.();
-
-  if (browserProcess && !browserProcess.killed) {
-    try {
-      browserProcess.kill("SIGKILL");
-    } catch {
-      // Best-effort cleanup after Puppeteer close has already timed out.
-    }
-  }
-  try {
-    browser.disconnect();
-  } catch {
-    // Best-effort cleanup after Puppeteer close has already timed out.
-  }
-}
-
 export async function createCaptureSession(
   serverUrl: string,
   outputDir: string,
@@ -718,7 +698,8 @@ export async function closeCaptureSession(session: CaptureSession): Promise<void
     const pageClosed = await waitForCloseWithTimeout(session.page.close());
     if (!pageClosed) {
       console.warn("[FrameCapture] Timed out closing page; forcing browser process shutdown");
-      forceKillBrowserProcess(session.browser);
+      forceReleaseBrowser(session.browser);
+      session.browserReleased = true;
     }
     session.pageReleased = true;
   }
@@ -728,7 +709,7 @@ export async function closeCaptureSession(session: CaptureSession): Promise<void
     );
     if (!browserClosed) {
       console.warn("[FrameCapture] Timed out closing browser; forcing browser process shutdown");
-      forceKillBrowserProcess(session.browser);
+      forceReleaseBrowser(session.browser);
     }
     session.browserReleased = true;
   }

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

@@ -189,6 +189,34 @@ describe("createFileServer", () => {
       rmSync(workspaceDir, { recursive: true, force: true });
     }
   });
+
+  it("decodes percent-encoded reserved characters in URL path segments", async () => {
+    const projectDir = mkdtempSync(join(tmpdir(), "hf-file-server-reserved-chars-"));
+
+    try {
+      const subDir = join(projectDir, "video#1");
+      mkdirSync(subDir, { recursive: true });
+      writeFileSync(join(projectDir, "index.html"), "<!doctype html><html></html>");
+      writeFileSync(join(subDir, "frame.jpg"), "fake-jpg");
+
+      const server = await createFileServer({
+        projectDir,
+        preHeadScripts: [],
+        headScripts: [],
+        bodyScripts: [],
+      });
+
+      try {
+        const res = await fetch(`${server.url}/video%231/frame.jpg`);
+        expect(res.status).toBe(200);
+        expect(await res.text()).toBe("fake-jpg");
+      } finally {
+        server.close();
+      }
+    } finally {
+      rmSync(projectDir, { recursive: true, force: true });
+    }
+  });
 });
 
 describe("HF_EARLY_STUB + HF_BRIDGE_SCRIPT integration", () => {

packages/producer/src/services/fileServer.ts

@@ -450,8 +450,17 @@ export function createFileServer(options: FileServerOptions): Promise<FileServer
     let requestPath = c.req.path;
     if (requestPath === "/") requestPath = "/index.html";
 
-    // Remove leading slash
+    const relativePath = requestPath
-    const relativePath = requestPath.replace(/^\//, "");
+      .replace(/^\//, "")
+      .split("/")
+      .map((seg) => {
+        try {
+          return decodeURIComponent(seg);
+        } catch {
+          return seg;
+        }
+      })
+      .join("/");
 
     // Resolve against compiledDir first (preferred — overrides project files
     // for compositions emitted by the build), then projectDir as fallback.

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

@@ -1,8 +1,8 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
-import { join } from "node:path";
+import { join, win32 } from "node:path";
 import { tmpdir } from "node:os";
-import type { EngineConfig } from "@hyperframes/engine";
+import type { EngineConfig, ExtractedFrames } from "@hyperframes/engine";
 import type { CompiledComposition } from "./htmlCompiler.js";
 
 import {
@@ -18,6 +18,7 @@ import {
   findMissingFrameRanges,
   getNextRetryWorkerCount,
   isRecoverableParallelCaptureError,
+  materializeExtractedFramesForCompiledDir,
   projectBrowserEndToCompositionTimeline,
   resolveRenderWorkerCount,
   selectCaptureCalibrationFrames,
@@ -140,6 +141,91 @@ describe("createCompiledFrameSrcResolver", () => {
 
     expect(resolver("/tmp/hf-job/video-frames/frame_00001.jpg")).toBeNull();
   });
+
+  it("resolves symlinked cache frames when materialized under compiledDir", () => {
+    const resolver = createCompiledFrameSrcResolver("/tmp/hf-job/compiled");
+
+    expect(resolver("/tmp/hf-job/compiled/__hyperframes_video_frames/vid1/frame_00001.jpg")).toBe(
+      "/__hyperframes_video_frames/vid1/frame_00001.jpg",
+    );
+
+    expect(resolver("/tmp/cache/abc123/frame_00001.jpg")).toBeNull();
+  });
+
+  it("encodes reserved characters in frame path segments", () => {
+    const resolver = createCompiledFrameSrcResolver("/tmp/hf-job/compiled");
+
+    expect(
+      resolver("/tmp/hf-job/compiled/__hyperframes_video_frames/video#1/frame_00001.jpg"),
+    ).toBe("/__hyperframes_video_frames/video%231/frame_00001.jpg");
+
+    expect(
+      resolver("/tmp/hf-job/compiled/__hyperframes_video_frames/video?q=1/frame_00001.jpg"),
+    ).toBe("/__hyperframes_video_frames/video%3Fq%3D1/frame_00001.jpg");
+  });
+});
+
+describe("materializeExtractedFramesForCompiledDir", () => {
+  function createExtractedFrames(
+    outputDir: string,
+    framePath: string,
+  ): Pick<ExtractedFrames, "videoId" | "outputDir" | "framePaths"> {
+    return {
+      videoId: "video-1",
+      outputDir,
+      framePaths: new Map([[0, framePath]]),
+    };
+  }
+
+  it("leaves Windows frame paths already under compiledDir unchanged", () => {
+    const compiledDir = win32.resolve("C:\\compiled");
+    const outputDir = win32.join(compiledDir, "__hyperframes_video_frames", "video-1");
+    const framePath = win32.join(outputDir, "frame_000001.jpg");
+    const extracted = createExtractedFrames(outputDir, framePath);
+
+    materializeExtractedFramesForCompiledDir([extracted], compiledDir, {
+      pathModule: win32,
+      fileSystem: {
+        existsSync: () => {
+          throw new Error("inside compiledDir should not touch the filesystem");
+        },
+        mkdirSync: () => {
+          throw new Error("inside compiledDir should not mkdir");
+        },
+        symlinkSync: () => {
+          throw new Error("inside compiledDir should not symlink");
+        },
+      },
+    });
+
+    expect(extracted.outputDir).toBe(outputDir);
+    expect(extracted.framePaths.get(0)).toBe(framePath);
+  });
+
+  it("remaps Windows cache frames under compiledDir using only the frame basename", () => {
+    const compiledDir = win32.resolve("C:\\compiled");
+    const outputDir = win32.resolve("D:\\cache\\abc123");
+    const framePath = win32.join(outputDir, "frame_000001.jpg");
+    const extracted = createExtractedFrames(outputDir, framePath);
+    const symlinks: Array<{ target: string; path: string }> = [];
+
+    materializeExtractedFramesForCompiledDir([extracted], compiledDir, {
+      pathModule: win32,
+      fileSystem: {
+        existsSync: () => false,
+        mkdirSync: () => undefined,
+        symlinkSync: (target, path) => {
+          symlinks.push({ target, path });
+        },
+      },
+    });
+
+    const linkPath = win32.join(compiledDir, "__hyperframes_video_frames", "video-1");
+    expect(extracted.outputDir).toBe(linkPath);
+    expect(extracted.framePaths.get(0)).toBe(win32.join(linkPath, "frame_000001.jpg"));
+    expect(extracted.framePaths.get(0)).not.toContain(outputDir);
+    expect(symlinks).toEqual([{ target: outputDir, path: linkPath }]);
+  });
 });
 
 describe("writeCompiledArtifacts — external assets on Windows drive-letter paths (GH #321)", () => {

packages/producer/src/services/renderOrchestrator.ts

@@ -26,12 +26,14 @@ import {
   writeFileSync,
   copyFileSync,
   appendFileSync,
+  symlinkSync,
 } from "fs";
 import { parseHTML } from "linkedom";
 import {
   type EngineConfig,
   resolveConfig,
   extractAllVideoFrames,
+  type ExtractedFrames,
   type ExtractionPhaseBreakdown,
   createFrameLookupTable,
   type VideoElement,
@@ -93,7 +95,7 @@ import {
   type ElementStackingInfo,
   type HfTransitionMeta,
 } from "@hyperframes/engine";
-import { join, dirname, resolve, relative, isAbsolute } from "path";
+import { join, dirname, resolve, relative, isAbsolute, basename } from "path";
 import { randomUUID } from "crypto";
 import { freemem } from "os";
 import { fileURLToPath } from "url";
@@ -685,6 +687,72 @@ export function createCompiledFrameSrcResolver(
   };
 }
 
+type MaterializedExtractedFrames = Pick<ExtractedFrames, "videoId" | "outputDir" | "framePaths">;
+
+type MaterializePathModule = {
+  resolve: (...segments: string[]) => string;
+  join: (...segments: string[]) => string;
+  dirname: (path: string) => string;
+  basename: (path: string) => string;
+  relative: (from: string, to: string) => string;
+  isAbsolute: (path: string) => boolean;
+};
+
+type MaterializeFileSystem = {
+  existsSync: (path: string) => boolean;
+  mkdirSync: (path: string, options: { recursive: true }) => unknown;
+  symlinkSync: (target: string, path: string) => unknown;
+};
+
+type MaterializeExtractedFramesOptions = {
+  pathModule?: MaterializePathModule;
+  fileSystem?: MaterializeFileSystem;
+};
+
+const materializePathModule: MaterializePathModule = {
+  resolve,
+  join,
+  dirname,
+  basename,
+  relative,
+  isAbsolute,
+};
+
+const materializeFileSystem: MaterializeFileSystem = {
+  existsSync,
+  mkdirSync,
+  symlinkSync,
+};
+
+export function materializeExtractedFramesForCompiledDir(
+  extracted: MaterializedExtractedFrames[],
+  compiledDir: string,
+  options: MaterializeExtractedFramesOptions = {},
+): void {
+  const pathModule = options.pathModule ?? materializePathModule;
+  const fileSystem = options.fileSystem ?? materializeFileSystem;
+  const resolvedCompiledDir = pathModule.resolve(compiledDir);
+  const compiledFrameRoot = pathModule.join(resolvedCompiledDir, "__hyperframes_video_frames");
+
+  for (const ext of extracted) {
+    const resolvedOut = pathModule.resolve(ext.outputDir);
+    if (isPathInside(resolvedOut, resolvedCompiledDir, { pathModule })) continue;
+
+    const linkPath = pathModule.join(compiledFrameRoot, ext.videoId);
+    if (!fileSystem.existsSync(linkPath)) {
+      fileSystem.mkdirSync(pathModule.dirname(linkPath), { recursive: true });
+      fileSystem.symlinkSync(resolvedOut, linkPath);
+    }
+
+    const remapped = new Map<number, string>();
+    for (const [idx, framePath] of ext.framePaths) {
+      remapped.set(idx, pathModule.join(linkPath, pathModule.basename(framePath)));
+    }
+    ext.framePaths = remapped;
+    ext.outputDir = linkPath;
+  }
+}
+
 export function applyRenderModeHints(
   cfg: EngineConfig,
   compiled: CompiledComposition,
@@ -2294,6 +2362,8 @@ export async function executeRenderJob(
       );
       assertNotAborted();
 
+      materializeExtractedFramesForCompiledDir(extractionResult.extracted, compiledDir);
+
       if (extractionResult.extracted.length > 0) {
         frameLookup = createFrameLookupTable(composition.videos, extractionResult.extracted);
       }

packages/producer/src/utils/paths.test.ts

@@ -12,7 +12,7 @@
  */
 
 import { describe, expect, it } from "vitest";
-import { resolve } from "node:path";
+import { resolve, win32 } from "node:path";
 
 import { isPathInside, toExternalAssetKey } from "./paths.js";
 
@@ -47,6 +47,16 @@ describe("isPathInside", () => {
   it("normalises trailing slashes on parent", () => {
     expect(isPathInside(resolve("/foo/bar/baz"), resolve("/foo/bar/"))).toBe(true);
   });
+
+  it("handles Windows paths under the parent directory", () => {
+    expect(
+      isPathInside(
+        win32.resolve("C:\\compiled\\__hyperframes_video_frames\\video\\frame_000001.jpg"),
+        win32.resolve("C:\\compiled"),
+        { pathModule: win32 },
+      ),
+    ).toBe(true);
+  });
 });
 
 describe("toExternalAssetKey", () => {

packages/producer/src/utils/paths.ts

@@ -2,7 +2,13 @@
  * Path resolution utilities for the render pipeline.
  */
 
-import { resolve, basename, join, relative, isAbsolute } from "node:path";
+import {
+  basename,
+  join,
+  resolve as nodeResolve,
+  relative as nodeRelative,
+  isAbsolute as nodeIsAbsolute,
+} from "node:path";
 
 export interface RenderPaths {
   absoluteProjectDir: string;
@@ -11,7 +17,17 @@ export interface RenderPaths {
 
 const DEFAULT_RENDERS_DIR =
   process.env.PRODUCER_RENDERS_DIR ??
-  resolve(new URL(import.meta.url).pathname, "../../..", "renders");
+  nodeResolve(new URL(import.meta.url).pathname, "../../..", "renders");
+
+type PathModuleLike = {
+  resolve: (...segments: string[]) => string;
+  relative: (from: string, to: string) => string;
+  isAbsolute: (path: string) => boolean;
+};
+
+type IsPathInsideOptions = {
+  pathModule?: PathModuleLike;
+};
 
 /**
  * Cross-platform containment check.
@@ -25,15 +41,22 @@ const DEFAULT_RENDERS_DIR =
  * Both inputs are normalised via `resolve()` so callers don't need to.
  * Equality counts as "inside" (a directory contains itself).
  */
-export function isPathInside(childPath: string, parentPath: string): boolean {
+export function isPathInside(
-  const absChild = resolve(childPath);
+  childPath: string,
-  const absParent = resolve(parentPath);
+  parentPath: string,
+  options: IsPathInsideOptions = {},
+): boolean {
+  const resolvePath = options.pathModule?.resolve ?? nodeResolve;
+  const relativePath = options.pathModule?.relative ?? nodeRelative;
+  const isPathAbsolute = options.pathModule?.isAbsolute ?? nodeIsAbsolute;
+  const absChild = resolvePath(childPath);
+  const absParent = resolvePath(parentPath);
   if (absChild === absParent) return true;
-  const rel = relative(absParent, absChild);
+  const rel = relativePath(absParent, absChild);
   // `relative()` returns "" when paths are equal, ".." or "..\\foo" when child
   // is above the parent, and an absolute path when they live on different
   // drives/volumes (Windows) — none of which count as "inside".
-  return rel !== "" && !rel.startsWith("..") && !isAbsolute(rel);
+  return rel !== "" && !rel.startsWith("..") && !isPathAbsolute(rel);
 }
 
 /**
@@ -91,10 +114,10 @@ export function resolveRenderPaths(
   outputPath: string | null | undefined,
   rendersDir: string = DEFAULT_RENDERS_DIR,
 ): RenderPaths {
-  const absoluteProjectDir = resolve(projectDir);
+  const absoluteProjectDir = nodeResolve(projectDir);
   const projectName = basename(absoluteProjectDir);
   const resolvedOutputPath = outputPath ?? join(rendersDir, `${projectName}.mp4`);
-  const absoluteOutputPath = resolve(resolvedOutputPath);
+  const absoluteOutputPath = nodeResolve(resolvedOutputPath);
 
   return { absoluteProjectDir, absoluteOutputPath };
 }