fix(desktop): make gateway file saves failure-atomic so a failed download never destroys an existing file

`pumpStreamToFile` opened the user-chosen destination with
`fs.createWriteStream`, which truncates the target the instant it opens,
and its error path then unlinked that same path. When a user picked an
existing file in the Save dialog (and confirmed the overwrite) and the
gateway dropped mid-stream, the original was gone: truncated first,
deleted second, with nothing written in its place. The data-URL
compatibility fallback (`saveGatewayFileViaDataUrl`) had the same class
of bug via `fs.promises.writeFile`, which truncates before the write
completes.

Both paths now go through one failure-atomic primitive. Bytes land in a
short, randomly named sibling temp file (`.hermes-download-<hex>.part`,
same directory so the final step is a same-volume rename), created with
`flags: 'wx'`, and are renamed onto the destination only after the whole
body has been written and the descriptor released. The destination is
never opened before that point, so a failed download leaves whatever was
there untouched.

- Ownership-gated cleanup: the temp file is unlinked only after the
  stream's 'open' event proved THIS operation created it. An exclusive
  create that fails before open (EEXIST collision, EACCES, missing
  parent) never removes a file that belongs to someone else.
- `WriteStream.close(cb)` rather than `end(cb)` before renaming: `end`'s
  callback fires on 'finish' while the fd may still be open, and Windows
  refuses to rename a file with an open handle. Falls back to `end` for
  stream shapes without `close`.
- The failure path waits for 'close' (bounded by a 2s grace period)
  before unlinking, for the same reason: `destroy()` releases the fd
  asynchronously and an unlink racing the open handle would leak the
  `.part` file on Windows.
- A rename failure (destination locked, permissions) removes the owned
  temp file and rejects; nothing is left behind.
- Fixed-length temp name so a long user-chosen filename cannot push it
  past the filesystem's name limit.
- `fsPumpDeps()` is the single production deps factory (`'wx'` create,
  `fs.promises.rename`, `fs.promises.unlink`); `writeBufferToFile()`
  routes the data-URL fallback through the same pump. `PumpDeps` gains
  `rename` and a `tempPathFor` test seam.

Tests. Fakes: temp-then-rename on success, close-before-rename ordering,
the regression itself (destination neither opened nor unlinked when the
response fails mid-stream), write-error cleanup, close-before-unlink
ordering, rename-failure cleanup, pre-open EEXIST leaves the colliding
file alone, `writeBufferToFile` success and post-open write failure, the
temp-name length bound, and the `main.ts` wiring. Real filesystem
(`gateway-file-download.fs.test.ts`, exact production deps in a scratch
dir): completed download replaces the destination with no temp left;
mid-stream failure leaves the pre-existing destination byte-for-byte
with no `.part`; failure into a fresh name leaves nothing; seeded temp
path survives a pre-open EEXIST with no rename; rename failure (directory
at the destination) cleans the owned temp; data-URL fallback success and
missing-directory failure.

Adds the contributor email mapping required by the attribution check.

Fixes #96597

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015u8q2pHVPZmxpSrgkt94jC

apps/desktop/electron/gateway-file-download-transport.test.ts

@@ -51,6 +51,21 @@ test('finalizeGatewayDownload prompts a save dialog then streams the response',
 
   assert.match(fn, /dialog\.showSaveDialog/)
   assert.match(fn, /pumpStreamToFile\(/)
+  // Production deps come from one place so the streaming save and the data-URL
+  // fallback share the exclusive-create + rename contract (#96597).
+  assert.match(fn, /fsPumpDeps\(\)/)
+  assert.doesNotMatch(fn, /fs\.createWriteStream/)
   // HTTP errors carry their status so a 404 can trigger the fallback.
   assert.match(fn, /error\.statusCode = statusCode/)
 })
