fix(desktop): preserve destination on artifact export failure - #4833
Conversation
bcaafd5 to
68ddf8e
Compare
|
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 0656abcb995774b8b81a0ddb593c761d90d8528b against current main (03b5a2cc5a76897b919e691ee4f242fa2e7c9e36).
The change replaces the destructive remove-then-rename export path with same-directory staging/backup replacement, adds classified save errors and localized copy, and seeds an artifact-export fixture.
Findings:
- P2: restoration failure is swallowed, so a failed replacement can still remove the original destination path.
- P3: backup cleanup failure is reported as a source transfer failure after the new destination is already committed.
Validation: full build:test; Desktop typecheck; focused artifact/workbar and fixture tests; direct fixture seed/read; renderer architecture (101/101); format, Biome, ASF headers, and diff checks. Exact compiled-module fault injection reproduced both findings. The full Desktop suite had 2,155 passes, 0 failures, and 8 unrelated MCP OAuth timer cancellations. Hosted test and package checks were still running on this exact head when this review was posted.
I did not run a native macOS/Windows interactive save flow locally.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } catch (error) { | ||
| if (destinationBackedUp) { | ||
| await rename(backupPath, targetPath).catch(() => undefined); | ||
| destinationBackedUp = false; |
There was a problem hiding this comment.
P2 — Preserve the original destination when restoration itself fails.
This restore error is swallowed and destinationBackedUp is cleared unconditionally. On this exact head I let the backup rename succeed, then made the destination directory non-writable before the staging install. Both the install and the real backup restore failed; the handler returned replace_failed, targetPath was ENOENT, and the only copy of ORIGINAL was the randomly named .bak. The renderer meanwhile says “The original file was kept.”
Keep the backup state until restoration succeeds, and surface a recovery path (or otherwise guarantee the original remains at targetPath). The checked-in artifact IPC test covers only the successful save, so this failure branch is currently unguarded.
| } | ||
| if (destinationBackedUp) { | ||
| destinationBackedUp = false; | ||
| await rm(backupPath, { force: true }); |
There was a problem hiding this comment.
P3 — Treat post-commit backup cleanup separately from source transfer.
At this point stagingPath has already been renamed into targetPath. If removing the backup fails (for example EBUSY on Windows), the cleared flag sends the error to the outer catch, which wraps it as source_failed. Exact-head fault injection returned source_failed while targetPath already contained NEW and the .bak still contained ORIGINAL. This tells the user the transfer failed even though the save committed and leaves an orphan backup. Make cleanup best-effort or report it separately without turning a completed save into a source failure.
|
Addressed the two review findings within #4832 scope. The export path now writes and fsyncs a same-directory staging file, then performs one rename to the destination. It never removes or renames the existing destination first, so a failed replacement leaves the original path untouched; this also removes the restore-failure and post-commit backup-cleanup states. No new error category or product behavior was added. Validation: |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head fb23f7408584defb2e50ab15bf19fadfa15de298.
The previous P2 and P3 are fixed: replacement now stages beside the destination and performs a single atomic overwrite rename without moving the original away first, and the backup/restore cleanup state machine has been removed.
I found one new non-blocking P3 inline: a staging-file sync() or close() failure is still reported as source_failed, even though the source stream completed and the destination write failed. This produces the wrong recovery message in the UI; it should be classified as target_write_failed.
Validation passed with Node 24.18.1 and npm 11.19.0: clean install, full build:test, Desktop typecheck, artifact IPC tests 3/3, Biome, ASF headers, git diff --check, hosted exact-head test and package, and a clean patch-preserving merge onto current main (12ebc45e6fe43b2eebd7dfe61f5c1fa92c0c873a). A deterministic injected sync() EIO preserved the original destination and removed staging, but returned source_failed. I did not run the native macOS/Windows interactive save path.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| await handle.close().catch(() => undefined); | ||
| await rm(stagingPath, { force: true }).catch(() => undefined); | ||
| if (!(error instanceof ArtifactMaterializationError)) { | ||
| throw new ArtifactMaterializationError(writingStaging ? "target_write_failed" : "source_failed", error); |
There was a problem hiding this comment.
[P3] Classify staging flush/close failures as destination-write failures
writingStaging is reset after the last chunk, before handle.sync() and handle.close(). If either operation fails, this catch therefore wraps the error as source_failed, even though the source stream completed and the failure occurred while persisting the destination staging file. The UI renders that as “The generated file transfer was interrupted” instead of “The destination is not writable.” A deterministic sync() EIO injection returns { ok: false, reason: "source_failed" } while preserving the original target and cleaning the temporary file. Please wrap the flush/close phase as target_write_failed (and add a focused failure-path regression).
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head fb23f7408584defb2e50ab15bf19fadfa15de298. The earlier replacement-safety P2 and backup-cleanup P3 are fixed, and I found no P0/P1. One P2 remains in the contribution record.
P2 — update the PR body to describe the shipped design and declare AI use
The current PR body still says the export path moves the destination into a backup, restores that backup after installation failure, and deletes it after success. The final commit removed that entire state machine. This head now writes and flushes a same-directory staging file, then performs one replacement rename(stagingPath, targetPath); if that rename fails, the original destination was never moved away. The later issue comment describes this correctly, but the canonical PR description and its review/validation claims still describe code that no longer exists.
Please update the Fix and Validation sections to document the single-rename design. Also complete the repository's required AI-use declaration: CONTRIBUTING.md says every PR must state whether generative tooling contributed substantively, and this body currently says neither. If it did, name the tool and add Generated-by trailers to each affected commit; otherwise state that no generative tool made a substantive contribution.
For the code path itself, I verified both sides on macOS: replacing an existing destination succeeds and installs NEW; making the existing destination immutable causes replace_failed, leaves its content as ORIGINAL, and leaves no staging artifact. The current-head sync()/close() classification issue already reported in the preceding review also remains; I did not duplicate that inline finding.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 7ba4cb5da5908c1e414681c36f8e792fd09ac560. The previous P3 is fixed: once streamArtifact completes, failures from staging-file sync() or close() are now classified as target_write_failed rather than source_failed. A deterministic sync() EIO injection returned the corrected result while preserving the original destination and removing the staging file.
I found no remaining P0-P3 issue in this increment. Desktop main and renderer typechecks, the focused artifact IPC suite (3/3), changed-file Biome, repository formatting, ASF header audit, diff check, exact-head hosted test and package checks, and a clean merge onto current main passed. The checked-in suite still lacks a regression for final flush failure, and I did not run native macOS or Windows interactive save behavior.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 7ba4cb5 against main (bc08195fd5), +78/-13, no test changes; two passes.
The problem is real and the fix is the right one: main runs rm(target) before rename(staging, target), so any rename failure after the rm leaves nothing at the destination. Dropping the rm is the whole fix; rename replaces an existing file atomically on POSIX and libuv passes MOVEFILE_REPLACE_EXISTING on Windows, so a failed rename leaves the original untouched, which is what #4832 asks for. The staging file is already same-directory, so there is no cross-device case. Worth saying in the body: the loss needs the rm to succeed and the rename to then fail, which is why the reproduction is fault injection; the immutable-file check in the review above fails the rm too, so it exercises the new path, not the old loss.
One P2, then smaller items.
P2: the new result contract has no test, and the fixture it ships instead has no consumer. The PR adds four ArtifactSaveFailureReason values and three-locale copy for them, but runtime-host-artifacts-ipc-main.test.ts still only covers the success path. The existing harness already has a fake client, so source_failed (make streamArtifact reject) and size_mismatch (return a different total) are two short cases that fail on main, which returns write_failed for both; replace_failed is reachable by pointing the save dialog at an existing non-empty directory, and the assertion that matters is that the directory is still there afterwards. The artifact-export fixture scenario, on the other hand, is 30 lines of main-process seeding that acquire the interactive root lease and write the artifact store, and E2eFixtureScenario's own comment says a scenario exists only when a current E2E, audit or smoke entry point consumes it; nothing does (apps/desktop/e2e, scripts/audit-alignment.mjs). Either add the E2E that drives Save As through it, or drop the scenario and keep the manual check as a note in the body.
Smaller:
app:openArtifactPathcalls the samematerializeArtifactand still flattens every failure toopen-failed(runtime-host-artifacts-ipc-main.ts:117-119), so the four new reasons reach Save As but not Show in Finder. Either map them there too or say in the body that only Save As is covered.size_mismatchcopy: en says the size changed during transfer, which is what the code checks (:246compares byte counts); zh-CN and zh-TW say 内容校验失败 / 內容校驗失敗, and there is no content check. Make the Chinese say the size changed.writingStaging/streamCompletedclassify a failure by when it happened rather than where it came from. I walked every throw site at this head and they all land on the right reason, so this is a readability note only: throwingArtifactMaterializationError('target_write_failed')from thehandle.writeloop and aroundsync/closewould say the same thing without the two flags.- Fixture session name is hard-coded zh-CN. Dev-only, so fine if the fixture stays, but it is the kind of string the locale ratchet will flag next time someone touches the file.
- Three of the four commits carry no
Generated-bytrailer while the body says Codex contributed. The squash body will carry the declaration; just make sure the trailer survives.
Manual acceptance: Save As over an existing file succeeds and replaces it; Save As into a read-only location shows the destination message; three locales for the four new sentences.
Evidence boundary: static read against main; no build, no suites, no Windows.
AI-assisted review: drafted with Maka in two passes; I verified the rm/rename path, every throw site in materializeArtifact, the test harness, the fixture consumers and the ArtifactSource union myself.
7ba4cb5 to
f71f516
Compare
|
Addressed the latest review points:
Local focused suite: 12/12 passed; build, Biome, and diff checks passed. |
290b9ef to
13d6e7e
Compare
|
Follow-up pushed as The branch now contains one affected commit (the earlier fixture-only commit was removed), with a valid |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 13d6e7e6e371e6444c336a0d8597aa4432644734.
The previous flush/close classification gap is now covered by a deterministic Save As fault matrix. The new tests exercise successful overwrite, source interruption, both byte-count mismatch paths, rename and directory replacement failures, and write/sync/close failures against real temporary files. Each failure checks the public result, preserves the original destination, and leaves no staging file. The Chinese size-mismatch copy now also describes the byte-count failure accurately, and the obsolete artifact-export fixture was removed after its Electron consumer moved below that layer.
I found no remaining P0-P3 issue in the current branch. Local validation passed: clean install, npm run build:test, Desktop 2172/2172, Core 824/824, focused artifact IPC 12/12, Desktop typecheck, changed-file Biome, repository format, ASF headers, and git diff --check.
This head is not merge-ready: GitHub reports CONFLICTING/DIRTY against current main 411512bd9cb698f668eccbae35adf41e71d7cf68, with conflicts in the Artifact save-result type, failure-copy mapping, and locale catalog. No hosted checks are present, so the resolved/rebased head needs another review and its own checks. I did not run native macOS or Windows interactive Save As behavior.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Resolved the PR conflict against the latest The resolution keeps upstream's newer artifact child-result behavior and applies the scoped Save As failure contract/tests on top. The focused artifact IPC suite remains 12/12 locally. A full workspace rebuild is currently blocked by unrelated upstream |
|
CI failure follow-up:
Pushed as |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 3735b8554ad724ee17188e56416fb82430b85076 against current main (ef2a724537188aaafc4e3a8f2340b137109e6c5f).
The conflict resolution preserves the current upstream artifact model while keeping the Save As change scoped. materializeArtifact() writes to a same-directory staging file, checks both reported and written byte counts, syncs and closes the file, and commits it with one replacement rename. The checked-in real-temporary-file matrix covers successful overwrite plus stream, byte-count, write, sync, close, rename, and directory-target failures; each failure preserves the existing destination and removes staging state.
I found no new P0-P3 issue in the current diff.
Local validation passed: clean install; build:test; Desktop typecheck and full tests (2220/2220); focused artifact IPC tests (12/12); Desktop renderer/Storybook builds; changed-file Biome; repository format, locale, ASF-header, and diff checks; and a clean, patch-preserving merge-tree onto current main. The hosted package check passed.
The required hosted test check is still red, so this head is not merge-ready yet. Its two observed attempts failed outside this PR's five-file artifact diff: first on the pre-existing Storybook project-groups focus scenario, which reproduced on both base and head under full-suite concurrency but passed 10/10 in isolation on each; then on skill-draft-lifecycle.spec.ts, whose source is byte-identical between base and head. The latter failing case passed 3/3 locally on this exact head after a full Desktop build. These results do not indicate a regression from this PR, but the required check still needs a green terminal result.
I did not run native macOS or Windows interactive Save As behavior.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
The latest failed |
3735b85 to
6f4a734
Compare
Generated-by: OpenAI Codex
6f4a734 to
5eff126
Compare
Generated-by: OpenAI Codex
|
Rebased onto the merged latest The previous rebased tree accidentally retained unrelated legacy artifact model fields ( Validation on the rebased tree:
The new CI run is queued. |
github-actions
Bot
added
effort/M
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 19e301539cc10c337274784900637c79b4b584a3. I found no new P0-P3 issues in this revision.
The scoped Save As implementation remains unchanged from the previously reviewed version: runtime-host-artifacts-ipc-main.ts:186-245 writes an exclusive same-directory staging file, verifies both byte counts, syncs and closes it, and then performs one replacement rename while classifying source, destination-write, size, and replacement failures. The real-temporary-file regression matrix at runtime-host-artifacts-ipc-main.test.ts:35-117 is also byte-identical and covers successful overwrite plus stream, count, write, sync, close, rename, and directory-target failures.
Relative to the prior reviewed head, the only substantive change restores current main compatibility: packages/core/src/artifacts.ts:196-205 retains the legacy write_failed result member, and artifact-pane.tsx:602-618 maps both that member and target_write_failed to the same destination-write message. This avoids narrowing the existing result contract while preserving the new detailed failure reasons.
Validation passed: clean install, build:test, focused artifact IPC 12/12, Desktop 2236/2236, Desktop typecheck, renderer and Storybook builds, renderer architecture 101/101 with the base-relative ratchet, Biome, repository format, locale hygiene, ASF headers, and git diff --check. The merge tree against current main a5a99a6337997b1928e07f943f1d1b7ce72baab5 is clean. Hosted test is green; hosted package is still running. I did not run the native macOS or Windows Save As flow.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
5 participants
Problem
Artifact export could remove an existing destination before replacement completed. If the final rename failed, the new file was not installed and the old destination was gone. Source-stream, size-validation, and final destination-write failures also lacked distinct user-facing reasons.
Fix
Validation
npm run build:testpassed.replace_failedfor a failed replacement andtarget_write_failedfor staging flush/close failures.git diff --checkand changed-file Biome checks passed.AI use
Generative tooling contributed substantively to this patch. The single affected commit carries
Generated-by: OpenAI Codex.Fixes #4832