Address review comments

Author: EhabY

src/core/binaryLock.ts

@@ -6,6 +6,16 @@ import { type Logger } from "../logging/logger";
 
 import * as downloadProgress from "./downloadProgress";
 
+/**
+ * Timeout to detect stale lock files and take over from stuck processes.
+ * This value is intentionally small so we can quickly takeover.
+ */
+const STALE_TIMEOUT_MS = 15000;
+
+const LOCK_POLL_INTERVAL_MS = 500;
+
+type LockRelease = () => Promise<void>;
+
 /**
  * Manages file locking for binary downloads to coordinate between multiple
  * VS Code windows downloading the same binary.
@@ -23,7 +33,7 @@ export class BinaryLock {
 	async acquireLockOrWait(
 		binPath: string,
 		progressLogPath: string,
-	): Promise<{ release: () => Promise<void>; waited: boolean }> {
+	): Promise<{ release: LockRelease; waited: boolean }> {
 		const release = await this.safeAcquireLock(binPath);
 		if (release) {
 			return { release, waited: false };
@@ -43,12 +53,10 @@ export class BinaryLock {
 	 * Attempt to acquire a lock on the binary file.
 	 * Returns the release function if successful, null if lock is already held.
 	 */
-	private async safeAcquireLock(
-		path: string,
-	): Promise<(() => Promise<void>) | null> {
+	private async safeAcquireLock(path: string): Promise<LockRelease | null> {
 		try {
 			const release = await lockfile.lock(path, {
-				stale: downloadProgress.STALE_TIMEOUT_MS,
+				stale: STALE_TIMEOUT_MS,
 				retries: 0,
 				realpath: false,
 			});
@@ -69,44 +77,50 @@ export class BinaryLock {
 	private async monitorDownloadProgress(
 		binPath: string,
 		progressLogPath: string,
-	): Promise<() => Promise<void>> {
+	): Promise<LockRelease> {
 		return await this.vscodeProposed.window.withProgress(
 			{
 				location: vscode.ProgressLocation.Notification,
 				title: "Another window is downloading the Coder CLI binary",
 				cancellable: false,
 			},
 			async (progress) => {
-				return new Promise<() => Promise<void>>((resolve, reject) => {
-					const interval = setInterval(async () => {
+				return new Promise<LockRelease>((resolve, reject) => {
+					const poll = async () => {
 						try {
-							const currentProgress =
-								await downloadProgress.readProgress(progressLogPath);
-							if (currentProgress) {
-								const totalBytesPretty =
-									currentProgress.totalBytes === null
-										? "unknown"
-										: prettyBytes(currentProgress.totalBytes);
-								const message =
-									currentProgress.status === "verifying"
-										? "Verifying signature..."
-										: `${prettyBytes(currentProgress.bytesDownloaded)} / ${totalBytesPretty}`;
-								progress.report({ message });
-							}
-
+							await this.updateProgressMonitor(progressLogPath, progress);
 							const release = await this.safeAcquireLock(binPath);
 							if (release) {
-								clearInterval(interval);
-								this.output.debug("Download completed by another process");
 								return resolve(release);
 							}
+							// Schedule next poll only after current one completes
+							setTimeout(poll, LOCK_POLL_INTERVAL_MS);
 						} catch (error) {
-							clearInterval(interval);
 							reject(error);
 						}
-					}, 500);
+					};
+					poll().catch((error) => reject(error));
 				});
 			},
 		);
 	}
+
+	private async updateProgressMonitor(
+		progressLogPath: string,
+		progress: vscode.Progress<{ message?: string }>,
+	): Promise<void> {
+		const currentProgress =
+			await downloadProgress.readProgress(progressLogPath);
+		if (currentProgress) {
+			const totalBytesPretty =
+				currentProgress.totalBytes === null
+					? "unknown"
+					: prettyBytes(currentProgress.totalBytes);
+			const message =
+				currentProgress.status === "verifying"
+					? "Verifying signature..."
+					: `${prettyBytes(currentProgress.bytesDownloaded)} / ${totalBytesPretty}`;
+			progress.report({ message });
+		}
+	}
 }

src/core/cliManager.ts

@@ -197,9 +197,8 @@ export class CliManager {
 		reason: string,
 	): Promise<boolean> {
 		const choice = await this.vscodeProposed.window.showErrorMessage(
-			`${reason}. Use version ${version} anyway?`,
+			`${reason}. Run version ${version} anyway?`,
 			"Run",
-			"Exit",
 		);
 		return choice === "Run";
 	}
@@ -273,6 +272,7 @@ export class CliManager {
 			if (await this.promptUseExistingBinary(existingCheck.version, message)) {
 				return binPath;
 			}
+			throw error;
 		}
 
 		// Try .old-* binaries as fallback

src/core/cliUtils.ts

@@ -122,11 +122,15 @@ export async function findOldBinaries(binPath: string): Promise<string[]> {
 			.map((f) => path.join(binDir, f));
 
 		// Sort by modification time, most recent first
-		const stats = await Promise.all(
+		const stats = await Promise.allSettled(
 			oldBinaries.map(async (f) => ({
 				path: f,
 				mtime: (await fs.stat(f)).mtime,
 			})),
+		).then((result) =>
+			result
+				.filter((promise) => promise.status === "fulfilled")
+				.map((promise) => promise.value),
 		);
 		stats.sort((a, b) => b.mtime.getTime() - a.mtime.getTime());
 		return stats.map((s) => s.path);
@@ -149,10 +153,6 @@ export function maybeWrapFileLockError(
 	}
 	return error;
 }
-export function isFileLockError(error: unknown): boolean {
-	const code = (error as NodeJS.ErrnoException).code;
-	return code === "EBUSY" || code === "EPERM";
-}
 
 /**
  * Return the etag (sha1) of the path.  Throw if unable to hash the file.

src/core/downloadProgress.ts

@@ -1,8 +1,6 @@
 import { promises as fs } from "node:fs";
 import * as path from "node:path";
 
-export const STALE_TIMEOUT_MS = 15000;
-
 export interface DownloadProgress {
 	bytesDownloaded: number;
 	totalBytes: number | null;
@@ -38,11 +36,5 @@ export async function readProgress(
 }
 
 export async function clearProgress(logPath: string): Promise<void> {
-	try {
-		await fs.rm(logPath, { force: true });
-	} catch (error) {
-		if ((error as NodeJS.ErrnoException).code !== "ENOENT") {
-			throw error;
-		}
-	}
+	await fs.rm(logPath, { force: true });
 }

test/unit/core/downloadProgress.test.ts

@@ -65,7 +65,6 @@ describe("downloadProgress", () => {
 				bytesDownloaded: 1500,
 				totalBytes: 10000,
 				status: "downloading",
-				timestamp: Date.now(),
 			};
 
 			await fs.writeFile(testLogPath, JSON.stringify(expectedProgress) + "\n");