+
+test('data-URL fallback writes through the same failure-atomic primitive, never writeFile in place', () => {
+  const fn = extract('async function saveGatewayFileViaDataUrl', '\n// Mint a single-use WS ticket')
+
+  assert.match(fn, /dialog\.showSaveDialog/)
+  assert.match(fn, /writeBufferToFile\(/)
+  assert.match(fn, /fsPumpDeps\(\)/)
+  // A direct writeFile truncates an existing destination before the write
+  // completes; a mid-write failure would destroy it (#96597).
+  assert.doesNotMatch(fn, /fs\.promises\.writeFile/)
+})

apps/desktop/electron/gateway-file-download.fs.test.ts

@@ -0,0 +1,152 @@
+// Real-filesystem witnesses for the failure-atomic save contract (#96597).
+//
+// The unit tests in gateway-file-download.test.ts prove the pump's control flow
+// against fakes. These run the exact production deps (`fsPumpDeps()`) against
+// node:fs in a scratch directory and assert the user-visible invariants
+// byte-for-byte: a pre-existing destination survives every failure mode this
+// harness can force, a pre-existing file at the temp name survives a pre-open
+// collision, and no owned `.part` file is ever left behind.
+
+import assert from 'node:assert/strict'
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+import { Readable } from 'node:stream'
+
+import { afterEach, beforeEach, test } from 'vitest'
+
+import { fsPumpDeps, pumpStreamToFile, writeBufferToFile } from './gateway-file-download'
+
+let dir = ''
+
+beforeEach(async () => {
+  dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'hermes-download-fs-'))
+})
+
+afterEach(async () => {
+  await fs.promises.rm(dir, { force: true, recursive: true })
+})
+
+// A body that delivers `chunks` then fails with `error` (or ends cleanly when
+// `error` is omitted). Readable satisfies the pump's ReadableLike shape.
+function body(chunks: string[], error?: Error): Readable {
+  let i = 0
+
+  return new Readable({
+    read() {
+      if (i < chunks.length) {
+        this.push(Buffer.from(chunks[i++]))
+
+        return
+      }
+
+      if (error) {
+        this.destroy(error)
+
+        return
+      }
+
+      this.push(null)
+    }
+  })
+}
+
+async function listing(): Promise<string[]> {
+  return (await fs.promises.readdir(dir)).sort()
+}
+
+test('a completed download replaces the destination and leaves no temp file', async () => {
+  const dest = path.join(dir, 'report.bin')
+
+  await fs.promises.writeFile(dest, 'OLD CONTENT')
+
+  await pumpStreamToFile(body(['new ', 'content']), dest, fsPumpDeps())
+
+  assert.equal(await fs.promises.readFile(dest, 'utf8'), 'new content')
+  assert.deepEqual(await listing(), ['report.bin'])
+})
+
+test('a download that fails mid-stream leaves the pre-existing destination byte-for-byte and no temp file', async () => {
+  const dest = path.join(dir, 'report.bin')
+  const original = Buffer.from('OLD CONTENT THAT MUST SURVIVE')
+
+  await fs.promises.writeFile(dest, original)
+
+  await assert.rejects(
+    pumpStreamToFile(body(['partial'], new Error('socket hang up')), dest, fsPumpDeps()),
+    /socket hang up/
+  )
+
+  assert.ok(original.equals(await fs.promises.readFile(dest)), 'destination bytes must be unchanged')
+  assert.deepEqual(await listing(), ['report.bin'], 'no .part file may remain')
+})
+
+test('a download into a name with no existing file that fails leaves nothing behind', async () => {
+  const dest = path.join(dir, 'fresh.bin')
+
+  await assert.rejects(pumpStreamToFile(body(['partial'], new Error('reset')), dest, fsPumpDeps()), /reset/)
+
+  assert.deepEqual(await listing(), [])
+})
+
+// The reviewer-requested regression: seed the candidate temp path with known
+// bytes, force the exclusive open to fail with EEXIST, and prove those bytes
+// remain untouched and no rename occurred.
+test('a pre-open EEXIST collision leaves the seeded temp file and the destination untouched', async () => {
+  const dest = path.join(dir, 'report.bin')
+  const pinnedTemp = path.join(dir, '.hermes-download-pinned.part')
+  const seeded = Buffer.from('SOMEONE ELSES BYTES')
+  const original = Buffer.from('OLD CONTENT')
+
+  await fs.promises.writeFile(dest, original)
+  await fs.promises.writeFile(pinnedTemp, seeded)
+
+  const deps = { ...fsPumpDeps(), tempPathFor: () => pinnedTemp }
+
+  await assert.rejects(pumpStreamToFile(body(['new content']), dest, deps), (err: NodeJS.ErrnoException) => {
+    assert.equal(err.code, 'EEXIST')
+
+    return true
+  })
+
+  assert.ok(seeded.equals(await fs.promises.readFile(pinnedTemp)), 'the colliding file must not be unlinked')
+  assert.ok(original.equals(await fs.promises.readFile(dest)), 'destination must not be renamed over')
+  assert.deepEqual(await listing(), ['.hermes-download-pinned.part', 'report.bin'])
+})
+
+test('a failed final rename removes the owned temp file and leaves the destination as it was', async () => {
+  // A directory at the destination makes rename(2) fail on every platform.
+  const dest = path.join(dir, 'report.bin')
+
+  await fs.promises.mkdir(dest)
+  await fs.promises.writeFile(path.join(dest, 'keep.txt'), 'inside')
+
+  await assert.rejects(pumpStreamToFile(body(['new content']), dest, fsPumpDeps()))
+
+  assert.ok((await fs.promises.stat(dest)).isDirectory(), 'destination directory must survive')
+  assert.equal(await fs.promises.readFile(path.join(dest, 'keep.txt'), 'utf8'), 'inside')
+  assert.deepEqual(await listing(), ['report.bin'], 'the owned temp file must be cleaned up')
+})
+
+test('writeBufferToFile replaces the destination atomically and leaves no temp file', async () => {
+  const dest = path.join(dir, 'fallback.bin')
+
+  await fs.promises.writeFile(dest, 'OLD CONTENT')
+
+  await writeBufferToFile(Buffer.from('data-url payload'), dest, fsPumpDeps())
+
+  assert.equal(await fs.promises.readFile(dest, 'utf8'), 'data-url payload')
+  assert.deepEqual(await listing(), ['fallback.bin'])
+})
+
+test('writeBufferToFile into a missing directory fails without creating anything', async () => {
+  const dest = path.join(dir, 'missing-subdir', 'fallback.bin')
+
+  await assert.rejects(writeBufferToFile(Buffer.from('payload'), dest, fsPumpDeps()), (err: NodeJS.ErrnoException) => {
+    assert.equal(err.code, 'ENOENT')
+
+    return true
+  })
+
+  assert.deepEqual(await listing(), [])
+})

