diff --git a/package.json b/package.json index 78d39819..e1946da1 100644 --- a/package.json +++ b/package.json @@ -351,6 +351,7 @@ "jsonc-parser": "^3.3.1", "openpgp": "^6.2.2", "pretty-bytes": "^7.1.0", + "proper-lockfile": "^4.1.2", "proxy-agent": "^6.5.0", "semver": "^7.7.3", "ua-parser-js": "1.0.40", @@ -361,6 +362,7 @@ "@types/eventsource": "^3.0.0", "@types/glob": "^7.1.3", "@types/node": "^22.14.1", + "@types/proper-lockfile": "^4.1.4", "@types/semver": "^7.7.1", "@types/ua-parser-js": "0.7.36", "@types/vscode": "^1.73.0", diff --git a/src/core/binaryLock.ts b/src/core/binaryLock.ts new file mode 100644 index 00000000..6e334453 --- /dev/null +++ b/src/core/binaryLock.ts @@ -0,0 +1,126 @@ +import prettyBytes from "pretty-bytes"; +import * as lockfile from "proper-lockfile"; +import * as vscode from "vscode"; + +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; + +/** + * Manages file locking for binary downloads to coordinate between multiple + * VS Code windows downloading the same binary. + */ +export class BinaryLock { + constructor( + private readonly vscodeProposed: typeof vscode, + private readonly output: Logger, + ) {} + + /** + * Acquire the lock, or wait for another process if the lock is held. + * Returns the lock release function and a flag indicating if we waited. + */ + async acquireLockOrWait( + binPath: string, + progressLogPath: string, + ): Promise<{ release: LockRelease; waited: boolean }> { + const release = await this.safeAcquireLock(binPath); + if (release) { + return { release, waited: false }; + } + + this.output.info( + "Another process is downloading the binary, monitoring progress", + ); + const newRelease = await this.monitorDownloadProgress( + binPath, + progressLogPath, + ); + return { release: newRelease, waited: true }; + } + + /** + * 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 { + try { + const release = await lockfile.lock(path, { + stale: STALE_TIMEOUT_MS, + retries: 0, + realpath: false, + }); + return release; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ELOCKED") { + throw error; + } + return null; + } + } + + /** + * Monitor download progress from another process by polling the progress log + * and attempting to acquire the lock. Shows a VS Code progress notification. + * Returns the lock release function once the download completes. + */ + private async monitorDownloadProgress( + binPath: string, + progressLogPath: string, + ): Promise { + 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((resolve, reject) => { + const poll = async () => { + try { + await this.updateProgressMonitor(progressLogPath, progress); + const release = await this.safeAcquireLock(binPath); + if (release) { + return resolve(release); + } + // Schedule next poll only after current one completes + setTimeout(poll, LOCK_POLL_INTERVAL_MS); + } catch (error) { + reject(error); + } + }; + poll().catch((error) => reject(error)); + }); + }, + ); + } + + private async updateProgressMonitor( + progressLogPath: string, + progress: vscode.Progress<{ message?: string }>, + ): Promise { + 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 }); + } + } +} diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 4e8833fe..5e0b3d26 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"; @@ -15,15 +15,21 @@ import { errToStr } from "../api/api-helper"; import { type Logger } from "../logging/logger"; import * as pgp from "../pgp"; +import { BinaryLock } from "./binaryLock"; import * as cliUtils from "./cliUtils"; +import * as downloadProgress from "./downloadProgress"; import { type PathResolver } from "./pathResolver"; export class CliManager { + private readonly binaryLock: BinaryLock; + constructor( private readonly vscodeProposed: typeof vscode, private readonly output: Logger, private readonly pathResolver: PathResolver, - ) {} + ) { + this.binaryLock = new BinaryLock(vscodeProposed, output); + } /** * Download and return the path to a working binary for the deployment with @@ -97,143 +103,342 @@ export class CliManager { throw new Error("Unable to download CLI because downloads are disabled"); } - // Remove any left-over old or temporary binaries and signatures. - const removed = await cliUtils.rmOld(binPath); - removed.forEach(({ fileName, error }) => { - if (error) { - this.output.warn("Failed to remove", fileName, error); - } else { - this.output.info("Removed", fileName); - } - }); - - // Figure out where to get the binary. - const binName = cliUtils.name(); - const configSource = cfg.get("binarySource"); - const binSource = - configSource && String(configSource).trim().length > 0 - ? String(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) : ""; - this.output.info("Using ETag", etag); - - // Download the binary to a temporary file. + // Create the `bin` folder if it doesn't exist await fs.mkdir(path.dirname(binPath), { recursive: true }); - const tempFile = - binPath + ".temp-" + Math.random().toString(36).substring(8); - const writeStream = createWriteStream(tempFile, { - autoClose: true, - mode: 0o755, - }); - const client = restClient.getAxiosInstance(); - const status = await this.download(client, binSource, writeStream, { - "Accept-Encoding": "gzip", - "If-None-Match": `"${etag}"`, - }); + const progressLogPath = binPath + ".progress.log"; + + let lockResult: + | { release: () => Promise; waited: boolean } + | undefined; + let latestVersion = parsedVersion; + try { + lockResult = await this.binaryLock.acquireLockOrWait( + binPath, + progressLogPath, + ); + this.output.info("Acquired download lock"); - switch (status) { - case 200: { - if (cfg.get("disableSignatureVerification")) { + // If we waited for another process, re-check if binary is now ready + if (lockResult.waited) { + const latestBuildInfo = await restClient.getBuildInfo(); + this.output.info("Got latest server version", latestBuildInfo.version); + + const recheckAfterWait = await this.checkBinaryVersion( + binPath, + latestBuildInfo.version, + ); + if (recheckAfterWait.matches) { this.output.info( - "Skipping binary signature verification due to settings", + "Using existing binary since it matches the latest server version", ); - } else { - await this.verifyBinarySignatures(client, tempFile, [ - // A signature placed at the same level as the binary. It must be - // named exactly the same with an appended `.asc` (such as - // coder-windows-amd64.exe.asc or coder-linux-amd64.asc). - binSource + ".asc", - // The releases.coder.com bucket does not include the leading "v", - // and unlike what we get from buildinfo it uses a truncated version - // with only major.minor.patch. The signature name follows the same - // rule as above. - `https://releases.coder.com/coder-cli/${parsedVersion.major}.${parsedVersion.minor}.${parsedVersion.patch}/${binName}.asc`, - ]); + return binPath; } - // Move the old binary to a backup location first, just in case. And, - // on Linux at least, you cannot write onto a binary that is in use so - // moving first works around that (delete would also work). - if (stat !== undefined) { - const oldBinPath = - binPath + ".old-" + Math.random().toString(36).substring(8); - this.output.info( - "Moving existing binary to", - path.basename(oldBinPath), + // Parse the latest version for download + const latestParsedVersion = semver.parse(latestBuildInfo.version); + if (!latestParsedVersion) { + throw new Error( + `Got invalid version from deployment: ${latestBuildInfo.version}`, ); - await fs.rename(binPath, oldBinPath); } + latestVersion = latestParsedVersion; + } - // 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); + return await this.performBinaryDownload( + restClient, + latestVersion, + binPath, + progressLogPath, + ); + } catch (error) { + // Unified error handling - check for fallback binaries and prompt user + return await this.handleAnyBinaryFailure( + error, + binPath, + buildInfo.version, + ); + } finally { + if (lockResult) { + await lockResult.release(); + this.output.info("Released download lock"); + } + } + } - // For debugging, to see if the binary only partially downloaded. - const newStat = await cliUtils.stat(binPath); + /** + * Check if a binary exists and matches the expected version. + */ + private async checkBinaryVersion( + binPath: string, + expectedVersion: string, + ): Promise<{ version: string | null; matches: boolean }> { + const stat = await cliUtils.stat(binPath); + if (!stat) { + return { version: null, matches: false }; + } + + try { + const version = await cliUtils.version(binPath); + return { + version, + matches: version === expectedVersion, + }; + } catch (error) { + this.output.warn(`Unable to get version of binary: ${errToStr(error)}`); + return { version: null, matches: false }; + } + } + + /** + * Prompt the user to use an existing binary version. + */ + private async promptUseExistingBinary( + version: string, + reason: string, + ): Promise { + const choice = await this.vscodeProposed.window.showErrorMessage( + `${reason}. Run version ${version} anyway?`, + "Run", + ); + return choice === "Run"; + } + + /** + * Replace the existing binary with the downloaded temp file. + * Throws WindowsFileLockError if binary is in use. + */ + private async replaceExistingBinary( + binPath: string, + tempFile: string, + ): Promise { + const oldBinPath = + binPath + ".old-" + Math.random().toString(36).substring(8); + + try { + // Step 1: Move existing binary to backup (if it exists) + const stat = await cliUtils.stat(binPath); + if (stat) { this.output.info( - "Downloaded binary size is", - prettyBytes(newStat?.size || 0), + "Moving existing binary to", + path.basename(oldBinPath), ); + await fs.rename(binPath, oldBinPath); + } - // Make sure we can execute this new binary. - const version = await cliUtils.version(binPath); - this.output.info("Downloaded binary version is", version); + // Step 2: Move temp to final location + this.output.info("Moving downloaded file to", path.basename(binPath)); + await fs.rename(tempFile, binPath); + } catch (error) { + throw cliUtils.maybeWrapFileLockError(error, binPath); + } + + // For debugging, to see if the binary only partially downloaded. + const newStat = await cliUtils.stat(binPath); + this.output.info( + "Downloaded binary size is", + prettyBytes(newStat?.size || 0), + ); + + // Make sure we can execute this new binary. + const version = await cliUtils.version(binPath); + this.output.info("Downloaded binary version is", version); + } + /** + * Unified handler for any binary-related failure. + * Checks for existing or old binaries and prompts user once. + */ + private async handleAnyBinaryFailure( + error: unknown, + binPath: string, + expectedVersion: string, + ): Promise { + 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; } - case 304: { - this.output.info("Using existing binary since server returned a 304"); + // Version mismatch - prompt user + if (await this.promptUseExistingBinary(existingCheck.version, message)) { return binPath; } - case 404: { - vscode.window - .showErrorMessage( - "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", - "Open an Issue", - ) - .then((value) => { - if (!value) { - return; - } - const os = cliUtils.goos(); - const arch = cliUtils.goarch(); - const params = new URLSearchParams({ - title: `Support the \`${os}-${arch}\` platform`, - body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`, - }); - const uri = vscode.Uri.parse( - `https://github.com/coder/vscode-coder/issues/new?${params.toString()}`, - ); - vscode.env.openExternal(uri); - }); - throw new Error("Platform not supported"); + throw error; + } + + // Try .old-* binaries as fallback + 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; } - default: { - vscode.window - .showErrorMessage( - "Failed to download binary. Please open an issue.", - "Open an Issue", - ) - .then((value) => { - if (!value) { - return; - } - const params = new URLSearchParams({ - title: `Failed to download binary on \`${cliUtils.goos()}-${cliUtils.goarch()}\``, - body: `Received status code \`${status}\` when downloading the binary.`, - }); - const uri = vscode.Uri.parse( - `https://github.com/coder/vscode-coder/issues/new?${params.toString()}`, - ); - vscode.env.openExternal(uri); + } + + // No fallback available or user declined - re-throw original error + throw error; + } + + private async performBinaryDownload( + restClient: Api, + parsedVersion: semver.SemVer, + binPath: string, + progressLogPath: string, + ): Promise { + const cfg = vscode.workspace.getConfiguration("coder"); + const tempFile = + binPath + ".temp-" + Math.random().toString(36).substring(8); + + try { + const removed = await cliUtils.rmOld(binPath); + for (const { fileName, error } of removed) { + if (error) { + this.output.warn("Failed to remove", fileName, error); + } else { + this.output.info("Removed", fileName); + } + } + + // Figure out where to get the binary. + const binName = cliUtils.name(); + const configSource = cfg.get("binarySource"); + const binSource = configSource?.trim() ? 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 stat = await cliUtils.stat(binPath); + const etag = stat ? await cliUtils.eTag(binPath) : ""; + this.output.info("Using ETag", etag || ""); + + // Download the binary to a temporary file. + const writeStream = createWriteStream(tempFile, { + autoClose: true, + mode: 0o755, + }); + + const onProgress = async ( + bytesDownloaded: number, + totalBytes: number | null, + ) => { + await downloadProgress.writeProgress(progressLogPath, { + bytesDownloaded, + totalBytes, + status: "downloading", + }); + }; + + const client = restClient.getAxiosInstance(); + const status = await this.download( + client, + binSource, + writeStream, + { + "Accept-Encoding": "gzip", + "If-None-Match": `"${etag}"`, + }, + onProgress, + ); + + switch (status) { + case 200: { + await downloadProgress.writeProgress(progressLogPath, { + bytesDownloaded: 0, + totalBytes: null, + status: "verifying", }); - throw new Error("Failed to download binary"); + + if (cfg.get("disableSignatureVerification")) { + this.output.info( + "Skipping binary signature verification due to settings", + ); + } else { + await this.verifyBinarySignatures(client, tempFile, [ + // A signature placed at the same level as the binary. It must be + // named exactly the same with an appended `.asc` (such as + // coder-windows-amd64.exe.asc or coder-linux-amd64.asc). + binSource + ".asc", + // The releases.coder.com bucket does not include the leading "v", + // and unlike what we get from buildinfo it uses a truncated version + // with only major.minor.patch. The signature name follows the same + // rule as above. + `https://releases.coder.com/coder-cli/${parsedVersion.major}.${parsedVersion.minor}.${parsedVersion.patch}/${binName}.asc`, + ]); + } + + // Replace existing binary (handles both renames + Windows lock) + await this.replaceExistingBinary(binPath, tempFile); + + return binPath; + } + case 304: { + this.output.info("Using existing binary since server returned a 304"); + return binPath; + } + case 404: { + vscode.window + .showErrorMessage( + "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", + "Open an Issue", + ) + .then((value) => { + if (!value) { + return; + } + const os = cliUtils.goos(); + const arch = cliUtils.goarch(); + const params = new URLSearchParams({ + title: `Support the \`${os}-${arch}\` platform`, + body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`, + }); + const uri = vscode.Uri.parse( + `https://github.com/coder/vscode-coder/issues/new?${params.toString()}`, + ); + vscode.env.openExternal(uri); + }); + throw new Error("Platform not supported"); + } + default: { + vscode.window + .showErrorMessage( + "Failed to download binary. Please open an issue.", + "Open an Issue", + ) + .then((value) => { + if (!value) { + return; + } + const params = new URLSearchParams({ + title: `Failed to download binary on \`${cliUtils.goos()}-${cliUtils.goarch()}\``, + body: `Received status code \`${status}\` when downloading the binary.`, + }); + const uri = vscode.Uri.parse( + `https://github.com/coder/vscode-coder/issues/new?${params.toString()}`, + ); + vscode.env.openExternal(uri); + }); + throw new Error("Failed to download binary"); + } } + } finally { + await downloadProgress.clearProgress(progressLogPath); } } @@ -246,6 +451,10 @@ export class CliManager { source: string, writeStream: WriteStream, headers?: AxiosRequestConfig["headers"], + onProgress?: ( + bytesDownloaded: number, + totalBytes: number | null, + ) => Promise, ): Promise { const baseUrl = client.defaults.baseURL; @@ -306,6 +515,17 @@ export class CliManager { ? undefined : (buffer.byteLength / contentLength) * 100, }); + if (onProgress) { + onProgress( + written, + Number.isNaN(contentLength) ? null : contentLength, + ).catch((error) => { + this.output.warn( + "Failed to write progress log:", + errToStr(error), + ); + }); + } }); }); diff --git a/src/core/cliUtils.ts b/src/core/cliUtils.ts index cc92a345..2297cf77 100644 --- a/src/core/cliUtils.ts +++ b/src/core/cliUtils.ts @@ -1,10 +1,20 @@ -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"; + +/** + * Custom error thrown when a binary file is locked (typically on Windows). + */ +export class FileLockError extends Error { + constructor(binPath: string) { + super(`Binary is in use: ${binPath}`); + this.name = "WindowsFileLockError"; + } +} /** * Stat the path or undefined if the path does not exist. Throw if unable to @@ -77,7 +87,8 @@ export async function rmOld(binPath: string): Promise { if ( fileName.includes(".old-") || fileName.includes(".temp-") || - fileName.endsWith(".asc") + fileName.endsWith(".asc") || + fileName.endsWith(".progress.log") ) { try { await fs.rm(path.join(binDir, file), { force: true }); @@ -97,6 +108,52 @@ export async function rmOld(binPath: string): Promise { } } +/** + * Find all .old-* binaries in the same directory as the given binary path. + * Returns paths sorted by modification time (most recent first). + */ +export async function findOldBinaries(binPath: string): Promise { + const binDir = path.dirname(binPath); + const binName = path.basename(binPath); + try { + const files = await fs.readdir(binDir); + const oldBinaries = files + .filter((f) => f.startsWith(binName) && f.includes(".old-")) + .map((f) => path.join(binDir, f)); + + // Sort by modification time, most recent first + 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); + } catch (error) { + // If directory doesn't exist, return empty array + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return []; + } + throw error; + } +} + +export function maybeWrapFileLockError( + error: unknown, + binPath: string, +): unknown { + const code = (error as NodeJS.ErrnoException).code; + if (code === "EBUSY" || code === "EPERM") { + return new FileLockError(binPath); + } + return error; +} + /** * Return the etag (sha1) of the path. Throw if unable to hash the file. */ diff --git a/src/core/downloadProgress.ts b/src/core/downloadProgress.ts new file mode 100644 index 00000000..600c3139 --- /dev/null +++ b/src/core/downloadProgress.ts @@ -0,0 +1,44 @@ +import * as fs from "node:fs/promises"; +import * as path from "node:path"; + +export interface DownloadProgress { + bytesDownloaded: number; + totalBytes: number | null; + status: "downloading" | "verifying"; +} + +export async function writeProgress( + logPath: string, + progress: DownloadProgress, +): Promise { + await fs.mkdir(path.dirname(logPath), { recursive: true }); + await fs.writeFile(logPath, JSON.stringify({ ...progress }) + "\n"); +} + +export async function readProgress( + logPath: string, +): Promise { + try { + const content = await fs.readFile(logPath, "utf-8"); + const progress = JSON.parse(content) as DownloadProgress; + if ( + typeof progress.bytesDownloaded !== "number" || + (typeof progress.totalBytes !== "number" && + progress.totalBytes !== null) || + (progress.status !== "downloading" && progress.status !== "verifying") + ) { + return null; + } + return progress; + } catch { + return null; + } +} + +export async function clearProgress(logPath: string): Promise { + try { + await fs.rm(logPath, { force: true }); + } catch { + // If we cannot remove it now then we'll do it in the next startup + } +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 2ef46716..faf2a72d 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -1,3 +1,4 @@ +import { type IncomingMessage } from "node:http"; import { vi } from "vitest"; import * as vscode from "vscode"; @@ -8,7 +9,7 @@ import { type Logger } from "@/logging/logger"; * Use this to set configuration values that will be returned by vscode.workspace.getConfiguration(). */ export class MockConfigurationProvider { - private config = new Map(); + private readonly config = new Map(); constructor() { this.setupVSCodeMock(); @@ -298,3 +299,54 @@ export function createMockLogger(): Logger { error: vi.fn(), }; } + +export function createMockStream( + content: string, + options: { + chunkSize?: number; + delay?: number; + // If defined will throw an error instead of closing normally + error?: NodeJS.ErrnoException; + } = {}, +): IncomingMessage { + const { chunkSize = 8, delay = 1, error } = options; + + const buffer = Buffer.from(content); + let position = 0; + let closeCallback: ((...args: unknown[]) => void) | null = null; + let errorCallback: ((error: Error) => void) | null = null; + + return { + on: vi.fn((event: string, callback: (...args: unknown[]) => void) => { + if (event === "data") { + const sendChunk = () => { + if (position < buffer.length) { + const chunk = buffer.subarray( + position, + Math.min(position + chunkSize, buffer.length), + ); + position += chunkSize; + callback(chunk); + if (position < buffer.length) { + setTimeout(sendChunk, delay); + } else { + setImmediate(() => { + if (error && errorCallback) { + errorCallback(error); + } else if (closeCallback) { + closeCallback(); + } + }); + } + } + }; + setTimeout(sendChunk, delay); + } else if (event === "error") { + errorCallback = callback; + } else if (event === "close") { + closeCallback = callback; + } + }), + destroy: vi.fn(), + } as unknown as IncomingMessage; +} diff --git a/test/unit/core/binaryLock.test.ts b/test/unit/core/binaryLock.test.ts new file mode 100644 index 00000000..bab76e1a --- /dev/null +++ b/test/unit/core/binaryLock.test.ts @@ -0,0 +1,160 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { BinaryLock } from "@/core/binaryLock"; +import * as downloadProgress from "@/core/downloadProgress"; + +import { + createMockLogger, + MockProgressReporter, +} from "../../mocks/testHelpers"; + +vi.mock("vscode"); + +// Mock proper-lockfile +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(), +})); + +// Mock downloadProgress module +vi.mock("@/core/downloadProgress", () => ({ + STALE_TIMEOUT_MS: 15000, + readProgress: vi.fn(), + writeProgress: vi.fn(), + clearProgress: vi.fn(), +})); + +describe("BinaryLock", () => { + let binaryLock: BinaryLock; + let mockLogger: ReturnType; + let mockProgress: MockProgressReporter; + let mockRelease: () => Promise; + + const createLockError = () => { + const error = new Error("Lock is busy") as NodeJS.ErrnoException; + error.code = "ELOCKED"; + return error; + }; + + beforeEach(() => { + mockLogger = createMockLogger(); + mockProgress = new MockProgressReporter(); + mockRelease = vi.fn().mockResolvedValue(undefined); + + binaryLock = new BinaryLock(vscode, mockLogger); + }); + + describe("acquireLockOrWait", () => { + it("should acquire lock immediately when available", async () => { + const { lock } = await import("proper-lockfile"); + vi.mocked(lock).mockResolvedValue(mockRelease); + + const result = await binaryLock.acquireLockOrWait( + "/path/to/binary", + "/path/to/progress.log", + ); + + expect(result.release).toBe(mockRelease); + expect(result.waited).toBe(false); + expect(lock).toHaveBeenCalledWith("/path/to/binary", { + stale: 15000, + retries: 0, + realpath: false, + }); + }); + + it("should wait and monitor progress when lock is held", async () => { + const { lock } = await import("proper-lockfile"); + + vi.mocked(lock) + .mockRejectedValueOnce(createLockError()) + .mockResolvedValueOnce(mockRelease); + + vi.mocked(downloadProgress.readProgress).mockResolvedValue({ + bytesDownloaded: 1024, + totalBytes: 2048, + status: "downloading", + }); + + const result = await binaryLock.acquireLockOrWait( + "/path/to/binary", + "/path/to/progress.log", + ); + + expect(result.release).toBe(mockRelease); + expect(result.waited).toBe(true); + + const reports = mockProgress.getProgressReports(); + expect(reports.length).toBeGreaterThan(0); + expect(reports[0].message).toBe("1.02 kB / 2.05 kB"); + }); + + it.each([ + { + name: "downloading with known size", + progress: { + bytesDownloaded: 5000000, + totalBytes: 10000000, + status: "downloading" as const, + }, + expectedMessage: "5 MB / 10 MB", + }, + { + name: "downloading with unknown size", + progress: { + bytesDownloaded: 1024, + totalBytes: null, + status: "downloading" as const, + }, + expectedMessage: "1.02 kB / unknown", + }, + { + name: "verifying signature", + progress: { + bytesDownloaded: 0, + totalBytes: null, + status: "verifying" as const, + }, + expectedMessage: "Verifying signature...", + }, + ])( + "should report progress while waiting: $name", + async ({ progress, expectedMessage }) => { + const { lock } = await import("proper-lockfile"); + + let callCount = 0; + vi.mocked(lock).mockImplementation(() => { + callCount++; + if (callCount === 1) { + return Promise.reject(createLockError()); + } + return Promise.resolve(mockRelease); + }); + + vi.mocked(downloadProgress.readProgress).mockResolvedValue(progress); + + await binaryLock.acquireLockOrWait( + "/path/to/binary", + "/path/to/progress.log", + ); + + const reports = mockProgress.getProgressReports(); + expect(reports.length).toBeGreaterThan(0); + expect(reports[0].message).toContain(expectedMessage); + }, + ); + + it("should re-throw non-ELOCKED errors", async () => { + const { lock } = await import("proper-lockfile"); + const testError = new Error("Filesystem error"); + vi.mocked(lock).mockRejectedValue(testError); + + await expect( + binaryLock.acquireLockOrWait( + "/path/to/binary", + "/path/to/progress.log", + ), + ).rejects.toThrow("Filesystem error"); + }); + }); +}); diff --git a/test/unit/core/cliManager.concurrent.test.ts b/test/unit/core/cliManager.concurrent.test.ts new file mode 100644 index 00000000..457d8a31 --- /dev/null +++ b/test/unit/core/cliManager.concurrent.test.ts @@ -0,0 +1,191 @@ +/** + * This file tests that multiple concurrent calls to fetchBinary properly coordinate + * using proper-lockfile to prevent race conditions. Unlike the main cliManager.test.ts, + * this test uses the real filesystem and doesn't mock the locking library to verify + * actual file-level coordination. + */ +import { type AxiosInstance } from "axios"; +import { type Api } from "coder/site/src/api/api"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { CliManager } from "@/core/cliManager"; +import * as cliUtils from "@/core/cliUtils"; +import { PathResolver } from "@/core/pathResolver"; +import * as pgp from "@/pgp"; + +import { + createMockLogger, + createMockStream, + MockConfigurationProvider, + MockProgressReporter, +} from "../../mocks/testHelpers"; + +vi.mock("@/pgp"); +vi.mock("@/core/cliUtils", async () => { + const actual = await vi.importActual("@/core/cliUtils"); + return { + ...actual, + goos: vi.fn(), + goarch: vi.fn(), + name: vi.fn(), + version: vi.fn(), + }; +}); + +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.version).mockResolvedValue(version); + vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); +} + +function createMockApi( + version: string, + options: { + chunkSize?: number; + delay?: number; + error?: NodeJS.ErrnoException; + } = {}, +): Api { + const mockAxios = { + get: vi.fn().mockImplementation(() => + Promise.resolve({ + status: 200, + headers: { "content-length": "17" }, + data: createMockStream(`mock-binary-v${version}`, options), + }), + ), + defaults: { baseURL: "https://test.coder.com" }, + } as unknown as AxiosInstance; + + return { + getAxiosInstance: () => mockAxios, + getBuildInfo: vi.fn().mockResolvedValue({ version }), + } as unknown as Api; +} + +function setupManager(testDir: string): CliManager { + const _mockProgress = new MockProgressReporter(); + const mockConfig = new MockConfigurationProvider(); + mockConfig.set("coder.disableSignatureVerification", true); + + return new CliManager( + vscode, + createMockLogger(), + new PathResolver(testDir, "/code/log"), + ); +} + +describe("CliManager Concurrent Downloads", () => { + let testDir: string; + + beforeEach(async () => { + testDir = await fs.mkdtemp( + path.join(os.tmpdir(), "climanager-concurrent-"), + ); + }); + + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it("handles multiple concurrent downloads without race conditions", async () => { + const manager = setupManager(testDir); + setupCliUtilsMocks("1.2.3"); + const mockApi = createMockApi("1.2.3"); + + const label = "test.coder.com"; + const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); + + const downloads = await Promise.all([ + manager.fetchBinary(mockApi, label), + manager.fetchBinary(mockApi, label), + manager.fetchBinary(mockApi, label), + ]); + + expect(downloads).toHaveLength(3); + for (const result of downloads) { + expect(result).toBe(binaryPath); + } + + // 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(); + }); + + it("redownloads when version mismatch is detected concurrently", async () => { + const manager = setupManager(testDir); + setupCliUtilsMocks("1.2.3"); + vi.mocked(cliUtils.version).mockImplementation(async (binPath) => { + const fileContent = await fs.readFile(binPath, { + encoding: "utf-8", + }); + return fileContent.includes("1.2.3") ? "1.2.3" : "2.0.0"; + }); + + // First call downloads 1.2.3, next two expect 2.0.0 (server upgraded) + const mockApi1 = createMockApi("1.2.3", { delay: 100 }); + const mockApi2 = createMockApi("2.0.0"); + + const label = "test.coder.com"; + const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); + + // Start first call and give it time to acquire the lock + const firstDownload = manager.fetchBinary(mockApi1, label); + // Wait for the lock to be acquired before starting concurrent calls + await new Promise((resolve) => setTimeout(resolve, 50)); + + const downloads = await Promise.all([ + firstDownload, + manager.fetchBinary(mockApi2, label), + manager.fetchBinary(mockApi2, label), + ]); + + expect(downloads).toHaveLength(3); + for (const result of downloads) { + expect(result).toBe(binaryPath); + } + + // Binary should be updated to 2.0.0, lock/progress files cleaned up + await expect(fs.access(binaryPath)).resolves.toBeUndefined(); + const finalContent = await fs.readFile(binaryPath, "utf8"); + expect(finalContent).toContain("v2.0.0"); + await expect(fs.access(binaryPath + ".lock")).rejects.toThrow(); + await expect(fs.access(binaryPath + ".progress.log")).rejects.toThrow(); + }); + + it.each([ + { + name: "disk storage insufficient", + code: "ENOSPC", + message: "no space left on device", + }, + { + name: "connection timeout", + code: "ETIMEDOUT", + message: "connection timed out", + }, + ])("handles $name error during download", async ({ code, message }) => { + const manager = setupManager(testDir); + setupCliUtilsMocks("1.2.3"); + + const error = new Error(`${code}: ${message}`); + (error as NodeJS.ErrnoException).code = code; + const mockApi = createMockApi("1.2.3", { error }); + + const label = "test.coder.com"; + const binaryPath = path.join(testDir, label, "bin", "coder-linux-amd64"); + + await expect(manager.fetchBinary(mockApi, label)).rejects.toThrow( + `Unable to download binary: ${code}: ${message}`, + ); + + await expect(fs.access(binaryPath + ".lock")).rejects.toThrow(); + }); +}); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index d4f16c87..95755d31 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -1,11 +1,11 @@ import globalAxios, { type AxiosInstance } from "axios"; import { type Api } from "coder/site/src/api/api"; -import EventEmitter from "events"; -import * as fs from "fs"; -import { type IncomingMessage } from "http"; import { fs as memfs, vol } from "memfs"; -import * as os from "os"; -import * as path from "path"; +import EventEmitter from "node:events"; +import * as fs from "node:fs"; +import { type IncomingMessage } from "node:http"; +import * as os from "node:os"; +import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; @@ -16,6 +16,7 @@ import * as pgp from "@/pgp"; import { createMockLogger, + createMockStream, MockConfigurationProvider, MockProgressReporter, MockUserInteraction, @@ -24,7 +25,6 @@ import { expectPathsEqual } from "../../utils/platform"; vi.mock("os"); vi.mock("axios"); -vi.mock("@/pgp"); vi.mock("fs", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); @@ -42,6 +42,14 @@ vi.mock("fs/promises", async () => { }; }); +// Mock lockfile to bypass file locking in tests +vi.mock("proper-lockfile", () => ({ + lock: () => Promise.resolve(() => Promise.resolve()), + check: () => Promise.resolve(false), +})); + +vi.mock("@/pgp"); + vi.mock("@/core/cliUtils", async () => { const actual = await vi.importActual("@/core/cliUtils"); @@ -676,11 +684,11 @@ describe("CliManager", () => { } function withSignatureResponses(statuses: number[]): void { - statuses.forEach((status) => { + for (const status of statuses) { const data = status === 200 ? createMockStream("mock-signature-content") : undefined; withHttpResponse(status, {}, data); - }); + } } function withHttpResponse( @@ -730,70 +738,26 @@ describe("CliManager", () => { withHttpResponse(200, { "content-length": "1024" }, errorStream); } } - - function createMockStream( - content: string, - options: { chunkSize?: number; delay?: number } = {}, - ): IncomingMessage { - const { chunkSize = 8, delay = 1 } = options; - - const buffer = Buffer.from(content); - let position = 0; - let closeCallback: ((...args: unknown[]) => void) | null = null; - - return { - on: vi.fn((event: string, callback: (...args: unknown[]) => void) => { - if (event === "data") { - // Send data in chunks - const sendChunk = () => { - if (position < buffer.length) { - const chunk = buffer.subarray( - position, - Math.min(position + chunkSize, buffer.length), - ); - position += chunkSize; - callback(chunk); - if (position < buffer.length) { - setTimeout(sendChunk, delay); - } else { - // All chunks sent - use setImmediate to ensure close happens - // after all synchronous operations and I/O callbacks complete - setImmediate(() => { - if (closeCallback) { - closeCallback(); - } - }); - } - } - }; - setTimeout(sendChunk, delay); - } else if (event === "close") { - closeCallback = callback; - } - }), - destroy: vi.fn(), - } as unknown as IncomingMessage; - } - - function createVerificationError(msg: string): pgp.VerificationError { - const error = new pgp.VerificationError( - pgp.VerificationErrorCode.Invalid, - msg, - ); - vi.mocked(error.summary).mockReturnValue("Signature does not match"); - return error; - } - - 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 readdir(dir: string): string[] { - return memfs.readdirSync(dir) as string[]; - } }); + +function createVerificationError(msg: string): pgp.VerificationError { + const error = new pgp.VerificationError( + pgp.VerificationErrorCode.Invalid, + msg, + ); + vi.mocked(error.summary).mockReturnValue("Signature does not match"); + return error; +} + +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 readdir(dir: string): string[] { + return memfs.readdirSync(dir) as string[]; +} diff --git a/test/unit/core/downloadProgress.test.ts b/test/unit/core/downloadProgress.test.ts new file mode 100644 index 00000000..b39e82b6 --- /dev/null +++ b/test/unit/core/downloadProgress.test.ts @@ -0,0 +1,102 @@ +import { promises as fs } from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import * as downloadProgress from "@/core/downloadProgress"; + +describe("downloadProgress", () => { + let testDir: string; + let testLogPath: string; + + beforeEach(async () => { + testDir = await fs.mkdtemp( + path.join(os.tmpdir(), "download-progress-test-"), + ); + testLogPath = path.join(testDir, "test.progress.log"); + }); + + afterEach(async () => { + try { + await fs.rm(testDir, { recursive: true, force: true }); + } catch { + // Ignore + } + }); + + describe("writeProgress", () => { + it("should write and overwrite progress", async () => { + await downloadProgress.writeProgress(testLogPath, { + bytesDownloaded: 1000, + totalBytes: 10000, + status: "downloading", + }); + const first = JSON.parse( + (await fs.readFile(testLogPath, "utf-8")).trim(), + ); + expect(first.bytesDownloaded).toBe(1000); + + await downloadProgress.writeProgress(testLogPath, { + bytesDownloaded: 2000, + totalBytes: null, + status: "verifying", + }); + const second = JSON.parse( + (await fs.readFile(testLogPath, "utf-8")).trim(), + ); + expect(second.bytesDownloaded).toBe(2000); + expect(second.totalBytes).toBeNull(); + }); + + it("should create nested directories", async () => { + const nestedPath = path.join(testDir, "nested", "dir", "progress.log"); + await downloadProgress.writeProgress(nestedPath, { + bytesDownloaded: 500, + totalBytes: 5000, + status: "downloading", + }); + expect(await fs.readFile(nestedPath, "utf-8")).toBeTruthy(); + }); + }); + + describe("readProgress", () => { + it("should read progress from log file", async () => { + const expectedProgress = { + bytesDownloaded: 1500, + totalBytes: 10000, + status: "downloading", + }; + + await fs.writeFile(testLogPath, JSON.stringify(expectedProgress) + "\n"); + const progress = await downloadProgress.readProgress(testLogPath); + expect(progress).toEqual(expectedProgress); + }); + + it("should return null for missing, empty, or invalid files", async () => { + expect( + await downloadProgress.readProgress(path.join(testDir, "nonexistent")), + ).toBeNull(); + + await fs.writeFile(testLogPath, ""); + expect(await downloadProgress.readProgress(testLogPath)).toBeNull(); + + await fs.writeFile(testLogPath, "invalid json"); + expect(await downloadProgress.readProgress(testLogPath)).toBeNull(); + + await fs.writeFile(testLogPath, JSON.stringify({ incomplete: true })); + expect(await downloadProgress.readProgress(testLogPath)).toBeNull(); + }); + }); + + describe("clearProgress", () => { + it("should remove existing file or ignore missing file", async () => { + await fs.writeFile(testLogPath, "test"); + await downloadProgress.clearProgress(testLogPath); + await expect(fs.readFile(testLogPath)).rejects.toThrow(); + + await expect( + downloadProgress.clearProgress(path.join(testDir, "nonexistent")), + ).resolves.toBeUndefined(); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index ea35d101..b2527a90 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1288,6 +1288,13 @@ resolved "https://registry.yarnpkg.com/@types/normalize-package-data/-/normalize-package-data-2.4.4.tgz#56e2cc26c397c038fab0e3a917a12d5c5909e901" integrity sha512-37i+OaWTh9qeK4LSHPsyRC7NahnGotNuZvjLSgcPzblpHB3rrCJxAOgI5gCdKm7coonsaX1Of0ILiTcnZjbfxA== +"@types/proper-lockfile@^4.1.4": + version "4.1.4" + resolved "https://registry.yarnpkg.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz#cd9fab92bdb04730c1ada542c356f03620f84008" + integrity sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ== + dependencies: + "@types/retry" "*" + "@types/responselike@^1.0.0": version "1.0.3" resolved "https://registry.yarnpkg.com/@types/responselike/-/responselike-1.0.3.tgz#cc29706f0a397cfe6df89debfe4bf5cea159db50" @@ -1295,6 +1302,11 @@ dependencies: "@types/node" "*" +"@types/retry@*": + version "0.12.5" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.5.tgz#f090ff4bd8d2e5b940ff270ab39fd5ca1834a07e" + integrity sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw== + "@types/sarif@^2.1.7": version "2.1.7" resolved "https://registry.yarnpkg.com/@types/sarif/-/sarif-2.1.7.tgz#dab4d16ba7568e9846c454a8764f33c5d98e5524" @@ -6825,6 +6837,15 @@ progress@^2.0.0, progress@^2.0.3: resolved "https://registry.yarnpkg.com/progress/-/progress-2.0.3.tgz#7e8cf8d8f5b8f239c1bc68beb4eb78567d572ef8" integrity sha512-7PiHtLll5LdnKIMw100I+8xJXR5gW2QwWYkT6iJva0bXitZKa/XMrSbdmg3r2Xnaidz9Qumd0VPaMrZlF9V9sA== +proper-lockfile@^4.1.2: + version "4.1.2" + resolved "https://registry.yarnpkg.com/proper-lockfile/-/proper-lockfile-4.1.2.tgz#c8b9de2af6b2f1601067f98e01ac66baa223141f" + integrity sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA== + dependencies: + graceful-fs "^4.2.4" + retry "^0.12.0" + signal-exit "^3.0.2" + proxy-agent@^6.5.0: version "6.5.0" resolved "https://registry.yarnpkg.com/proxy-agent/-/proxy-agent-6.5.0.tgz#9e49acba8e4ee234aacb539f89ed9c23d02f232d" @@ -7686,6 +7707,11 @@ restore-cursor@^5.0.0: onetime "^7.0.0" signal-exit "^4.1.0" +retry@^0.12.0: + version "0.12.0" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b" + integrity sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow== + reusify@^1.0.4: version "1.0.4" resolved "https://registry.yarnpkg.com/reusify/-/reusify-1.0.4.tgz#90da382b1e126efc02146e90845a88db12925d76"