Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
An interrupted cloud download can report
ENOTEMPTYinstead of its network error and leave a staging directory behind. On Node 25.9.0, an async-generator source can rejectpipeline()while the destination WriteStream is still opening. Thefinallyblock then removes the staging directory concurrently with that open creating the staged file.Why
The staged output must be closed before its directory can be removed. Preserving the old destination is not sufficient if cleanup hides the actual failure or leaves partial artifacts.
How
Register a close promise before starting the pipeline and await it before removing the staging directory. This preserves the atomic replacement behavior introduced by #3840, including the old destination and caller-provided cancellation reason. No dependency or public API changes.
Test plan
Validation on this isolated download-only branch:
bunx vitest run src/cloud/download.test.tsfrompackages/cli: 12 tests passed. The regression uses a manually delayed file-open callback and checks completion ordering, the original error, preserved output, and absence of staging residue.bunx oxlint packages/cli/src/cloud/download.ts packages/cli/src/cloud/download.test.ts: 0 warnings and 0 errors.bunx oxfmt --check packages/cli/src/cloud/download.ts packages/cli/src/cloud/download.test.ts: both files passed.git diff --check.fallow audit --base origin/main --fail-on-issues(no issues in the 2 changed files), type checks for core, Studio, and scripts, and commitlint. The hook type checks used physical copies of build artifacts from unchanged base packages; no CLI or producer artifacts were copied.Earlier reproduction on macOS arm64 / Node 25.9.0 established that the delayed-open regression fails on the original implementation and passes with this change. A separate 100-call interrupted-stream run produced 19
ENOTEMPTYerrors before the fix, with cleanup starting before close in all 100 calls. After the fix, all 100 calls preserved the network error and none cleaned up before close. The previous output survived all calls in both runs. These are the earlier reproduction results, not a new stress run on this isolated branch.Earlier validation with this fix combined with the separate init-audio and GIF-loop patches passed the full CLI suite (3,213 tests; 3 skipped), CLI build/typecheck, and repository lint/format checks. Those broader checks were not rerun on this isolated download-only branch.
Node 22 and other operating systems were not exercised in the reproduction; it does not establish an identical failure rate on those runtimes. The earlier full workspace test run had separate baseline/environment failures: an existing producer AAC loudness assertion and Player/Studio tests conflicting with Node 25's native Web Storage. The AAC assertion also failed with the base commit's original implementation; disabling native Web Storage passed all 370 Player tests and the 120 tests in the affected Studio files.
Additional verification of the final combined state: after confirming that the original audit checkout contained exactly the three reviewed patches on
f6041d7597c8c53d381d4c27feb86b305b3317a4,HYPERFRAMES_NO_TELEMETRY=1 bun run buildandHYPERFRAMES_NO_TELEMETRY=1 bun run --filter '*' typecheckwere rerun and both passed. Before/after source and diff hashes are identical. These new whole-workspace results cover the final three-fix combination; this standalone branch's own focused tests and hook checks remain documented separately above.Current-upstream integration check: the three reviewed patches were also applied to a separate checkout of
a0a6244be7a40956c5343f5846609e584ee5c6b5. With independent dependencies and a freshly rebuilt current Core runtime, 388 focused tests passed: audio route/transport (97), capture classification (19), render orchestration (222), GIF preparation/compiler (10), and init/download (40). A real local Chrome/FFmpeg smoke check reused the init-generated audio element in a minimal two-second scene with a finite-loop GIF anddata-loop="false"; output was 160×90 H.264/AAC, 10 frames, 2.000 seconds, with one GIF playback and a 1.6-second tail hold. The screenshot retry branches were checked by unit tests, not an injected browser failure. Source/diff hashes remained unchanged. This verifies the combined patches against that upstream SHA; the independent commit and its original base remain unchanged.