fix: address PR #596 review issues - #597
Conversation
…oding) - P1: symlink cached extraction frames into compiledDir so the served-frame fast path works when extractCacheDir is set - P2: replace raw forceKillBrowserProcess with forceReleaseBrowser that atomically clears pool state before killing the Chrome process - P3: decode percent-encoded path segments in the file server so reserved characters in video IDs round-trip correctly
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Staff review follow-up: one blocking issue remains.
P1: packages/producer/src/services/renderOrchestrator.ts:2301-2310 breaks Windows video extraction paths.
The cache materialization guard uses resolve(compiledDir) + "/", which is false for normal Windows paths under compiledDir because they use backslashes. That means even non-cache extracted frames are treated as outside compiledDir on Windows and enter the remap block. The remap then uses framePath.slice(framePath.lastIndexOf("/") + 1), so a Windows path with backslashes is treated as the basename and gets joined back under linkPath, producing invalid paths like:
C:\compiled__hyperframes_video_frames\vid\C:\compiled__hyperframes_video_frames\vid\frame_00001.jpg
Those files do not exist, so served-frame injection will 404/fail for Windows video renders, not just the cache-enabled path.
Suggested fix: use the existing cross-platform isPathInside(resolvedOut, compiledDir) check for the containment test, and basename(framePath) from node:path for frame names. Please add a path.win32 regression test for this materialization/remap logic; the current tests only cover POSIX-style paths.
I did not run the PR test suite locally because gh pr checkout/diff were intermittently failing against GitHub from this machine; this review is based on the PR file snapshots from commit 3ca71c1.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit fc0a5de.
The Windows path regression from the previous review looks fixed: cache materialization now uses the cross-platform isPathInside helper with an injected win32 path module in tests, and remaps frame paths with path.basename instead of slicing on '/'. The new materializeExtractedFramesForCompiledDir tests cover both already-under-compiledDir Windows paths and cache paths on a different Windows drive.
I also rechecked the original PR #596 review points:
- Cache-enabled frame extraction now materializes cached frames under compiledDir before frame lookup creation, so createCompiledFrameSrcResolver can return served URLs.
- Browser force-release now clears pooled state before killing/disconnecting the browser.
- Percent-encoded reserved path segments are decoded by the file server, with a regression test for '#'.
Relevant CI is passing: Build, Test, Typecheck, Lint, Format, CLI smoke, Tests on windows-latest, and Render on windows-latest. Some broad regression shards are still pending, but I do not see code-review blockers in this patch.
Merge activity
|
## Summary Fixes three issues identified in the [post-merge review](heygen-com#596 (review)) of PR heygen-com#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
2 participants
Summary
Fixes three issues identified in the post-merge review of PR #596:
P1 (cache bypass): When
extractCacheDiris set, extracted frames live outsidecompiledDir, socreateCompiledFrameSrcResolverrejects them and every frame falls back to base64 data URIs. Fix: symlink cached frame directories intocompiledDir/__hyperframes_video_frames/after extraction and remapframePathsso the served-frame fast path works.P2 (pooled browser stale state):
closeCaptureSessionforce-killed the Chrome process on timeout via rawSIGKILLwithout clearingpooledBrowser/pooledBrowserRefCount, leaving other sessions with a dead browser reference. Fix: addforceReleaseBrowser()inbrowserManagerthat atomically clears pool state before killing the process.P3 (reserved chars in URLs):
createCompiledFrameSrcResolverencodes path segments withencodeURIComponent, but the file server usedc.req.path(which only appliesdecodeURI) to look up files on disk. Video IDs containing#,?, or%produced 404s. Fix: applydecodeURIComponentper path segment in the file server's catch-all route.Test plan
createCompiledFrameSrcResolvertests: symlinked cache paths resolve to served URLs; cache-external paths return null; reserved characters encode correctlyforceReleaseBrowsertests: kills process + disconnects; tolerates already-killed processcreateFileServertest:video%231/frame.jpgserves file fromvideo#1/frame.jpgon disk