Fix race condition when downloading binaries from different windows

src/core/cliManager.ts

@@ -3,10 +3,10 @@ import globalAxios, {
 	type AxiosRequestConfig,
 } from "axios";
 import { type Api } from "coder/site/src/api/api";
-import { createWriteStream, type WriteStream } from "fs";
-import fs from "fs/promises";
-import { type IncomingMessage } from "http";
-import path from "path";
+import { createWriteStream, type WriteStream } from "node:fs";
+import fs from "node:fs/promises";
+import { type IncomingMessage } from "node:http";
+import path from "node:path";
 import prettyBytes from "pretty-bytes";
 import * as semver from "semver";
 import * as vscode from "vscode";
@@ -99,26 +99,26 @@ export class CliManager {
 
 		// Remove any left-over old or temporary binaries and signatures.
 		const removed = await cliUtils.rmOld(binPath);
-		removed.forEach(({ fileName, error }) => {
+		for (const { fileName, error, skipped } of removed) {
 			if (error) {
 				this.output.warn("Failed to remove", fileName, error);
+			} else if (skipped) {
+				this.output.debug("Skipped", fileName, "(file too new)");
 			} else {
 				this.output.info("Removed", fileName);
 			}
-		});
+		}
 
 		// Figure out where to get the binary.
 		const binName = cliUtils.name();
-		const configSource = cfg.get("binarySource");
+		const configSource = cfg.get<string>("binarySource") ?? "";
 		const binSource =
-			configSource && String(configSource).trim().length > 0
-				? String(configSource)
-				: "/bin/" + binName;
+			configSource.trim().length > 0 ? configSource : "/bin/" + binName;
 		this.output.info("Downloading binary from", binSource);
 
 		// Ideally we already caught that this was the right version and returned
 		// early, but just in case set the ETag.
-		const etag = stat !== undefined ? await cliUtils.eTag(binPath) : "";
+		const etag = stat === undefined ? "" : await cliUtils.eTag(binPath);
 		this.output.info("Using ETag", etag);
 
 		// Download the binary to a temporary file.
@@ -165,13 +165,57 @@ export class CliManager {
 						"Moving existing binary to",
 						path.basename(oldBinPath),
 					);
-					await fs.rename(binPath, oldBinPath);
+					try {
+						await fs.rename(binPath, oldBinPath);
+					} catch (error) {
+						// That's fine since we are trying to move the file anyway
+						if ((error as NodeJS.ErrnoException).code !== "ENOENT") {
+							throw error;
+						}
+						this.output.debug(
+							"Binary already moved by another process, continuing",
+						);
+					}
 				}
 
 				// Then move the temporary binary into the right place.
 				this.output.info("Moving downloaded file to", path.basename(binPath));
 				await fs.mkdir(path.dirname(binPath), { recursive: true });
-				await fs.rename(tempFile, binPath);
+				try {
+					await fs.rename(tempFile, binPath);
+				} catch (error) {
+					const errCode = (error as NodeJS.ErrnoException).code;
+					// On Windows, fs.rename fails if the target exists. On POSIX systems,
+					// it atomically replaces the target. If another process already wrote
+					// the binary and it's the correct version, we can consider this a
+					// success since both processes are installing the same version.
+					if (
+						errCode === "EPERM" ||
+						errCode === "EACCES" ||
+						errCode === "EBUSY"
+					) {
+						this.output.debug(
+							"Binary may be in use or already exists, verifying version",
+						);
+						const existingStat = await cliUtils.stat(binPath);
+						if (existingStat !== undefined) {
+							try {
+								const existingVersion = await cliUtils.version(binPath);
+								const expectedVersion = buildInfo.version;
+								if (existingVersion === expectedVersion) {
+									this.output.info(
+										"Binary already installed by another process with correct version, cleaning up temp file",
+									);
+									await fs.rm(tempFile, { force: true });
+									return binPath;
+								}
+							} catch {
+								// If we can't verify the version, fall through to throw original error
+							}
+						}
+					}
+					throw error;
+				}
 
 				// For debugging, to see if the binary only partially downloaded.
 				const newStat = await cliUtils.stat(binPath);

src/core/cliUtils.ts

