fix(desktop): keep the previous artifact destination when its replacement fails

apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts

@@ -18,11 +18,11 @@
  */
 
 import assert from "node:assert/strict";
-import { mkdtemp, readFile, rm } from "node:fs/promises";
+import { mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises";
 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 = (
@@ -190,3 +190,92 @@ 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<void>,
+    ) {
+      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<string, Handler>();
+  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({
+      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<void>,
+        ) {
+          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 });
+  }
+});

apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts

@@ -182,12 +182,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<void> =
+    async (stagingPath, targetPath) => {
+      await rename(stagingPath, targetPath);
+    },
 ): Promise<void> {
   await mkdir(dirname(targetPath), { recursive: true });
   const stagingPath = join(
@@ -220,8 +233,10 @@ async function materializeArtifact(
     }
     await handle.sync();
     await handle.close();
-    await rm(targetPath, { force: true });
-    await rename(stagingPath, targetPath);
+    // 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).
+    await replaceDestination(stagingPath, targetPath);
   } catch (error) {
     await handle.close().catch(() => undefined);
     await rm(stagingPath, { force: true }).catch(() => undefined);