Fix race condition when downloading binaries from different windows

Author: EhabY

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.

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,7 +91,17 @@ 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 });

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",
 		]);