Repository navigation
fix(engine): audio scratch files Windows refuses to delete no longer fail a render - #6474
Conversation
…ls a finished audio mix
…the audio mix finishes
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 0 nits · head f465e9cb (pinned 47204d20 has been superseded)
- blocker (CI): the required
Preflight (lint + format)failsoxfmt --checkonpackages/engine/src/utils/renameWithRetry.test.ts, which meansregression,Render on windows-latestandTests on windows-latestcan't pass.47204d20failed the sameformat:check. Runoxfmton that file.
Scope
I read the full diff and checked all 20+ renameWithRetry callers in core/engine/producer, the audio-path rmSync sites, and the 47204d20 → f465e9cb delta. The removeScratch swaps are fine: each one is a best-effort cleanup in a finally or after the result.
— Genesis
Edit accuracy: accurate 271 (base branch 271), smooth 155 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — 0 resolved, 2 open, 0 new · ce894665 → 63917f64. The nextStep split only refactors the loop. The copy fallback is still the default in the shared renameWithRetry/renameWithRetryAsync, so it still applies to artifactTransaction.commit and the other atomic publishers (blocker, renameWithRetry.ts). The process-wide left list in removeScratch.ts:3 is unchanged. If you'd rather keep the fallback global, please reply on that thread with the reasoning.
— Genesis
There was a problem hiding this comment.
Verdict: APPROVE — 2 resolved, 0 open, 0 new · 63917f64 → 7b5892be. The copy fallback is now opt-in (copyFallback) and used only at the three audio scratch renames. A failed copy in enforceAacTruePeak returns success: false, as before. Leftovers are scoped to each run with AsyncLocalStorage. Every run on this head is green. (b7d8f1bd failed format; this head only drops a blank line.)
— Genesis
What was wrong
On Windows,
renderwith audio failed withAudio mix failed; output would be video-only: EPERM: operation not permitted(0.8.198 and 0.8.199 reports; people got a render only after pointing TEMP/TMP at a folder they could write to). The mix writes its pieces into scratch folders under the system temp folder and tidies them up infinallyblocks, with a plainrmSyncorrenameSync. Where the temp folder refuses deletes (a locked-down or protected TEMP), that tidy-up throws, and the throw replaces a mix that had already worked.I reproduced it in CI on a Windows runner with the released 0.8.204: TEMP set to a folder that denies delete on its children (
icacls /deny (OI)(CI)(DE,DC)), one project with an audio track. It fails withEPERM: operation not permitted, unlink. With the first half of this change the failure moves toEPERM: operation not permitted, rename, which is the reported message; with all of it, the same render finishes with a video and an audio stream.Fix
removeScratch(engine, new, 11 lines) removes a scratch file or folder and leaves it behind if the system refuses. The mix's scratch folders, the PCM source the true-peak guard reads, the AAC encode folder, the audio effect host folder and the pad/trim scratch all use it, so no tidy-up can fail a finished mix.renameWithRetry/renameWithRetryAsynctake{ copyFallback: true }: after the existing retries are spent on Windows, copy the file to the target and try to remove the source, instead of failing. It is off by default, so the atomic publishers (the output file, the baked volume WAV, the caches, the plan files) keep rename-or-throw. Only the three audio scratch renames opt in: the summed group track and the kept encoder input inaudioMixer, and the true-peak corrected file inaudioPadTrim. If the copy fails, the rename error is thrown unchanged.[audio] scratch left behindwith a count when a removal was refused. The count is per run (collectScratchLeftovers, an async-local collector around the stage), so concurrent renders do not see each other's leftovers and nothing accumulates outside a run.Design calls:
Why scratch is not moved under the render's work folder
Every audio scratch folder except one (the audio effect host page, now covered by
removeScratch) is already created inside the render's own work folder, next to the mixed audio. That work folder is itself created under the system temp folder on Windows and macOS (resolveRenderWorkDirPrefix), so a temp folder that denies delete denies the work folder's cleanup too: moving the scratch changes nothing there. Where a removal is refused,removeScratchnow records the path and the audio stage logs[audio] scratch left behindwith the count once per render, so a leak is visible instead of silent. Whether the render's work folder should live somewhere else on Windows is a separate decision and not part of this PR.Test
removeScratch.test.ts: removes a folder and a file, and returns normally when the removal throws.renameWithRetry.test.ts: withcopyFallback, after the retries are spent on win32 it copies, sync and async; if the copy fails it throws the rename error; without it, a refused rename throws and nothing is copied.removeScratch.test.ts: a copy that cannot clear its source leaves it, and the leftover goes to the run that made it only (two runs side by side, one removal outside any run).windows-latest, TEMP denying delete on its children, one project with an audio track; throwaway workflow, not in this PR):EPERM ... unlink: https://github.com/heygen-com/hyperframes/actions/runs/40382570717EPERM ... rename(the reported message): https://github.com/heygen-com/hyperframes/actions/runs/40387489758What I did NOT exercise
A real user machine and the exact reported environment (the reporters' TEMP restriction could differ from an ACL that denies delete). The CI check shows the delete-denied case only; other causes of the same rename error (a held file that never clears) are covered by the copy fallback in principle, not by a run.
Why under 100 lines
About 40 lines of source and 50 of tests; the rest is the two import lines of each touched file. It does not fold into other open PRs: it shares no file with them.