apps/desktop/electron/gateway-file-download.test.ts

@@ -1,17 +1,21 @@
 import assert from 'node:assert/strict'
 import { EventEmitter } from 'node:events'
+import path from 'node:path'
 
 import { test } from 'vitest'
 
 import { pathForRegistryBackendRequest } from './connection-config'
+import type { PumpDeps } from './gateway-file-download'
 import {
+  downloadTempPath,
   filenameFromContentDisposition,
   gatewayFilePath,
   gatewayFileRequestPaths,
   isNotFoundError,
   parseDataUrlToBuffer,
   pumpStreamToFile,
-  resolveGatewayFileBackend
+  resolveGatewayFileBackend,
+  writeBufferToFile
 } from './gateway-file-download'
 
 // A Readable-like response driven manually in tests.
@@ -40,9 +44,15 @@ class FakeWriteStream extends EventEmitter {
   destroyed = false
   private writeReturns: boolean[]
 
-  constructor(writeReturns: boolean[] = []) {
+  constructor(writeReturns: boolean[] = [], { opens = true }: { opens?: boolean } = {}) {
     super()
     this.writeReturns = writeReturns
+
+    // Like fs.WriteStream: 'open' fires once the exclusive create succeeded.
+    // `opens: false` models a create that fails before any file exists.
+    if (opens) {
+      queueMicrotask(() => this.emit('open'))
+    }
   }
 
   write(chunk: Buffer): boolean {
@@ -56,22 +66,81 @@ class FakeWriteStream extends EventEmitter {
     cb()
   }
 
+  // Like fs.WriteStream: the descriptor is released asynchronously and 'close'
+  // fires afterwards.
   destroy() {
     this.destroyed = true
+    queueMicrotask(() => this.emit('close'))
   }
 }
 
-test('pumpStreamToFile streams chunks to the destination without buffering the whole body', async () => {
-  const res = new FakeResponse()
-  const ws = new FakeWriteStream()
+// Deps recorder shared by the pumpStreamToFile tests: captures every path the
+// pump opens, renames, or unlinks so each test can assert the destination itself
+// was never touched before the body finished.
+function recordingDeps(ws: FakeWriteStream, { renameError }: { renameError?: Error } = {}) {
+  const opened: string[] = []
+  const renamed: Array<[string, string]> = []
   const unlinked: string[] = []
 
-  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', {
-    createWriteStream: () => ws as never,
-    unlink: async p => {
+  const deps: PumpDeps = {
+    createWriteStream: (p: string) => {
+      opened.push(p)
+
+      return ws as never
+    },
+    rename: async (from: string, to: string) => {
+      if (renameError) {
+        throw renameError
+      }
+
+      renamed.push([from, to])
+    },
+    unlink: async (p: string) => {
       unlinked.push(p)
     }
-  })
+  }
+
+  return { deps, opened, renamed, unlinked }
+}
+
+// Separator-agnostic: path.join emits backslashes on Windows, so the expectation
+// is "short hidden .part name, same directory as the destination", not a
+// literal POSIX string.
+const TEMP_BASENAME = /^\.hermes-download-[0-9a-f]{8}\.part$/
+
+// path.join normalizes separators (``/tmp`` -> ``\\tmp`` on Windows) while the
+// literal destination strings in these tests do not, so compare normalized forms.
+function assertTempPathBeside(tempPath: string, destPath: string) {
+  assert.equal(
+    path.normalize(path.dirname(tempPath)),
+    path.normalize(path.dirname(destPath)),
+    'temp file must sit beside the destination'
+  )
+  assert.match(path.basename(tempPath), TEMP_BASENAME)
+}
+
+test('downloadTempPath stays beside the destination with a short, random per-call name', () => {
+  const a = downloadTempPath('/tmp/out.bin')
+  const b = downloadTempPath('/tmp/out.bin')
+
+  assertTempPathBeside(a, '/tmp/out.bin')
+  assertTempPathBeside(b, '/tmp/out.bin')
+  assert.notEqual(a, b, 'two concurrent saves into the same directory must not share a temp file')
+
+  // The temp name must not grow with the user's filename: a destination near the
+  // filesystem's name limit still gets a temp file that fits beside it.
+  const longName = `/downloads/${'x'.repeat(250)}.bin`
+
+  assert.equal(path.normalize(path.dirname(downloadTempPath(longName))), path.normalize(path.dirname(longName)))
+  assert.ok(path.basename(downloadTempPath(longName)).length < 40)
+})
+
+test('pumpStreamToFile streams chunks into a sibling temp file, then renames it onto the destination', async () => {
+  const res = new FakeResponse()
+  const ws = new FakeWriteStream()
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
 
   res.emit('data', Buffer.from('abc'))
   res.emit('data', Buffer.from('def'))
@@ -81,17 +150,51 @@ test('pumpStreamToFile streams chunks to the destination without buffering the w
 
   assert.equal(Buffer.concat(ws.chunks).toString('utf8'), 'abcdef')
   assert.equal(ws.ended, true)
+  assert.equal(opened.length, 1)
+  assertTempPathBeside(opened[0], '/tmp/out.bin')
+  assert.deepEqual(renamed, [[opened[0], '/tmp/out.bin']])
   assert.deepEqual(unlinked, []) // success -> no cleanup
 })
 
+test('pumpStreamToFile waits for the descriptor to close before renaming when the stream supports close()', async () => {
+  const res = new FakeResponse()
+  const order: string[] = []
+
+  class ClosingWriteStream extends FakeWriteStream {
+    close(cb: (err?: Error | null) => void) {
+      order.push('close')
+      // Like fs.WriteStream: end the stream, release the fd, then call back.
+      this.ended = true
+      setTimeout(() => cb(), 0)
+    }
+  }
+
+  const ws = new ClosingWriteStream()
+  const { deps, renamed } = recordingDeps(ws)
+
+  deps.rename = async (from, to) => {
+    order.push('rename')
+    renamed.push([from, to])
+  }
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
+
+  res.emit('data', Buffer.from('abc'))
+  res.emit('end')
+
+  await promise
+
+  assert.deepEqual(order, ['close', 'rename'])
+  assert.equal(renamed.length, 1)
+  assert.equal(renamed[0][1], '/tmp/out.bin')
+})
+
 test('pumpStreamToFile applies backpressure: pauses on a full buffer and resumes on drain', async () => {
   const res = new FakeResponse()
   const ws = new FakeWriteStream([false]) // first write signals "buffer full"
+  const { deps } = recordingDeps(ws)
 
-  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', {
-    createWriteStream: () => ws as never,
-    unlink: async () => {}
-  })
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
 
   res.emit('data', Buffer.from('big-chunk'))
   assert.equal(res.paused, true, 'source should be paused when write() returns false')
@@ -104,43 +207,166 @@ test('pumpStreamToFile applies backpressure: pauses on a full buffer and resumes
   await promise
 })
 
-test('pumpStreamToFile unlinks the partial file and rejects on a write error', async () => {
+test('pumpStreamToFile removes only the temp file and rejects on a write error', async () => {
   const res = new FakeResponse()
   const ws = new FakeWriteStream()
-  const unlinked: string[] = []
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
 
-  const promise = pumpStreamToFile(res as never, '/tmp/partial.bin', {
-    createWriteStream: () => ws as never,
-    unlink: async p => {
-      unlinked.push(p)
-    }
-  })
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
 
   res.emit('data', Buffer.from('abc'))
   ws.emit('error', new Error('ENOSPC: disk full'))
 
   await assert.rejects(promise, /disk full/)
-  assert.deepEqual(unlinked, ['/tmp/partial.bin'])
+  assert.deepEqual(unlinked, [opened[0]])
+  assertTempPathBeside(unlinked[0], '/tmp/out.bin')
+  assert.deepEqual(renamed, [], 'a failed body must never be moved onto the destination')
   assert.equal(res.destroyed, true, 'source should be torn down on write failure')
 })
 
-test('pumpStreamToFile unlinks the partial file and rejects on a response error', async () => {
+test('pumpStreamToFile waits for the write stream to close before unlinking the temp file', async () => {
   const res = new FakeResponse()
-  const ws = new FakeWriteStream()
-  const unlinked: string[] = []
+  const order: string[] = []
 
-  const promise = pumpStreamToFile(res as never, '/tmp/partial.bin', {
-    createWriteStream: () => ws as never,
-    unlink: async p => {
-      unlinked.push(p)
+  class SlowCloseWriteStream extends FakeWriteStream {
+    destroy() {
+      this.destroyed = true
+      order.push('destroy')
+      // Release the fd later than a microtask: cleanup must still wait for it.
+      setTimeout(() => {
+        order.push('close')
+        this.emit('close')
+      }, 5)
     }
-  })
+  }
+
+  const ws = new SlowCloseWriteStream()
+  const { deps, opened, unlinked } = recordingDeps(ws)
+
+  deps.unlink = async (p: string) => {
+    order.push('unlink')
+    unlinked.push(p)
+  }
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
 
   res.emit('data', Buffer.from('abc'))
   res.emit('error', new Error('socket hang up'))
 
   await assert.rejects(promise, /socket hang up/)
-  assert.deepEqual(unlinked, ['/tmp/partial.bin'])
+  assert.deepEqual(order, ['destroy', 'close', 'unlink'])
+  assert.deepEqual(unlinked, [opened[0]])
+})
+
+// Ownership gate: an exclusive create can fail BEFORE this pump owns anything at
+// the temp path (EEXIST on a collision). Cleanup must not unlink a file it did
+// not create, or the destructive class moves from the destination to the temp
+// name.
+test('pumpStreamToFile never unlinks a temp path it did not create when the exclusive open fails', async () => {
+  const res = new FakeResponse()
+  const ws = new FakeWriteStream([], { opens: false })
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
+
+  const eexist: any = new Error("EEXIST: file already exists, open '/tmp/.hermes-download-deadbeef.part'")
+
+  eexist.code = 'EEXIST'
+  ws.emit('error', eexist)
+
+  await assert.rejects(promise, /EEXIST/)
+  assert.equal(opened.length, 1, 'one create attempt')
+  assert.deepEqual(unlinked, [], 'the colliding file belongs to someone else and must survive')
+  assert.deepEqual(renamed, [])
+  assert.equal(res.destroyed, true)
+})
+
+test('pumpStreamToFile honours tempPathFor so a regression can pin the temp path', async () => {
+  const res = new FakeResponse()
+  const ws = new FakeWriteStream()
+  const { deps, opened, renamed } = recordingDeps(ws)
+
+  deps.tempPathFor = () => '/tmp/pinned.part'
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
+
+  res.emit('data', Buffer.from('abc'))
+  res.emit('end')
+
+  await promise
+
+  assert.deepEqual(opened, ['/tmp/pinned.part'])
+  assert.deepEqual(renamed, [['/tmp/pinned.part', '/tmp/out.bin']])
+})
+
+test('writeBufferToFile streams the buffer through the same temp-then-rename contract', async () => {
+  const ws = new FakeWriteStream()
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
+
+  await writeBufferToFile(Buffer.from('whole body'), '/tmp/out.bin', deps)
+
+  assert.equal(Buffer.concat(ws.chunks).toString('utf8'), 'whole body')
+  assert.equal(opened.length, 1)
+  assertTempPathBeside(opened[0], '/tmp/out.bin')
+  assert.deepEqual(renamed, [[opened[0], '/tmp/out.bin']])
+  assert.deepEqual(unlinked, [])
+})
+
+test('writeBufferToFile leaves the destination untouched when the write fails after open', async () => {
+  // fs.WriteStream surfaces a write failure before 'finish', never after, so
+  // the fake errors from write() itself.
+  class FailingWriteStream extends FakeWriteStream {
+    write(chunk: Buffer): boolean {
+      super.write(chunk)
+      this.emit('error', new Error('ENOSPC: disk full'))
+
+      return true
+    }
+  }
+
+  const ws = new FailingWriteStream()
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
+
+  await assert.rejects(writeBufferToFile(Buffer.from('whole body'), '/tmp/out.bin', deps), /disk full/)
+  assert.ok(!opened.includes('/tmp/out.bin'))
+  assert.deepEqual(unlinked, [opened[0]], 'only the owned temp file is removed')
+  assert.deepEqual(renamed, [])
+})
+
+// Regression for #96597: opening the destination directly truncated it as soon
+// as the stream opened, and the error path then unlinked it — so a gateway
+// hiccup mid-download destroyed a pre-existing file the user had chosen to
+// overwrite. The destination must be neither opened nor removed on failure.
+test('pumpStreamToFile leaves a pre-existing destination untouched when the response fails mid-stream', async () => {
+  const res = new FakeResponse()
+  const ws = new FakeWriteStream()
+  const { deps, opened, renamed, unlinked } = recordingDeps(ws)
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
+
+  res.emit('data', Buffer.from('abc'))
+  res.emit('error', new Error('socket hang up'))
+
+  await assert.rejects(promise, /socket hang up/)
+  assert.ok(!opened.includes('/tmp/out.bin'), 'destination must not be opened (and truncated) before the body lands')
+  assert.ok(!unlinked.includes('/tmp/out.bin'), 'destination must not be removed on failure')
+  assert.deepEqual(unlinked, [opened[0]])
+  assert.deepEqual(renamed, [])
+})
+
+test('pumpStreamToFile removes the temp file and rejects when the final rename fails', async () => {
+  const res = new FakeResponse()
+  const ws = new FakeWriteStream()
+  const { deps, opened, unlinked } = recordingDeps(ws, { renameError: new Error('EPERM: destination locked') })
+
+  const promise = pumpStreamToFile(res as never, '/tmp/out.bin', deps)
+
+  res.emit('data', Buffer.from('abc'))
+  res.emit('end')
+
+  await assert.rejects(promise, /destination locked/)
+  assert.deepEqual(unlinked, [opened[0]], 'the temp file must not be left behind after a failed rename')
+  assert.ok(!unlinked.includes('/tmp/out.bin'))
 })
 
 test('parseDataUrlToBuffer decodes base64 payloads', () => {

apps/desktop/electron/gateway-file-download.ts

@@ -5,11 +5,15 @@
 // The transport wrappers (token / OAuth) live in main.ts because they need
 // main-process singletons (https/http, electronNet, the OAuth session). They
 // delegate the byte-moving to `pumpStreamToFile` here, which streams the
-// response to a user-selected destination with backpressure and cleans up a
-// partial file on error — so a large download never has to be buffered whole in
-// the native process.
+// response into a sibling temp file with backpressure and renames it onto the
+// user-selected destination only once the body has landed in full — so a large
+// download never has to be buffered whole in the native process, and a failed
+// one never touches a file that was already at the destination.
 
+import crypto from 'node:crypto'
+import fs from 'node:fs'
 import path from 'node:path'
+import { Readable } from 'node:stream'
 
 // Minimal shape of the response objects we consume. Both Node's
 // http.IncomingMessage and Electron net's IncomingMessage satisfy it.
@@ -25,14 +29,58 @@ export interface ReadableLike {
 export interface WriteStreamLike {
   write(chunk: Buffer): boolean
   end(cb: () => void): void
+  // fs.WriteStream's close() ends the stream and calls back only after the
+  // descriptor is released. end()'s callback fires on 'finish', while the fd can
+  // still be open — and Windows refuses to rename a file with an open handle.
+  close?(cb: (err?: Error | null) => void): void
   destroy(err?: Error): void
   on(event: 'error', listener: (err: Error) => void): unknown
-  once(event: 'drain', listener: () => void): unknown
+  // 'open' is the ownership signal: only after it fires did THIS pump create
+  // the temp file, and only then may cleanup unlink it.
+  once(event: 'close' | 'drain' | 'open', listener: () => void): unknown
 }
 
 export interface PumpDeps {
-  createWriteStream: (destPath: string) => WriteStreamLike
-  unlink: (destPath: string) => Promise<unknown>
+  // Must open the temp path exclusively (`flags: 'wx'`): the pump relies on
+  // creating a brand-new file, never on truncating or following something that
+  // already sits at that name.
+  createWriteStream: (tempPath: string) => WriteStreamLike
+  rename: (fromPath: string, toPath: string) => Promise<unknown>
+  unlink: (tempPath: string) => Promise<unknown>
+  // Test seam: pick the temp path deterministically so a regression can seed
+  // it and prove a pre-open collision leaves the seeded file untouched.
+  tempPathFor?: (destPath: string) => string
+}
+
+// Production deps: exclusive create on the real filesystem. Shared by the
+// streaming save and the data-URL fallback in main.ts, and exercised directly
+// by the real-filesystem tests so the guarantees are proven against node:fs,
+// not only against fakes.
+export function fsPumpDeps(): PumpDeps {
+  return {
+    createWriteStream: tempPath => fs.createWriteStream(tempPath, { flags: 'wx' }),
+    rename: (fromPath, toPath) => fs.promises.rename(fromPath, toPath),
+    unlink: tempPath => fs.promises.unlink(tempPath)
+  }
+}
+
+// How long to wait for a destroyed write stream to emit 'close' before giving
+// up and unlinking anyway. fs.WriteStream always emits it; the grace period only
+// protects against a stream shape that never does.
+const CLOSE_GRACE_MS = 2000
+
+// Resolve once `ws` has released its descriptor. destroy() closes the fd
+// asynchronously, and Windows rejects unlink/rename on a path whose handle is
+// still open, so cleanup must not run until 'close' has fired.
+function awaitClosed(ws: WriteStreamLike): Promise<void> {
+  return new Promise(resolve => {
+    const timer = setTimeout(resolve, CLOSE_GRACE_MS)
+
+    ws.once('close', () => {
+      clearTimeout(timer)
+      resolve()
+    })
+  })
 }
 
 export interface GatewayFileBackendDeps<T> {
@@ -79,15 +127,57 @@ export async function resolveGatewayFileBackend<T>(
   return { connection, connectionId, profile }
 }
 
-// Stream `res` into `destPath`, honoring backpressure. On any read/write error
-// the write stream is torn down and the (partial) destination file is removed
-// before the returned promise rejects, so a failed download never leaves a
-// truncated file behind.
+// Sibling temp name for an in-flight download. It lives in the destination's own
+// directory so the final step is a same-volume rename (and stays inside whatever
+// directory the save dialog approved). The name is short and fixed rather than
+// derived from the destination's basename so a long user-chosen filename cannot
+// push the temp name past the filesystem limit, and the random suffix keeps two
+// concurrent saves into the same directory from sharing a temp file. The leading
+// dot hides the in-flight file in Finder/ls while it exists.
+export function downloadTempPath(destPath: string): string {
+  return path.join(path.dirname(destPath), `.hermes-download-${crypto.randomBytes(4).toString('hex')}.part`)
+}
+
+// Stream `res` to `destPath`, honoring backpressure. Bytes land in a sibling
+// temp file first and are renamed onto `destPath` only after the whole body has
+// been written and the descriptor released. The destination itself is never
+// opened before that point, so a download that fails part-way leaves any file
+// already at `destPath` exactly as it was — only the temp file is removed before
+// the returned promise rejects. (Opening `destPath` directly truncated it on the
+// spot and the error path then unlinked it, destroying a pre-existing file the
+// user had chosen to overwrite; #96597.)
 export function pumpStreamToFile(res: ReadableLike, destPath: string, deps: PumpDeps): Promise<void> {
   return new Promise((resolve, reject) => {
-    const ws = deps.createWriteStream(destPath)
+    const tempPath = (deps.tempPathFor ?? downloadTempPath)(destPath)
+    const ws = deps.createWriteStream(tempPath)
     let failed = false
 
+    // Ownership gate. An exclusive open can fail BEFORE this pump has created
+    // anything at `tempPath` (EEXIST on a collision, EACCES, a missing parent);
+    // in that case the path belongs to someone else and cleanup must not touch
+    // it. fs.WriteStream emits 'open' exactly when the create succeeded.
+    let owned = false
+
+    ws.once('open', () => {
+      owned = true
+    })
+
+    // `.then(() => dep())` rather than `Promise.resolve(dep())` so a dep that
+    // throws synchronously still lands on the rejection path instead of escaping
+    // the stream callback it was invoked from.
+    const discardTemp = (): Promise<void> => {
+      if (!owned) {
+        return Promise.resolve()
+      }
+
+      return Promise.resolve()
+        .then(() => deps.unlink(tempPath))
+        .then(
+          () => {},
+          () => {} // best effort
+        )
+    }
+
     const fail = (err: Error) => {
       if (failed) {
         return
@@ -101,15 +191,60 @@ export function pumpStreamToFile(res: ReadableLike, destPath: string, deps: Pump
         // best effort — the socket may already be closed
       }
 
+      // Register the 'close' listener BEFORE destroy(): on a stream that is
+      // already tearing down after its own 'error', 'close' can follow on the
+      // next tick.
+      const closed = awaitClosed(ws)
+
       try {
         ws.destroy()
       } catch {
         // best effort
       }
 
-      Promise.resolve(deps.unlink(destPath))
-        .catch(() => {})
-        .then(() => reject(err))
+      closed.then(discardTemp).then(() => reject(err))
+    }
+
+    // Flush and release the temp file, then move it into place. A rename failure
+    // (destination locked, permissions) must not leave the temp file behind.
+    const finish = () => {
+      const onClosed = (err?: Error | null) => {
+        if (failed) {
+          return
+        }
+
+        if (err) {
+          fail(err)
+
+          return
+        }
+
+        Promise.resolve()
+          .then(() => deps.rename(tempPath, destPath))
+          .then(
+            () => {
+              // A failure that raced the rename has already taken the reject
+              // path; never report success on top of it.
+              if (!failed) {
+                resolve()
+              }
+            },
+            (renameErr: Error) => {
+              if (failed) {
+                return
+              }
+
+              failed = true
+              discardTemp().then(() => reject(renameErr))
+            }
+          )
+      }
+
+      if (typeof ws.close === 'function') {
+        ws.close(onClosed)
+      } else {
+        ws.end(() => onClosed())
+      }
     }
 
     ws.on('error', fail)
@@ -140,11 +275,20 @@ export function pumpStreamToFile(res: ReadableLike, destPath: string, deps: Pump
         return
       }
 
-      ws.end(() => resolve())
+      finish()
     })
   })
 }
 
+// Write an in-memory body to `destPath` with the same failure-atomic contract as
+// `pumpStreamToFile` (temp file, exclusive create, close, rename). Used by the
+// data-URL compatibility fallback, which has the whole body up front; a plain
+// `fs.promises.writeFile(destPath, buffer)` would truncate an existing file
+// before the write completes and so could destroy it on a mid-write failure.
+export function writeBufferToFile(buffer: Buffer, destPath: string, deps: PumpDeps): Promise<void> {
+  return pumpStreamToFile(Readable.from([buffer]), destPath, deps)
+}
+
 // Decode a `data:[<mime>][;base64],<payload>` URL into a Buffer. Used by the
 // compatibility fallback that reads through the capped `/api/fs/read-data-url`
 // route when the gateway predates `/api/fs/download`.

apps/desktop/electron/main.ts

@@ -187,12 +187,14 @@ import { createFirstRunSetupGate } from './first-run-setup-gate'
 import { registerFsIpc } from './fs-ipc'
 import {
   filenameFromContentDisposition,
+  fsPumpDeps,
   gatewayFilePath,
   gatewayFileRequestPaths,
   isNotFoundError,
   parseDataUrlToBuffer,
   pumpStreamToFile,
-  resolveGatewayFileBackend
+  resolveGatewayFileBackend,
+  writeBufferToFile
 } from './gateway-file-download'
 import { probeGatewayWebSocket } from './gateway-ws-probe'
 import { registerGitIpc } from './git-ipc'
@@ -7709,10 +7711,9 @@ async function finalizeGatewayDownload(res, statusCode, headers, ctx: any = {})
   }
 
   try {
-    await pumpStreamToFile(res, result.filePath, {
-      createWriteStream: (destPath: string) => fs.createWriteStream(destPath),
-      unlink: (destPath: string) => fs.promises.unlink(destPath)
-    })
+    // Failure-atomic: exclusive temp create beside the destination, rename into
+    // place only once the body is complete (#96597).
+    await pumpStreamToFile(res, result.filePath, fsPumpDeps())
   } catch (error) {
     ctx.abort?.()
     throw error
@@ -7864,7 +7865,9 @@ async function saveGatewayFileViaDataUrl(
     return { canceled: true, saved: false }
   }
 
-  await fs.promises.writeFile(result.filePath, buffer)
+  // Same failure-atomic contract as the streaming path: a direct writeFile
+  // truncates an existing destination before the write completes (#96597).
+  await writeBufferToFile(buffer, result.filePath, fsPumpDeps())
 
   return { path: result.filePath, saved: true }
 }

contributors/emails/amansk@gmail.com

@@ -0,0 +1,2 @@
+amansk
+# PR #96608 (desktop: failure-atomic gateway downloads, #96597)