From 2884db78861153de8a94e53abdc80b68d52893b1 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 12 Nov 2025 12:43:41 +0300 Subject: [PATCH 1/3] Fix race condition when downloading binaries from different windows --- src/core/cliManager.ts | 22 +++++++------- src/core/cliUtils.ts | 39 ++++++++++++++++++------ test/unit/core/cliUtils.test.ts | 53 ++++++++++++++++++++++++++++----- 3 files changed, 86 insertions(+), 28 deletions(-) diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 4e8833fe..ad25cc24 100644 --- a/src/core/cliManager.ts +++ b/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("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. diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index cc92a345..1f2d44be 100644 --- a/src/core/cliUtils.ts +++ b/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 { 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 { 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 { 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 }); diff --git a/test/unit/core/cliUtils.test.ts b/test/unit/core/cliUtils.test.ts index dd1c56f0..fbfc1101 100644 --- a/test/unit/core/cliUtils.test.ts +++ b/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", ]); From 73a9ef260ced524263ba9380ea2e1746daa8bdaa Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 12 Nov 2025 13:51:41 +0300 Subject: [PATCH 2/3] Fix test --- test/unit/core/cliManager.test.ts | 37 ++++++++++++++++++------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index d4f16c87..df7dee0b 100644 --- a/test/unit/core/cliManager.test.ts +++ b/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 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); +} From 4631e82a10fc2a8a24195be119929aba8fd86d9b Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 13 Nov 2025 11:21:43 +0300 Subject: [PATCH 3/3] More error handling --- src/core/cliManager.ts | 48 +++++++++++++++++++++++++++++-- src/core/cliUtils.ts | 7 ++++- test/unit/core/cliManager.test.ts | 2 +- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index ad25cc24..680359e5 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -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); diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index 1f2d44be..5af04a32 100644 --- a/src/core/cliUtils.ts +++ b/src/core/cliUtils.ts @@ -104,7 +104,12 @@ export async function rmOld(binPath: string): Promise { 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 }); + } } } } diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index df7dee0b..53f2b1a1 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -353,7 +353,7 @@ describe("CliManager", () => { 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 + // New files won't be deleted (could be another download in-progress) memfs.writeFileSync(path.join(BINARY_DIR, "coder.asc"), "signature"); withSuccessfulDownload();