fix(render): publish artifacts atomically

[jrusso1020]

Summary

Prevent a failed or cancelled render from overwriting a valid existing output with a partial artifact.

How it works

• Reserve a private transaction directory beside the final destination so staging is collision-safe and stays on the same filesystem.
• Render into a staging path inside that directory.
• Verify the opened staged file or PNG-sequence directory is readable and non-empty.
• Publish it to the requested output path only after the render pipeline succeeds.
• On failure or cancellation, remove the transaction directory and leave the previous destination untouched.

File outputs such as MP4, WebM, MOV, and GIF are replaced with a single rename, so readers see either the complete old file or the complete new file. A newly created PNG-sequence directory also publishes with one rename. Replacing an existing non-empty PNG-sequence directory uses backup and restore; that path is recoverable from ordinary errors but is not atomic or crash-safe.

The private, atomically reserved staging directory and descriptor-based file validation also avoid predictable temporary-file and path-check/file-open races.

Why

The renderer previously wrote directly to the caller-visible output path. If encoding, validation, or cancellation failed partway through, callers could be left with a partial artifact instead of the last known-good output.

Validation

• File replacement, validation failure, injected promotion failure, and cancellation rollback tests
• Private transaction-directory permissions and cleanup tests
• PNG-sequence publication and empty-directory rejection tests
• Full Producer unit suite
• Producer typecheck and repository lint/format/typecheck hooks
• Required CI on the current post-merge restack

[jrusso1020]

