fix: use platform-specific name for temp/cleanup in shared directories

Always download to the platform-specific binary name (coder-linux-amd64)
for temp files, old backups, signatures, and lock files. After a
successful download, rename the result to the user's configured file
name while still holding the lock to avoid races with other windows.

Scoped rmOld to only clean files prefixed with the binary basename so
it does not accidentally remove unrelated files when the target
directory is shared (e.g. /usr/bin).

Additional improvements:
- Rename cliUtils.name() to fullName() to pair with simpleName()
- Replace ResolvedBinary kind "file"/"dir" with source "file-path"/"directory"
  to clarify that stat always describes the binary, not the directory
- Fix binaryDestination description: file paths are version-checked and
  updated on mismatch, not used without downloading
- Update resolveBinaryPath JSDoc to remove misleading "as-is" language
- Add fallback path to error handler so file-path destinations check
  the user's original binary when the download path has no usable binary
- Simplify handleAnyBinaryFailure with tryCandidate closure and deferred
  findOldBinaries to avoid unnecessary I/O when earlier candidates match
- On error recovery, rename fallback to resolved.binPath so the binary
  always ends up where the user expects
- Add "starting download" back to the no-binary-found log message
- Improve test coverage: file destination error fallback, simple name
  error fallback, platform-specific file destination, old backup restore
- Unify test structure: shared disableSignatureVerification in top-level
  beforeEach, shared withFailedDownload helper, Binary Resolution group
- Remove flaky lock/progress file assertions from concurrent tests

Author: EhabY

package.json

@@ -62,7 +62,7 @@
 					"default": ""
 				},
 				"coder.binaryDestination": {
-					"markdownDescription": "The path to the Coder CLI binary or the directory containing it. When set to a file path (e.g., `/usr/bin/coder`), the extension uses it directly without downloading. When set to a directory, the extension looks for the CLI inside it (downloading if enabled). Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.",
+					"markdownDescription": "The path to the Coder CLI binary or the directory containing it. When set to a file path (e.g., `/usr/bin/coder`), the extension checks its version and downloads a replacement if it does not match the server (and downloads are enabled). When set to a directory, the extension looks for the CLI inside it (downloading if enabled). Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.",
 					"type": "string",
 					"default": ""
 				},

src/core/cliManager.ts

@@ -30,8 +30,8 @@ import type { CliCredentialManager } from "./cliCredentialManager";
 import type { PathResolver } from "./pathResolver";
 
 type ResolvedBinary =