@@ -1,10 +1,16 @@
-import { execFile, type ExecFileException } from "child_process";
-import * as crypto from "crypto";
-import { createReadStream, type Stats } from "fs";
-import fs from "fs/promises";
-import os from "os";
-import path from "path";
-import { promisify } from "util";
+import { execFile, type ExecFileException } from "node:child_process";
+import * as crypto from "node:crypto";
+import { createReadStream, type Stats } from "node:fs";
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { promisify } from "node:util";
+
+/**
+ * Minimum age in milliseconds before cleaning up temporary files.
+ * This prevents deleting files that are actively being created by another window.
+ */
+const MIN_TEMP_FILE_AGE_MS = 30 * 60 * 1000;
 
 /**
  * Stat the path or undefined if the path does not exist.  Throw if unable to
@@ -60,7 +66,11 @@ export async function version(binPath: string): Promise<string> {
 	return json.version;
 }
 
-export type RemovalResult = { fileName: string; error: unknown };
+export type RemovalResult = {
+	fileName: string;
+	error: unknown;
+	skipped?: boolean;
+};
 
 /**
  * Remove binaries in the same directory as the specified path that have a
@@ -72,6 +82,7 @@ export async function rmOld(binPath: string): Promise<RemovalResult[]> {
 	try {
 		const files = await fs.readdir(binDir);
 		const results: RemovalResult[] = [];
+		const now = Date.now();
 		for (const file of files) {
 			const fileName = path.basename(file);
 			if (
@@ -80,10 +91,25 @@ export async function rmOld(binPath: string): Promise<RemovalResult[]> {
 				fileName.endsWith(".asc")
 			) {
 				try {
-					await fs.rm(path.join(binDir, file), { force: true });
+					const filePath = path.join(binDir, file);
+					const stats = await fs.stat(filePath);
+					const fileAge = now - stats.mtimeMs;
+
+					// Do not delete recently created files, they could be downloads in-progress
+					if (fileAge < MIN_TEMP_FILE_AGE_MS) {
+						results.push({ fileName, error: undefined, skipped: true });
+						continue;
+					}
+
+					await fs.rm(filePath, { force: true });
 					results.push({ fileName, error: undefined });
 				} catch (error) {
-					results.push({ fileName, error });
+					// That's fine since we were trying to delete this file anyway
+					if ((error as NodeJS.ErrnoException)?.code === "ENOENT") {
+						results.push({ fileName, error: undefined });
+					} else {
+						results.push({ fileName, error });
+					}
 				}
 			}
 		}

test/unit/core/cliManager.test.ts

@@ -350,10 +350,11 @@ describe("CliManager", () => {
 		it("cleans up old files before download", async () => {
 			// Create old temporary files and signature files
 			vol.mkdirSync(BINARY_DIR, { recursive: true });
-			memfs.writeFileSync(path.join(BINARY_DIR, "coder.old-xyz"), "old");
-			memfs.writeFileSync(path.join(BINARY_DIR, "coder.temp-abc"), "temp");
+			writeOldFile(path.join(BINARY_DIR, "coder.old-xyz"), "old");
+			writeOldFile(path.join(BINARY_DIR, "coder.temp-abc"), "temp");
+			writeOldFile(path.join(BINARY_DIR, "keeper.txt"), "keep");
+			// New files won't be deleted (could be another download in-progress)
 			memfs.writeFileSync(path.join(BINARY_DIR, "coder.asc"), "signature");
-			memfs.writeFileSync(path.join(BINARY_DIR, "keeper.txt"), "keep");
 
 			withSuccessfulDownload();
 			await manager.fetchBinary(mockApi, "test");
@@ -365,7 +366,7 @@ describe("CliManager", () => {
 			expect(memfs.existsSync(path.join(BINARY_DIR, "coder.temp-abc"))).toBe(
 				false,
 			);
-			expect(memfs.existsSync(path.join(BINARY_DIR, "coder.asc"))).toBe(false);
+			expect(memfs.existsSync(path.join(BINARY_DIR, "coder.asc"))).toBe(true);
 			expect(memfs.existsSync(path.join(BINARY_DIR, "keeper.txt"))).toBe(true);
 		});
 
@@ -783,17 +784,23 @@ describe("CliManager", () => {
 		vi.mocked(error.summary).mockReturnValue("Signature does not match");
 		return error;
 	}
+});
 
-	function mockBinaryContent(version: string): string {
-		return `mock-binary-v${version}`;
-	}
+function mockBinaryContent(version: string): string {
+	return `mock-binary-v${version}`;
+}
 
-	function expectFileInDir(dir: string, pattern: string): string | undefined {
-		const files = readdir(dir);
-		return files.find((f) => f.includes(pattern));
-	}
+function expectFileInDir(dir: string, pattern: string): string | undefined {
+	const files = readdir(dir);
+	return files.find((f) => f.includes(pattern));
+}
 
-	function readdir(dir: string): string[] {
-		return memfs.readdirSync(dir) as string[];
-	}
-});
+function readdir(dir: string): string[] {
+	return memfs.readdirSync(dir) as string[];
+}
+
+function writeOldFile(filePath: string, content: string): void {
+	const oldTime = Date.now() - 60 * 60 * 1000; // 60 minutes ago
+	memfs.writeFileSync(filePath, content);
+	memfs.utimesSync(filePath, oldTime / 1000, oldTime / 1000);
+}

test/unit/core/cliUtils.test.ts

@@ -1,6 +1,6 @@
-import fs from "fs/promises";
-import os from "os";
-import path from "path";
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
 import { beforeAll, describe, expect, it } from "vitest";
 
 import * as cliUtils from "@/core/cliUtils";
@@ -95,17 +95,45 @@ describe("CliUtils", () => {
 		expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([]);
 
 		await fs.mkdir(binDir, { recursive: true });
+
+		// Create old files that should be removed.
+		const oldTime = Date.now() - 60 * 60 * 1000; // 60 minutes ago
 		await fs.writeFile(path.join(binDir, "bin.old-1"), "echo hello");
+		await fs.utimes(
+			path.join(binDir, "bin.old-1"),
+			oldTime / 1000,
+			oldTime / 1000,
+		);
 		await fs.writeFile(path.join(binDir, "bin.old-2"), "echo hello");
+		await fs.utimes(
+			path.join(binDir, "bin.old-2"),
+			oldTime / 1000,
+			oldTime / 1000,
+		);
 		await fs.writeFile(path.join(binDir, "bin.temp-1"), "echo hello");
-		await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello");
-		await fs.writeFile(path.join(binDir, "bin1"), "echo hello");
-		await fs.writeFile(path.join(binDir, "bin2"), "echo hello");
+		await fs.utimes(
+			path.join(binDir, "bin.temp-1"),
+			oldTime / 1000,
+			oldTime / 1000,
+		);
 		await fs.writeFile(path.join(binDir, "bin.asc"), "echo hello");
+		await fs.utimes(
+			path.join(binDir, "bin.asc"),
+			oldTime / 1000,
+			oldTime / 1000,
+		);
+
+		// Create new files that should be skipped.
+		await fs.writeFile(path.join(binDir, "bin.temp-2"), "echo hello");
 		await fs.writeFile(path.join(binDir, "bin.old-1.asc"), "echo hello");
 		await fs.writeFile(path.join(binDir, "bin.temp-2.asc"), "echo hello");
 
-		expect(await cliUtils.rmOld(path.join(binDir, "bin1"))).toStrictEqual([
+		// Regular files that should never be removed.
+		await fs.writeFile(path.join(binDir, "bin1"), "echo hello");
+		await fs.writeFile(path.join(binDir, "bin2"), "echo hello");
+
+		const results = await cliUtils.rmOld(path.join(binDir, "bin1"));
+		expect(results).toStrictEqual([
 			{
 				fileName: "bin.asc",
 				error: undefined,
@@ -117,6 +145,7 @@ describe("CliUtils", () => {
 			{
 				fileName: "bin.old-1.asc",
 				error: undefined,
+				skipped: true,
 			},
 			{
 				fileName: "bin.old-2",
@@ -129,14 +158,22 @@ describe("CliUtils", () => {
 			{
 				fileName: "bin.temp-2",
 				error: undefined,
+				skipped: true,
 			},
 			{
 				fileName: "bin.temp-2.asc",
 				error: undefined,
+				skipped: true,
 			},
 		]);
 
-		expect(await fs.readdir(path.join(tmp, "bins"))).toStrictEqual([
+		// Verify old files were removed and new files were kept.
+		const remaining = await fs.readdir(path.join(tmp, "bins"));
+		remaining.sort();
+		expect(remaining).toStrictEqual([
+			"bin.old-1.asc",
+			"bin.temp-2",
+			"bin.temp-2.asc",
 			"bin1",
 			"bin2",
 		]);