From a7e7de6407a509e45539ff41a986f76689539036 Mon Sep 17 00:00:00 2001 From: Amandeep Khurana Date: Thu, 27 Aug 2026 12:44:54 -0700 Subject: [PATCH] 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-.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 Claude-Session: https://claude.ai/code/session_015u8q2pHVPZmxpSrgkt94jC --- .../gateway-file-download-transport.test.ts | 15 + .../electron/gateway-file-download.fs.test.ts | 152 +++++++++ .../electron/gateway-file-download.test.ts | 288 ++++++++++++++++-- .../desktop/electron/gateway-file-download.ts | 174 ++++++++++- apps/desktop/electron/main.ts | 15 +- contributors/emails/amansk@gmail.com | 2 + 6 files changed, 594 insertions(+), 52 deletions(-) create mode 100644 apps/desktop/electron/gateway-file-download.fs.test.ts create mode 100644 contributors/emails/amansk@gmail.com diff --git a/apps/desktop/electron/gateway-file-download-transport.test.ts b/apps/desktop/electron/gateway-file-download-transport.test.ts index 631d3aee21..128510ebbf 100644 --- a/apps/desktop/electron/gateway-file-download-transport.test.ts +++ b/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/) +}) diff --git a/apps/desktop/electron/gateway-file-download.fs.test.ts b/apps/desktop/electron/gateway-file-download.fs.test.ts new file mode 100644 index 0000000000..6b56cd01d7 --- /dev/null +++ b/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 { + 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(), []) +}) diff --git a/apps/desktop/electron/gateway-file-download.test.ts b/apps/desktop/electron/gateway-file-download.test.ts index fcadd46e8b..a04265dbd1 100644 --- a/apps/desktop/electron/gateway-file-download.test.ts +++ b/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', () => { diff --git a/apps/desktop/electron/gateway-file-download.ts b/apps/desktop/electron/gateway-file-download.ts index 40fa98ced3..15d09964bf 100644 --- a/apps/desktop/electron/gateway-file-download.ts +++ b/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 + // 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 + unlink: (tempPath: string) => Promise + // 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 { + return new Promise(resolve => { + const timer = setTimeout(resolve, CLOSE_GRACE_MS) + + ws.once('close', () => { + clearTimeout(timer) + resolve() + }) + }) } export interface GatewayFileBackendDeps { @@ -79,15 +127,57 @@ export async function resolveGatewayFileBackend( 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 { 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 => { + 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 { + return pumpStreamToFile(Readable.from([buffer]), destPath, deps) +} + // Decode a `data:[][;base64],` 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`. diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index fbf80f6bc9..b0cc01cfa3 100644 --- a/apps/desktop/electron/main.ts +++ b/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 } } diff --git a/contributors/emails/amansk@gmail.com b/contributors/emails/amansk@gmail.com new file mode 100644 index 0000000000..c905da9a58 --- /dev/null +++ b/contributors/emails/amansk@gmail.com @@ -0,0 +1,2 @@ +amansk +# PR #96608 (desktop: failure-atomic gateway downloads, #96597)