From 3883c1932c0ec4761f4096ccb85f00d6369b27cc Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Thu, 23 Jul 2026 22:26:18 +0000 Subject: [PATCH] Discard the temporary file when a download fails downloadFile opens a write stream before fetching, but only closed the stream and removed the file it was writing to when the server answered with a bad status. A request that never reached the server, or a connection dropped part way through the body, left both the temporary file and an open file handle behind. Mods are downloaded through this in a Promise.all over every mod in a pack, so one bad moment on the network scatters temporary files across the mods directory. They are not harmless: createTempWriteStream picks the first free name, so each leftover file permanently shifts the name used for later downloads of the same mod, as the existing "should not overwrite existing temp files" test describes. Move the fetch and the body write into a try block and discard the stream and its file on the way out. Destroy rather than end the stream, as the content is being thrown away, and skip both if the stream has already closed so that a failure after the body was written does not wait forever for a close event that has already fired. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/lib/src/file_ops.ts | 49 +++++++++++++++++++++++++++++------- test/lib/file_ops.js | 32 +++++++++++++++++++++++ 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/packages/lib/src/file_ops.ts b/packages/lib/src/file_ops.ts index d1dbf11f..fbaaff5b 100644 --- a/packages/lib/src/file_ops.ts +++ b/packages/lib/src/file_ops.ts @@ -279,6 +279,32 @@ export async function createTempWriteStream(filePath: string) { return [tempFilePath, writeStream] as const; } +/** + * Discard a partially written download + * + * Destroys the write stream and removes the file it was writing to, so a + * download which failed part way through does not leave a stray temporary + * file and an open file handle behind. Safe to call whether or not the + * stream has already closed. + * + * @param writeStream - stream the download was written to. + * @param tempFilePath - path the stream was writing to. + */ +async function discardPartialDownload(writeStream: stream.Writable, tempFilePath: string) { + if (!writeStream.closed) { + writeStream.destroy(); + await events.once(writeStream, "close"); + } + try { + await fs.unlink(tempFilePath); + } catch (err: any) { + // Nothing to discard if the file never made it to disk. + if (err.code !== "ENOENT") { + throw err; + } + } +} + /** * Download a file by fetching the given url and streaming the body content to it. * @@ -305,16 +331,21 @@ export async function downloadFile( [tempFilePath, writeStream] = await createTempWriteStream(filePath); } - // Fetch stream bytes - const response = await fetch(url); - if (!response.ok || !response.body) { - writeStream.end(); - await events.once(writeStream, "close"); - await fs.unlink(tempFilePath); - throw new Error(`Failed to download ${url}: ${response.status} ${response.statusText}`); + // Fetch stream bytes. Anything that goes wrong from here until the + // body has been written leaves a temporary file that is of no use to + // anyone, including the request never reaching the server at all. + try { + const response = await fetch(url); + if (!response.ok || !response.body) { + throw new Error(`Failed to download ${url}: ${response.status} ${response.statusText}`); + } + const writer = stream.Writable.toWeb(writeStream); + await response.body.pipeTo(writer); + + } catch (err) { + await discardPartialDownload(writeStream, tempFilePath); + throw err; } - const writer = stream.Writable.toWeb(writeStream); - await response.body.pipeTo(writer); let targetFilePath = filePath; if (overwriteMode === "overwrite") { diff --git a/test/lib/file_ops.js b/test/lib/file_ops.js index 7e503326..acff0eb6 100644 --- a/test/lib/file_ops.js +++ b/test/lib/file_ops.js @@ -366,6 +366,38 @@ describe("lib/file_ops", function() { assert.equal(writtenContent, "original content"); }); + it("should not leave a temporary file behind if the request fails", async function () { + // Bind a server only to get a port nothing is listening on, so + // that the fetch is refused before there is a response at all. + const deadServer = http.createServer(); + await util.promisify(deadServer.listen.bind(deadServer))(); + const deadUrl = new URL("/simple-file", baseUrl); + deadUrl.port = deadServer.address().port; + await util.promisify(deadServer.close.bind(deadServer))(); + + const downloadPath = path.join(downloadDir, "refused.txt"); + await assert.rejects(lib.downloadFile(deadUrl, downloadPath, "overwrite")); + await assert.rejects( + fs.readFile(path.join(downloadDir, "refused.tmp.txt")), + { code: "ENOENT" }, + ); + await assert.rejects(fs.readFile(downloadPath), { code: "ENOENT" }); + }); + + it("should not leave a temporary file behind if the download is interrupted", async function () { + const downloadPath = path.join(downloadDir, "interrupted.txt"); + onStream = (res) => { + // Cut the connection with the body half written. + res.destroy(); + }; + await assert.rejects(lib.downloadFile(new URL("/stream", baseUrl), downloadPath, "overwrite")); + await assert.rejects( + fs.readFile(path.join(downloadDir, "interrupted.tmp.txt")), + { code: "ENOENT" }, + ); + await assert.rejects(fs.readFile(downloadPath), { code: "ENOENT" }); + }); + it("should throw if overwriteMode is rename and directory disappeared", async function () { if (process.platform === "win32") { // Can't remove the directory while the stream is running on Windows.