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) <noreply@anthropic.com>

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") {

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.