-	| { binPath: string; kind: "file" | "dir"; stat: Stats }
-	| { binPath: string; kind: "not-found" };
+	| { binPath: string; stat: Stats; source: "file-path" | "directory" }
+	| { binPath: string; source: "not-found" };
 
 export class CliManager {
 	private readonly binaryLock: BinaryLock;
@@ -51,7 +51,7 @@ export class CliManager {
 	public async locateBinary(url: string): Promise<string> {
 		const safeHostname = toSafeHost(url);
 		const resolved = await this.resolveBinaryPath(safeHostname);
-		if (resolved.kind === "not-found") {
+		if (resolved.source === "not-found") {
 			throw new Error(`No CLI binary found at ${resolved.binPath}`);
 		}
 		return resolved.binPath;
@@ -60,9 +60,10 @@ export class CliManager {
 	/**
 	 * Resolve the CLI binary path from the configured cache path.
 	 *
-	 * Returns "file" when the cache path is an existing file (use as-is),
-	 * "dir" when a binary was found inside the directory, or "not-found"
-	 * with the platform-specific path for the caller to download into.
+	 * Returns "file-path" when the cache path is an existing file (checked for
+	 * version match and updated if needed), "directory" when a binary was found
+	 * inside the directory, or "not-found" with the platform-specific path for
+	 * the caller to download into.
 	 */
 	private async resolveBinaryPath(
 		safeHostname: string,
@@ -71,29 +72,29 @@ export class CliManager {
 		const cacheStat = await cliUtils.stat(cachePath);
 
 		if (cacheStat?.isFile()) {
-			return { binPath: cachePath, kind: "file", stat: cacheStat };
+			return { binPath: cachePath, stat: cacheStat, source: "file-path" };
 		}
 
-		const fullNamePath = path.join(cachePath, cliUtils.name());
+		const fullNamePath = path.join(cachePath, cliUtils.fullName());
 
 		// Path does not exist yet; return the platform-specific path to download.
 		if (!cacheStat) {
-			return { binPath: fullNamePath, kind: "not-found" };
+			return { binPath: fullNamePath, source: "not-found" };
 		}
 
 		// Directory exists; check platform-specific name, then simple name.
 		const fullStat = await cliUtils.stat(fullNamePath);
 		if (fullStat) {
-			return { binPath: fullNamePath, kind: "dir", stat: fullStat };
+			return { binPath: fullNamePath, stat: fullStat, source: "directory" };
 		}
 
 		const simpleNamePath = path.join(cachePath, cliUtils.simpleName());
 		const simpleStat = await cliUtils.stat(simpleNamePath);
 		if (simpleStat) {
-			return { binPath: simpleNamePath, kind: "dir", stat: simpleStat };
+			return { binPath: simpleNamePath, stat: simpleStat, source: "directory" };
 		}
 
-		return { binPath: fullNamePath, kind: "not-found" };
+		return { binPath: fullNamePath, source: "not-found" };
 	}
 
 	/**
@@ -134,10 +135,12 @@ export class CliManager {
 		}
 
 		const resolved = await this.resolveBinaryPath(safeHostname);
-		this.output.debug("Resolved binary path", resolved.binPath, resolved.kind);
+		this.output.debug(
+			`Resolved binary: ${resolved.binPath} (${resolved.source})`,
+		);
 
 		// Check existing binary version when one was found.
-		if (resolved.kind !== "not-found") {
+		if (resolved.source !== "not-found") {
 			this.output.debug(
 				"Existing binary size is",
 				prettyBytes(resolved.stat.size),
@@ -164,20 +167,19 @@ export class CliManager {
 				);
 			}
 		} else {
-			this.output.info("No existing binary found");
+			this.output.info("No existing binary found, starting download");
 		}
 
 		if (!enableDownloads) {
 			this.output.warn("Unable to download CLI because downloads are disabled");
 			throw new Error("Unable to download CLI because downloads are disabled");
 		}
 
-		// File destinations download to the same path. Directory destinations
-		// always use the platform-specific binary name.
-		const downloadBinPath =
-			resolved.kind === "file"
-				? resolved.binPath
-				: path.join(path.dirname(resolved.binPath), cliUtils.name());
+		// Always download using the platform-specific name.
+		const downloadBinPath = path.join(
+			path.dirname(resolved.binPath),
+			cliUtils.fullName(),
+		);
 
 		// Create the `bin` folder if it doesn't exist
 		await fs.mkdir(path.dirname(downloadBinPath), { recursive: true });
@@ -195,6 +197,7 @@ export class CliManager {
 			this.output.debug("Acquired download lock");
 
 			// If we waited for another process, re-check if binary is now ready
+			let needsDownload = true;
 			if (lockResult.waited) {
 				const latestBuildInfo = await restClient.getBuildInfo();
 				this.output.debug("Got latest server version", latestBuildInfo.version);
@@ -207,32 +210,52 @@ export class CliManager {
 					this.output.debug(
 						"Using existing binary since it matches the latest server version",
 					);
-					return downloadBinPath;
+					needsDownload = false;
+				} else {
+					const latestParsedVersion = semver.parse(latestBuildInfo.version);
+					if (!latestParsedVersion) {
+						throw new Error(
+							`Got invalid version from deployment: ${latestBuildInfo.version}`,
+						);
+					}
+					latestVersion = latestParsedVersion;
 				}
+			}
 
-				// Parse the latest version for download
-				const latestParsedVersion = semver.parse(latestBuildInfo.version);
-				if (!latestParsedVersion) {
-					throw new Error(
-						`Got invalid version from deployment: ${latestBuildInfo.version}`,
-					);
-				}
-				latestVersion = latestParsedVersion;
+			if (needsDownload) {
+				await this.performBinaryDownload(
+					restClient,
+					latestVersion,
+					downloadBinPath,
+					progressLogPath,
+				);
 			}
 
-			return await this.performBinaryDownload(
-				restClient,
-				latestVersion,
-				downloadBinPath,
-				progressLogPath,
-			);
+			// Rename to user-configured file path while we hold the lock.
+			if (
+				resolved.source === "file-path" &&
+				downloadBinPath !== resolved.binPath
+			) {
+				this.output.info(
+					"Renaming downloaded binary to",
+					path.basename(resolved.binPath),
+				);
+				await fs.rename(downloadBinPath, resolved.binPath);
+				return resolved.binPath;
+			}
+			return downloadBinPath;
 		} catch (error) {
-			// Unified error handling - check for fallback binaries and prompt user
-			return await this.handleAnyBinaryFailure(
+			const fallback = await this.handleAnyBinaryFailure(
 				error,
 				downloadBinPath,
 				buildInfo.version,
+				resolved.binPath !== downloadBinPath ? resolved.binPath : undefined,
 			);
+			// Move the fallback to the expected path if needed.
+			if (fallback !== resolved.binPath) {
+				await fs.rename(fallback, resolved.binPath);
+			}
+			return resolved.binPath;
 		} finally {
 			if (lockResult) {
 				await lockResult.release();
@@ -321,54 +344,59 @@ export class CliManager {
 	}
 
 	/**
-	 * Unified handler for any binary-related failure.
-	 * Checks for existing or old binaries and prompts user once.
+	 * Try fallback binaries after a download failure, prompting the user once
+	 * if the best candidate is a version mismatch.
 	 */
 	private async handleAnyBinaryFailure(
 		error: unknown,
 		binPath: string,
 		expectedVersion: string,
+		fallbackBinPath?: string,
 	): Promise<string> {
 		const message =
 			error instanceof cliUtils.FileLockError
 				? "Unable to update the Coder CLI binary because it's in use"
 				: "Failed to update CLI binary";
 
-		// Try existing binary first
-		const existingCheck = await this.checkBinaryVersion(
-			binPath,
-			expectedVersion,
-		);
-		if (existingCheck.version) {
-			// Perfect match - use without prompting
-			if (existingCheck.matches) {
-				return binPath;
+		// Returns the path if usable, undefined if not found.
+		// Throws the original error if the user declines a mismatch.
+		const tryCandidate = async (
+			candidate: string,
+		): Promise<string | undefined> => {
+			const check = await this.checkBinaryVersion(candidate, expectedVersion);
+			if (!check.version) {
+				return undefined;
 			}
-			// Version mismatch - prompt user
-			if (await this.promptUseExistingBinary(existingCheck.version, message)) {
-				return binPath;
+			if (
+				!check.matches &&
+				!(await this.promptUseExistingBinary(check.version, message))
+			) {
+				throw error;
+			}
+			return candidate;
+		};
+
+		const primary = await tryCandidate(binPath);
+		if (primary) {
+			return primary;
+		}
+
+		if (fallbackBinPath) {
+			const fallback = await tryCandidate(fallbackBinPath);
+			if (fallback) {
+				return fallback;
 			}
-			throw error;
 		}
 
-		// Try .old-* binaries as fallback
+		// Last resort: try the most recent .old-* backup.
 		const oldBinaries = await cliUtils.findOldBinaries(binPath);
 		if (oldBinaries.length > 0) {
-			const oldCheck = await this.checkBinaryVersion(
-				oldBinaries[0],
-				expectedVersion,
-			);
-			if (
-				oldCheck.version &&
-				(oldCheck.matches ||
-					(await this.promptUseExistingBinary(oldCheck.version, message)))
-			) {
-				await fs.rename(oldBinaries[0], binPath);
-				return binPath;
+			const old = await tryCandidate(oldBinaries[0]);
+			if (old) {
+				return old;
 			}
 		}
 
-		// No fallback available or user declined - re-throw original error
 		throw error;
 	}
 
@@ -392,7 +420,7 @@ export class CliManager {
 			}
 
 			// Figure out where to get the binary.
-			const binName = cliUtils.name();
+			const binName = cliUtils.fullName();
 			const configSource = cfg.get<string>("binarySource");
 			const binSource = configSource?.trim() ? configSource : "/bin/" + binName;
 			this.output.info("Downloading binary from", binSource);

src/core/cliUtils.ts

@@ -41,16 +41,18 @@ export interface RemovalResult {
  */
 export async function rmOld(binPath: string): Promise<RemovalResult[]> {
 	const binDir = path.dirname(binPath);
+	const binBase = path.basename(binPath);
 	try {
 		const files = await fs.readdir(binDir);
 		const results: RemovalResult[] = [];
 		for (const file of files) {
 			const fileName = path.basename(file);
 			if (
-				fileName.includes(".old-") ||
-				fileName.includes(".temp-") ||
-				fileName.endsWith(".asc") ||
-				fileName.endsWith(".progress.log")
+				fileName.startsWith(binBase) &&
+				(fileName.includes(".old-") ||
+					fileName.includes(".temp-") ||
+					fileName.endsWith(".asc") ||
+					fileName.endsWith(".progress.log"))
 			) {
 				try {
 					await fs.rm(path.join(binDir, file), { force: true });
@@ -139,7 +141,7 @@ export async function eTag(binPath: string): Promise<string> {
 /**
  * Return the platform-specific binary name (e.g. "coder-linux-amd64").
  */
-export function name(): string {
+export function fullName(): string {
 	const os = goos();
 	const arch = goarch();
 	let binName = `coder-${os}-${arch}`;
@@ -153,7 +155,7 @@ export function name(): string {
 /**
  * Return the simple binary name ("coder" or "coder.exe" on Windows).
  * This is the name used by package managers, as opposed to the
- * platform-specific name returned by name().
+ * platform-specific name returned by fullName().
  */
 export function simpleName(): string {
 	return goos() === "windows" ? "coder.exe" : "coder";

test/unit/core/cliManager.concurrent.test.ts

@@ -33,7 +33,7 @@ vi.mock("@/core/cliUtils", async () => {
 		...actual,
 		goos: vi.fn(),
 		goarch: vi.fn(),
-		name: vi.fn(),
+		fullName: vi.fn(),
 	};
 });
 
@@ -48,7 +48,7 @@ vi.mock("@/core/cliExec", async () => {
 function setupCliUtilsMocks(version: string) {
 	vi.mocked(cliUtils.goos).mockReturnValue("linux");
 	vi.mocked(cliUtils.goarch).mockReturnValue("amd64");
-	vi.mocked(cliUtils.name).mockReturnValue("coder-linux-amd64");
+	vi.mocked(cliUtils.fullName).mockReturnValue("coder-linux-amd64");
 	vi.mocked(cliExec.version).mockResolvedValue(version);
 	vi.mocked(pgp.readPublicKeys).mockResolvedValue([]);
 }
@@ -122,7 +122,7 @@ describe("CliManager Concurrent Downloads", () => {
 			expect(result).toBe(binaryPath);
 		}
 
-		// Verify binary exists and lock/progress files are cleaned up
+		// Verify binary exists, and lock/progress files are cleaned up
 		await expect(fs.access(binaryPath)).resolves.toBeUndefined();
 		await expect(fs.access(binaryPath + ".lock")).rejects.toThrow();
 		await expect(fs.access(binaryPath + ".progress.log")).rejects.toThrow();
@@ -161,7 +161,7 @@ describe("CliManager Concurrent Downloads", () => {
 			expect(result).toBe(binaryPath);
 		}
 
-		// Binary should be updated to 2.0.0, lock/progress files cleaned up
+		// Binary should be updated to 2.0.0, and lock/progress files are cleaned up
 		await expect(fs.access(binaryPath)).resolves.toBeUndefined();
 		const finalContent = await fs.readFile(binaryPath, "utf8");
 		expect(finalContent).toContain("v2.0.0");