Skip to content

fix(producer): copy extracted frames on Windows to avoid symlink EPERM [P0] - #2025

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/win32-copy-extracted-frames
Jul 7, 2026
Merged

fix(producer): copy extracted frames on Windows to avoid symlink EPERM [P0]#2025
miguel-heygen merged 1 commit into
mainfrom
fix/win32-copy-extracted-frames

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Problem

Local video renders on Windows without Developer Mode/Administrator fail at the video_extract stage: renderOrchestrator → runExtractVideosStage materializes each video's extracted frames into the compiled dir via symlinkSync with no materializeSymlinks flag, and symlinkSync throws EPERM there. Reported repeatedly (users worked around it by patching cli.js to pass materializeSymlinks: process.platform==='win32', or by falling back to snapshot-frame + manual ffmpeg mux).

The distributed plan() path already copies (materializeSymlinks: true) — only the local render caller was missing it.

Fix

Pass materializeSymlinks: shouldCopyExtractedFrames(process.platform) at the local runExtractVideosStage call in renderOrchestrator.ts:

  • win32 → copy the frames (symlinks need privilege there)
  • darwin/linux → symlink (cheap; unchanged)

New pure shouldCopyExtractedFrames(platform) helper in extractVideosStage.ts documents the rationale and is unit-tested. This sidesteps the symlink entirely on Windows — independent of, and complementary to, the EPERM→cpSync fallback in open PR #1959 (which catches the throw if a symlink is ever attempted elsewhere).

Tests

extractVideosStage.test.tsshouldCopyExtractedFrames: win32 → copy (true), darwin/linux → symlink (false). Pre-commit typecheck + fallow + oxlint/oxfmt all green.

The local render path (renderOrchestrator → runExtractVideosStage) materialized
each video's extracted frames into the compiled dir via symlinkSync, with no
materializeSymlinks flag. On Windows without Developer Mode/Administrator,
symlinkSync throws EPERM, so local video renders failed at the video_extract
stage (users worked around it with materializeSymlinks patches / snapshot-frame
hacks). The distributed plan() path already copies (materializeSymlinks: true).

Pass materializeSymlinks: shouldCopyExtractedFrames(process.platform) at the
local caller — copy on win32 (symlinks unavailable), symlink elsewhere (cheap).
New pure shouldCopyExtractedFrames() helper + unit tests.
@miguel-heygen miguel-heygen changed the title fix(producer): copy extracted frames on Windows to avoid symlink EPERM [P0] fix(producer): copy extracted frames on Windows to avoid symlink EPERM Jul 7, 2026
@miguel-heygen
miguel-heygen marked this pull request as ready for review July 7, 2026 19:04

@vanceingalls vanceingalls left a comment

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.

R1 — #2025 at 14ece7e532ba2ac4c5376467342a7b28e0dd03a5

🟢 LGTM. Small, well-scoped, correct. Two flags: one required-check blocker, one cosmetic nit.

Correctness

  • materializeSymlinks?: boolean is a pre-existing field on ExtractVideosStageInput (verified at head, extractVideosStage.ts:~74 — the doc comment already explains why the distributed plan() path forces true). The local renderOrchestrator caller was the only site that omitted it, silently defaulting to falsesymlinkSync → EPERM on Windows without Dev Mode. This PR wires the flag through cleanly.
  • Helper is pure and platform-string-driven — trivial to unit test, trivially correct. Naming is a bit meta (shouldCopyExtractedFrames returns the value we pass as materializeSymlinks), but the two names semantically agree so it's fine.
  • Complementary to open PR #1959 (EPERM → cpSync fallback inside the extract stage itself): this PR sidesteps the symlink preemptively on win32; #1959 catches at throw-time. They compose well — this one covers the intended path, #1959 catches any escape.

Coverage

Helper unit-tested for the three relevant platforms. The orchestrator wiring is validated by the Windows render verification CI job (SUCCESS on the older SHA at 18:32; re-triggered at 19:03 for the current head, Render on windows-latest still IN_PROGRESS at review time — worth waiting for green before merge).

Blocker

  • Semantic PR title CI check FAILED on the current head (SUCCESS on the earlier SHA at 18:24, FAILURE at 19:02). The [P0] prefix breaks conventional-commits parsing. Either drop the prefix from the title (keep the body as-is), or move the [P0] marker to the end / a label. If this is a required check, it'll block merge.

Optional nit

  • Windows users who have Developer Mode or admin enabled don't need the copy — symlinkSync would work for them, and copying is measurably slower for large extractions. This PR always copies on win32 regardless. Right call for now (correctness > perf, and detecting Dev Mode is a syscall dance you'd rather not add here), but worth remembering if the copy step ever shows up in perf profiles from win32 users.

R1 by Via

@miguel-heygen miguel-heygen changed the title [P0] fix(producer): copy extracted frames on Windows to avoid symlink EPERM fix(producer): copy extracted frames on Windows to avoid symlink EPERM [P0] Jul 7, 2026
miguel-heygen added a commit that referenced this pull request Jul 7, 2026
…copy path

Addresses review nits on the frame-staging fallback:
- Widen the symlink no-privilege catch from EPERM/EACCES to also include
  UNKNOWN (some Windows builds surface a symlink privilege denial as UNKNOWN).
- Wrap the EEXIST stale-entry recovery around BOTH staging branches, not just
  the symlink one: after #2025 Windows uses the eager cpSync path, which can
  collide with a dangling symlink left by a prior Linux run — now it clears the
  stale entry and re-stages either way.
- Emit a one-time INFO log when symlinking degrades to copying, so a heavier
  Windows render is self-explanatory.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

@via thanks for the review. The semantic-title check was failing on the [P0] prefix — moved the priority tag to the end of the title so it parses as a conventional commit again (green now). No code change. Agree on letting the windows-latest render job go green before merge; it was mid-run at the new head.

