From 88ff31e907cb16faf1f4084d975b5199cc1797af Mon Sep 17 00:00:00 2001 From: Adarsh Date: Sun, 6 Sep 2026 01:26:37 +0530 Subject: [PATCH 1/2] fix(desktop): keep the previous artifact destination when its replacement fails app:saveArtifactAs removed the destination file before renaming the staging file into place, so a rename that failed after a successful unlink deleted a file the user already had: the destination was gone and the save reported write_failed (#4832, reproduced with injected EIO on the final rename). rename(2) replaces an existing destination atomically on every platform, so the unlink was both unnecessary and destructive. The replacement now runs directly against the staged file: a failed rename leaves the previous destination byte-identical, and the staging file is cleaned up by the existing catch. materializeArtifact is exported with an injected replacement step so the fault-injection test can fail the final replacement deterministically on every platform. --- .../runtime-host-artifacts-ipc-main.test.ts | 92 ++++++++++++++++++- .../main/runtime-host-artifacts-ipc-main.ts | 20 +++- 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts b/apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts index 77f5186af0..63daa434c1 100644 --- a/apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts +++ b/apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts @@ -23,7 +23,7 @@ import { syncBuiltinESMExports } from "node:module"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { registerRuntimeHostArtifactsIpc } from "../runtime-host-artifacts-ipc-main.js"; +import { materializeArtifact, registerRuntimeHostArtifactsIpc } from "../runtime-host-artifacts-ipc-main.js"; type Handler = (event: unknown, ...args: any[]) => unknown; type StreamArtifact = ( @@ -279,3 +279,93 @@ test("Attachment byte IPC stops a stream that exceeds its preview admission", as { ok: false, reason: "too_large" }, ); }); + +test("A failed final replacement leaves the previous destination intact", async () => { + const root = await mkdtemp(join(tmpdir(), "maka-host-artifact-ipc-")); + const savedPath = join(root, "saved.bin"); + await writeFile(savedPath, "ORIGINAL"); + const content = Buffer.from("REPLACEMENT"); + const client = { + async streamArtifact( + _sessionId: string, + _artifactId: string, + writeChunk: (chunk: Uint8Array) => Promise, + ) { + await writeChunk(content); + return content.byteLength; + }, + }; + + try { + await assert.rejects( + materializeArtifact( + client as never, + "session-1", + "artifact-1", + savedPath, + content.byteLength, + async () => { + throw Object.assign(new Error("injected EIO"), { code: "EIO" }); + }, + ), + ); + assert.equal(await readFile(savedPath, "utf8"), "ORIGINAL"); + assert.deepEqual(await readdir(root), ["saved.bin"]); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("app:saveArtifactAs replaces an existing destination on success", async () => { + const root = await mkdtemp(join(tmpdir(), "maka-host-artifact-ipc-")); + const savedPath = join(root, "saved.bin"); + await writeFile(savedPath, "ORIGINAL"); + const content = Buffer.from("REPLACEMENT"); + const handlers = new Map(); + const artifact = { + id: "artifact-1", + sessionId: "session-1", + turnId: "turn-1", + createdAt: 1, + name: "result.bin", + kind: "image", + sizeBytes: content.byteLength, + mimeType: "image/png", + status: "live", + } as const; + + try { + registerRuntimeHostArtifactsIpc({ + uiLocale: () => "zh-CN" as const, + ipcMain: { + handle: (channel, handler) => handlers.set(channel, handler as Handler), + }, + client: { + hostEpoch: "host-1", + async getArtifact() { + return artifact; + }, + async streamArtifact( + _sessionId: string, + _artifactId: string, + writeChunk: (chunk: Uint8Array) => Promise, + ) { + await writeChunk(content); + return content.byteLength; + }, + } as never, + mainWindowController: { + showSaveDialog: async () => ({ canceled: false, filePath: savedPath }), + } as never, + showItemInFolder() {}, + }); + + assert.deepEqual( + await handlers.get("app:saveArtifactAs")?.({}, "session-1", "artifact-1"), + { ok: true, saved: "result.bin" }, + ); + assert.equal(await readFile(savedPath, "utf8"), "REPLACEMENT"); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts b/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts index 6b136f81ba..bbe7bd9d8c 100644 --- a/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts +++ b/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts @@ -185,12 +185,25 @@ export function registerRuntimeHostAttachmentPreviewIpc( ); } -async function materializeArtifact( +/** + * Streams the artifact to a staging file beside the destination, then + * replaces the destination in one atomic step. Exported for the + * fault-injection test of #4832: `replaceDestination` lets a test fail the + * final replacement to prove the previous destination survives; production + * always uses `rename`, which replaces an existing destination atomically on + * every platform (no unlink first — that would lose the destination if the + * rename failed). + */ +export async function materializeArtifact( client: DesktopRuntimeHostClient, sessionId: string, artifactId: string, targetPath: string, expectedBytes: number, + replaceDestination: (stagingPath: string, targetPath: string) => Promise = + async (stagingPath, targetPath) => { + await rename(stagingPath, targetPath); + }, ): Promise { await mkdir(dirname(targetPath), { recursive: true }); const stagingPath = join( @@ -228,8 +241,11 @@ async function materializeArtifact( } await handle.sync(); await handle.close(); + // rename(2) replaces an existing destination in one atomic step on every + // platform; unlinking the destination first would turn any rename + // failure into a lost destination (#4832). try { - await rename(stagingPath, targetPath); + await replaceDestination(stagingPath, targetPath); } catch (error) { throw new ArtifactMaterializationError("replace_failed", error); } From f18b7ce2743ae0a61de35c00fcf917ac2bd34fcb Mon Sep 17 00:00:00 2001 From: Adarsh Date: Fri, 11 Sep 2026 23:04:57 +0530 Subject: [PATCH 2/2] fix(desktop): fsync the artifact directory after a successful rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review P3 on #4875: handle.sync() covers the staging file's content but not the rename's directory entry, so a crash could leave the old destination content plus a leftover staging file. The same best-effort syncDirectory tier the other desktop write paths use now runs after the replacement lands (skipped on Windows, where the directory handle cannot be opened this way and the rename already persists the entry). The materializeArtifact docs also state what the fault-injection test does and does not cover: both production call sites pass nothing, so the default is rename, and the injected failure deliberately does not cover the default path — it is asserted on success only. --- .../main/runtime-host-artifacts-ipc-main.ts | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts b/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts index bbe7bd9d8c..69688c66fc 100644 --- a/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts +++ b/apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts @@ -192,7 +192,11 @@ export function registerRuntimeHostAttachmentPreviewIpc( * final replacement to prove the previous destination survives; production * always uses `rename`, which replaces an existing destination atomically on * every platform (no unlink first — that would lose the destination if the - * rename failed). + * rename failed). Both production call sites pass nothing, so the default + * is `rename`; the injected failure does not cover the default path itself, + * and making a real `rename` fail portably without flakiness is hard, so + * the default path is only asserted on success (the save-dialog happy-path + * test goes through it). */ export async function materializeArtifact( client: DesktopRuntimeHostClient, @@ -249,6 +253,11 @@ export async function materializeArtifact( } catch (error) { throw new ArtifactMaterializationError("replace_failed", error); } + // The staging file's content is synced, but the rename's directory + // entry is not: after a crash some filesystems can show the old + // destination content plus a leftover staging file. Best-effort, since + // the user's file is already saved once the rename landed. + await syncDirectory(dirname(targetPath)).catch(() => undefined); } catch (error) { await handle.close().catch(() => undefined); await rm(stagingPath, { force: true }).catch(() => undefined); @@ -272,6 +281,20 @@ class ArtifactMaterializationError extends Error { } } +// Same durability tier as the other desktop write paths: a directory +// fsync after a rename so the new directory entry survives a crash. +// Windows cannot open a directory handle this way, and its rename +// already persists the entry, so it is skipped there. +async function syncDirectory(path: string): Promise { + if (process.platform === 'win32') return; + const handle = await open(path, 'r'); + try { + await handle.sync(); + } finally { + await handle.close(); + } +} + const ARTIFACT_DIALOG_COPY = { 'zh-CN': { saveAs: (name: string) => `另存为 ${name}` }, 'zh-TW': { saveAs: (name: string) => `另存為 ${name}` },