fix(cloud): preserve existing output when downloads fail

[jrusso1020]

A failed cloud render download could truncate an existing --output file and then delete it during cleanup. This was reproduced during the independent review of CodeQL #418.

Download into a unique private staging directory beside the destination, verify the declared length, wait for all writes to finish, then atomically replace the output. Failure or cancellation removes only the staged bytes. Node's pipeline propagates stream and flush errors; caller-provided cancellation errors are retained. Successful overwrite, progress callbacks, existing file permissions, and writes through existing or dangling output symlinks remain supported.

Validation: 11 focused download tests pass; three new failure regressions fail on the original implementation (truncated body, cancellation during progress, interrupted response). Tests also cover successful replacement, permissions, symlinks and staging cleanup. Full CLI suite: 3,171 passed / 3 skipped. Full workspace build, CLI typecheck, lint/format and Fallow pass. The final cancellation-reason adjustment also passes the focused suite.

The URL remains the authenticated cloud API's render result; this change addresses destination publication rather than changing the supported download capability. CodeQL may still flag the intentional response-to-file flow; no alert disposition will be claimed without independent post-fix review. Atomic replacement changes the destination inode; existing hard links keep their prior inode contents, and the existing trusted-parent-directory assumption remains.

[miga-heygen]

Reviewed at head 68c9ae68 (1 commit on 962c9540/main; 2 files, +146/−93). No prior reviews on this PR.

This closes the defect from the CodeQL #418 triage: the old createWriteStream(destPath) truncated an existing --output before the first byte arrived and unlinkSync removed it on failure. Now the destination is only touched by renameSync after a complete, length-verified, fully flushed staged file.

Strengths

• download.ts:71 — the stage is mkdtemp'd in dirname(destination) after symlink resolution, so the rename is same-directory (same filesystem, atomic; no EXDEV). The temp dir is 0700 and unique per call, so two concurrent downloads to the same output can't collide; last rename wins with a complete file either way.
• download.ts:75-91 — pipeline resolves only after the write stream has emitted close (probed on Node 22.22: closed=true, fd=null before renameSync), so the fd is released before the rename. That matters on Windows, where renaming over a file with an open handle fails. Late write errors (ENOSPC on flush) now reject instead of being swallowed by the old closeFile.
• download.ts:92-97 — abort ordering: pipeline({signal}) tears down both streams, throwIfAborted() covers an abort landing between flush and rename, and the caller's own reason is re-thrown in place of Node's AbortError. Removing the re-throw fails preserves the previous output when cancelled during progress.
• download.ts:106-113 — writing through a caller-selected symlink (existing or dangling) is preserved and tested, rather than replacing the link with a regular file; the 40-hop cap turns a link loop into a clear error instead of ELOOP deep in mkdtemp.

Mutation check (download.test.ts, 11 tests)

mutant	result
write straight to destination (old shape)	4 red incl. both preservation tests
drop content-length check	2 red
drop chmodSync	mode test red
drop rmSync(stage)	2 red (readdirSync(dir) asserts)
drop abort-reason re-throw	cancelled during progress red
don't follow symlinks	symlink test red
drop throwIfAborted() after pipeline	survives — race window between flush and rename, defensible without a test
wx → w on the staged file	survives — equivalent, the stage dir is private and unique
stage in os.tmpdir() instead of beside the destination	survives — see nit below

Non-blocking

• nit download.ts:71 — "staged beside the destination" is the property that makes the rename atomic, and no test pins it (the tmpdir mutant survives because the test dir is under tmpdir). An onProgress callback that asserts readdirSync(dirname(dest)) contains one .hf-download-* entry would pin it cheaply.
• nit download.ts:93-94 — behaviour shifts worth a line in the docblock: (a) a read-only destination (0o444) was previously unwritable (EACCES on open) and is now replaced via the directory, then chmod'd back to 0o444; (b) owner/group of the previous file are not carried over; (c) the destination's directory must now be writable even when the file itself was, since the stage lives there. All consistent with the stated trusted-parent-directory assumption; (d) hard links keep the old inode, as the PR body already says.
• nit download.ts:100 — leftover .hf-download-* directories are only possible on SIGKILL/power loss (handled aborts clean up). Fine for a CLI; a sweep of stale .hf-download-* beside the destination on the next run would be the belt-and-braces version.
• nit — no fsync before renameSync. Not needed for this defect (the old file is never truncated), but for crash-durability of the new file on non-ext4 filesystems the usual atomic-replace recipe fsyncs the staged fd first.
• nit commands/cloud/render.ts:855 — the comment still says downloadToFile "cleans up the partial file on error"; now it preserves the existing output. One-line update.
• Length is verified against content-length only; the presigned S3 response carries no usable content checksum for multipart objects, so that is the right stopping point.

CI at this head: every required check is green except Tests on windows-latest, which failed on two tests this PR does not touch — src/capture/contactSheet.test.ts > writes PNG output when the output path uses a .png extension (60s timeout) and src/commands/init.test.ts > requires an explicit source in non-interactive mode (expected -1 to be 1). Neither file imports cloud/download; src/cloud/download.test.ts itself passed on the Windows runner (11 tests, symlink case skipped by design). The same lane was green on main at 962c9540 twenty minutes earlier, so this reads as runner flake, but a required gate is red and I don't approve over one. Local: 11/11 focused tests at this head.

Verdict: COMMENT (Ready — will approve on a green Windows rerun at this head)
Reasoning: The staged-then-rename shape is correct end to end — same-directory stage, flush-then-close before rename, abort/flush/rename ordering all pinned by tests that go red when mutated — and the one surviving non-equivalent mutant is a test-coverage nit, not a defect. The only thing between this and APPROVE is the required Windows lane, red on two unrelated tests.

— Miga (pr-review)

[miga-heygen]

Reviewed at head 3aa62ef8 (2 commits on 962c9540/main). Interdiff from 68c9ae68 (my COMMENT review above, which stands as the full read): download.test.ts:145-148 asserts exactly one private .hf-download-* stage exists beside the destination mid-transfer; download.ts:47-49 documents the parent-writable / new-inode / mode-kept / owner-group-not / hard-link / read-only semantics; render.ts:855 comment refreshed. No runtime change.

Re-verified at this head

• 11/11 focused tests locally.
• The stage-location mutant (mkdtemp in os.tmpdir() instead of dirname(destination)) that survived at 68c9ae68 now fails keeps the old output visible until successful replacement and preserves its mode. The only real coverage gap from the previous review is closed.
• All required checks green at this head, including Tests on windows-latest; the previous head's Windows retry also passed, confirming the earlier red was runner flake on unrelated tests.

Stale-temp sweeping and fsync remain optional, as marked before.

Verdict: APPROVE
Reasoning: Correct staged-then-atomic-replace fix for the --output data-loss defect, with flush/abort/rename ordering, symlink write-through, mode retention and now stage placement all pinned by tests that go red under mutation; CI green at the exact head.

— Miga (pr-review)