@miguel-heygen
miguel-heygen merged commit 1aa39d4 into main Jul 7, 2026
86 of 88 checks passed
@miguel-heygen
miguel-heygen deleted the fix/win32-copy-extracted-frames branch July 7, 2026 21:10
miguel-heygen added a commit that referenced this pull request Jul 7, 2026
…s EPERM

* fix(producer): fall back to copying extracted frames when symlink hits EPERM

materializeExtractedFramesForCompiledDir stages each video's extracted
frames into the compiled dir via a single symlink (the in-process
renderer's default; distributed plan() already copies via
materializeSymlinks). Windows without Developer Mode (or Administrator)
cannot create symlinks and rejects with EPERM, so high/standard-quality
renders failed there — while draft quality worked because it avoids the
symlinked-cache path entirely.

Fix: a new stageExtractedFrameDir helper catches EPERM/EACCES from
symlinkSync and falls back to the same recursive cpSync the
materializeSymlinks path already uses. The extra disk is far better than
a hard render failure on a default Windows configuration. Non-permission
errors (ENOSPC, etc.) still propagate so real failures aren't masked as
silent copies. Extracting the helper also keeps the main function under
the complexity gate.

Test: two new cases via the injected fileSystem — symlinkSync throwing
EPERM triggers exactly one recursive cpSync (frames still remapped under
compiledDir), and a non-permission error (ENOSPC) rethrows without
falling back to copy. Full renderOrchestrator suite (81) passes.

* fix(producer): recover from a stale dangling frame-symlink (EEXIST)

Follow-up to this PR's EPERM copy fallback, from a further Windows report: the
symlink fails with EEXIST after the extraction cache is GC'd. A prior render's
symlink at the compiled linkPath dangles once its target is removed; the
caller's existsSync() guard follows the dead link and reads it as absent, so
staging runs again, but the link file still exists and symlinkSync collides
with EEXIST -> the render hard-fails.

Catch EEXIST in stageExtractedFrameDir, clear the stale entry (rmSync), and
re-stage (link, or copy on EPERM/EACCES). Factored the link-or-copy into a
helper reused by both the first attempt and the retry. rmSync is an optional
injected fs method (default fs supplies it; only the EEXIST path calls it).
New unit test covers the dangling-symlink recovery.

* fix(producer): widen symlink fallback to UNKNOWN and cover EEXIST on copy path

Addresses review nits on the frame-staging fallback:
- Widen the symlink no-privilege catch from EPERM/EACCES to also include
  UNKNOWN (some Windows builds surface a symlink privilege denial as UNKNOWN).
- Wrap the EEXIST stale-entry recovery around BOTH staging branches, not just
  the symlink one: after #2025 Windows uses the eager cpSync path, which can
  collide with a dangling symlink left by a prior Linux run — now it clears the
  stale entry and re-stages either way.
- Emit a one-time INFO log when symlinking degrades to copying, so a heavier
  Windows render is self-explanatory.
dahans-msft2 pushed a commit to dahans-msft2/hyperframes that referenced this pull request Aug 6, 2026
…s EPERM

* fix(producer): fall back to copying extracted frames when symlink hits EPERM

materializeExtractedFramesForCompiledDir stages each video's extracted
frames into the compiled dir via a single symlink (the in-process
renderer's default; distributed plan() already copies via
materializeSymlinks). Windows without Developer Mode (or Administrator)
cannot create symlinks and rejects with EPERM, so high/standard-quality
renders failed there — while draft quality worked because it avoids the
symlinked-cache path entirely.

Fix: a new stageExtractedFrameDir helper catches EPERM/EACCES from
symlinkSync and falls back to the same recursive cpSync the
materializeSymlinks path already uses. The extra disk is far better than
a hard render failure on a default Windows configuration. Non-permission
errors (ENOSPC, etc.) still propagate so real failures aren't masked as
silent copies. Extracting the helper also keeps the main function under
the complexity gate.

Test: two new cases via the injected fileSystem — symlinkSync throwing
EPERM triggers exactly one recursive cpSync (frames still remapped under
compiledDir), and a non-permission error (ENOSPC) rethrows without
falling back to copy. Full renderOrchestrator suite (81) passes.

* fix(producer): recover from a stale dangling frame-symlink (EEXIST)

Follow-up to this PR's EPERM copy fallback, from a further Windows report: the
symlink fails with EEXIST after the extraction cache is GC'd. A prior render's
symlink at the compiled linkPath dangles once its target is removed; the
caller's existsSync() guard follows the dead link and reads it as absent, so
staging runs again, but the link file still exists and symlinkSync collides
with EEXIST -> the render hard-fails.

Catch EEXIST in stageExtractedFrameDir, clear the stale entry (rmSync), and
re-stage (link, or copy on EPERM/EACCES). Factored the link-or-copy into a
helper reused by both the first attempt and the retry. rmSync is an optional
injected fs method (default fs supplies it; only the EEXIST path calls it).
New unit test covers the dangling-symlink recovery.

* fix(producer): widen symlink fallback to UNKNOWN and cover EEXIST on copy path

Addresses review nits on the frame-staging fallback:
- Widen the symlink no-privilege catch from EPERM/EACCES to also include
  UNKNOWN (some Windows builds surface a symlink privilege denial as UNKNOWN).
- Wrap the EEXIST stale-entry recovery around BOTH staging branches, not just
  the symlink one: after heygen-com#2025 Windows uses the eager cpSync path, which can
  collide with a dangling symlink left by a prior Linux run — now it clears the
  stale entry and re-stages either way.
- Emit a one-time INFO log when symlinking degrades to copying, so a heavier
  Windows render is self-explanatory.
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.

2 participants