• refactor(repo): resolve changed-code audit #2174
• test(engine): isolate peer abort regression #2173
• refactor(cli): decompose render phases #2172
• refactor(cli): centralize process lifecycle #2171
• fix(cloudrun): publish adapter contract #2170
• test(repo): execute packed subpaths #2169
• refactor(repo): centralize package subpaths #2168
• refactor(core): own edit protocol contract #2167
• test(compiler): enforce preview render parity #2166
• refactor(studio): gate SDK operation families #2165
• refactor(studio): gate Acorn writer migration #2164
• refactor(producer): make capture plans immutable #2163
• refactor(producer): unify render requests #2162
• refactor(engine): type capture failures #2161
• refactor(engine): manage child process lifecycles #2160
• refactor(engine): add fingerprinted browser leases #2159
• refactor(render): centralize execution ownership #2158
• refactor(core): unify composition contract #2157
• fix(studio): enforce optimistic file concurrency #2156
• fix(studio): make sdk cutover transactional #2155
• fix(render): publish artifacts atomically #2154 👈 (View in Graphite)
• fix(render): surface structured outcomes #2153
• fix(player): own connection and media resources #2152
• fix(core): preserve runtime transport contract #2151
• fix(player): version runtime protocol #2150
• test(repo): verify packed consumers #2149
• test(repo): enforce workspace contracts #2148
• fix(core): enforce strict runtime safety #2147
• test(producer): gate source tests by execution lane #2146
• fix(core): make timing masks safe for source loaders #2145
• chore(repo): forbid tracked generated artifacts #2144
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Independent top-layer review on bb006241 (layer diff against #2153).

Staging beside the destination and validating before promotion correctly protects against encode/validation/cancellation failures (packages/producer/src/services/renderOrchestrator.ts:1532, :2950, :3040).

Blocker — packages/producer/src/services/render/artifactTransaction.ts:89: replacing an existing artifact is a two-syscall destination → backup, then staging → destination sequence (:90-92). Between those calls the destination does not exist, so concurrent readers can observe ENOENT; a process crash in that window leaves only the hidden backup. That is not atomic publication, which is the core contract of this PR. Use a replacement/recovery design that keeps a valid destination continuously addressable, and add a test/failure-injection case covering the handoff window rather than only final end states.

The layer checks are green aside from pending Graphite mergeability. This finding is independent of the parent #2153 verdict.

Verdict: REQUEST CHANGES
Reasoning: Validation and rollback are improved, but the promotion algorithm still exposes an absent-artifact window and cannot provide the advertised atomicity guarantee.

— Home

[james-russo-rames-d-jusso]

Reviewed at bb00624.

Clean, focused, well-scoped. Staging beside the destination on the same filesystem for atomic-rename semantics is the right shape, and assertReadableNonEmptyFile (non-empty, non-symlink, readable via 1-byte probe) is a thorough gate before promotion. The interaction with #2153's RenderQualityError throw is correct: the outer catch calls artifactTransaction.rollback() first via safeCleanup, so a strict-mode media failure never leaves a staged artifact orphaned. State machine is idempotent — rollback short-circuits when state isn't active, and commit's own inner recovery on second-rename failure stays in active so a follow-on rollback becomes a no-op restore.

Nits

• commit() embeds an inline backup-restore path (rmSync(destinationPath, ...) + renameSync(backupPath, destinationPath) on second-rename failure) that partially mirrors rollback()'s restore path with different conditions. Fine to keep for intent-clarity; a shared _restoreBackup() helper would trim it slightly. Non-blocking.

What I didn't verify

• The failed-second-rename recovery path — coverable only via a mocked filesystem (e.g. spying renameSync to throw once). Not exercised by the test suite; the logic reads correctly but is unverified in CI.
• Concurrent renders to the same outputPath: each transaction gets UUID-suffixed staging + backup, so intermediates are always valid single artifacts, but two racing commits pick a nondeterministic winner. Only a concern if the caller assumed linearization.

— Review by Rames D Jusso

[vanceingalls]

R1 by Via

PR #2154 — fix(render): publish artifacts atomically
Verdict: 🔴 BLOCK — sustains Miguel's atomicity finding + adds resource-leak concern
Head: bb006241 on stack-base #2153@050fad62
Status: mergeable=unstable (Graphite check in-progress); CI green (regression + player-perf + preview-regression all green); prior reviews — miguel-heygen CHANGES_REQUESTED @ 20:07Z, james-russo-rames-d-jusso COMMENTED @ 20:11Z.

Summary of change
Introduces ArtifactTransaction (file/directory) that stages beside the destination, validates the staged artifact (lstat non-symlink non-empty + 1-byte fd probe; recursive for dirs), then commits via dest→backup + staging→dest two-rename sequence with inner backup-restore on second-rename failure. renderOrchestrator.executeRenderJob writes encode/assemble stages to stagedOutputPath, validates, does debug-copy + workDir cleanup, then commit() — then flips job.outputPath and status=complete. Catch path calls rollback() via safeCleanup.

Findings

🔴 [Sustain Miguel] Two-syscall publish is not atomic — violates the PR's own thesis — packages/producer/src/services/render/artifactTransaction.ts:89-92. commit() does renameSync(dest, backup) THEN renameSync(staging, dest). Between them the destination doesn't exist. Concurrent readers see ENOENT; a SIGKILL/host-crash/ENOSPC in the window leaves the caller-visible artifact gone and only a hidden .stem.hf-backup-<uuid> file remains, with no GC to find it. PR body says "atomically promote" — this delivers "validated, then near-atomic promote with a race window." Refute-attempt: with single-writer per-job semantics the concurrent-reader case is theoretical, and the dest-already-exists case only fires on retries (where losing the prior artifact is arguably acceptable). But the stated contract fails on both axes, and Node has fs.rename(staging, dest) as a single atomic call for FILES on both POSIX and Windows (Windows MoveFileEx with MOVEFILE_REPLACE_EXISTING). File path could be one-syscall; only the directory case genuinely needs a workaround (renameat2 RENAME_EXCHANGE Linux-only, or symlink-swap). Right fix: single-rename for files; document the directory-case tradeoff.

🟡 Orphaned staging/backup on abnormal exit — no GC — artifactTransaction.ts:106-112. rollback() only runs from the outer catch. SIGKILL, container-OOM, ENOSPC after staging bytes but before catch (e.g. inside runEncodeStage's own writers), power loss — all leave .stem.hf-staging-<uuid>{.ext} (and possibly .hf-backup-<uuid> if second-rename failed and inner recovery aborted) permanently in the output dir. No pruning at start-up, no age-based cleanup. Refute-attempt: if the parent dir is per-job ephemeral, orphans die with it. But dirname(outputPath) is the caller's output dir (typically shared across jobs, based on PRODUCER_RENDERS_DIR), not workDir — orphans accumulate. Suggest a startup sweep of .*.hf-{staging,backup}-* older than N hours in the same directory the transaction targets.

🟡 Double validation on happy path — orchestrator calls artifactTransaction.validate() at renderOrchestrator.ts:2950, then commit() at :3035 internally calls validate() again (artifactTransaction.ts:65). For png-sequence with thousands of frames, that's 2× lstat+open+read+close per frame. Non-blocking, but either drop the explicit call at :2950 or add a skipValidate flag / _validated guard.

⚪ Debug-copy from staging before commit — renderOrchestrator.ts:3019-3021 copies stagedOutputPath to workDir/output${videoExt} in debug mode. If the subsequent commit() fails and we roll back, the debug copy still lives in workDir. Probably fine (debug-only, workDir is inspection-mode by definition), noting for completeness.

⚪ "completed" checkpoint tag dropped — renderOrchestrator.ts:2955. Old code emitted observability.checkpoint("pipeline", "completed", ...); new code emits "artifact validated". Free-form string, not an enum, so no compile-time break — but Datadog dashboards / log-search alerts filtering on pipeline: completed will silently lose signal. Worth a heads-up + follow-up query update if any dashboards depend on it.

Peer-lens acknowledgment
Miguel's block already covers the primary atomicity thesis. Rames' code-clean LGTM-with-nits (state-machine idempotency, shared _restoreBackup() helper suggestion, uncovered second-rename-failure path, concurrent-writer nondeterminism) is accurate; my BLOCK sustains Miguel and adds the orphan-cleanup + double-validate not covered elsewhere.

Cross-checks

• Head SHA current: yes (bb00624189eac897a335eb69628b1d12b400f144)
• Prior reviewers: miguel-heygen (BLOCK), james-russo-rames-d-jusso (COMMENTED, effective LGTM w/ nits)
• Mergeability: unstable (Graphite mergeability_check in_progress; substantive CI green)
• POSIX vs Windows rename semantics handled: partial — Node renameSync normalizes across platforms (Windows uses MoveFileEx with MOVEFILE_REPLACE_EXISTING), so the 2-step is cross-platform-consistent but suboptimal for files (single-rename would be truly atomic on both). Directory-case is genuinely hard on either OS.
• Every publish site uses new atomic path: yes — grep of outputPath|stagedOutputPath in renderOrchestrator.ts at head confirms encode (:2880), assemble (:2938), debug-copy (:3019-3021) all read/write through stagedOutputPath; job.outputPath = outputPath (the public field) only assigned after commit().
• Test coverage: real (imports ./artifactTransaction.js, not aspirational). Covers 5 happy/rollback paths for file + directory. Missing: second-rename-failure recovery (Rames noted), concurrent-writer race, abnormal-exit orphan behavior.

R1 by Via

[jrusso1020]

Valid — addressed on the current head.\n\n- Existing file artifacts now publish with a single replacement rename. We no longer move the old file away first, so readers see either the complete old file or the complete new file; a failed rename leaves the old file untouched.\n- Added injected-filesystem coverage that observes the old destination during the sole replacement call, forces that call to fail, and verifies the destination stays byte-identical and addressable.\n- Documented the real portability boundary: Node's portable filesystem API cannot atomically replace an already-existing non-empty PNG-sequence directory. That path retains recoverable backup handoff semantics instead of claiming the same one-syscall guarantee as files.\n\nValidation: 6 ArtifactTransaction tests; Producer typecheck; repository lint/format/typecheck hooks; and real Chrome/FFmpeg render runs. The MP4 path passed 100/100 visual checkpoints on both this layer and the combined stack head. The PNG-sequence render completed and published all 60 frames locally; its macOS byte-golden comparison differs as the fixture docs predict for non-Docker alpha output, so the authoritative Linux/Docker regression remains fresh CI.

[miguel-heygen]

Re-reviewed current head aefb7fab46ad089a1a7e2be9d5f1f414851d09ae.

The layer-specific blocker from my prior review is addressed: file artifacts now publish with a single replacement rename, the failure-path test proves the old destination remains addressable, and the PR text/code explicitly narrow existing non-empty directory replacement to a recoverable (not atomic/crash-safe) handoff. I found no new blocker in this three-file layer.

I cannot approve the stack yet because required regression checks on this head are failing from the unresolved parent #2153 strict-by-default rollout/regressions. This review should become approval once the parent blocker is fixed and this head is rerun green.

Verdict: COMMENT (layer ready; stack blocked)
Reasoning: The artifact-transaction issue is fixed, but required inherited CI is red and the parent PR remains blocked.
— Magi

[jrusso1020]

Follow-up: the parent #2153 is now fully green on the compatibility-preserving head, and this #2154 head has completed all required GitHub CI and regression shards successfully. The only remaining pending status is Graphite mergeability. Miguel, the conditions from your layer-ready follow-up are now satisfied; please update the stale request-changes review when convenient.

[miguel-heygen]

Re-reviewed current head 8fc6645c30dfea24618b7b34bd4d5bf15419c542 after the parent rewrite and full CI rerun.

The layer remains a narrow three-file artifact-publication change. File outputs validate in sibling staging and replace the destination with one rename; injected promotion failure leaves the old file continuously addressable. Existing non-empty directory replacement is explicitly documented as a recoverable, non-atomic handoff rather than overclaiming crash-safe visibility, and validation/cancellation paths preserve the prior destination. The orchestrator publishes completion only after commit and rolls staging back on every error path.

Focused artifact-transaction verification passed 6/6 tests. The inherited parent is now approved and green; this head has all Windows, preview, player, and eight regression shards green. The only live status is Graphite mergeability bookkeeping, not a test failure or code blocker.

Verdict: APPROVE
Reasoning: The original absent-destination window is removed for file artifacts, the directory limitation is accurately bounded, failure behavior is covered, and the complete code-bearing CI fleet is green on the resolved parent.

— Deepwork

The base branch was changed.

[miguel-heygen]

The current-head hardening is good: private transaction directories are atomically reserved with mkdtempSync, descriptor-based validation closes the prior TOCTOU window, every encode/assemble/debug producer writes through stagedOutputPath, and the focused artifact transaction suite passes 7/7. The original file-replacement blocker is resolved.

There is one remaining concurrency correctness blocker in the existing-directory path.

At packages/producer/src/services/render/artifactTransaction.ts:135-145, rollback can delete another transaction's successfully committed directory. A deterministic interleaving is:

1. T1 renames the old destination to T1's backup at line 135.
2. During that gap, T2 observes no destination, takes the one-rename branch at lines 117-127, publishes its complete staging directory, and returns committed.
3. T1's staging rename at line 137 then fails because T2's non-empty directory occupies the destination.
4. T1's catch sees the destination at lines 141-143, recursively deletes it, and restores the old backup at line 144.

T2 has already reported success, but its complete artifact is removed and the destination silently reverts to the pre-render version. The collision-safe staging directories do not protect the shared destination, and concurrent direct/CLI renders can target the same PNG-sequence path.

Please serialize directory commits per destination, or make recovery ownership-aware so it never removes a destination that appeared after this transaction's handoff. Add a deterministic two-transaction interleaving test; current directory tests cover happy promotion and validation failure, but not second-rename/concurrent recovery.

Freshness: reviewed exact head f65003bd561c9df7f7cb1a079f710eaf57390b74; all current checks are green and there are zero unresolved prior threads.

Verdict: REQUEST CHANGES
Reasoning: Concurrent directory commits can make one transaction delete another transaction's already-successful output; recovery must not remove a destination it does not own.

— Deepwork

[jrusso1020]

Valid

[jrusso1020]

Fixed on current head 1fd5fe7. Directory recovery no longer deletes a destination that appeared after this transaction moved the prior directory aside; it restores its backup only while the destination remains unclaimed, otherwise the concurrent publisher keeps ownership. Added deterministic tests for both ordinary second-rename failure recovery and Miguel’s exact two-transaction interleaving. Local validation: artifact transaction 9/9, full Producer unit lane green, Producer typecheck green, and repository lint/format green. Please re-review.