diff --git a/packages/producer/src/services/render/shared.ts b/packages/producer/src/services/render/shared.ts index 8c26224d17..c3704a75f9 100644 --- a/packages/producer/src/services/render/shared.ts +++ b/packages/producer/src/services/render/shared.ts @@ -293,7 +293,10 @@ type MaterializePathModule = { type MaterializeFileSystem = { existsSync: (path: string) => boolean; mkdirSync: (path: string, options: { recursive: true }) => unknown; - symlinkSync: (target: string, path: string) => unknown; + // `type` is only ever passed as "junction": the Windows-privilege-free + // fallback below requests one explicitly. On POSIX Node ignores the argument + // (always a normal symlink), so the default fs-backed impl needs no change. + symlinkSync: (target: string, path: string, type?: "junction") => unknown; cpSync: (src: string, dest: string, options: { recursive: true }) => unknown; // Optional: only the stale-entry (EEXIST) recovery path calls it, and the // default fileSystem always supplies it. Test doubles that never trigger @@ -399,31 +402,71 @@ export function createMemorySampler(intervalMs: number = 250): MemorySampler { * external callers should use `executeRenderJob` instead. */ // Stage one video's extracted-frame dir into the compiled dir. Default is a -// single symlink (cheap; the in-process renderer); `materializeSymlinks` copies +// single link (cheap; the in-process renderer); `materializeSymlinks` copies // instead (distributed plan() needs a self-contained dir). On Windows without -// Developer Mode/Administrator symlink creation is rejected with EPERM/EACCES, -// which failed high/standard renders — degrade to a copy there rather than -// throwing. Non-permission errors still propagate so real failures aren't hidden. -// One-time guard for the symlink→copy fallback notice below. +// Developer Mode/Administrator a plain symlink is rejected (EPERM/EACCES/UNKNOWN), +// which failed high/standard renders — fall back to a junction (privilege-free) +// and only to a full copy when even that is unavailable (SMB/NFS/exFAT). See +// linkOrCopyFrameDir. Non-capability errors still propagate so real failures +// aren't hidden. +// One-time guard for the link→copy fallback notice below. let warnedSymlinkFallback = false; -// Create the symlink, degrading to a copy on Windows' no-symlink-privilege -// errors (EPERM/EACCES, plus UNKNOWN — some Windows builds surface a symlink -// privilege denial as an UNKNOWN-coded error rather than EPERM). Non-permission -// errors propagate. +// Filesystem-capability errors: the link operation is unsupported here, not a +// real failure. Codes vary by platform and mount — EPERM/EACCES (Windows +// symlink privilege), UNKNOWN (some Windows builds surface a privilege denial +// this way), and EINVAL/ENOSYS/EOPNOTSUPP/ENOTSUP (SMB/NFS/exFAT rejecting a +// link outright). Any of these means "fall through to a copy", never abort. +function isSymlinkCapabilityError(code: string | undefined): boolean { + return ( + code === "EPERM" || + code === "EACCES" || + code === "UNKNOWN" || + code === "EINVAL" || + code === "ENOSYS" || + code === "EOPNOTSUPP" || + code === "ENOTSUP" + ); +} + +// Stage the frame dir with a link, degrading to a copy only when the filesystem +// can't link at all. Three tiers: +// 1. Plain symlink — the cheap default (POSIX, Windows with Developer Mode). +// 2. Junction — Windows' privilege-free directory link. When a plain symlink +// is rejected for lack of privilege (EPERM/EACCES/UNKNOWN), a junction +// needs neither Developer Mode nor Administrator and still stages the dir +// at link speed, avoiding a full recursive copy of every frame. On POSIX +// Node ignores the "junction" type, but we only reach here on a privilege +// error, which POSIX doesn't raise — so this tier is Windows-only in +// practice. Junctions need a local absolute target and don't exist on +// SMB/NFS/exFAT, so a capability error here drops to tier 3. +// 3. Recursive copy — the last resort for mounts with no link support. +// Non-capability errors (e.g. EEXIST stale entry, ENOSPC) propagate unchanged. function linkOrCopyFrameDir(fileSystem: MaterializeFileSystem, src: string, dest: string): void { try { fileSystem.symlinkSync(src, dest); + return; } catch (err) { const code = (err as NodeJS.ErrnoException | undefined)?.code; if (code !== "EPERM" && code !== "EACCES" && code !== "UNKNOWN") throw err; - // Copying is measurably slower than symlinking, so surface the degrade once - // — it explains a render that suddenly got heavier and saves a support + // Tier 2: try a junction before paying for a copy. + try { + fileSystem.symlinkSync(src, dest, "junction"); + return; + } catch (junctionErr) { + const junctionCode = (junctionErr as NodeJS.ErrnoException | undefined)?.code; + // Only a capability error means "junctions aren't available here" — keep + // going to the copy. Anything else (a stale EEXIST entry the outer handler + // must clear, a real ENOSPC) has to surface. + if (!isSymlinkCapabilityError(junctionCode)) throw junctionErr; + } + // Tier 3: copying is measurably slower than linking, so surface the degrade + // once — it explains a render that suddenly got heavier and saves a support // round-trip diagnosing slow frame staging on Windows. if (!warnedSymlinkFallback) { warnedSymlinkFallback = true; defaultLogger.info( - `[Render] Symlinking extracted frames was rejected (${code}); copying them into the compiled dir instead. Expected on Windows without Developer Mode/Administrator.`, + `[Render] Linking extracted frames was rejected (${code}) and a junction was unavailable; copying them into the compiled dir instead. Expected on SMB/NFS/exFAT mounts with no link support.`, ); } fileSystem.cpSync(src, dest, { recursive: true }); diff --git a/packages/producer/src/services/render/stages/extractVideosStage.test.ts b/packages/producer/src/services/render/stages/extractVideosStage.test.ts index 640d90f526..bf4003ec35 100644 --- a/packages/producer/src/services/render/stages/extractVideosStage.test.ts +++ b/packages/producer/src/services/render/stages/extractVideosStage.test.ts @@ -1,6 +1,34 @@ -import { describe, expect, it } from "vitest"; -import { appendAutoDetectedVideoAudio, shouldCopyExtractedFrames } from "./extractVideosStage.js"; -import type { ExtractedFrames, VideoElement } from "@hyperframes/engine"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + appendAutoDetectedVideoAudio, + runExtractVideosStage, + type ExtractVideosStageInput, +} from "./extractVideosStage.js"; +import type { EngineConfig, ExtractedFrames, VideoElement } from "@hyperframes/engine"; +import type { RenderJob } from "../../renderOrchestrator.js"; + +// Stub only the two boundaries the staging path crosses, so the regression +// below runs the REAL `runExtractVideosStage` (the exact function the render +// orchestrator invokes) without ffmpeg or disk: +// - `extractAllVideoFrames` returns an empty extraction so the stage reaches +// its `materializeExtractedFramesForCompiledDir` call without decoding. +// - `materializeExtractedFramesForCompiledDir` is spied so we can assert the +// `materializeSymlinks` value the caller propagated. Everything else in +// both modules stays real. +const { extractAllVideoFramesMock, materializeSpy } = vi.hoisted(() => ({ + extractAllVideoFramesMock: vi.fn(async () => ({ extracted: [] as ExtractedFrames[] })), + materializeSpy: vi.fn(), +})); + +vi.mock("@hyperframes/engine", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, extractAllVideoFrames: extractAllVideoFramesMock }; +}); + +vi.mock("../shared.js", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, materializeExtractedFramesForCompiledDir: materializeSpy }; +}); function makeVideo(overrides: Partial = {}): VideoElement { return { @@ -82,13 +110,59 @@ describe("appendAutoDetectedVideoAudio", () => { }); }); -describe("shouldCopyExtractedFrames", () => { - it("copies frames on Windows (symlinkSync throws EPERM without Developer Mode)", () => { - expect(shouldCopyExtractedFrames("win32")).toBe(true); +// Regression for the win32 bypass: the render orchestrator forced an eager copy +// on Windows (`materializeSymlinks: shouldCopyExtractedFrames(process.platform)`, +// which was `true`), so `stageExtractedFrameDirOnce` copied directly and the +// symlink → junction → copy ladder in `linkOrCopyFrameDir` was never reached on +// the very platform it was written to optimize. These tests pin the actual +// `runExtractVideosStage` contract at both call sites so it can't regress: the +// local render path must NOT request an eager copy, and only the distributed +// `plan()` may — proven at the stage the orchestrator really calls, not a helper. +describe("runExtractVideosStage — frame staging mode", () => { + afterEach(() => { + extractAllVideoFramesMock.mockClear(); + materializeSpy.mockClear(); + }); + + function makeInput(overrides: Partial = {}): ExtractVideosStageInput { + return { + projectDir: "/proj", + compiledDir: "/proj/compiled", + // force-sdr skips the HDR color-space probes, so the only engine call the + // stage makes is the mocked `extractAllVideoFrames`. + job: { + config: { fps: 30, hdrMode: "force-sdr", videoFrameFormat: "auto" }, + } as unknown as RenderJob, + cfg: {} as EngineConfig, + composition: { + duration: 5, + videos: [makeVideo()], + audios: [], + images: [], + width: 1920, + height: 1080, + }, + abortSignal: undefined, + assertNotAborted: () => {}, + ...overrides, + }; + } + + it("does not request an eager copy on the local render path (win32 must reach the symlink→junction→copy ladder, not bypass it)", async () => { + await runExtractVideosStage(makeInput({ materializeSymlinks: false })); + expect(materializeSpy).toHaveBeenCalledTimes(1); + expect(materializeSpy.mock.calls[0]?.[2]).toEqual({ materializeSymlinks: false }); + }); + + it("leaves staging on the ladder when the caller omits the flag (the safe default the local renderer relies on)", async () => { + await runExtractVideosStage(makeInput()); + expect(materializeSpy).toHaveBeenCalledTimes(1); + expect(materializeSpy.mock.calls[0]?.[2]?.materializeSymlinks).not.toBe(true); }); - it("symlinks on macOS and Linux (cheaper, symlinks allowed)", () => { - expect(shouldCopyExtractedFrames("darwin")).toBe(false); - expect(shouldCopyExtractedFrames("linux")).toBe(false); + it("requests an eager copy only when the distributed plan() explicitly asks for a self-contained dir", async () => { + await runExtractVideosStage(makeInput({ materializeSymlinks: true })); + expect(materializeSpy).toHaveBeenCalledTimes(1); + expect(materializeSpy.mock.calls[0]?.[2]).toEqual({ materializeSymlinks: true }); }); }); diff --git a/packages/producer/src/services/render/stages/extractVideosStage.ts b/packages/producer/src/services/render/stages/extractVideosStage.ts index 7f133b789d..bb793b2909 100644 --- a/packages/producer/src/services/render/stages/extractVideosStage.ts +++ b/packages/producer/src/services/render/stages/extractVideosStage.ts @@ -65,10 +65,16 @@ export interface ExtractVideosStageInput { abortSignal: AbortSignal | undefined; assertNotAborted: () => void; /** - * Whether to materialize symlinks into real files when staging extracted - * frames inside `compiledDir`. Default `false` preserves the in-process - * renderer's behavior (single symlink per video). Distributed `plan()` - * passes `true` so the planDir is self-contained. + * Force an eager recursive COPY when staging extracted frames into + * `compiledDir`, instead of the default symlink → junction → copy ladder + * (see `linkOrCopyFrameDir` in `../shared.ts`). + * + * Set `true` ONLY for distributed `plan()`, whose planDir must be + * self-contained across machines (symlinks/junctions don't survive S3/GCS + * round-trips). The local in-process renderer must leave this `false` (its + * default) so Windows-without-Developer-Mode stages a privilege-free junction + * at link speed rather than copying every frame — do NOT re-couple this to + * `process.platform`. Default `false`. */ materializeSymlinks?: boolean; } @@ -96,18 +102,6 @@ export interface ExtractVideosStageResult { videoExtractMs: number; } -/** - * Whether the extract stage should COPY frames into the compiled dir instead of - * symlinking them. Windows without Developer Mode / Administrator can't create - * symlinks (`symlinkSync` throws EPERM), which failed local video renders; copy - * there instead. Elsewhere symlinking is cheaper, so keep it. (The distributed - * `plan()` path already forces copying for a different reason — a self-contained - * planDir — by passing `materializeSymlinks: true` explicitly.) - */ -export function shouldCopyExtractedFrames(platform: NodeJS.Platform): boolean { - return platform === "win32"; -} - export async function runExtractVideosStage( input: ExtractVideosStageInput, ): Promise { diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index 818694973c..1d3af7ea80 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -624,15 +624,18 @@ describe("materializeExtractedFramesForCompiledDir", () => { }); // fallow-ignore-next-line code-duplication - it("falls back to copying frames when symlinkSync fails with EPERM (Windows, no Developer Mode)", () => { - // Windows without Developer Mode/Administrator rejects symlink creation with - // EPERM — high/standard-quality renders failed here while draft worked. The - // helper must degrade to a recursive copy instead of throwing. + it("falls back to copying frames only when both the symlink and the junction fail (EPERM, no link support)", () => { + // Windows without Developer Mode/Administrator rejects a plain symlink with + // EPERM. The helper first tries a junction; when that ALSO fails with a + // capability error (a mount with no link support at all — SMB/exFAT), it + // must degrade to a recursive copy instead of throwing. Both the plain + // symlink and the junction attempt must precede the copy. 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 copies: Array<{ src: string; dest: string; recursive: boolean }> = []; + const linkAttempts: Array<{ type: "junction" | undefined }> = []; // fallow-ignore-next-line code-duplication materializeExtractedFramesForCompiledDir([extracted], compiledDir, { @@ -640,7 +643,8 @@ describe("materializeExtractedFramesForCompiledDir", () => { fileSystem: { existsSync: () => false, mkdirSync: () => undefined, - symlinkSync: () => { + symlinkSync: (_target, _path, type) => { + linkAttempts.push({ type }); const err: NodeJS.ErrnoException = new Error("EPERM: operation not permitted, symlink"); err.code = "EPERM"; throw err; @@ -652,11 +656,52 @@ describe("materializeExtractedFramesForCompiledDir", () => { }); const linkPath = win32.join(compiledDir, "__hyperframes_video_frames", "video-1"); + // Plain symlink first, then a junction, then the copy. + expect(linkAttempts).toEqual([{ type: undefined }, { type: "junction" }]); expect(copies).toEqual([{ src: outputDir, dest: linkPath, recursive: true }]); expect(extracted.outputDir).toBe(linkPath); expect(extracted.framePaths.get(0)).toBe(win32.join(linkPath, "frame_000001.jpg")); }); + // fallow-ignore-next-line code-duplication + it("stages via a junction (no copy) when a plain symlink is rejected but the junction succeeds (Windows, no Developer Mode)", () => { + // The common Windows-without-Developer-Mode case: a plain directory symlink + // needs privilege and is rejected (EPERM), but a junction needs none. The + // helper must stage the dir with the junction at link speed and NOT fall + // through to a full recursive copy of every frame. + 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 junctions: Array<{ target: string; path: string }> = []; + + // fallow-ignore-next-line code-duplication + materializeExtractedFramesForCompiledDir([extracted], compiledDir, { + pathModule: win32, + fileSystem: { + existsSync: () => false, + mkdirSync: () => undefined, + symlinkSync: (target, path, type) => { + if (type !== "junction") { + const err: NodeJS.ErrnoException = new Error("EPERM: operation not permitted, symlink"); + err.code = "EPERM"; + throw err; + } + junctions.push({ target, path }); + }, + cpSync: () => { + throw new Error("junction fast-path should not invoke cpSync"); + }, + }, + }); + + const linkPath = win32.join(compiledDir, "__hyperframes_video_frames", "video-1"); + expect(junctions).toEqual([{ target: outputDir, path: linkPath }]); + 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); + }); + it("rethrows a non-permission symlink error instead of masking it with a copy", () => { const compiledDir = win32.resolve("C:\\compiled"); const outputDir = win32.resolve("D:\\cache\\abc123"); @@ -728,10 +773,11 @@ describe("materializeExtractedFramesForCompiledDir", () => { }); // fallow-ignore-next-line code-duplication - it("falls back to copying when symlinkSync fails with UNKNOWN (some Windows privilege denials)", () => { - // Some Windows builds surface a no-symlink-privilege denial as an - // UNKNOWN-coded error rather than EPERM/EACCES — it must still degrade to a - // copy, not hard-fail the render. + it("falls back to copying when both the symlink and the junction fail with UNKNOWN (some Windows privilege denials)", () => { + // Some Windows builds surface a no-link-privilege denial as an UNKNOWN-coded + // error rather than EPERM/EACCES. When both the plain symlink and the + // junction fall over this way, the helper must still degrade to a copy, not + // hard-fail the render. const compiledDir = win32.resolve("C:\\compiled"); const outputDir = win32.resolve("D:\\cache\\abc123"); const framePath = win32.join(outputDir, "frame_000001.jpg"); @@ -760,6 +806,38 @@ describe("materializeExtractedFramesForCompiledDir", () => { expect(extracted.framePaths.get(0)).toBe(win32.join(linkPath, "frame_000001.jpg")); }); + // fallow-ignore-next-line code-duplication + it("propagates a non-capability junction error (does not mask it with a copy)", () => { + // If the plain symlink is a privilege error but the junction attempt fails + // for a genuine reason (here ENOSPC), that must surface — silently copying + // would hide a real disk problem. + 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); + + expect(() => + materializeExtractedFramesForCompiledDir([extracted], compiledDir, { + pathModule: win32, + fileSystem: { + existsSync: () => false, + mkdirSync: () => undefined, + symlinkSync: (_target, _path, type) => { + const err: NodeJS.ErrnoException = + type === "junction" + ? new Error("ENOSPC: no space left, junction") + : new Error("EPERM: operation not permitted, symlink"); + err.code = type === "junction" ? "ENOSPC" : "EPERM"; + throw err; + }, + cpSync: () => { + throw new Error("must not fall back to copy for a non-capability junction error"); + }, + }, + }), + ).toThrow(/ENOSPC/); + }); + // fallow-ignore-next-line code-duplication it("clears a stale entry and re-copies when the eager-copy path (materializeSymlinks) hits EEXIST", () => { // #2025 routes Windows through the eager-copy branch. Reusing a dir a prior diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index cdd68428e4..944f61db4f 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -124,10 +124,7 @@ import { import { type HdrPerfCollector, type HdrPerfSummary } from "./render/hdrPerf.js"; import { runCompileStage } from "./render/stages/compileStage.js"; import { runProbeStage } from "./render/stages/probeStage.js"; -import { - runExtractVideosStage, - shouldCopyExtractedFrames, -} from "./render/stages/extractVideosStage.js"; +import { runExtractVideosStage } from "./render/stages/extractVideosStage.js"; import { runAudioStage } from "./render/stages/audioStage.js"; import { runCaptureStage } from "./render/stages/captureStage.js"; import { @@ -1868,9 +1865,14 @@ export async function executeRenderJob( composition, abortSignal, assertNotAborted, - // Copy (don't symlink) extracted frames on Windows — symlinkSync throws - // EPERM there without Developer Mode/admin, which failed local renders. - materializeSymlinks: shouldCopyExtractedFrames(process.platform), + // Local in-process render: never force an eager copy. Frame staging + // routes through materializeExtractedFramesForCompiledDir's + // symlink → junction → copy ladder (see linkOrCopyFrameDir), which + // handles Windows-without-Developer-Mode by staging a privilege-free + // junction at link speed and copies only as the last resort (SMB/NFS/ + // exFAT). Eager copy (materializeSymlinks: true) is reserved for the + // distributed plan(), whose planDir must be self-contained. + materializeSymlinks: false, }), ); const {