From c79bacf1906ae4d925d36305a1e89ff89afd6542 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 8 Jun 2026 10:39:10 +0000 Subject: [PATCH 1/7] feat: add telemetry for CLI and remote setup --- src/core/cliCredentialManager.ts | 15 +- src/core/cliManager.ts | 488 ++++++++++++------ src/instrumentation/remoteSetup.ts | 8 +- src/instrumentation/websocket.ts | 49 +- src/remote/remote.ts | 116 +++-- src/websocket/reconnectingWebSocket.ts | 15 +- test/mocks/testHelpers.ts | 5 +- test/unit/core/cliCredentialManager.test.ts | 30 +- test/unit/core/cliManager.test.ts | 93 +++- test/unit/instrumentation/remoteSetup.test.ts | 36 +- test/unit/instrumentation/websocket.test.ts | 41 +- .../websocket/reconnectingWebSocket.test.ts | 7 +- 12 files changed, 667 insertions(+), 236 deletions(-) diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 245f1a5e1a..37fe4ddca7 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -31,6 +31,11 @@ const execFileAsync = promisify(execFile); type KeyringFeature = "keyringAuth" | "keyringTokenRead"; +export interface CliCredentialStoreResult { + readonly mode: "keyring" | "file"; + readonly credentialSource: "session_token" | "empty_token"; +} + const EXEC_TIMEOUT_MS = 60_000; const EXEC_LOG_INTERVAL_MS = 5_000; @@ -71,13 +76,15 @@ export class CliCredentialManager { * * Keyring and files are mutually exclusive, never both. */ - public storeToken( + public async storeToken( url: string, token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - return this.credentialTelemetry.traceStore(configs, async (span) => { + ): Promise { + const credentialSource = token === "" ? "empty_token" : "session_token"; + let mode: CliCredentialStoreResult["mode"] = "file"; + await this.credentialTelemetry.traceStore(configs, async (span) => { const binPath = await this.resolveKeyringBinary( url, configs, @@ -88,9 +95,11 @@ export class CliCredentialManager { await this.writeCredentialFiles(url, token); return; } + mode = "keyring"; span.setProperty("category", "keyring"); await this.storeKeyringToken(binPath, url, token, configs, options); }); + return { mode, credentialSource }; } private async storeKeyringToken( diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index b65f0f0a6c..58ab207a75 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,6 +10,7 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; +import { isAbortError } from "../error/errorUtils"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; @@ -38,6 +39,26 @@ type ResolvedBinary = type CliDownloadReason = "missing" | "version_mismatch" | "unreadable"; +type CliResolveOutcome = + | "cache_hit" + | "downloaded" + | "lock_wait_cache_hit" + | "download_disabled_fallback" + | "fallback_to_existing_binary"; + +type CliVersionCheckOutcome = "missing" | "match" | "mismatch" | "unreadable"; + +type CliConfigureFailureCategory = + | "cancelled" + | "filesystem" + | "credential_store" + | "unknown"; + +type CliResolveFailureCategory = + | "downloads_disabled" + | "download" + | "fallback_declined"; + type CliVerifyResult = | { kind: "verified" } | { kind: "bypassed" } @@ -124,157 +145,265 @@ export class CliManager { * downloads being disabled. */ public async fetchBinary(restClient: Api): Promise { - const baseUrl = restClient.getAxiosInstance().defaults.baseURL; - if (!baseUrl) { - throw new Error("REST client has no base URL configured"); - } - const safeHostname = toSafeHost(baseUrl); - const cfg = vscode.workspace.getConfiguration("coder"); - // Settings can be undefined when set to their defaults (true in this case), - // so explicitly check against false. - const enableDownloads = cfg.get("enableDownloads") !== false; - this.output.debug( - "Downloads are", - enableDownloads ? "enabled" : "disabled", - ); + return this.telemetry.trace("cli.resolve", async (span) => { + const baseUrl = restClient.getAxiosInstance().defaults.baseURL; + if (!baseUrl) { + span.setProperty("failureCategory", "unknown"); + throw new Error("REST client has no base URL configured"); + } + const safeHostname = toSafeHost(baseUrl); + const cfg = vscode.workspace.getConfiguration("coder"); + // Settings can be undefined when set to their defaults (true in this case), + // so explicitly check against false. + const enableDownloads = cfg.get("enableDownloads") !== false; + span.setProperty("downloadsEnabled", enableDownloads); + this.output.debug( + "Downloads are", + enableDownloads ? "enabled" : "disabled", + ); - // Get the build info to compare with the existing binary version, if any, - // and to log for debugging. - const buildInfo = await restClient.getBuildInfo(); - this.output.info("Got server version", buildInfo.version); - const parsedVersion = semver.parse(buildInfo.version); - if (!parsedVersion) { - throw new Error( - `Got invalid version from deployment: ${buildInfo.version}`, + const resolved = await span.phase("cache_lookup", async (cacheSpan) => { + const result = await this.resolveBinaryPath(safeHostname); + cacheSpan.setProperty("source", result.source); + return result; + }); + span.setProperty("cacheSource", resolved.source); + this.output.debug( + `Resolved binary: ${resolved.binPath} (${resolved.source})`, ); - } - const resolved = await this.resolveBinaryPath(safeHostname); - this.output.debug( - `Resolved binary: ${resolved.binPath} (${resolved.source})`, - ); + const versionResult = await span.phase( + "version_check", + async (versionSpan) => { + // Get the build info to compare with the existing binary version, if any, + // and to log for debugging. + const buildInfo = await restClient.getBuildInfo(); + this.output.info("Got server version", buildInfo.version); + const parsedVersion = semver.parse(buildInfo.version); + if (!parsedVersion) { + throw new Error( + `Got invalid version from deployment: ${buildInfo.version}`, + ); + } - let existingVersion: string | null = null; - let downloadReason: CliDownloadReason; - if (resolved.source === "not-found") { - downloadReason = "missing"; - this.output.info("No existing binary found, starting download"); - } else { - this.output.debug( - "Existing binary size is", - prettyBytes(resolved.stat.size), + let existingVersion: string | null = null; + let downloadReason: CliDownloadReason; + let outcome: CliVersionCheckOutcome; + if (resolved.source === "not-found") { + downloadReason = "missing"; + outcome = "missing"; + this.output.info("No existing binary found, starting download"); + } else { + this.output.debug( + "Existing binary size is", + prettyBytes(resolved.stat.size), + ); + try { + existingVersion = await cliVersion(resolved.binPath); + this.output.debug("Existing binary version is", existingVersion); + downloadReason = "version_mismatch"; + outcome = + existingVersion === buildInfo.version ? "match" : "mismatch"; + } catch (error) { + this.output.warn( + "Unable to get version of existing binary, downloading instead", + error, + ); + downloadReason = "unreadable"; + outcome = "unreadable"; + } + } + versionSpan.setProperty("outcome", outcome); + return { + buildInfo, + parsedVersion, + existingVersion, + downloadReason, + outcome, + }; + }, ); - try { - existingVersion = await cliVersion(resolved.binPath); - this.output.debug("Existing binary version is", existingVersion); - downloadReason = "version_mismatch"; - } catch (error) { + span.setProperty("versionCheck", versionResult.outcome); + const { buildInfo, parsedVersion, existingVersion, downloadReason } = + versionResult; + + if (existingVersion === buildInfo.version) { + this.output.debug("Existing binary matches server version"); + span.setProperty("outcome", "cache_hit" satisfies CliResolveOutcome); + return resolved.binPath; + } + + await span.phase("download_decision", (decisionSpan) => { + decisionSpan.setProperty("reason", downloadReason); + decisionSpan.setProperty("downloadsEnabled", enableDownloads); + decisionSpan.setProperty( + "outcome", + enableDownloads + ? "download" + : existingVersion + ? "fallback" + : "blocked", + ); + return Promise.resolve(); + }); + span.setProperty("downloadReason", downloadReason); + + if (!enableDownloads) { + if (existingVersion) { + this.output.info( + "Using existing binary despite version mismatch because downloads are disabled", + ); + span.setProperty( + "outcome", + "download_disabled_fallback" satisfies CliResolveOutcome, + ); + return resolved.binPath; + } this.output.warn( - "Unable to get version of existing binary, downloading instead", - error, + "Unable to download CLI because downloads are disabled", + ); + span.setProperty( + "failureCategory", + "downloads_disabled" satisfies CliResolveFailureCategory, + ); + throw new Error( + "Unable to download CLI because downloads are disabled", ); - downloadReason = "unreadable"; } - } - if (existingVersion === buildInfo.version) { - this.output.debug("Existing binary matches server version"); - return resolved.binPath; - } - - if (!enableDownloads) { if (existingVersion) { this.output.info( - "Using existing binary despite version mismatch because downloads are disabled", + "Downloading since existing binary does not match the server version", ); - return resolved.binPath; } - this.output.warn("Unable to download CLI because downloads are disabled"); - throw new Error("Unable to download CLI because downloads are disabled"); - } - if (existingVersion) { - this.output.info( - "Downloading since existing binary does not match the server version", + // Always download using the platform-specific name. + const downloadBinPath = path.join( + path.dirname(resolved.binPath), + cliUtils.fullName(), ); - } - - // 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 }); - const progressLogPath = downloadBinPath + ".progress.log"; + // Create the `bin` folder if it doesn't exist + await fs.mkdir(path.dirname(downloadBinPath), { recursive: true }); + const progressLogPath = downloadBinPath + ".progress.log"; - let lockResult: - | { release: () => Promise; waited: boolean } - | undefined; - let latestVersion = parsedVersion; - try { - lockResult = await this.binaryLock.acquireLockOrWait( - downloadBinPath, - progressLogPath, - ); - this.output.debug("Acquired download lock"); - - // Another process may have finished the download while we waited. - if (lockResult.waited) { - const latestBuildInfo = await restClient.getBuildInfo(); - this.output.debug("Got latest server version", latestBuildInfo.version); + let lockResult: + | { release: () => Promise; waited: boolean } + | undefined; + let latestVersion = parsedVersion; + try { + lockResult = await span.phase("lock_wait", async (lockSpan) => { + const result = await this.binaryLock.acquireLockOrWait( + downloadBinPath, + progressLogPath, + ); + lockSpan.setProperty("waited", result.waited); + return result; + }); + this.output.debug("Acquired download lock"); + + // Another process may have finished the download while we waited. + if (lockResult.waited) { + const waitResult = await span.phase( + "lock_wait_recheck", + async (recheckSpan) => { + const latestBuildInfo = await restClient.getBuildInfo(); + this.output.debug( + "Got latest server version", + latestBuildInfo.version, + ); - const recheckAfterWait = await this.checkBinaryVersion( - downloadBinPath, - latestBuildInfo.version, - ); - if (recheckAfterWait.matches) { - this.output.debug("Binary already matches server version after wait"); - return await this.renameToFinalPath(resolved, downloadBinPath); - } + const recheckAfterWait = await this.checkBinaryVersion( + downloadBinPath, + latestBuildInfo.version, + ); + recheckSpan.setProperty( + "outcome", + recheckAfterWait.matches + ? "match" + : recheckAfterWait.version + ? "mismatch" + : "missing", + ); + if (recheckAfterWait.matches) { + return { matches: true as const }; + } - const latestParsedVersion = semver.parse(latestBuildInfo.version); - if (!latestParsedVersion) { - throw new Error( - `Got invalid version from deployment: ${latestBuildInfo.version}`, + const latestParsedVersion = semver.parse(latestBuildInfo.version); + if (!latestParsedVersion) { + throw new Error( + `Got invalid version from deployment: ${latestBuildInfo.version}`, + ); + } + return { + matches: false as const, + parsedVersion: latestParsedVersion, + }; + }, ); + if (waitResult.matches) { + this.output.debug( + "Binary already matches server version after wait", + ); + span.setProperty( + "outcome", + "lock_wait_cache_hit" satisfies CliResolveOutcome, + ); + return await this.renameToFinalPath(resolved, downloadBinPath); + } + latestVersion = waitResult.parsedVersion; } - latestVersion = latestParsedVersion; - } - return await this.telemetry.trace( - "cli.download", - async (span) => { - const downloadedBinPath = await this.performBinaryDownload( - restClient, - latestVersion, - downloadBinPath, - progressLogPath, - span, - ); - return this.renameToFinalPath(resolved, downloadedBinPath); - }, - { reason: downloadReason }, - ); - } catch (error) { - 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(); - this.output.debug("Released download lock"); + const result = await this.telemetry.trace( + "cli.download", + async (span) => { + const downloadedBinPath = await this.performBinaryDownload( + restClient, + latestVersion, + downloadBinPath, + progressLogPath, + span, + ); + return this.renameToFinalPath(resolved, downloadedBinPath); + }, + { reason: downloadReason }, + ); + span.setProperty("outcome", "downloaded" satisfies CliResolveOutcome); + return result; + } catch (error) { + return await span.phase( + "fallback_to_existing_binary", + async (fallbackSpan) => { + fallbackSpan.setProperty( + "failureCategory", + this.categorizeResolveFailure(error), + ); + 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); + } + span.setProperty( + "outcome", + "fallback_to_existing_binary" satisfies CliResolveOutcome, + ); + return resolved.binPath; + }, + ); + } finally { + if (lockResult) { + await lockResult.release(); + this.output.debug("Released download lock"); + } } - } + }); } /** @@ -876,34 +1005,55 @@ export class CliManager { throw new Error("URL is required to configure the CLI"); } - const configs = vscode.workspace.getConfiguration(); + return this.telemetry.trace("cli.configure", async (span) => { + const configs = vscode.workspace.getConfiguration(); + span.setProperty("silent", options?.silent === true); - if (options?.silent) { - try { - await this.cliCredentialManager.storeToken(url, token, configs); - } catch (error) { - this.handleStoreError(error); + if (options?.silent) { + try { + const result = await this.cliCredentialManager.storeToken( + url, + token, + configs, + ); + span.setProperty("configMode", result.mode); + span.setProperty("credentialSource", result.credentialSource); + } catch (error) { + span.setProperty( + "failureCategory", + this.categorizeConfigureFailure(error), + ); + this.handleStoreError(error); + } + return; } - return; - } - const result = await withCancellableProgress( - ({ signal }) => - this.cliCredentialManager.storeToken(url, token, configs, { signal }), - { - location: vscode.ProgressLocation.Notification, - title: `Storing credentials for ${url}`, - cancellable: true, - }, - ); - if (result.ok) { - return; - } - if (result.cancelled) { - this.output.info("Credential storage cancelled by user"); - return; - } - this.handleStoreError(result.error); + const result = await withCancellableProgress( + ({ signal }) => + this.cliCredentialManager.storeToken(url, token, configs, { signal }), + { + location: vscode.ProgressLocation.Notification, + title: `Storing credentials for ${url}`, + cancellable: true, + }, + ); + if (result.ok) { + span.setProperty("configMode", result.value.mode); + span.setProperty("credentialSource", result.value.credentialSource); + return; + } + if (result.cancelled) { + this.output.info("Credential storage cancelled by user"); + span.setProperty("failureCategory", "cancelled"); + span.markAborted(); + return; + } + span.setProperty( + "failureCategory", + this.categorizeConfigureFailure(result.error), + ); + this.handleStoreError(result.error); + }); } /** @@ -932,6 +1082,38 @@ export class CliManager { } } + private categorizeConfigureFailure( + error: unknown, + ): CliConfigureFailureCategory { + if (isAbortError(error)) { + return "cancelled"; + } + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (typeof code === "string" && code.startsWith("E")) { + return "filesystem"; + } + if (error instanceof Error) { + return "credential_store"; + } + return "unknown"; + } + + private categorizeResolveFailure(error: unknown): CliResolveFailureCategory { + if ( + error instanceof Error && + error.message === "Unable to download CLI because downloads are disabled" + ) { + return "downloads_disabled"; + } + if ( + error instanceof Error && + (error.message.includes("declined") || error.message.includes("aborted")) + ) { + return "fallback_declined"; + } + return "download"; + } + private handleStoreError(error: unknown): void { this.output.error("Failed to store credentials:", error); vscode.window diff --git a/src/instrumentation/remoteSetup.ts b/src/instrumentation/remoteSetup.ts index c3b49e46b0..c675366e80 100644 --- a/src/instrumentation/remoteSetup.ts +++ b/src/instrumentation/remoteSetup.ts @@ -1,10 +1,16 @@ import type { TelemetryService } from "../telemetry/service"; export type RemoteSetupPhase = + | "cli_resolve" + | "cli_configure" + | "compatibility_check" | "workspace_lookup" + | "workspace_monitor_setup" | "workspace_ready" | "resolve_agent" - | "ssh_config_write"; + | "ssh_config_write" + | "ssh_monitor_setup" + | "vscode_handoff"; /** Reason for a non-throwing early exit from `remote.setup`. */ export type RemoteSetupOutcome = "workspace_not_found" | "incompatible_server"; diff --git a/src/instrumentation/websocket.ts b/src/instrumentation/websocket.ts index 0db3760537..25bd4f2bf9 100644 --- a/src/instrumentation/websocket.ts +++ b/src/instrumentation/websocket.ts @@ -39,6 +39,42 @@ interface ReconnectCycle { readonly startMs: number; readonly reason: ConnectionStateReason; attempts: number; + maxBackoffMs: number; +} + +function bucketAttempts(attempts: number): string { + if (attempts <= 0) { + return "0"; + } + if (attempts === 1) { + return "1"; + } + if (attempts === 2) { + return "2"; + } + if (attempts <= 5) { + return "3-5"; + } + if (attempts <= 10) { + return "6-10"; + } + return "11+"; +} + +function bucketBackoff(ms: number): string { + if (ms <= 0) { + return "none"; + } + if (ms < 1000) { + return "<1s"; + } + if (ms < 5000) { + return "1-5s"; + } + if (ms < 30000) { + return "5-30s"; + } + return "30s+"; } interface DropOptions { @@ -136,6 +172,7 @@ export class WebSocketTelemetry { startMs: performance.now(), reason, attempts: 0, + maxBackoffMs: 0, }; } @@ -146,9 +183,17 @@ export class WebSocketTelemetry { } /** Drop and (re)open a reconnect cycle. */ - public retrying(reason: ConnectionStateReason, options: DropOptions): void { + public retrying( + reason: ConnectionStateReason, + options: DropOptions, + backoffMs: number, + ): void { this.dropped(options.cause, options.code, options.error); this.reconnectStarted(reason); + const cycle = this.#reconnectCycle; + if (cycle) { + cycle.maxBackoffMs = Math.max(cycle.maxBackoffMs, backoffMs); + } } #finishReconnect(outcome: ReconnectOutcome): void { @@ -161,6 +206,8 @@ export class WebSocketTelemetry { const properties: Record = { result: outcome.result, reason: cycle.reason, + attemptBucket: bucketAttempts(cycle.attempts), + maxBackoffBucket: bucketBackoff(cycle.maxBackoffMs), }; if (outcome.result === "error") { properties.terminationReason = outcome.terminationReason; diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 1e8012f247..6e0852350a 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -238,35 +238,45 @@ export class Remote { // Store for use in commands. this.commands.remoteWorkspaceClient = workspaceClient; - const binaryPath = await this.resolveRemoteBinary(workspaceClient); + const binaryPath = await tracer.phase("cli_resolve", () => + this.resolveRemoteBinary(workspaceClient), + ); // Write token to keyring or file if (baseUrl && token !== undefined) { - await this.cliManager.configure(baseUrl, token); + await tracer.phase("cli_configure", () => + this.cliManager.configure(baseUrl, token), + ); } // Listen for token changes for this deployment disposables.push(this.watchRemoteSessionAuth(context, workspaceClient)); - // First thing is to check the version. - const buildInfo = await workspaceClient.getBuildInfo(); - - let version: semver.SemVer | null = null; - try { - version = semver.parse(await cliVersion(binaryPath)); - } catch { - version = semver.parse(buildInfo.version); - } + const { featureSet, cliAuth } = await tracer.phase( + "compatibility_check", + async () => { + // First thing is to check the version. + const buildInfo = await workspaceClient.getBuildInfo(); + + let version: semver.SemVer | null; + try { + version = semver.parse(await cliVersion(binaryPath)); + } catch { + version = semver.parse(buildInfo.version); + } - const featureSet = featureSetForVersion(version); - const configDir = this.pathResolver.getGlobalConfigDir( - parts.safeHostname, - ); - const cliAuth = resolveCliAuth( - vscode.workspace.getConfiguration(), - featureSet, - baseUrl, - configDir, + const featureSet = featureSetForVersion(version); + const configDir = this.pathResolver.getGlobalConfigDir( + parts.safeHostname, + ); + const cliAuth = resolveCliAuth( + vscode.workspace.getConfiguration(), + featureSet, + baseUrl, + configDir, + ); + return { featureSet, cliAuth }; + }, ); // Server versions before v0.14.1 don't support the vscodessh command! @@ -312,10 +322,12 @@ export class Remote { }); // Watch the workspace for changes. - const monitor = await WorkspaceMonitor.create( - workspace, - workspaceClient, - this.serviceContainer, + const monitor = await tracer.phase("workspace_monitor_setup", () => + WorkspaceMonitor.create( + workspace, + workspaceClient, + this.serviceContainer, + ), ); disposables.push( monitor, @@ -391,15 +403,17 @@ export class Remote { } // Monitor SSH process and display network status - const sshMonitor = SshProcessMonitor.start({ - sshHost: parts.sshHost, - networkInfoPath: this.pathResolver.getNetworkInfoPath(), - proxyLogDir: logDir || undefined, - logger: this.logger, - codeLogDir: this.pathResolver.getCodeLogDir(), - remoteSshExtensionId: args.remoteSshExtensionId, - telemetry: this.serviceContainer.getTelemetryService(), - }); + const sshMonitor = await tracer.phase("ssh_monitor_setup", () => + SshProcessMonitor.start({ + sshHost: parts.sshHost, + networkInfoPath: this.pathResolver.getNetworkInfoPath(), + proxyLogDir: logDir || undefined, + logger: this.logger, + codeLogDir: this.pathResolver.getCodeLogDir(), + remoteSshExtensionId: args.remoteSshExtensionId, + telemetry: this.serviceContainer.getTelemetryService(), + }), + ); disposables.push(sshMonitor); this.commands.workspaceLogPath = sshMonitor.getLogFilePath(); @@ -468,23 +482,25 @@ export class Remote { throw ex; } - this.contextManager.set("coder.workspace.connected", true); - this.logger.info("Remote setup complete"); - - // Returning the URL and token allows the plugin to authenticate its own - // client, for example to display the list of workspaces belonging to this - // deployment in the sidebar. We use our own client in here for reasons - // explained above. - return { - safeHostname: parts.safeHostname, - url: baseUrl, - token: token ?? "", - dispose: () => { - disposables.forEach((d) => { - d.dispose(); - }); - }, - }; + return await tracer.phase("vscode_handoff", () => { + this.contextManager.set("coder.workspace.connected", true); + this.logger.info("Remote setup complete"); + + // Returning the URL and token allows the plugin to authenticate its own + // client, for example to display the list of workspaces belonging to this + // deployment in the sidebar. We use our own client in here for reasons + // explained above. + return { + safeHostname: parts.safeHostname, + url: baseUrl, + token: token ?? "", + dispose: () => { + disposables.forEach((d) => { + d.dispose(); + }); + }, + }; + }); } private async lookupWorkspace( diff --git a/src/websocket/reconnectingWebSocket.ts b/src/websocket/reconnectingWebSocket.ts index 6e49e830f5..a881599378 100644 --- a/src/websocket/reconnectingWebSocket.ts +++ b/src/websocket/reconnectingWebSocket.ts @@ -428,15 +428,18 @@ export class ReconnectingWebSocket< if (!this.#dispatch({ type: "SCHEDULE_RETRY" }, reason)) { return; } - this.#telemetry.retrying(reason, { - cause, - code: options.code, - error: options.error, - }); - const jitter = this.#backoffMs * this.#options.jitterFactor * (Math.random() * 2 - 1); const delayMs = Math.max(0, this.#backoffMs + jitter); + this.#telemetry.retrying( + reason, + { + cause, + code: options.code, + error: options.error, + }, + delayMs, + ); this.#logger.debug( `Reconnecting WebSocket in ${Math.round(delayMs)}ms for ${this.#route}`, diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index f715d9b4a6..40b53381f1 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -444,7 +444,10 @@ export class InMemorySecretStorage implements vscode.SecretStorage { export function createMockCliCredentialManager(): CliCredentialManager { return { - storeToken: vi.fn().mockResolvedValue(undefined), + storeToken: vi.fn().mockResolvedValue({ + mode: "file", + credentialSource: "session_token", + }), readToken: vi.fn().mockResolvedValue(undefined), deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 91799edf96..0798fc8d5c 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -178,7 +178,12 @@ describe("CliCredentialManager", () => { it("writes files when keyring is disabled", async () => { const { manager, sink } = setup(); - await manager.storeToken(TEST_URL, "my-token", configs); + await expect( + manager.storeToken(TEST_URL, "my-token", configs), + ).resolves.toEqual({ + mode: "file", + credentialSource: "session_token", + }); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); @@ -197,7 +202,12 @@ describe("CliCredentialManager", () => { stubExecFile({ stdout: "" }); const { manager, resolver, sink } = setup(); - await manager.storeToken(TEST_URL, "my-secret-token", configs); + await expect( + manager.storeToken(TEST_URL, "my-secret-token", configs), + ).resolves.toEqual({ + mode: "keyring", + credentialSource: "session_token", + }); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); @@ -220,13 +230,27 @@ describe("CliCredentialManager", () => { vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0"); const { manager } = setup(); - await manager.storeToken(TEST_URL, "token", configs); + await expect( + manager.storeToken(TEST_URL, "token", configs), + ).resolves.toEqual({ + mode: "file", + credentialSource: "session_token", + }); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("token"); }); + it("reports empty tokens as mTLS credential source", async () => { + const { manager } = setup(); + + await expect(manager.storeToken(TEST_URL, "", configs)).resolves.toEqual({ + mode: "file", + credentialSource: "empty_token", + }); + }); + it("throws when CLI exec fails", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ error: "login failed" }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 4e0c185d1b..ed557f1603 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -128,6 +128,8 @@ describe("CliManager", () => { const CONFIGURE_URL = "https://coder.example.com"; const TOKEN = "test-token"; + const configureEvent = () => telemetrySink.expectOne("cli.configure"); + function configure(options?: { silent?: boolean }) { return manager.configure(CONFIGURE_URL, TOKEN, options); } @@ -149,6 +151,15 @@ describe("CliManager", () => { expect.anything(), { signal: expect.any(AbortSignal) }, ); + expect(configureEvent()).toMatchObject({ + properties: { + result: "success", + silent: "false", + configMode: "file", + credentialSource: "session_token", + }, + measurements: { durationMs: expect.any(Number) }, + }); }); it("should skip progress when silent", async () => { @@ -160,6 +171,12 @@ describe("CliManager", () => { TOKEN, expect.anything(), ); + expect(configureEvent().properties).toMatchObject({ + result: "success", + silent: "true", + configMode: "file", + credentialSource: "session_token", + }); }); it("should throw when URL is empty", async () => { @@ -180,6 +197,13 @@ describe("CliManager", () => { expect.stringContaining("keyring unavailable"), "Open Settings", ); + expect(configureEvent()).toMatchObject({ + properties: { + result: "error", + failureCategory: "credential_store", + }, + error: { message: "keyring unavailable" }, + }); }, ); @@ -190,6 +214,10 @@ describe("CliManager", () => { await expect(configure()).resolves.not.toThrow(); expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); + expect(configureEvent().properties).toMatchObject({ + result: "aborted", + failureCategory: "cancelled", + }); }); }); @@ -796,11 +824,65 @@ describe("CliManager", () => { ); }); - it("does not emit cli.download when a cached binary already matches", async () => { + it("emits cli.resolve cache-hit phases without cli.download", async () => { withExistingBinary(TEST_VERSION); await manager.fetchBinary(mockApi); expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { + result: "success", + outcome: "cache_hit", + cacheSource: "directory", + versionCheck: "match", + }, + measurements: { durationMs: expect.any(Number) }, + }); + expect(event("cli.resolve.cache_lookup")).toMatchObject({ + properties: { source: "directory", result: "success" }, + }); + expect(event("cli.resolve.version_check")).toMatchObject({ + properties: { outcome: "match", result: "success" }, + }); + }); + + it("distinguishes disabled-download fallback", async () => { + mockConfig.set("coder.enableDownloads", false); + withExistingBinary("1.0.0"); + + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + + expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve.download_decision")).toMatchObject({ + properties: { + reason: "version_mismatch", + downloadsEnabled: "false", + outcome: "fallback", + result: "success", + }, + }); + expect(event("cli.resolve")).toMatchObject({ + properties: { + outcome: "download_disabled_fallback", + result: "success", + }, + }); + }); + + it("distinguishes disabled-download failure", async () => { + mockConfig.set("coder.enableDownloads", false); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Unable to download CLI because downloads are disabled", + ); + + expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { + failureCategory: "downloads_disabled", + result: "error", + }, + }); }); it("omits downloadedBytes when the server returns 304", async () => { @@ -813,6 +895,9 @@ describe("CliManager", () => { properties: { reason: "version_mismatch", result: "success" }, }); expect(e?.measurements.downloadedBytes).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { outcome: "downloaded", result: "success" }, + }); }); it("emits downloadedBytes when a download fails mid-stream", async () => { @@ -832,6 +917,12 @@ describe("CliManager", () => { error: { message: "Unable to download binary: connection reset" }, measurements: { downloadedBytes: Buffer.byteLength(partial) }, }); + expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ + properties: { failureCategory: "download", result: "error" }, + }); + expect(event("cli.resolve")).toMatchObject({ + properties: { result: "error" }, + }); }); describe("cli.download.verify", () => { diff --git a/test/unit/instrumentation/remoteSetup.test.ts b/test/unit/instrumentation/remoteSetup.test.ts index cd388acf2f..7e74e57d0d 100644 --- a/test/unit/instrumentation/remoteSetup.test.ts +++ b/test/unit/instrumentation/remoteSetup.test.ts @@ -29,19 +29,35 @@ describe("RemoteSetupTelemetry", () => { it("emits each named phase as a child of remote.setup with shared traceId", async () => { const { sink, remoteSetup } = makeHarness(); + const phases = [ + "cli_resolve", + "cli_configure", + "compatibility_check", + "workspace_lookup", + "workspace_monitor_setup", + "workspace_ready", + "resolve_agent", + "ssh_config_write", + "ssh_monitor_setup", + "vscode_handoff", + ] as const; await remoteSetup.trace(async (tracer) => { - await tracer.phase("workspace_lookup", () => "ws"); - await tracer.phase("ssh_config_write", () => Promise.resolve("cfg")); + for (const phase of phases) { + await tracer.phase(phase, () => phase); + } }); - const [lookup, sshWrite, parent] = sink.events; - expect(lookup.eventName).toBe("remote.setup.workspace_lookup"); - expect(sshWrite.eventName).toBe("remote.setup.ssh_config_write"); - expect(parent.eventName).toBe("remote.setup"); - expect(lookup.traceId).toBe(parent.traceId); - expect(sshWrite.traceId).toBe(parent.traceId); - expect(lookup.parentEventId).toBe(parent.eventId); - expect(sshWrite.parentEventId).toBe(parent.eventId); + const parent = sink.events.at(-1); + expect(parent).toBeDefined(); + expect(parent?.eventName).toBe("remote.setup"); + expect(sink.events.map((event) => event.eventName)).toEqual([ + ...phases.map((phase) => `remote.setup.${phase}`), + "remote.setup", + ]); + for (const phase of sink.events.slice(0, -1)) { + expect(phase.traceId).toBe(parent?.traceId); + expect(phase.parentEventId).toBe(parent?.eventId); + } }); it("markAborted records the outcome and flips result to aborted", async () => { diff --git a/test/unit/instrumentation/websocket.test.ts b/test/unit/instrumentation/websocket.test.ts index 0af7a2a7be..8546fb3f5c 100644 --- a/test/unit/instrumentation/websocket.test.ts +++ b/test/unit/instrumentation/websocket.test.ts @@ -44,6 +44,17 @@ describe("WebSocketTelemetry", () => { }); }); + it("normalizes websocket URLs before emitting the route", () => { + const { ws, sink } = setup(); + + ws.opened( + "wss://coder.example.com/api/v2/workspaces/123e4567-e89b-12d3-a456-426614174000/watch-ws?token=secret#fragment", + ); + + const [event] = sink.eventsNamed("connection.opened"); + expect(event.properties.route).toBe("/api/v2/workspaces/{id}/watch-ws"); + }); + it("uses 0 duration when connectStarted was not called", () => { const { ws, sink } = setup(); @@ -133,7 +144,12 @@ describe("WebSocketTelemetry", () => { const [event] = sink.eventsNamed("connection.reconnect_resolved"); expect(event).toMatchObject({ - properties: { result: "success", reason: "manual_reconnect" }, + properties: { + result: "success", + reason: "manual_reconnect", + attemptBucket: "1", + maxBackoffBucket: "none", + }, measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, }); }); @@ -148,6 +164,8 @@ describe("WebSocketTelemetry", () => { expect(event.properties).toEqual({ result: "error", reason: "manual_reconnect", + attemptBucket: "0", + maxBackoffBucket: "none", terminationReason: "unrecoverable_http", }); }); @@ -191,15 +209,26 @@ describe("WebSocketTelemetry", () => { const { ws, sink } = setup(); ws.opened("/api/test"); - ws.retrying("unexpected_close", { - cause: "unexpected_close", - code: 1006, - }); + ws.retrying( + "unexpected_close", + { + cause: "unexpected_close", + code: 1006, + }, + 250, + ); expect(sink.eventsNamed("connection.dropped")).toHaveLength(1); ws.opened("/api/test"); - expect(sink.eventsNamed("connection.reconnect_resolved")).toHaveLength(1); + expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ + { + properties: { + attemptBucket: "0", + maxBackoffBucket: "<1s", + }, + }, + ]); }); }); }); diff --git a/test/unit/websocket/reconnectingWebSocket.test.ts b/test/unit/websocket/reconnectingWebSocket.test.ts index 769625ab4c..6e2a2569fd 100644 --- a/test/unit/websocket/reconnectingWebSocket.test.ts +++ b/test/unit/websocket/reconnectingWebSocket.test.ts @@ -638,7 +638,12 @@ describe("ReconnectingWebSocket", () => { expect(sink.eventsNamed("connection.dropped")).toHaveLength(2); expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ { - properties: { result: "success", reason: "unexpected_close" }, + properties: { + result: "success", + reason: "unexpected_close", + attemptBucket: "1", + maxBackoffBucket: "<1s", + }, measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, }, ]); From 405cd92741a097877c3e12629075933a67e35b25 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 8 Jun 2026 11:35:39 +0000 Subject: [PATCH 2/7] refactor: isolate telemetry helpers --- src/core/cliCredentialManager.ts | 4 +- src/core/cliManager.ts | 577 ++++++++---------- src/instrumentation/cli.ts | 212 +++++++ src/instrumentation/remoteSetup.ts | 5 +- src/instrumentation/websocket.ts | 38 +- src/remote/remote.ts | 4 +- test/mocks/testHelpers.ts | 5 +- test/unit/core/cliCredentialManager.test.ts | 24 +- test/unit/core/cliManager.test.ts | 8 + test/unit/instrumentation/remoteSetup.test.ts | 4 +- test/unit/instrumentation/websocket.test.ts | 20 +- .../websocket/reconnectingWebSocket.test.ts | 11 +- 12 files changed, 509 insertions(+), 403 deletions(-) create mode 100644 src/instrumentation/cli.ts diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 37fe4ddca7..1b51495d78 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -33,7 +33,6 @@ type KeyringFeature = "keyringAuth" | "keyringTokenRead"; export interface CliCredentialStoreResult { readonly mode: "keyring" | "file"; - readonly credentialSource: "session_token" | "empty_token"; } const EXEC_TIMEOUT_MS = 60_000; @@ -82,7 +81,6 @@ export class CliCredentialManager { configs: Pick, options?: { signal?: AbortSignal }, ): Promise { - const credentialSource = token === "" ? "empty_token" : "session_token"; let mode: CliCredentialStoreResult["mode"] = "file"; await this.credentialTelemetry.traceStore(configs, async (span) => { const binPath = await this.resolveKeyringBinary( @@ -99,7 +97,7 @@ export class CliCredentialManager { span.setProperty("category", "keyring"); await this.storeKeyringToken(binPath, url, token, configs, options); }); - return { mode, credentialSource }; + return { mode }; } private async storeKeyringToken( diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 58ab207a75..d881a2af4d 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,7 +10,12 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; -import { isAbortError } from "../error/errorUtils"; +import { + CliTelemetry, + type CliDownloadReason, + type CliVersionCheckOutcome, + type CliResolveTrace, +} from "../instrumentation/cli"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; @@ -37,28 +42,6 @@ type ResolvedBinary = | { binPath: string; stat: Stats; source: "file-path" | "directory" } | { binPath: string; source: "not-found" }; -type CliDownloadReason = "missing" | "version_mismatch" | "unreadable"; - -type CliResolveOutcome = - | "cache_hit" - | "downloaded" - | "lock_wait_cache_hit" - | "download_disabled_fallback" - | "fallback_to_existing_binary"; - -type CliVersionCheckOutcome = "missing" | "match" | "mismatch" | "unreadable"; - -type CliConfigureFailureCategory = - | "cancelled" - | "filesystem" - | "credential_store" - | "unknown"; - -type CliResolveFailureCategory = - | "downloads_disabled" - | "download" - | "fallback_declined"; - type CliVerifyResult = | { kind: "verified" } | { kind: "bypassed" } @@ -71,14 +54,16 @@ type SingleVerifyResult = export class CliManager { private readonly binaryLock: BinaryLock; + private readonly cliTelemetry: CliTelemetry; constructor( private readonly output: Logger, private readonly pathResolver: PathResolver, private readonly cliCredentialManager: CliCredentialManager, - private readonly telemetry: TelemetryService, + telemetry: TelemetryService, ) { this.binaryLock = new BinaryLock(output); + this.cliTelemetry = new CliTelemetry(telemetry); } /** @@ -145,127 +130,55 @@ export class CliManager { * downloads being disabled. */ public async fetchBinary(restClient: Api): Promise { - return this.telemetry.trace("cli.resolve", async (span) => { + return this.cliTelemetry.resolve(async (trace) => { const baseUrl = restClient.getAxiosInstance().defaults.baseURL; if (!baseUrl) { - span.setProperty("failureCategory", "unknown"); + trace.setFailure("unknown"); throw new Error("REST client has no base URL configured"); } const safeHostname = toSafeHost(baseUrl); const cfg = vscode.workspace.getConfiguration("coder"); - // Settings can be undefined when set to their defaults (true in this case), - // so explicitly check against false. + // Settings can be undefined when set to their defaults (true in this + // case), so explicitly check against false. const enableDownloads = cfg.get("enableDownloads") !== false; - span.setProperty("downloadsEnabled", enableDownloads); + trace.setDownloadsEnabled(enableDownloads); this.output.debug( "Downloads are", enableDownloads ? "enabled" : "disabled", ); - const resolved = await span.phase("cache_lookup", async (cacheSpan) => { - const result = await this.resolveBinaryPath(safeHostname); - cacheSpan.setProperty("source", result.source); - return result; - }); - span.setProperty("cacheSource", resolved.source); - this.output.debug( - `Resolved binary: ${resolved.binPath} (${resolved.source})`, - ); - - const versionResult = await span.phase( - "version_check", - async (versionSpan) => { - // Get the build info to compare with the existing binary version, if any, - // and to log for debugging. - const buildInfo = await restClient.getBuildInfo(); - this.output.info("Got server version", buildInfo.version); - const parsedVersion = semver.parse(buildInfo.version); - if (!parsedVersion) { - throw new Error( - `Got invalid version from deployment: ${buildInfo.version}`, - ); - } - - let existingVersion: string | null = null; - let downloadReason: CliDownloadReason; - let outcome: CliVersionCheckOutcome; - if (resolved.source === "not-found") { - downloadReason = "missing"; - outcome = "missing"; - this.output.info("No existing binary found, starting download"); - } else { - this.output.debug( - "Existing binary size is", - prettyBytes(resolved.stat.size), - ); - try { - existingVersion = await cliVersion(resolved.binPath); - this.output.debug("Existing binary version is", existingVersion); - downloadReason = "version_mismatch"; - outcome = - existingVersion === buildInfo.version ? "match" : "mismatch"; - } catch (error) { - this.output.warn( - "Unable to get version of existing binary, downloading instead", - error, - ); - downloadReason = "unreadable"; - outcome = "unreadable"; - } - } - versionSpan.setProperty("outcome", outcome); - return { - buildInfo, - parsedVersion, - existingVersion, - downloadReason, - outcome, - }; - }, + const resolved = await trace.cacheLookup(() => + this.lookupBinary(safeHostname), ); - span.setProperty("versionCheck", versionResult.outcome); const { buildInfo, parsedVersion, existingVersion, downloadReason } = - versionResult; + await trace.versionCheck(() => + this.checkResolvedBinary(restClient, resolved), + ); if (existingVersion === buildInfo.version) { this.output.debug("Existing binary matches server version"); - span.setProperty("outcome", "cache_hit" satisfies CliResolveOutcome); + trace.setOutcome("cache_hit"); return resolved.binPath; } - await span.phase("download_decision", (decisionSpan) => { - decisionSpan.setProperty("reason", downloadReason); - decisionSpan.setProperty("downloadsEnabled", enableDownloads); - decisionSpan.setProperty( - "outcome", - enableDownloads - ? "download" - : existingVersion - ? "fallback" - : "blocked", - ); - return Promise.resolve(); - }); - span.setProperty("downloadReason", downloadReason); + await trace.downloadDecision( + downloadReason, + enableDownloads, + existingVersion !== null, + ); if (!enableDownloads) { if (existingVersion) { this.output.info( "Using existing binary despite version mismatch because downloads are disabled", ); - span.setProperty( - "outcome", - "download_disabled_fallback" satisfies CliResolveOutcome, - ); + trace.setOutcome("download_disabled_fallback"); return resolved.binPath; } this.output.warn( "Unable to download CLI because downloads are disabled", ); - span.setProperty( - "failureCategory", - "downloads_disabled" satisfies CliResolveFailureCategory, - ); + trace.setFailure("downloads_disabled"); throw new Error( "Unable to download CLI because downloads are disabled", ); @@ -277,133 +190,209 @@ export class CliManager { ); } - // Always download using the platform-specific name. - const downloadBinPath = path.join( - path.dirname(resolved.binPath), - cliUtils.fullName(), + return this.downloadBinary( + restClient, + trace, + resolved, + parsedVersion, + buildInfo.version, + downloadReason, ); + }); + } - // Create the `bin` folder if it doesn't exist - await fs.mkdir(path.dirname(downloadBinPath), { recursive: true }); - const progressLogPath = downloadBinPath + ".progress.log"; - - let lockResult: - | { release: () => Promise; waited: boolean } - | undefined; - let latestVersion = parsedVersion; - try { - lockResult = await span.phase("lock_wait", async (lockSpan) => { - const result = await this.binaryLock.acquireLockOrWait( - downloadBinPath, - progressLogPath, - ); - lockSpan.setProperty("waited", result.waited); - return result; - }); - this.output.debug("Acquired download lock"); - - // Another process may have finished the download while we waited. - if (lockResult.waited) { - const waitResult = await span.phase( - "lock_wait_recheck", - async (recheckSpan) => { - const latestBuildInfo = await restClient.getBuildInfo(); - this.output.debug( - "Got latest server version", - latestBuildInfo.version, - ); + private async lookupBinary(safeHostname: string): Promise { + const resolved = await this.resolveBinaryPath(safeHostname); + this.output.debug( + `Resolved binary: ${resolved.binPath} (${resolved.source})`, + ); + return resolved; + } - const recheckAfterWait = await this.checkBinaryVersion( - downloadBinPath, - latestBuildInfo.version, - ); - recheckSpan.setProperty( - "outcome", - recheckAfterWait.matches - ? "match" - : recheckAfterWait.version - ? "mismatch" - : "missing", - ); - if (recheckAfterWait.matches) { - return { matches: true as const }; - } + private async checkResolvedBinary( + restClient: Api, + resolved: ResolvedBinary, + ): Promise<{ + buildInfo: Awaited>; + parsedVersion: semver.SemVer; + existingVersion: string | null; + downloadReason: CliDownloadReason; + outcome: CliVersionCheckOutcome; + }> { + // Get the build info to compare with the existing binary version, if any, + // and to log for debugging. + const buildInfo = await restClient.getBuildInfo(); + this.output.info("Got server version", buildInfo.version); + const parsedVersion = semver.parse(buildInfo.version); + if (!parsedVersion) { + throw new Error( + `Got invalid version from deployment: ${buildInfo.version}`, + ); + } - const latestParsedVersion = semver.parse(latestBuildInfo.version); - if (!latestParsedVersion) { - throw new Error( - `Got invalid version from deployment: ${latestBuildInfo.version}`, - ); - } - return { - matches: false as const, - parsedVersion: latestParsedVersion, - }; - }, - ); - if (waitResult.matches) { - this.output.debug( - "Binary already matches server version after wait", - ); - span.setProperty( - "outcome", - "lock_wait_cache_hit" satisfies CliResolveOutcome, - ); - return await this.renameToFinalPath(resolved, downloadBinPath); - } - latestVersion = waitResult.parsedVersion; - } + if (resolved.source === "not-found") { + this.output.info("No existing binary found, starting download"); + return { + buildInfo, + parsedVersion, + existingVersion: null, + downloadReason: "missing", + outcome: "missing", + }; + } - const result = await this.telemetry.trace( - "cli.download", - async (span) => { - const downloadedBinPath = await this.performBinaryDownload( - restClient, - latestVersion, - downloadBinPath, - progressLogPath, - span, - ); - return this.renameToFinalPath(resolved, downloadedBinPath); - }, - { reason: downloadReason }, - ); - span.setProperty("outcome", "downloaded" satisfies CliResolveOutcome); - return result; - } catch (error) { - return await span.phase( - "fallback_to_existing_binary", - async (fallbackSpan) => { - fallbackSpan.setProperty( - "failureCategory", - this.categorizeResolveFailure(error), - ); - 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); - } - span.setProperty( - "outcome", - "fallback_to_existing_binary" satisfies CliResolveOutcome, - ); - return resolved.binPath; - }, + this.output.debug( + "Existing binary size is", + prettyBytes(resolved.stat.size), + ); + try { + const existingVersion = await cliVersion(resolved.binPath); + this.output.debug("Existing binary version is", existingVersion); + return { + buildInfo, + parsedVersion, + existingVersion, + downloadReason: "version_mismatch", + outcome: existingVersion === buildInfo.version ? "match" : "mismatch", + }; + } catch (error) { + this.output.warn( + "Unable to get version of existing binary, downloading instead", + error, + ); + return { + buildInfo, + parsedVersion, + existingVersion: null, + downloadReason: "unreadable", + outcome: "unreadable", + }; + } + } + + private async downloadBinary( + restClient: Api, + trace: CliResolveTrace, + resolved: ResolvedBinary, + parsedVersion: semver.SemVer, + serverVersion: string, + downloadReason: CliDownloadReason, + ): Promise { + // 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 }); + const progressLogPath = downloadBinPath + ".progress.log"; + + let lockResult: + | { release: () => Promise; waited: boolean } + | undefined; + let latestVersion = parsedVersion; + try { + lockResult = await trace.lockWait(() => + this.binaryLock.acquireLockOrWait(downloadBinPath, progressLogPath), + ); + this.output.debug("Acquired download lock"); + + if (lockResult.waited) { + const waitResult = await trace.lockWaitRecheck(() => + this.recheckBinaryAfterWait(restClient, downloadBinPath), ); - } finally { - if (lockResult) { - await lockResult.release(); - this.output.debug("Released download lock"); + if (waitResult.matches) { + this.output.debug("Binary already matches server version after wait"); + trace.setOutcome("lock_wait_cache_hit"); + return await this.renameToFinalPath(resolved, downloadBinPath); } + latestVersion = waitResult.parsedVersion; } - }); + + const result = await this.cliTelemetry.download( + downloadReason, + async (span) => { + const downloadedBinPath = await this.performBinaryDownload( + restClient, + latestVersion, + downloadBinPath, + progressLogPath, + span, + ); + return this.renameToFinalPath(resolved, downloadedBinPath); + }, + ); + trace.setOutcome("downloaded"); + return result; + } catch (error) { + return await trace.fallback(error, () => + this.fallbackToExistingBinary( + error, + downloadBinPath, + serverVersion, + resolved, + ), + ); + } finally { + if (lockResult) { + await lockResult.release(); + this.output.debug("Released download lock"); + } + } + } + + private async recheckBinaryAfterWait( + restClient: Api, + downloadBinPath: string, + ): Promise< + | { matches: true; outcome: CliVersionCheckOutcome } + | { + matches: false; + parsedVersion: semver.SemVer; + outcome: CliVersionCheckOutcome; + } + > { + const latestBuildInfo = await restClient.getBuildInfo(); + this.output.debug("Got latest server version", latestBuildInfo.version); + + const recheckAfterWait = await this.checkBinaryVersion( + downloadBinPath, + latestBuildInfo.version, + ); + if (recheckAfterWait.matches) { + return { matches: true, outcome: "match" }; + } + + const latestParsedVersion = semver.parse(latestBuildInfo.version); + if (!latestParsedVersion) { + throw new Error( + `Got invalid version from deployment: ${latestBuildInfo.version}`, + ); + } + return { + matches: false, + parsedVersion: latestParsedVersion, + outcome: recheckAfterWait.version ? "mismatch" : "missing", + }; + } + + private async fallbackToExistingBinary( + error: unknown, + downloadBinPath: string, + serverVersion: string, + resolved: ResolvedBinary, + ): Promise { + const fallback = await this.handleAnyBinaryFailure( + error, + downloadBinPath, + serverVersion, + 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; } /** @@ -1005,55 +994,51 @@ export class CliManager { throw new Error("URL is required to configure the CLI"); } - return this.telemetry.trace("cli.configure", async (span) => { - const configs = vscode.workspace.getConfiguration(); - span.setProperty("silent", options?.silent === true); - - if (options?.silent) { - try { - const result = await this.cliCredentialManager.storeToken( - url, - token, - configs, - ); - span.setProperty("configMode", result.mode); - span.setProperty("credentialSource", result.credentialSource); - } catch (error) { - span.setProperty( - "failureCategory", - this.categorizeConfigureFailure(error), - ); - this.handleStoreError(error); + const silent = options?.silent === true; + return this.cliTelemetry.configure( + { silent, hasToken: token !== "" }, + async (trace) => { + const configs = vscode.workspace.getConfiguration(); + + if (silent) { + try { + const { mode } = await this.cliCredentialManager.storeToken( + url, + token, + configs, + ); + trace.stored(mode); + } catch (error) { + trace.failed(error); + this.handleStoreError(error); + } + return; } - return; - } - const result = await withCancellableProgress( - ({ signal }) => - this.cliCredentialManager.storeToken(url, token, configs, { signal }), - { - location: vscode.ProgressLocation.Notification, - title: `Storing credentials for ${url}`, - cancellable: true, - }, - ); - if (result.ok) { - span.setProperty("configMode", result.value.mode); - span.setProperty("credentialSource", result.value.credentialSource); - return; - } - if (result.cancelled) { - this.output.info("Credential storage cancelled by user"); - span.setProperty("failureCategory", "cancelled"); - span.markAborted(); - return; - } - span.setProperty( - "failureCategory", - this.categorizeConfigureFailure(result.error), - ); - this.handleStoreError(result.error); - }); + const result = await withCancellableProgress( + ({ signal }) => + this.cliCredentialManager.storeToken(url, token, configs, { + signal, + }), + { + location: vscode.ProgressLocation.Notification, + title: `Storing credentials for ${url}`, + cancellable: true, + }, + ); + if (result.ok) { + trace.stored(result.value.mode); + return; + } + if (result.cancelled) { + this.output.info("Credential storage cancelled by user"); + trace.cancelled(); + return; + } + trace.failed(result.error); + this.handleStoreError(result.error); + }, + ); } /** @@ -1082,38 +1067,6 @@ export class CliManager { } } - private categorizeConfigureFailure( - error: unknown, - ): CliConfigureFailureCategory { - if (isAbortError(error)) { - return "cancelled"; - } - const code = (error as NodeJS.ErrnoException | undefined)?.code; - if (typeof code === "string" && code.startsWith("E")) { - return "filesystem"; - } - if (error instanceof Error) { - return "credential_store"; - } - return "unknown"; - } - - private categorizeResolveFailure(error: unknown): CliResolveFailureCategory { - if ( - error instanceof Error && - error.message === "Unable to download CLI because downloads are disabled" - ) { - return "downloads_disabled"; - } - if ( - error instanceof Error && - (error.message.includes("declined") || error.message.includes("aborted")) - ) { - return "fallback_declined"; - } - return "download"; - } - private handleStoreError(error: unknown): void { this.output.error("Failed to store credentials:", error); vscode.window diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts new file mode 100644 index 0000000000..8458c3243f --- /dev/null +++ b/src/instrumentation/cli.ts @@ -0,0 +1,212 @@ +import { isAbortError } from "../error/errorUtils"; + +import type { CallerPropertyValue } from "../telemetry/event"; +import type { TelemetryService } from "../telemetry/service"; +import type { Span } from "../telemetry/span"; + +export type CliCacheSource = "file-path" | "directory" | "not-found"; +export type CliDownloadReason = "missing" | "version_mismatch" | "unreadable"; +export type CliResolveOutcome = + | "cache_hit" + | "downloaded" + | "lock_wait_cache_hit" + | "download_disabled_fallback" + | "fallback_to_existing_binary"; +export type CliVersionCheckOutcome = + | "missing" + | "match" + | "mismatch" + | "unreadable"; +export type CliConfigureFailureCategory = + | "cancelled" + | "filesystem" + | "credential_store" + | "unknown"; +export type CliResolveFailureCategory = + | "downloads_disabled" + | "download" + | "fallback_declined"; + +export class CliTelemetry { + public constructor(private readonly telemetry: TelemetryService) {} + + public resolve(fn: (trace: CliResolveTrace) => Promise): Promise { + return this.telemetry.trace("cli.resolve", (span) => + fn(new CliResolveTrace(span)), + ); + } + + public download( + reason: CliDownloadReason, + fn: (span: Span) => Promise, + ): Promise { + return this.telemetry.trace("cli.download", fn, { reason }); + } + + public configure( + options: { silent: boolean; hasToken: boolean }, + fn: (trace: CliConfigureTrace) => Promise, + ): Promise { + return this.telemetry.trace("cli.configure", (span) => { + span.setProperty("silent", options.silent); + span.setProperty( + "credentialSource", + options.hasToken ? "session_token" : "empty_token", + ); + return fn(new CliConfigureTrace(span)); + }); + } +} + +export class CliResolveTrace { + public constructor(private readonly span: Span) {} + + public setDownloadsEnabled(enabled: boolean): void { + this.span.setProperty("downloadsEnabled", enabled); + } + + public setOutcome(outcome: CliResolveOutcome): void { + this.span.setProperty("outcome", outcome); + } + + public setFailure(category: CliResolveFailureCategory | "unknown"): void { + this.span.setProperty("failureCategory", category); + } + + public async cacheLookup( + fn: () => Promise, + ): Promise { + const result = await tracedPhase( + this.span, + "cache_lookup", + "source", + (r) => r.source, + fn, + ); + this.span.setProperty("cacheSource", result.source); + return result; + } + + public async versionCheck< + T extends { readonly outcome: CliVersionCheckOutcome }, + >(fn: () => Promise): Promise { + const result = await tracedPhase( + this.span, + "version_check", + "outcome", + (r) => r.outcome, + fn, + ); + this.span.setProperty("versionCheck", result.outcome); + return result; + } + + public downloadDecision( + reason: CliDownloadReason, + downloadsEnabled: boolean, + hasExistingBinary: boolean, + ): Promise { + this.span.setProperty("downloadReason", reason); + return this.span.phase("download_decision", (span) => { + span.setProperty("reason", reason); + span.setProperty("downloadsEnabled", downloadsEnabled); + span.setProperty( + "outcome", + downloadsEnabled + ? "download" + : hasExistingBinary + ? "fallback" + : "blocked", + ); + return Promise.resolve(); + }); + } + + public lockWait( + fn: () => Promise, + ): Promise { + return tracedPhase(this.span, "lock_wait", "waited", (r) => r.waited, fn); + } + + public lockWaitRecheck< + T extends { readonly outcome: CliVersionCheckOutcome }, + >(fn: () => Promise): Promise { + return tracedPhase( + this.span, + "lock_wait_recheck", + "outcome", + (r) => r.outcome, + fn, + ); + } + + public async fallback(error: unknown, fn: () => Promise): Promise { + const result = await this.span.phase( + "fallback_to_existing_binary", + async (span) => { + span.setProperty("failureCategory", categorizeResolveFailure(error)); + return fn(); + }, + ); + this.setOutcome("fallback_to_existing_binary"); + return result; + } +} + +export class CliConfigureTrace { + public constructor(private readonly span: Span) {} + + public stored(mode: "keyring" | "file"): void { + this.span.setProperty("configMode", mode); + } + + public cancelled(): void { + this.span.setProperty("failureCategory", "cancelled"); + this.span.markAborted(); + } + + public failed(error: unknown): void { + this.span.setProperty("failureCategory", categorizeConfigureFailure(error)); + } +} + +/** Run `fn` as a child phase, tagging the child span with `key = select(result)`. */ +function tracedPhase( + span: Span, + name: string, + key: string, + select: (result: T) => CallerPropertyValue, + fn: () => Promise, +): Promise { + return span.phase(name, async (child) => { + const result = await fn(); + child.setProperty(key, select(result)); + return result; + }); +} + +function categorizeConfigureFailure( + error: unknown, +): CliConfigureFailureCategory { + if (isAbortError(error)) { + return "cancelled"; + } + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (typeof code === "string" && code.startsWith("E")) { + return "filesystem"; + } + return error instanceof Error ? "credential_store" : "unknown"; +} + +function categorizeResolveFailure(error: unknown): CliResolveFailureCategory { + if (!(error instanceof Error)) { + return "download"; + } + const message = error.message.toLowerCase(); + if (message === "unable to download cli because downloads are disabled") { + return "downloads_disabled"; + } + return message.includes("declined") || message.includes("aborted") + ? "fallback_declined" + : "download"; +} diff --git a/src/instrumentation/remoteSetup.ts b/src/instrumentation/remoteSetup.ts index c675366e80..6a0d4a6e52 100644 --- a/src/instrumentation/remoteSetup.ts +++ b/src/instrumentation/remoteSetup.ts @@ -7,16 +7,17 @@ export type RemoteSetupPhase = | "workspace_lookup" | "workspace_monitor_setup" | "workspace_ready" - | "resolve_agent" + | "agent_resolve" | "ssh_config_write" | "ssh_monitor_setup" - | "vscode_handoff"; + | "connection_handoff"; /** Reason for a non-throwing early exit from `remote.setup`. */ export type RemoteSetupOutcome = "workspace_not_found" | "incompatible_server"; /** Helpers scoped to the remote.setup trace's lifetime. */ export interface RemoteSetupTracer { + /** Emit a typed child phase of `remote.setup`. */ phase(name: RemoteSetupPhase, fn: () => T | PromiseLike): Promise; /** Mark this setup as aborted with a typed reason; emits as `outcome` on the parent event. */ markAborted(reason: RemoteSetupOutcome): void; diff --git a/src/instrumentation/websocket.ts b/src/instrumentation/websocket.ts index 25bd4f2bf9..07e3161ac4 100644 --- a/src/instrumentation/websocket.ts +++ b/src/instrumentation/websocket.ts @@ -42,41 +42,6 @@ interface ReconnectCycle { maxBackoffMs: number; } -function bucketAttempts(attempts: number): string { - if (attempts <= 0) { - return "0"; - } - if (attempts === 1) { - return "1"; - } - if (attempts === 2) { - return "2"; - } - if (attempts <= 5) { - return "3-5"; - } - if (attempts <= 10) { - return "6-10"; - } - return "11+"; -} - -function bucketBackoff(ms: number): string { - if (ms <= 0) { - return "none"; - } - if (ms < 1000) { - return "<1s"; - } - if (ms < 5000) { - return "1-5s"; - } - if (ms < 30000) { - return "5-30s"; - } - return "30s+"; -} - interface DropOptions { cause: ConnectionDropCause; code?: number; @@ -206,14 +171,13 @@ export class WebSocketTelemetry { const properties: Record = { result: outcome.result, reason: cycle.reason, - attemptBucket: bucketAttempts(cycle.attempts), - maxBackoffBucket: bucketBackoff(cycle.maxBackoffMs), }; if (outcome.result === "error") { properties.terminationReason = outcome.terminationReason; } this.#telemetry.log("connection.reconnect_resolved", properties, { attempts: cycle.attempts, + maxBackoffMs: cycle.maxBackoffMs, totalDurationMs: performance.now() - cycle.startMs, }); } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 6e0852350a..8b8efd5d24 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -361,7 +361,7 @@ export class Remote { // Mark initial setup as complete so the monitor can start notifying about state changes monitor.markInitialSetupComplete(); - const agent = await tracer.phase("resolve_agent", () => + const agent = await tracer.phase("agent_resolve", () => this.resolveAgent(context, workspace, stateMachine), ); @@ -482,7 +482,7 @@ export class Remote { throw ex; } - return await tracer.phase("vscode_handoff", () => { + return await tracer.phase("connection_handoff", () => { this.contextManager.set("coder.workspace.connected", true); this.logger.info("Remote setup complete"); diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 40b53381f1..6d05df6000 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -444,10 +444,7 @@ export class InMemorySecretStorage implements vscode.SecretStorage { export function createMockCliCredentialManager(): CliCredentialManager { return { - storeToken: vi.fn().mockResolvedValue({ - mode: "file", - credentialSource: "session_token", - }), + storeToken: vi.fn().mockResolvedValue({ mode: "file" }), readToken: vi.fn().mockResolvedValue(undefined), deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 0798fc8d5c..29016c6ec7 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -180,10 +180,7 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "my-token", configs), - ).resolves.toEqual({ - mode: "file", - credentialSource: "session_token", - }); + ).resolves.toEqual({ mode: "file" }); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); @@ -204,10 +201,7 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "my-secret-token", configs), - ).resolves.toEqual({ - mode: "keyring", - credentialSource: "session_token", - }); + ).resolves.toEqual({ mode: "keyring" }); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); @@ -232,25 +226,13 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "token", configs), - ).resolves.toEqual({ - mode: "file", - credentialSource: "session_token", - }); + ).resolves.toEqual({ mode: "file" }); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); expect(memfs.readFileSync(SESSION_FILE, "utf8")).toBe("token"); }); - it("reports empty tokens as mTLS credential source", async () => { - const { manager } = setup(); - - await expect(manager.storeToken(TEST_URL, "", configs)).resolves.toEqual({ - mode: "file", - credentialSource: "empty_token", - }); - }); - it("throws when CLI exec fails", async () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); stubExecFile({ error: "login failed" }); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index ed557f1603..19dd7aea70 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -179,6 +179,14 @@ describe("CliManager", () => { }); }); + it("reports an empty token as the mTLS credential source", async () => { + await manager.configure(CONFIGURE_URL, ""); + + expect(configureEvent().properties).toMatchObject({ + credentialSource: "empty_token", + }); + }); + it("should throw when URL is empty", async () => { await expect(manager.configure("", TOKEN)).rejects.toThrow( "URL is required to configure the CLI", diff --git a/test/unit/instrumentation/remoteSetup.test.ts b/test/unit/instrumentation/remoteSetup.test.ts index 7e74e57d0d..4dec34433a 100644 --- a/test/unit/instrumentation/remoteSetup.test.ts +++ b/test/unit/instrumentation/remoteSetup.test.ts @@ -36,10 +36,10 @@ describe("RemoteSetupTelemetry", () => { "workspace_lookup", "workspace_monitor_setup", "workspace_ready", - "resolve_agent", + "agent_resolve", "ssh_config_write", "ssh_monitor_setup", - "vscode_handoff", + "connection_handoff", ] as const; await remoteSetup.trace(async (tracer) => { for (const phase of phases) { diff --git a/test/unit/instrumentation/websocket.test.ts b/test/unit/instrumentation/websocket.test.ts index 8546fb3f5c..708a3cddc5 100644 --- a/test/unit/instrumentation/websocket.test.ts +++ b/test/unit/instrumentation/websocket.test.ts @@ -144,13 +144,12 @@ describe("WebSocketTelemetry", () => { const [event] = sink.eventsNamed("connection.reconnect_resolved"); expect(event).toMatchObject({ - properties: { - result: "success", - reason: "manual_reconnect", - attemptBucket: "1", - maxBackoffBucket: "none", + properties: { result: "success", reason: "manual_reconnect" }, + measurements: { + attempts: 1, + maxBackoffMs: 0, + totalDurationMs: expect.any(Number), }, - measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, }); }); @@ -164,8 +163,6 @@ describe("WebSocketTelemetry", () => { expect(event.properties).toEqual({ result: "error", reason: "manual_reconnect", - attemptBucket: "0", - maxBackoffBucket: "none", terminationReason: "unrecoverable_http", }); }); @@ -222,12 +219,7 @@ describe("WebSocketTelemetry", () => { ws.opened("/api/test"); expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ - { - properties: { - attemptBucket: "0", - maxBackoffBucket: "<1s", - }, - }, + { measurements: { attempts: 0, maxBackoffMs: 250 } }, ]); }); }); diff --git a/test/unit/websocket/reconnectingWebSocket.test.ts b/test/unit/websocket/reconnectingWebSocket.test.ts index 6e2a2569fd..669cd1ab8d 100644 --- a/test/unit/websocket/reconnectingWebSocket.test.ts +++ b/test/unit/websocket/reconnectingWebSocket.test.ts @@ -638,13 +638,12 @@ describe("ReconnectingWebSocket", () => { expect(sink.eventsNamed("connection.dropped")).toHaveLength(2); expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ { - properties: { - result: "success", - reason: "unexpected_close", - attemptBucket: "1", - maxBackoffBucket: "<1s", + properties: { result: "success", reason: "unexpected_close" }, + measurements: { + attempts: 1, + maxBackoffMs: expect.any(Number), + totalDurationMs: expect.any(Number), }, - measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, }, ]); }); From 0e2120fe26a95432e549074fede0cd36fb134c4c Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 09:18:16 +0000 Subject: [PATCH 3/7] refactor: align telemetry attribute names --- src/core/cliManager.ts | 4 +- src/instrumentation/cli.ts | 25 ++++++------ src/instrumentation/websocket.ts | 12 +++--- test/unit/core/cliManager.test.ts | 38 +++++++++---------- test/unit/instrumentation/websocket.test.ts | 16 ++++---- .../websocket/reconnectingWebSocket.test.ts | 6 +-- 6 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index d881a2af4d..871dbb5963 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -647,7 +647,7 @@ export class CliManager { ); verifySpan.setProperty("outcome", result.kind); if (result.kind === "sig_not_found") { - verifySpan.setProperty("sigStatus", String(result.status)); + verifySpan.setProperty("sig_status", String(result.status)); } }); } @@ -708,7 +708,7 @@ export class CliManager { } } finally { if (bytesWritten > 0) { - downloadSpan.setMeasurement("downloadedBytes", bytesWritten); + downloadSpan.setMeasurement("downloaded_bytes", bytesWritten); } await downloadProgress.clearProgress(progressLogPath); } diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index 8458c3243f..b9f59c24fd 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -50,7 +50,7 @@ export class CliTelemetry { return this.telemetry.trace("cli.configure", (span) => { span.setProperty("silent", options.silent); span.setProperty( - "credentialSource", + "credential_source", options.hasToken ? "session_token" : "empty_token", ); return fn(new CliConfigureTrace(span)); @@ -62,7 +62,7 @@ export class CliResolveTrace { public constructor(private readonly span: Span) {} public setDownloadsEnabled(enabled: boolean): void { - this.span.setProperty("downloadsEnabled", enabled); + this.span.setProperty("downloads_enabled", enabled); } public setOutcome(outcome: CliResolveOutcome): void { @@ -70,7 +70,7 @@ export class CliResolveTrace { } public setFailure(category: CliResolveFailureCategory | "unknown"): void { - this.span.setProperty("failureCategory", category); + this.span.setProperty("failure_category", category); } public async cacheLookup( @@ -83,7 +83,7 @@ export class CliResolveTrace { (r) => r.source, fn, ); - this.span.setProperty("cacheSource", result.source); + this.span.setProperty("cache_source", result.source); return result; } @@ -97,7 +97,7 @@ export class CliResolveTrace { (r) => r.outcome, fn, ); - this.span.setProperty("versionCheck", result.outcome); + this.span.setProperty("version_check", result.outcome); return result; } @@ -106,10 +106,10 @@ export class CliResolveTrace { downloadsEnabled: boolean, hasExistingBinary: boolean, ): Promise { - this.span.setProperty("downloadReason", reason); + this.span.setProperty("download_reason", reason); return this.span.phase("download_decision", (span) => { span.setProperty("reason", reason); - span.setProperty("downloadsEnabled", downloadsEnabled); + span.setProperty("downloads_enabled", downloadsEnabled); span.setProperty( "outcome", downloadsEnabled @@ -144,7 +144,7 @@ export class CliResolveTrace { const result = await this.span.phase( "fallback_to_existing_binary", async (span) => { - span.setProperty("failureCategory", categorizeResolveFailure(error)); + span.setProperty("failure_category", categorizeResolveFailure(error)); return fn(); }, ); @@ -157,16 +157,19 @@ export class CliConfigureTrace { public constructor(private readonly span: Span) {} public stored(mode: "keyring" | "file"): void { - this.span.setProperty("configMode", mode); + this.span.setProperty("config_mode", mode); } public cancelled(): void { - this.span.setProperty("failureCategory", "cancelled"); + this.span.setProperty("failure_category", "cancelled"); this.span.markAborted(); } public failed(error: unknown): void { - this.span.setProperty("failureCategory", categorizeConfigureFailure(error)); + this.span.setProperty( + "failure_category", + categorizeConfigureFailure(error), + ); } } diff --git a/src/instrumentation/websocket.ts b/src/instrumentation/websocket.ts index 07e3161ac4..d0f3307a37 100644 --- a/src/instrumentation/websocket.ts +++ b/src/instrumentation/websocket.ts @@ -86,7 +86,7 @@ export class WebSocketTelemetry { this.#telemetry.log( "connection.opened", { route: normalizeRoute(route) }, - { connectDurationMs: now - start }, + { connect_duration_ms: now - start }, ); this.#finishReconnect({ result: "success" }); } @@ -105,10 +105,10 @@ export class WebSocketTelemetry { const properties: CallerProperties = { cause }; if (closeCode !== undefined) { - properties.closeCode = closeCode; + properties.close_code = closeCode; } const measurements = { - connectionDurationMs: performance.now() - openedAtMs, + connection_duration_ms: performance.now() - openedAtMs, }; if (error === undefined) { this.#telemetry.log("connection.dropped", properties, measurements); @@ -173,12 +173,12 @@ export class WebSocketTelemetry { reason: cycle.reason, }; if (outcome.result === "error") { - properties.terminationReason = outcome.terminationReason; + properties.termination_reason = outcome.terminationReason; } this.#telemetry.log("connection.reconnect_resolved", properties, { attempts: cycle.attempts, - maxBackoffMs: cycle.maxBackoffMs, - totalDurationMs: performance.now() - cycle.startMs, + max_backoff_ms: cycle.maxBackoffMs, + total_duration_ms: performance.now() - cycle.startMs, }); } } diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 19dd7aea70..49e4a30da0 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -155,8 +155,8 @@ describe("CliManager", () => { properties: { result: "success", silent: "false", - configMode: "file", - credentialSource: "session_token", + config_mode: "file", + credential_source: "session_token", }, measurements: { durationMs: expect.any(Number) }, }); @@ -174,8 +174,8 @@ describe("CliManager", () => { expect(configureEvent().properties).toMatchObject({ result: "success", silent: "true", - configMode: "file", - credentialSource: "session_token", + config_mode: "file", + credential_source: "session_token", }); }); @@ -183,7 +183,7 @@ describe("CliManager", () => { await manager.configure(CONFIGURE_URL, ""); expect(configureEvent().properties).toMatchObject({ - credentialSource: "empty_token", + credential_source: "empty_token", }); }); @@ -208,7 +208,7 @@ describe("CliManager", () => { expect(configureEvent()).toMatchObject({ properties: { result: "error", - failureCategory: "credential_store", + failure_category: "credential_store", }, error: { message: "keyring unavailable" }, }); @@ -224,7 +224,7 @@ describe("CliManager", () => { expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); expect(configureEvent().properties).toMatchObject({ result: "aborted", - failureCategory: "cancelled", + failure_category: "cancelled", }); }); }); @@ -827,7 +827,7 @@ describe("CliManager", () => { properties: { reason, result: "success" }, }); expect(e?.measurements.durationMs).toBeGreaterThanOrEqual(0); - expect(e?.measurements.downloadedBytes).toBe( + expect(e?.measurements.downloaded_bytes).toBe( Buffer.byteLength(mockBinaryContent(TEST_VERSION)), ); }); @@ -841,8 +841,8 @@ describe("CliManager", () => { properties: { result: "success", outcome: "cache_hit", - cacheSource: "directory", - versionCheck: "match", + cache_source: "directory", + version_check: "match", }, measurements: { durationMs: expect.any(Number) }, }); @@ -864,7 +864,7 @@ describe("CliManager", () => { expect(event("cli.resolve.download_decision")).toMatchObject({ properties: { reason: "version_mismatch", - downloadsEnabled: "false", + downloads_enabled: "false", outcome: "fallback", result: "success", }, @@ -887,13 +887,13 @@ describe("CliManager", () => { expect(event("cli.download")).toBeUndefined(); expect(event("cli.resolve")).toMatchObject({ properties: { - failureCategory: "downloads_disabled", + failure_category: "downloads_disabled", result: "error", }, }); }); - it("omits downloadedBytes when the server returns 304", async () => { + it("omits downloaded_bytes when the server returns 304", async () => { withExistingBinary("1.0.0"); withHttpResponse(304); await manager.fetchBinary(mockApi); @@ -902,13 +902,13 @@ describe("CliManager", () => { expect(e).toMatchObject({ properties: { reason: "version_mismatch", result: "success" }, }); - expect(e?.measurements.downloadedBytes).toBeUndefined(); + expect(e?.measurements.downloaded_bytes).toBeUndefined(); expect(event("cli.resolve")).toMatchObject({ properties: { outcome: "downloaded", result: "success" }, }); }); - it("emits downloadedBytes when a download fails mid-stream", async () => { + it("emits downloaded_bytes when a download fails mid-stream", async () => { const partial = "partial-binary"; withHttpResponse( 200, @@ -923,10 +923,10 @@ describe("CliManager", () => { expect(event("cli.download")).toMatchObject({ properties: { reason: "missing", result: "error" }, error: { message: "Unable to download binary: connection reset" }, - measurements: { downloadedBytes: Buffer.byteLength(partial) }, + measurements: { downloaded_bytes: Buffer.byteLength(partial) }, }); expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ - properties: { failureCategory: "download", result: "error" }, + properties: { failure_category: "download", result: "error" }, }); expect(event("cli.resolve")).toMatchObject({ properties: { result: "error" }, @@ -971,7 +971,7 @@ describe("CliManager", () => { { status: 404, message: "Signature not found" }, { status: 500, message: "Failed to download signature" }, ])( - "emits outcome=sig_not_found with sigStatus=$status when user runs without verification", + "emits outcome=sig_not_found with sig_status=$status when user runs without verification", async ({ status, message }) => { withSignatureResponses([status, status]); mockUI.setResponse(message, "Run without verification"); @@ -981,7 +981,7 @@ describe("CliManager", () => { expect(event("cli.download.verify")).toMatchObject({ properties: { outcome: "sig_not_found", - sigStatus: String(status), + sig_status: String(status), result: "success", }, }); diff --git a/test/unit/instrumentation/websocket.test.ts b/test/unit/instrumentation/websocket.test.ts index 708a3cddc5..2809c097e5 100644 --- a/test/unit/instrumentation/websocket.test.ts +++ b/test/unit/instrumentation/websocket.test.ts @@ -40,7 +40,7 @@ describe("WebSocketTelemetry", () => { const [event] = sink.eventsNamed("connection.opened"); expect(event).toMatchObject({ properties: { route: "/api/test" }, - measurements: { connectDurationMs: expect.any(Number) }, + measurements: { connect_duration_ms: expect.any(Number) }, }); }); @@ -61,7 +61,7 @@ describe("WebSocketTelemetry", () => { ws.opened("/api/test"); const [event] = sink.eventsNamed("connection.opened"); - expect(event.measurements.connectDurationMs).toBe(0); + expect(event.measurements.connect_duration_ms).toBe(0); }); }); @@ -83,9 +83,9 @@ describe("WebSocketTelemetry", () => { const [event] = sink.eventsNamed("connection.dropped"); expect(event.properties).toMatchObject({ cause: "unexpected_close", - closeCode: "1006", + close_code: "1006", }); - expect(event.measurements.connectionDurationMs).toEqual( + expect(event.measurements.connection_duration_ms).toEqual( expect.any(Number), ); }); @@ -147,8 +147,8 @@ describe("WebSocketTelemetry", () => { properties: { result: "success", reason: "manual_reconnect" }, measurements: { attempts: 1, - maxBackoffMs: 0, - totalDurationMs: expect.any(Number), + max_backoff_ms: 0, + total_duration_ms: expect.any(Number), }, }); }); @@ -163,7 +163,7 @@ describe("WebSocketTelemetry", () => { expect(event.properties).toEqual({ result: "error", reason: "manual_reconnect", - terminationReason: "unrecoverable_http", + termination_reason: "unrecoverable_http", }); }); @@ -219,7 +219,7 @@ describe("WebSocketTelemetry", () => { ws.opened("/api/test"); expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ - { measurements: { attempts: 0, maxBackoffMs: 250 } }, + { measurements: { attempts: 0, max_backoff_ms: 250 } }, ]); }); }); diff --git a/test/unit/websocket/reconnectingWebSocket.test.ts b/test/unit/websocket/reconnectingWebSocket.test.ts index 669cd1ab8d..250332f227 100644 --- a/test/unit/websocket/reconnectingWebSocket.test.ts +++ b/test/unit/websocket/reconnectingWebSocket.test.ts @@ -641,8 +641,8 @@ describe("ReconnectingWebSocket", () => { properties: { result: "success", reason: "unexpected_close" }, measurements: { attempts: 1, - maxBackoffMs: expect.any(Number), - totalDurationMs: expect.any(Number), + max_backoff_ms: expect.any(Number), + total_duration_ms: expect.any(Number), }, }, ]); @@ -664,7 +664,7 @@ describe("ReconnectingWebSocket", () => { expect(dropped).toHaveLength(1); expect(dropped[0].properties).toMatchObject({ cause: "normal_close", - closeCode: String(WebSocketCloseCode.GOING_AWAY), + close_code: String(WebSocketCloseCode.GOING_AWAY), }); expect( sink From abafd1f9b28b2ca6652e7fca841dccbf8f79efe6 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 13:27:55 +0000 Subject: [PATCH 4/7] refactor: simplify CLI telemetry instrumentation --- src/core/cliCredentialManager.ts | 13 +- src/core/cliManager.ts | 43 +- src/instrumentation/cli.ts | 138 +++--- test/mocks/testHelpers.ts | 2 +- test/unit/core/cliCredentialManager.test.ts | 6 +- test/unit/core/cliManager.telemetry.test.ts | 478 ++++++++++++++++++++ test/unit/core/cliManager.test.ts | 240 +--------- 7 files changed, 593 insertions(+), 327 deletions(-) create mode 100644 test/unit/core/cliManager.telemetry.test.ts diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 1b51495d78..245f1a5e1a 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -31,10 +31,6 @@ const execFileAsync = promisify(execFile); type KeyringFeature = "keyringAuth" | "keyringTokenRead"; -export interface CliCredentialStoreResult { - readonly mode: "keyring" | "file"; -} - const EXEC_TIMEOUT_MS = 60_000; const EXEC_LOG_INTERVAL_MS = 5_000; @@ -75,14 +71,13 @@ export class CliCredentialManager { * * Keyring and files are mutually exclusive, never both. */ - public async storeToken( + public storeToken( url: string, token: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { - let mode: CliCredentialStoreResult["mode"] = "file"; - await this.credentialTelemetry.traceStore(configs, async (span) => { + ): Promise { + return this.credentialTelemetry.traceStore(configs, async (span) => { const binPath = await this.resolveKeyringBinary( url, configs, @@ -93,11 +88,9 @@ export class CliCredentialManager { await this.writeCredentialFiles(url, token); return; } - mode = "keyring"; span.setProperty("category", "keyring"); await this.storeKeyringToken(binPath, url, token, configs, options); }); - return { mode }; } private async storeKeyringToken( diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 871dbb5963..df6d8bbc7b 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -11,7 +11,10 @@ import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; import { + CliDownloadsDisabledError, + CliFallbackDeclinedError, CliTelemetry, + type CliDownloadAction, type CliDownloadReason, type CliVersionCheckOutcome, type CliResolveTrace, @@ -52,6 +55,16 @@ type SingleVerifyResult = | { kind: "bypassed" } | { kind: "sig_unavailable"; status: number }; +function getDownloadAction( + downloadsEnabled: boolean, + hasExistingBinary: boolean, +): CliDownloadAction { + if (downloadsEnabled) { + return "download"; + } + return hasExistingBinary ? "fallback" : "blocked"; +} + export class CliManager { private readonly binaryLock: BinaryLock; private readonly cliTelemetry: CliTelemetry; @@ -141,7 +154,6 @@ export class CliManager { // Settings can be undefined when set to their defaults (true in this // case), so explicitly check against false. const enableDownloads = cfg.get("enableDownloads") !== false; - trace.setDownloadsEnabled(enableDownloads); this.output.debug( "Downloads are", enableDownloads ? "enabled" : "disabled", @@ -161,11 +173,10 @@ export class CliManager { return resolved.binPath; } - await trace.downloadDecision( - downloadReason, - enableDownloads, - existingVersion !== null, - ); + await trace.recordDownloadDecision({ + reason: downloadReason, + action: getDownloadAction(enableDownloads, existingVersion !== null), + }); if (!enableDownloads) { if (existingVersion) { @@ -178,10 +189,9 @@ export class CliManager { this.output.warn( "Unable to download CLI because downloads are disabled", ); + const error = new CliDownloadsDisabledError(); trace.setFailure("downloads_disabled"); - throw new Error( - "Unable to download CLI because downloads are disabled", - ); + throw error; } if (existingVersion) { @@ -523,7 +533,7 @@ export class CliManager { !check.matches && !(await this.promptUseExistingBinary(check.version, message)) ) { - throw error; + throw new CliFallbackDeclinedError(error); } return candidate; }; @@ -996,18 +1006,16 @@ export class CliManager { const silent = options?.silent === true; return this.cliTelemetry.configure( - { silent, hasToken: token !== "" }, + { + silent, + credentialSource: token === "" ? "empty_token" : "session_token", + }, async (trace) => { const configs = vscode.workspace.getConfiguration(); if (silent) { try { - const { mode } = await this.cliCredentialManager.storeToken( - url, - token, - configs, - ); - trace.stored(mode); + await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { trace.failed(error); this.handleStoreError(error); @@ -1027,7 +1035,6 @@ export class CliManager { }, ); if (result.ok) { - trace.stored(result.value.mode); return; } if (result.cancelled) { diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index b9f59c24fd..7558eb6a3e 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -1,11 +1,13 @@ import { isAbortError } from "../error/errorUtils"; -import type { CallerPropertyValue } from "../telemetry/event"; +import type { CallerProperties, CallerPropertyValue } from "../telemetry/event"; import type { TelemetryService } from "../telemetry/service"; import type { Span } from "../telemetry/span"; export type CliCacheSource = "file-path" | "directory" | "not-found"; export type CliDownloadReason = "missing" | "version_mismatch" | "unreadable"; +export type CliDownloadAction = "download" | "fallback" | "blocked"; +export type CliCredentialSource = "session_token" | "empty_token"; export type CliResolveOutcome = | "cache_hit" | "downloaded" @@ -27,6 +29,30 @@ export type CliResolveFailureCategory = | "download" | "fallback_declined"; +interface CliConfigureOptions { + readonly silent: boolean; + readonly credentialSource: CliCredentialSource; +} + +export class CliDownloadsDisabledError extends Error { + public constructor() { + super("Unable to download CLI because downloads are disabled"); + this.name = "CliDownloadsDisabledError"; + } +} + +export class CliFallbackDeclinedError extends Error { + public constructor(cause: unknown) { + super( + cause instanceof Error ? cause.message : "CLI binary fallback declined", + { + cause, + }, + ); + this.name = "CliFallbackDeclinedError"; + } +} + export class CliTelemetry { public constructor(private readonly telemetry: TelemetryService) {} @@ -44,27 +70,27 @@ export class CliTelemetry { } public configure( - options: { silent: boolean; hasToken: boolean }, + options: CliConfigureOptions, fn: (trace: CliConfigureTrace) => Promise, ): Promise { - return this.telemetry.trace("cli.configure", (span) => { - span.setProperty("silent", options.silent); - span.setProperty( - "credential_source", - options.hasToken ? "session_token" : "empty_token", - ); - return fn(new CliConfigureTrace(span)); - }); + return this.telemetry.trace("cli.configure", (span) => + fn(this.createConfigureTrace(span, options)), + ); + } + + private createConfigureTrace( + span: Span, + options: CliConfigureOptions, + ): CliConfigureTrace { + span.setProperty("silent", options.silent); + span.setProperty("credential_source", options.credentialSource); + return new CliConfigureTrace(span); } } export class CliResolveTrace { public constructor(private readonly span: Span) {} - public setDownloadsEnabled(enabled: boolean): void { - this.span.setProperty("downloads_enabled", enabled); - } - public setOutcome(outcome: CliResolveOutcome): void { this.span.setProperty("outcome", outcome); } @@ -79,8 +105,7 @@ export class CliResolveTrace { const result = await tracedPhase( this.span, "cache_lookup", - "source", - (r) => r.source, + { source: (r) => r.source }, fn, ); this.span.setProperty("cache_source", result.source); @@ -93,39 +118,28 @@ export class CliResolveTrace { const result = await tracedPhase( this.span, "version_check", - "outcome", - (r) => r.outcome, + { outcome: (r) => r.outcome }, fn, ); this.span.setProperty("version_check", result.outcome); return result; } - public downloadDecision( - reason: CliDownloadReason, - downloadsEnabled: boolean, - hasExistingBinary: boolean, - ): Promise { - this.span.setProperty("download_reason", reason); - return this.span.phase("download_decision", (span) => { - span.setProperty("reason", reason); - span.setProperty("downloads_enabled", downloadsEnabled); - span.setProperty( - "outcome", - downloadsEnabled - ? "download" - : hasExistingBinary - ? "fallback" - : "blocked", - ); - return Promise.resolve(); + public recordDownloadDecision(options: { + readonly reason: CliDownloadReason; + readonly action: CliDownloadAction; + }): Promise { + this.span.setProperty("download_reason", options.reason); + return this.phase("download_decision", { + reason: options.reason, + outcome: options.action, }); } public lockWait( fn: () => Promise, ): Promise { - return tracedPhase(this.span, "lock_wait", "waited", (r) => r.waited, fn); + return tracedPhase(this.span, "lock_wait", { waited: (r) => r.waited }, fn); } public lockWaitRecheck< @@ -134,8 +148,7 @@ export class CliResolveTrace { return tracedPhase( this.span, "lock_wait_recheck", - "outcome", - (r) => r.outcome, + { outcome: (r) => r.outcome }, fn, ); } @@ -144,22 +157,36 @@ export class CliResolveTrace { const result = await this.span.phase( "fallback_to_existing_binary", async (span) => { - span.setProperty("failure_category", categorizeResolveFailure(error)); - return fn(); + try { + const result = await fn(); + span.setProperty("failure_category", categorizeResolveFailure(error)); + return result; + } catch (fallbackError) { + span.setProperty( + "failure_category", + categorizeResolveFailure(fallbackError), + ); + throw fallbackError; + } }, ); this.setOutcome("fallback_to_existing_binary"); return result; } + + private phase(name: string, properties: CallerProperties): Promise { + return this.span.phase(name, (span) => { + for (const [key, value] of Object.entries(properties)) { + span.setProperty(key, value); + } + return Promise.resolve(); + }); + } } export class CliConfigureTrace { public constructor(private readonly span: Span) {} - public stored(mode: "keyring" | "file"): void { - this.span.setProperty("config_mode", mode); - } - public cancelled(): void { this.span.setProperty("failure_category", "cancelled"); this.span.markAborted(); @@ -173,17 +200,17 @@ export class CliConfigureTrace { } } -/** Run `fn` as a child phase, tagging the child span with `key = select(result)`. */ function tracedPhase( span: Span, name: string, - key: string, - select: (result: T) => CallerPropertyValue, + properties: Readonly CallerPropertyValue>>, fn: () => Promise, ): Promise { return span.phase(name, async (child) => { const result = await fn(); - child.setProperty(key, select(result)); + for (const [key, select] of Object.entries(properties)) { + child.setProperty(key, select(result)); + } return result; }); } @@ -202,14 +229,11 @@ function categorizeConfigureFailure( } function categorizeResolveFailure(error: unknown): CliResolveFailureCategory { - if (!(error instanceof Error)) { - return "download"; - } - const message = error.message.toLowerCase(); - if (message === "unable to download cli because downloads are disabled") { + if (error instanceof CliDownloadsDisabledError) { return "downloads_disabled"; } - return message.includes("declined") || message.includes("aborted") - ? "fallback_declined" - : "download"; + if (error instanceof CliFallbackDeclinedError) { + return "fallback_declined"; + } + return "download"; } diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 6d05df6000..f715d9b4a6 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -444,7 +444,7 @@ export class InMemorySecretStorage implements vscode.SecretStorage { export function createMockCliCredentialManager(): CliCredentialManager { return { - storeToken: vi.fn().mockResolvedValue({ mode: "file" }), + storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), deleteToken: vi.fn().mockResolvedValue(undefined), } as unknown as CliCredentialManager; diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 29016c6ec7..854e43adae 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -180,7 +180,7 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "my-token", configs), - ).resolves.toEqual({ mode: "file" }); + ).resolves.toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); @@ -201,7 +201,7 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "my-secret-token", configs), - ).resolves.toEqual({ mode: "keyring" }); + ).resolves.toBeUndefined(); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); @@ -226,7 +226,7 @@ describe("CliCredentialManager", () => { await expect( manager.storeToken(TEST_URL, "token", configs), - ).resolves.toEqual({ mode: "file" }); + ).resolves.toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); diff --git a/test/unit/core/cliManager.telemetry.test.ts b/test/unit/core/cliManager.telemetry.test.ts new file mode 100644 index 0000000000..64861a972c --- /dev/null +++ b/test/unit/core/cliManager.telemetry.test.ts @@ -0,0 +1,478 @@ +import globalAxios, { type AxiosInstance } from "axios"; +import { type Api } from "coder/site/src/api/api"; +import { fs as memfs, vol } from "memfs"; +import * as os from "node:os"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import * as cliExec from "@/core/cliExec"; +import { CliManager } from "@/core/cliManager"; +import { PathResolver } from "@/core/pathResolver"; +import * as pgp from "@/pgp"; + +import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; +import { + createMockCliCredentialManager, + createMockLogger, + createMockStream, + MockConfigurationProvider, + MockProgressReporter, + MockUserInteraction, +} from "../../mocks/testHelpers"; +import { expectPathsEqual } from "../../utils/platform"; + +import type * as fs from "node:fs"; + +import type { CliCredentialManager } from "@/core/cliCredentialManager"; + +vi.mock("os"); +vi.mock("axios"); + +vi.mock("fs", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return { + ...memfs.fs, + default: memfs.fs, + }; +}); + +vi.mock("fs/promises", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return { + ...memfs.fs.promises, + default: memfs.fs.promises, + }; +}); + +vi.mock("proper-lockfile", () => ({ + lock: () => Promise.resolve(() => Promise.resolve()), + check: () => Promise.resolve(false), +})); + +vi.mock("@/pgp"); + +vi.mock("@/core/cliExec", async () => { + const actual = + await vi.importActual("@/core/cliExec"); + return { + ...actual, + version: vi.fn(), + }; +}); + +describe("CliManager telemetry", () => { + let manager: CliManager; + let mockConfig: MockConfigurationProvider; + let mockProgress: MockProgressReporter; + let mockUI: MockUserInteraction; + let mockApi: Api; + let mockAxios: AxiosInstance; + let mockCredManager: CliCredentialManager; + let telemetrySink: TestSink; + + const TEST_VERSION = "1.2.3"; + const TEST_URL = "https://test.coder.com"; + const BASE_PATH = "/path/base"; + const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; + const PLATFORM = "linux"; + const ARCH = "amd64"; + const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; + const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; + + beforeEach(() => { + vi.resetAllMocks(); + vol.reset(); + + mockApi = createMockApi(TEST_VERSION, TEST_URL); + mockAxios = mockApi.getAxiosInstance(); + vi.mocked(globalAxios.create).mockReturnValue(mockAxios); + mockConfig = new MockConfigurationProvider(); + mockProgress = new MockProgressReporter(); + mockUI = new MockUserInteraction(); + mockCredManager = createMockCliCredentialManager(); + telemetrySink = new TestSink(); + manager = new CliManager( + createMockLogger(), + new PathResolver(BASE_PATH, "/code/log"), + mockCredManager, + createTestTelemetryService(telemetrySink), + ); + + vi.mocked(os.platform).mockReturnValue(PLATFORM); + vi.mocked(os.arch).mockReturnValue(ARCH); + vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); + + mockConfig.set("coder.disableSignatureVerification", true); + }); + + afterEach(async () => { + mockProgress?.setCancellation(false); + vi.clearAllTimers(); + await new Promise((resolve) => setImmediate(resolve)); + vol.reset(); + }); + + describe("cli.configure", () => { + const CONFIGURE_URL = "https://coder.example.com"; + const TOKEN = "test-token"; + + const configureEvent = () => telemetrySink.expectOne("cli.configure"); + + function configure(options?: { silent?: boolean }) { + return manager.configure(CONFIGURE_URL, TOKEN, options); + } + + it("emits credential source and silent mode on success", async () => { + await configure(); + + expect(configureEvent()).toMatchObject({ + properties: { + result: "success", + silent: "false", + credential_source: "session_token", + }, + measurements: { durationMs: expect.any(Number) }, + }); + }); + + it("emits silent=true when progress is suppressed", async () => { + await configure({ silent: true }); + + expect(configureEvent().properties).toMatchObject({ + result: "success", + silent: "true", + credential_source: "session_token", + }); + }); + + it("reports empty token as the mTLS credential source", async () => { + await manager.configure(CONFIGURE_URL, ""); + + expect(configureEvent().properties).toMatchObject({ + credential_source: "empty_token", + }); + }); + + it.each([{ silent: false }, { silent: true }])( + "emits credential_store failure category on failure (silent=$silent)", + async (options) => { + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); + + await expect(configure(options)).rejects.toThrow("keyring unavailable"); + expect(configureEvent()).toMatchObject({ + properties: { + result: "error", + failure_category: "credential_store", + }, + error: { message: "keyring unavailable" }, + }); + }, + ); + + it("emits cancelled failure category when user cancels", async () => { + const error = new Error("The operation was aborted"); + error.name = "AbortError"; + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce(error); + + await expect(configure()).resolves.not.toThrow(); + expect(configureEvent().properties).toMatchObject({ + result: "aborted", + failure_category: "cancelled", + }); + }); + }); + + describe("cli.resolve and cli.download", () => { + const event = (name: string) => + telemetrySink.events.find((e) => e.eventName === name); + + it.each([ + { reason: "missing", setup: () => {} }, + { reason: "version_mismatch", setup: () => withExistingBinary("1.0.0") }, + { reason: "unreadable", setup: () => withCorruptedBinary() }, + ])("emits cli.download with reason=$reason", async ({ reason, setup }) => { + setup(); + withSuccessfulDownload(); + await manager.fetchBinary(mockApi); + + const e = event("cli.download"); + expect(e).toMatchObject({ + properties: { reason, result: "success" }, + }); + expect(e?.measurements.durationMs).toBeGreaterThanOrEqual(0); + expect(e?.measurements.downloaded_bytes).toBe( + Buffer.byteLength(mockBinaryContent(TEST_VERSION)), + ); + }); + + it("emits cli.resolve cache-hit phases without cli.download", async () => { + withExistingBinary(TEST_VERSION); + await manager.fetchBinary(mockApi); + + expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { + result: "success", + outcome: "cache_hit", + cache_source: "directory", + version_check: "match", + }, + measurements: { durationMs: expect.any(Number) }, + }); + expect(event("cli.resolve.cache_lookup")).toMatchObject({ + properties: { source: "directory", result: "success" }, + }); + expect(event("cli.resolve.version_check")).toMatchObject({ + properties: { outcome: "match", result: "success" }, + }); + }); + + it("distinguishes disabled-download fallback", async () => { + mockConfig.set("coder.enableDownloads", false); + withExistingBinary("1.0.0"); + + expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + + expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve.download_decision")).toMatchObject({ + properties: { + reason: "version_mismatch", + outcome: "fallback", + result: "success", + }, + }); + expect(event("cli.resolve")).toMatchObject({ + properties: { + outcome: "download_disabled_fallback", + result: "success", + }, + }); + }); + + it("distinguishes disabled-download failure", async () => { + mockConfig.set("coder.enableDownloads", false); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Unable to download CLI because downloads are disabled", + ); + + expect(event("cli.download")).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { + failure_category: "downloads_disabled", + result: "error", + }, + }); + }); + + it("distinguishes fallback declined from download failure", async () => { + withExistingBinary("1.0.0"); + vi.mocked(cliExec.version).mockResolvedValueOnce("1.0.0"); + withHttpResponse( + 200, + { "content-length": "1024" }, + createMockStream("partial-binary", { + error: new Error("connection reset"), + }), + ); + mockUI.setResponse( + "Failed to update CLI binary. Run version 1.0.0 anyway?", + undefined, + ); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Unable to download binary: connection reset", + ); + + expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ + properties: { failure_category: "fallback_declined", result: "error" }, + }); + }); + + it("omits downloaded_bytes when the server returns 304", async () => { + withExistingBinary("1.0.0"); + withHttpResponse(304); + await manager.fetchBinary(mockApi); + + const e = event("cli.download"); + expect(e).toMatchObject({ + properties: { reason: "version_mismatch", result: "success" }, + }); + expect(e?.measurements.downloaded_bytes).toBeUndefined(); + expect(event("cli.resolve")).toMatchObject({ + properties: { outcome: "downloaded", result: "success" }, + }); + }); + + it("emits downloaded_bytes when a download fails mid-stream", async () => { + const partial = "partial-binary"; + withHttpResponse( + 200, + { "content-length": "1024" }, + createMockStream(partial, { error: new Error("connection reset") }), + ); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Unable to download binary: connection reset", + ); + + expect(event("cli.download")).toMatchObject({ + properties: { reason: "missing", result: "error" }, + error: { message: "Unable to download binary: connection reset" }, + measurements: { downloaded_bytes: Buffer.byteLength(partial) }, + }); + expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ + properties: { failure_category: "download", result: "error" }, + }); + expect(event("cli.resolve")).toMatchObject({ + properties: { result: "error" }, + }); + }); + + describe("cli.download.verify", () => { + beforeEach(() => { + mockConfig.set("coder.disableSignatureVerification", false); + withSuccessfulDownload(); + }); + + it("emits outcome=verified on valid signature", async () => { + withSignatureResponses([200]); + await manager.fetchBinary(mockApi); + + const verify = event("cli.download.verify"); + const download = event("cli.download"); + expect(verify).toMatchObject({ + properties: { outcome: "verified", result: "success" }, + }); + expect(verify?.measurements.durationMs).toBeGreaterThanOrEqual(0); + expect(verify?.traceId).toBe(download?.traceId); + expect(verify?.parentEventId).toBe(download?.eventId); + }); + + it("emits outcome=bypassed when user runs anyway despite invalid signature", async () => { + withSignatureResponses([200]); + vi.mocked(pgp.verifySignature).mockRejectedValueOnce( + createVerificationError("Invalid signature"), + ); + mockUI.setResponse("Signature does not match", "Run anyway"); + + await manager.fetchBinary(mockApi); + + expect(event("cli.download.verify")).toMatchObject({ + properties: { outcome: "bypassed", result: "success" }, + }); + }); + + it.each([ + { status: 404, message: "Signature not found" }, + { status: 500, message: "Failed to download signature" }, + ])( + "emits outcome=sig_not_found with sig_status=$status when user runs without verification", + async ({ status, message }) => { + withSignatureResponses([status, status]); + mockUI.setResponse(message, "Run without verification"); + + await manager.fetchBinary(mockApi); + + expect(event("cli.download.verify")).toMatchObject({ + properties: { + outcome: "sig_not_found", + sig_status: String(status), + result: "success", + }, + }); + }, + ); + + it("emits error when verification is aborted", async () => { + withSignatureResponses([200]); + vi.mocked(pgp.verifySignature).mockRejectedValueOnce( + createVerificationError("Invalid signature"), + ); + mockUI.setResponse("Signature does not match", undefined); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Signature verification aborted", + ); + + expect(event("cli.download.verify")).toMatchObject({ + properties: { result: "error" }, + error: { message: "Signature verification aborted" }, + }); + }); + }); + }); + + function createMockApi(version: string, url: string): Api { + const axios = { + defaults: { baseURL: url }, + get: vi.fn(), + } as unknown as AxiosInstance; + return { + getBuildInfo: vi.fn().mockResolvedValue({ version }), + getAxiosInstance: () => axios, + } as unknown as Api; + } + + function withExistingBinary(version: string, dir: string = BINARY_DIR) { + vol.mkdirSync(dir, { recursive: true }); + memfs.writeFileSync(`${dir}/${BINARY_NAME}`, mockBinaryContent(version), { + mode: 0o755, + }); + vi.mocked(cliExec.version).mockResolvedValueOnce(version); + } + + function withCorruptedBinary() { + vol.mkdirSync(BINARY_DIR, { recursive: true }); + memfs.writeFileSync(BINARY_PATH, "corrupted-binary-content", { + mode: 0o755, + }); + vi.mocked(cliExec.version).mockRejectedValueOnce(new Error("corrupted")); + } + + function withSuccessfulDownload(opts?: { + headers?: Record; + }) { + const stream = createMockStream(mockBinaryContent(TEST_VERSION)); + withHttpResponse( + 200, + opts?.headers ?? { "content-length": "1024" }, + stream, + ); + vi.mocked(cliExec.version).mockResolvedValue(TEST_VERSION); + } + + function withSignatureResponses(statuses: number[]): void { + for (const status of statuses) { + const data = + status === 200 ? createMockStream("mock-signature-content") : undefined; + withHttpResponse(status, {}, data); + } + } + + function withHttpResponse( + status: number, + headers: Record = {}, + data?: unknown, + ) { + vi.mocked(mockAxios.get).mockResolvedValueOnce({ + status, + headers, + data, + }); + } +}); + +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}`; +} diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 49e4a30da0..3059e709d1 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -15,7 +15,7 @@ import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { isKeyringEnabled } from "@/settings/cli"; -import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; +import { createTestTelemetryService } from "../../mocks/telemetry"; import { createMockCliCredentialManager, createMockLogger, @@ -77,7 +77,6 @@ describe("CliManager", () => { let mockApi: Api; let mockAxios: AxiosInstance; let mockCredManager: CliCredentialManager; - let telemetrySink: TestSink; const TEST_VERSION = "1.2.3"; const TEST_URL = "https://test.coder.com"; @@ -99,12 +98,11 @@ describe("CliManager", () => { mockProgress = new MockProgressReporter(); mockUI = new MockUserInteraction(); mockCredManager = createMockCliCredentialManager(); - telemetrySink = new TestSink(); manager = new CliManager( createMockLogger(), new PathResolver(BASE_PATH, "/code/log"), mockCredManager, - createTestTelemetryService(telemetrySink), + createTestTelemetryService(), ); // Mock only what's necessary @@ -128,8 +126,6 @@ describe("CliManager", () => { const CONFIGURE_URL = "https://coder.example.com"; const TOKEN = "test-token"; - const configureEvent = () => telemetrySink.expectOne("cli.configure"); - function configure(options?: { silent?: boolean }) { return manager.configure(CONFIGURE_URL, TOKEN, options); } @@ -151,15 +147,6 @@ describe("CliManager", () => { expect.anything(), { signal: expect.any(AbortSignal) }, ); - expect(configureEvent()).toMatchObject({ - properties: { - result: "success", - silent: "false", - config_mode: "file", - credential_source: "session_token", - }, - measurements: { durationMs: expect.any(Number) }, - }); }); it("should skip progress when silent", async () => { @@ -171,20 +158,6 @@ describe("CliManager", () => { TOKEN, expect.anything(), ); - expect(configureEvent().properties).toMatchObject({ - result: "success", - silent: "true", - config_mode: "file", - credential_source: "session_token", - }); - }); - - it("reports an empty token as the mTLS credential source", async () => { - await manager.configure(CONFIGURE_URL, ""); - - expect(configureEvent().properties).toMatchObject({ - credential_source: "empty_token", - }); }); it("should throw when URL is empty", async () => { @@ -205,13 +178,6 @@ describe("CliManager", () => { expect.stringContaining("keyring unavailable"), "Open Settings", ); - expect(configureEvent()).toMatchObject({ - properties: { - result: "error", - failure_category: "credential_store", - }, - error: { message: "keyring unavailable" }, - }); }, ); @@ -222,10 +188,6 @@ describe("CliManager", () => { await expect(configure()).resolves.not.toThrow(); expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); - expect(configureEvent().properties).toMatchObject({ - result: "aborted", - failure_category: "cancelled", - }); }); }); @@ -809,204 +771,6 @@ describe("CliManager", () => { ); }); - describe("Telemetry", () => { - const event = (name: string) => - telemetrySink.events.find((e) => e.eventName === name); - - it.each([ - { reason: "missing", setup: () => {} }, - { reason: "version_mismatch", setup: () => withExistingBinary("1.0.0") }, - { reason: "unreadable", setup: () => withCorruptedBinary() }, - ])("emits cli.download with reason=$reason", async ({ reason, setup }) => { - setup(); - withSuccessfulDownload(); - await manager.fetchBinary(mockApi); - - const e = event("cli.download"); - expect(e).toMatchObject({ - properties: { reason, result: "success" }, - }); - expect(e?.measurements.durationMs).toBeGreaterThanOrEqual(0); - expect(e?.measurements.downloaded_bytes).toBe( - Buffer.byteLength(mockBinaryContent(TEST_VERSION)), - ); - }); - - it("emits cli.resolve cache-hit phases without cli.download", async () => { - withExistingBinary(TEST_VERSION); - await manager.fetchBinary(mockApi); - - expect(event("cli.download")).toBeUndefined(); - expect(event("cli.resolve")).toMatchObject({ - properties: { - result: "success", - outcome: "cache_hit", - cache_source: "directory", - version_check: "match", - }, - measurements: { durationMs: expect.any(Number) }, - }); - expect(event("cli.resolve.cache_lookup")).toMatchObject({ - properties: { source: "directory", result: "success" }, - }); - expect(event("cli.resolve.version_check")).toMatchObject({ - properties: { outcome: "match", result: "success" }, - }); - }); - - it("distinguishes disabled-download fallback", async () => { - mockConfig.set("coder.enableDownloads", false); - withExistingBinary("1.0.0"); - - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); - - expect(event("cli.download")).toBeUndefined(); - expect(event("cli.resolve.download_decision")).toMatchObject({ - properties: { - reason: "version_mismatch", - downloads_enabled: "false", - outcome: "fallback", - result: "success", - }, - }); - expect(event("cli.resolve")).toMatchObject({ - properties: { - outcome: "download_disabled_fallback", - result: "success", - }, - }); - }); - - it("distinguishes disabled-download failure", async () => { - mockConfig.set("coder.enableDownloads", false); - - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( - "Unable to download CLI because downloads are disabled", - ); - - expect(event("cli.download")).toBeUndefined(); - expect(event("cli.resolve")).toMatchObject({ - properties: { - failure_category: "downloads_disabled", - result: "error", - }, - }); - }); - - it("omits downloaded_bytes when the server returns 304", async () => { - withExistingBinary("1.0.0"); - withHttpResponse(304); - await manager.fetchBinary(mockApi); - - const e = event("cli.download"); - expect(e).toMatchObject({ - properties: { reason: "version_mismatch", result: "success" }, - }); - expect(e?.measurements.downloaded_bytes).toBeUndefined(); - expect(event("cli.resolve")).toMatchObject({ - properties: { outcome: "downloaded", result: "success" }, - }); - }); - - it("emits downloaded_bytes when a download fails mid-stream", async () => { - const partial = "partial-binary"; - withHttpResponse( - 200, - { "content-length": "1024" }, - createMockStream(partial, { error: new Error("connection reset") }), - ); - - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( - "Unable to download binary: connection reset", - ); - - expect(event("cli.download")).toMatchObject({ - properties: { reason: "missing", result: "error" }, - error: { message: "Unable to download binary: connection reset" }, - measurements: { downloaded_bytes: Buffer.byteLength(partial) }, - }); - expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ - properties: { failure_category: "download", result: "error" }, - }); - expect(event("cli.resolve")).toMatchObject({ - properties: { result: "error" }, - }); - }); - - describe("cli.download.verify", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", false); - withSuccessfulDownload(); - }); - - it("emits outcome=verified on valid signature", async () => { - withSignatureResponses([200]); - await manager.fetchBinary(mockApi); - - const verify = event("cli.download.verify"); - const download = event("cli.download"); - expect(verify).toMatchObject({ - properties: { outcome: "verified", result: "success" }, - }); - expect(verify?.measurements.durationMs).toBeGreaterThanOrEqual(0); - expect(verify?.traceId).toBe(download?.traceId); - expect(verify?.parentEventId).toBe(download?.eventId); - }); - - it("emits outcome=bypassed when user runs anyway despite invalid signature", async () => { - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", "Run anyway"); - - await manager.fetchBinary(mockApi); - - expect(event("cli.download.verify")).toMatchObject({ - properties: { outcome: "bypassed", result: "success" }, - }); - }); - - it.each([ - { status: 404, message: "Signature not found" }, - { status: 500, message: "Failed to download signature" }, - ])( - "emits outcome=sig_not_found with sig_status=$status when user runs without verification", - async ({ status, message }) => { - withSignatureResponses([status, status]); - mockUI.setResponse(message, "Run without verification"); - - await manager.fetchBinary(mockApi); - - expect(event("cli.download.verify")).toMatchObject({ - properties: { - outcome: "sig_not_found", - sig_status: String(status), - result: "success", - }, - }); - }, - ); - - it("emits error when verification is aborted", async () => { - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", undefined); - - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( - "Signature verification aborted", - ); - - expect(event("cli.download.verify")).toMatchObject({ - properties: { result: "error" }, - error: { message: "Signature verification aborted" }, - }); - }); - }); - }); - describe("File System Operations", () => { it("creates binary directory", async () => { expect(memfs.existsSync(BINARY_DIR)).toBe(false); From 0a2011e924d3246102f8c4fa900b86adc56a7b81 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 9 Jun 2026 18:20:44 +0300 Subject: [PATCH 5/7] refactor: simplify CLI telemetry wrappers and share test harness - Make fetchBinary/configure thin telemetry wrappers over private impls; thread trace as the trailing arg and group value params into an options object - Inline getDownloadAction and fold cli.ts phase helpers (downloadDecision, tracedPhase) for a smaller instrumentation surface - Categorize configure failures via a typed CredentialFileError instead of error-shape heuristics - Extract remote.setup compatibility check into checkCompatibility - Share a setupCliManager() harness across the cliManager unit and telemetry tests; per-test setup with local mocks, no beforeEach --- src/core/cliManager.ts | 228 +++++---- src/instrumentation/cli.ts | 133 +++-- src/remote/remote.ts | 59 ++- test/unit/core/cliManager.telemetry.test.ts | 497 ++++++++----------- test/unit/core/cliManager.test.ts | 514 ++++++++------------ test/unit/core/cliManagerHarness.ts | 237 +++++++++ 6 files changed, 839 insertions(+), 829 deletions(-) create mode 100644 test/unit/core/cliManagerHarness.ts diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index df6d8bbc7b..474c8372f6 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -14,6 +14,7 @@ import { CliDownloadsDisabledError, CliFallbackDeclinedError, CliTelemetry, + type CliConfigureTrace, type CliDownloadAction, type CliDownloadReason, type CliVersionCheckOutcome, @@ -55,16 +56,6 @@ type SingleVerifyResult = | { kind: "bypassed" } | { kind: "sig_unavailable"; status: number }; -function getDownloadAction( - downloadsEnabled: boolean, - hasExistingBinary: boolean, -): CliDownloadAction { - if (downloadsEnabled) { - return "download"; - } - return hasExistingBinary ? "fallback" : "blocked"; -} - export class CliManager { private readonly binaryLock: BinaryLock; private readonly cliTelemetry: CliTelemetry; @@ -142,73 +133,82 @@ export class CliManager { * unable to download a working binary, whether because of network issues or * downloads being disabled. */ - public async fetchBinary(restClient: Api): Promise { - return this.cliTelemetry.resolve(async (trace) => { - const baseUrl = restClient.getAxiosInstance().defaults.baseURL; - if (!baseUrl) { - trace.setFailure("unknown"); - throw new Error("REST client has no base URL configured"); - } - const safeHostname = toSafeHost(baseUrl); - const cfg = vscode.workspace.getConfiguration("coder"); - // Settings can be undefined when set to their defaults (true in this - // case), so explicitly check against false. - const enableDownloads = cfg.get("enableDownloads") !== false; - this.output.debug( - "Downloads are", - enableDownloads ? "enabled" : "disabled", - ); + public fetchBinary(restClient: Api): Promise { + return this.cliTelemetry.resolve((trace) => + this.resolveBinary(restClient, trace), + ); + } - const resolved = await trace.cacheLookup(() => - this.lookupBinary(safeHostname), - ); - const { buildInfo, parsedVersion, existingVersion, downloadReason } = - await trace.versionCheck(() => - this.checkResolvedBinary(restClient, resolved), - ); + private async resolveBinary( + restClient: Api, + trace: CliResolveTrace, + ): Promise { + const baseUrl = restClient.getAxiosInstance().defaults.baseURL; + if (!baseUrl) { + trace.setFailure("unknown"); + throw new Error("REST client has no base URL configured"); + } + const safeHostname = toSafeHost(baseUrl); + const cfg = vscode.workspace.getConfiguration("coder"); + // Settings can be undefined when set to their defaults (true in this + // case), so explicitly check against false. + const enableDownloads = cfg.get("enableDownloads") !== false; + this.output.debug( + "Downloads are", + enableDownloads ? "enabled" : "disabled", + ); - if (existingVersion === buildInfo.version) { - this.output.debug("Existing binary matches server version"); - trace.setOutcome("cache_hit"); - return resolved.binPath; - } + const resolved = await trace.cacheLookup(() => + this.lookupBinary(safeHostname), + ); + const { buildInfo, parsedVersion, existingVersion, downloadReason } = + await trace.versionCheck(() => + this.checkResolvedBinary(restClient, resolved), + ); - await trace.recordDownloadDecision({ - reason: downloadReason, - action: getDownloadAction(enableDownloads, existingVersion !== null), - }); + if (existingVersion === buildInfo.version) { + this.output.debug("Existing binary matches server version"); + trace.setOutcome("cache_hit"); + return resolved.binPath; + } - if (!enableDownloads) { - if (existingVersion) { - this.output.info( - "Using existing binary despite version mismatch because downloads are disabled", - ); - trace.setOutcome("download_disabled_fallback"); - return resolved.binPath; - } - this.output.warn( - "Unable to download CLI because downloads are disabled", - ); - const error = new CliDownloadsDisabledError(); - trace.setFailure("downloads_disabled"); - throw error; - } + let action: CliDownloadAction; + if (enableDownloads) { + action = "download"; + } else { + action = existingVersion !== null ? "fallback" : "blocked"; + } + await trace.downloadDecision(downloadReason, action); + if (!enableDownloads) { if (existingVersion) { this.output.info( - "Downloading since existing binary does not match the server version", + "Using existing binary despite version mismatch because downloads are disabled", ); + trace.setOutcome("download_disabled_fallback"); + return resolved.binPath; } + this.output.warn("Unable to download CLI because downloads are disabled"); + trace.setFailure("downloads_disabled"); + throw new CliDownloadsDisabledError(); + } + + if (existingVersion) { + this.output.info( + "Downloading since existing binary does not match the server version", + ); + } - return this.downloadBinary( - restClient, - trace, + return this.downloadBinary( + restClient, + { resolved, parsedVersion, - buildInfo.version, + serverVersion: buildInfo.version, downloadReason, - ); - }); + }, + trace, + ); } private async lookupBinary(safeHostname: string): Promise { @@ -282,12 +282,15 @@ export class CliManager { private async downloadBinary( restClient: Api, + options: { + resolved: ResolvedBinary; + parsedVersion: semver.SemVer; + serverVersion: string; + downloadReason: CliDownloadReason; + }, trace: CliResolveTrace, - resolved: ResolvedBinary, - parsedVersion: semver.SemVer, - serverVersion: string, - downloadReason: CliDownloadReason, ): Promise { + const { resolved, parsedVersion, serverVersion, downloadReason } = options; // Always download using the platform-specific name. const downloadBinPath = path.join( path.dirname(resolved.binPath), @@ -308,7 +311,7 @@ export class CliManager { this.output.debug("Acquired download lock"); if (lockResult.waited) { - const waitResult = await trace.lockWaitRecheck(() => + const waitResult = await trace.lockRecheck(() => this.recheckBinaryAfterWait(restClient, downloadBinPath), ); if (waitResult.matches) { @@ -324,9 +327,11 @@ export class CliManager { async (span) => { const downloadedBinPath = await this.performBinaryDownload( restClient, - latestVersion, - downloadBinPath, - progressLogPath, + { + parsedVersion: latestVersion, + binPath: downloadBinPath, + progressLogPath, + }, span, ); return this.renameToFinalPath(resolved, downloadedBinPath); @@ -564,11 +569,14 @@ export class CliManager { private async performBinaryDownload( restClient: Api, - parsedVersion: semver.SemVer, - binPath: string, - progressLogPath: string, + options: { + parsedVersion: semver.SemVer; + binPath: string; + progressLogPath: string; + }, downloadSpan: Span, ): Promise { + const { parsedVersion, binPath, progressLogPath } = options; const cfg = vscode.workspace.getConfiguration("coder"); const tempFile = tempFilePath(binPath, "temp"); @@ -1010,42 +1018,46 @@ export class CliManager { silent, credentialSource: token === "" ? "empty_token" : "session_token", }, - async (trace) => { - const configs = vscode.workspace.getConfiguration(); - - if (silent) { - try { - await this.cliCredentialManager.storeToken(url, token, configs); - } catch (error) { - trace.failed(error); - this.handleStoreError(error); - } - return; - } + (trace) => this.storeCredentials({ url, token, silent }, trace), + ); + } - const result = await withCancellableProgress( - ({ signal }) => - this.cliCredentialManager.storeToken(url, token, configs, { - signal, - }), - { - location: vscode.ProgressLocation.Notification, - title: `Storing credentials for ${url}`, - cancellable: true, - }, - ); - if (result.ok) { - return; - } - if (result.cancelled) { - this.output.info("Credential storage cancelled by user"); - trace.cancelled(); - return; - } - trace.failed(result.error); - this.handleStoreError(result.error); + private async storeCredentials( + options: { url: string; token: string; silent: boolean }, + trace: CliConfigureTrace, + ): Promise { + const { url, token, silent } = options; + const configs = vscode.workspace.getConfiguration(); + + if (silent) { + try { + await this.cliCredentialManager.storeToken(url, token, configs); + } catch (error) { + trace.failed(error); + this.handleStoreError(error); + } + return; + } + + const result = await withCancellableProgress( + ({ signal }) => + this.cliCredentialManager.storeToken(url, token, configs, { signal }), + { + location: vscode.ProgressLocation.Notification, + title: `Storing credentials for ${url}`, + cancellable: true, }, ); + if (result.ok) { + return; + } + if (result.cancelled) { + this.output.info("Credential storage cancelled by user"); + trace.cancelled(); + return; + } + trace.failed(result.error); + this.handleStoreError(result.error); } /** diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index 7558eb6a3e..f80f4d5301 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -1,6 +1,8 @@ import { isAbortError } from "../error/errorUtils"; -import type { CallerProperties, CallerPropertyValue } from "../telemetry/event"; +import { CredentialFileError } from "./credentials"; + +import type { CallerPropertyValue } from "../telemetry/event"; import type { TelemetryService } from "../telemetry/service"; import type { Span } from "../telemetry/span"; @@ -73,18 +75,11 @@ export class CliTelemetry { options: CliConfigureOptions, fn: (trace: CliConfigureTrace) => Promise, ): Promise { - return this.telemetry.trace("cli.configure", (span) => - fn(this.createConfigureTrace(span, options)), - ); - } - - private createConfigureTrace( - span: Span, - options: CliConfigureOptions, - ): CliConfigureTrace { - span.setProperty("silent", options.silent); - span.setProperty("credential_source", options.credentialSource); - return new CliConfigureTrace(span); + return this.telemetry.trace("cli.configure", (span) => { + span.setProperty("silent", options.silent); + span.setProperty("credential_source", options.credentialSource); + return fn(new CliConfigureTrace(span)); + }); } } @@ -99,58 +94,49 @@ export class CliResolveTrace { this.span.setProperty("failure_category", category); } - public async cacheLookup( + public cacheLookup( fn: () => Promise, ): Promise { - const result = await tracedPhase( - this.span, - "cache_lookup", - { source: (r) => r.source }, - fn, - ); - this.span.setProperty("cache_source", result.source); - return result; + return this.tracedPhase("cache_lookup", fn, (r) => r.source, { + child: "source", + parent: "cache_source", + }); } - public async versionCheck< - T extends { readonly outcome: CliVersionCheckOutcome }, - >(fn: () => Promise): Promise { - const result = await tracedPhase( - this.span, - "version_check", - { outcome: (r) => r.outcome }, - fn, - ); - this.span.setProperty("version_check", result.outcome); - return result; + public versionCheck( + fn: () => Promise, + ): Promise { + return this.tracedPhase("version_check", fn, (r) => r.outcome, { + child: "outcome", + parent: "version_check", + }); } - public recordDownloadDecision(options: { - readonly reason: CliDownloadReason; - readonly action: CliDownloadAction; - }): Promise { - this.span.setProperty("download_reason", options.reason); - return this.phase("download_decision", { - reason: options.reason, - outcome: options.action, + public lockWait( + fn: () => Promise, + ): Promise { + return this.tracedPhase("lock_wait", fn, (r) => r.waited, { + child: "waited", }); } - public lockWait( + public lockRecheck( fn: () => Promise, ): Promise { - return tracedPhase(this.span, "lock_wait", { waited: (r) => r.waited }, fn); + return this.tracedPhase("lock_wait_recheck", fn, (r) => r.outcome, { + child: "outcome", + }); } - public lockWaitRecheck< - T extends { readonly outcome: CliVersionCheckOutcome }, - >(fn: () => Promise): Promise { - return tracedPhase( - this.span, - "lock_wait_recheck", - { outcome: (r) => r.outcome }, - fn, - ); + public downloadDecision( + reason: CliDownloadReason, + action: CliDownloadAction, + ): Promise { + this.span.setProperty("download_reason", reason); + return this.span.phase("download_decision", () => Promise.resolve(), { + reason, + outcome: action, + }); } public async fallback(error: unknown, fn: () => Promise): Promise { @@ -174,13 +160,25 @@ export class CliResolveTrace { return result; } - private phase(name: string, properties: CallerProperties): Promise { - return this.span.phase(name, (span) => { - for (const [key, value] of Object.entries(properties)) { - span.setProperty(key, value); - } - return Promise.resolve(); + /** + * Run `fn` as a child phase tagged with `select(result)`, mirroring it onto + * the parent when `keys.parent` is given. + */ + private async tracedPhase( + name: string, + fn: () => Promise, + select: (result: T) => CallerPropertyValue, + keys: { readonly child: string; readonly parent?: string }, + ): Promise { + const result = await this.span.phase(name, async (child) => { + const value = await fn(); + child.setProperty(keys.child, select(value)); + return value; }); + if (keys.parent) { + this.span.setProperty(keys.parent, select(result)); + } + return result; } } @@ -200,29 +198,14 @@ export class CliConfigureTrace { } } -function tracedPhase( - span: Span, - name: string, - properties: Readonly CallerPropertyValue>>, - fn: () => Promise, -): Promise { - return span.phase(name, async (child) => { - const result = await fn(); - for (const [key, select] of Object.entries(properties)) { - child.setProperty(key, select(result)); - } - return result; - }); -} - function categorizeConfigureFailure( error: unknown, ): CliConfigureFailureCategory { if (isAbortError(error)) { return "cancelled"; } - const code = (error as NodeJS.ErrnoException | undefined)?.code; - if (typeof code === "string" && code.startsWith("E")) { + // A CredentialFileError is a file-write failure; anything else is keyring/CLI. + if (error instanceof CredentialFileError) { return "filesystem"; } return error instanceof Error ? "credential_store" : "unknown"; diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 8b8efd5d24..129cf3db19 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -254,29 +254,13 @@ export class Remote { const { featureSet, cliAuth } = await tracer.phase( "compatibility_check", - async () => { - // First thing is to check the version. - const buildInfo = await workspaceClient.getBuildInfo(); - - let version: semver.SemVer | null; - try { - version = semver.parse(await cliVersion(binaryPath)); - } catch { - version = semver.parse(buildInfo.version); - } - - const featureSet = featureSetForVersion(version); - const configDir = this.pathResolver.getGlobalConfigDir( - parts.safeHostname, - ); - const cliAuth = resolveCliAuth( - vscode.workspace.getConfiguration(), - featureSet, + () => + this.checkCompatibility({ + workspaceClient, + binaryPath, baseUrl, - configDir, - ); - return { featureSet, cliAuth }; - }, + safeHostname: parts.safeHostname, + }), ); // Server versions before v0.14.1 don't support the vscodessh command! @@ -707,6 +691,37 @@ export class Remote { } } + /** + * Resolve the feature set and CLI auth, falling back to the server version + * when the CLI version can't be read. + */ + private async checkCompatibility(options: { + workspaceClient: Api; + binaryPath: string; + baseUrl: string; + safeHostname: string; + }): Promise<{ featureSet: FeatureSet; cliAuth: CliAuth }> { + const { workspaceClient, binaryPath, baseUrl, safeHostname } = options; + const buildInfo = await workspaceClient.getBuildInfo(); + + let version: semver.SemVer | null; + try { + version = semver.parse(await cliVersion(binaryPath)); + } catch { + version = semver.parse(buildInfo.version); + } + + const featureSet = featureSetForVersion(version); + const configDir = this.pathResolver.getGlobalConfigDir(safeHostname); + const cliAuth = resolveCliAuth( + vscode.workspace.getConfiguration(), + featureSet, + baseUrl, + configDir, + ); + return { featureSet, cliAuth }; + } + private watchRemoteSessionAuth( context: RemoteSetupContext, workspaceClient: CoderApi, diff --git a/test/unit/core/cliManager.telemetry.test.ts b/test/unit/core/cliManager.telemetry.test.ts index 64861a972c..cbfbc4851a 100644 --- a/test/unit/core/cliManager.telemetry.test.ts +++ b/test/unit/core/cliManager.telemetry.test.ts @@ -1,46 +1,30 @@ -import globalAxios, { type AxiosInstance } from "axios"; -import { type Api } from "coder/site/src/api/api"; -import { fs as memfs, vol } from "memfs"; -import * as os from "node:os"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; - -import * as cliExec from "@/core/cliExec"; -import { CliManager } from "@/core/cliManager"; -import { PathResolver } from "@/core/pathResolver"; -import * as pgp from "@/pgp"; - -import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; -import { - createMockCliCredentialManager, - createMockLogger, - createMockStream, - MockConfigurationProvider, - MockProgressReporter, - MockUserInteraction, -} from "../../mocks/testHelpers"; +import { afterEach, describe, expect, it, vi } from "vitest"; + import { expectPathsEqual } from "../../utils/platform"; -import type * as fs from "node:fs"; +import { + BINARY_PATH, + type CliManagerHarness, + flushPendingIO, + makeAbortError, + mockBinaryContent, + setupCliManager, + TEST_VERSION, +} from "./cliManagerHarness"; -import type { CliCredentialManager } from "@/core/cliCredentialManager"; +import type * as fs from "node:fs"; vi.mock("os"); vi.mock("axios"); vi.mock("fs", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); - return { - ...memfs.fs, - default: memfs.fs, - }; + return { ...memfs.fs, default: memfs.fs }; }); vi.mock("fs/promises", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); - return { - ...memfs.fs.promises, - default: memfs.fs.promises, - }; + return { ...memfs.fs.promises, default: memfs.fs.promises }; }); vi.mock("proper-lockfile", () => ({ @@ -53,130 +37,72 @@ vi.mock("@/pgp"); vi.mock("@/core/cliExec", async () => { const actual = await vi.importActual("@/core/cliExec"); - return { - ...actual, - version: vi.fn(), - }; + return { ...actual, version: vi.fn() }; }); describe("CliManager telemetry", () => { - let manager: CliManager; - let mockConfig: MockConfigurationProvider; - let mockProgress: MockProgressReporter; - let mockUI: MockUserInteraction; - let mockApi: Api; - let mockAxios: AxiosInstance; - let mockCredManager: CliCredentialManager; - let telemetrySink: TestSink; - - const TEST_VERSION = "1.2.3"; - const TEST_URL = "https://test.coder.com"; - const BASE_PATH = "/path/base"; - const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; - const PLATFORM = "linux"; - const ARCH = "amd64"; - const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; - const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; - - beforeEach(() => { - vi.resetAllMocks(); - vol.reset(); - - mockApi = createMockApi(TEST_VERSION, TEST_URL); - mockAxios = mockApi.getAxiosInstance(); - vi.mocked(globalAxios.create).mockReturnValue(mockAxios); - mockConfig = new MockConfigurationProvider(); - mockProgress = new MockProgressReporter(); - mockUI = new MockUserInteraction(); - mockCredManager = createMockCliCredentialManager(); - telemetrySink = new TestSink(); - manager = new CliManager( - createMockLogger(), - new PathResolver(BASE_PATH, "/code/log"), - mockCredManager, - createTestTelemetryService(telemetrySink), - ); - - vi.mocked(os.platform).mockReturnValue(PLATFORM); - vi.mocked(os.arch).mockReturnValue(ARCH); - vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); - - mockConfig.set("coder.disableSignatureVerification", true); - }); - - afterEach(async () => { - mockProgress?.setCancellation(false); - vi.clearAllTimers(); - await new Promise((resolve) => setImmediate(resolve)); - vol.reset(); - }); + afterEach(flushPendingIO); describe("cli.configure", () => { - const CONFIGURE_URL = "https://coder.example.com"; + const URL = "https://coder.example.com"; const TOKEN = "test-token"; - const configureEvent = () => telemetrySink.expectOne("cli.configure"); - - function configure(options?: { silent?: boolean }) { - return manager.configure(CONFIGURE_URL, TOKEN, options); - } + it.each([ + { + name: "session token", + token: TOKEN, + options: undefined, + props: { silent: "false", credential_source: "session_token" }, + }, + { + name: "silent mode", + token: TOKEN, + options: { silent: true }, + props: { silent: "true", credential_source: "session_token" }, + }, + { + name: "empty token as mTLS", + token: "", + options: undefined, + props: { credential_source: "empty_token" }, + }, + ])("emits $name on success", async ({ token, options, props }) => { + const { manager, event } = setupCliManager(); - it("emits credential source and silent mode on success", async () => { - await configure(); + await manager.configure(URL, token, options); - expect(configureEvent()).toMatchObject({ - properties: { - result: "success", - silent: "false", - credential_source: "session_token", - }, + expect(event("cli.configure")).toMatchObject({ + properties: { result: "success", ...props }, measurements: { durationMs: expect.any(Number) }, }); }); - it("emits silent=true when progress is suppressed", async () => { - await configure({ silent: true }); - - expect(configureEvent().properties).toMatchObject({ - result: "success", - silent: "true", - credential_source: "session_token", - }); - }); - - it("reports empty token as the mTLS credential source", async () => { - await manager.configure(CONFIGURE_URL, ""); - - expect(configureEvent().properties).toMatchObject({ - credential_source: "empty_token", - }); - }); - it.each([{ silent: false }, { silent: true }])( "emits credential_store failure category on failure (silent=$silent)", - async (options) => { + async ({ silent }) => { + const { manager, mockCredManager, event } = setupCliManager(); vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( new Error("keyring unavailable"), ); - await expect(configure(options)).rejects.toThrow("keyring unavailable"); - expect(configureEvent()).toMatchObject({ - properties: { - result: "error", - failure_category: "credential_store", - }, + await expect(manager.configure(URL, TOKEN, { silent })).rejects.toThrow( + "keyring unavailable", + ); + expect(event("cli.configure")).toMatchObject({ + properties: { result: "error", failure_category: "credential_store" }, error: { message: "keyring unavailable" }, }); }, ); - it("emits cancelled failure category when user cancels", async () => { - const error = new Error("The operation was aborted"); - error.name = "AbortError"; - vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce(error); + it("emits cancelled failure category when the user cancels", async () => { + const { manager, mockCredManager, expectProps } = setupCliManager(); + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + makeAbortError(), + ); - await expect(configure()).resolves.not.toThrow(); - expect(configureEvent().properties).toMatchObject({ + await expect(manager.configure(URL, TOKEN)).resolves.not.toThrow(); + expectProps("cli.configure", { result: "aborted", failure_category: "cancelled", }); @@ -184,33 +110,43 @@ describe("CliManager telemetry", () => { }); describe("cli.resolve and cli.download", () => { - const event = (name: string) => - telemetrySink.events.find((e) => e.eventName === name); - - it.each([ - { reason: "missing", setup: () => {} }, - { reason: "version_mismatch", setup: () => withExistingBinary("1.0.0") }, - { reason: "unreadable", setup: () => withCorruptedBinary() }, - ])("emits cli.download with reason=$reason", async ({ reason, setup }) => { - setup(); - withSuccessfulDownload(); - await manager.fetchBinary(mockApi); - - const e = event("cli.download"); - expect(e).toMatchObject({ - properties: { reason, result: "success" }, - }); - expect(e?.measurements.durationMs).toBeGreaterThanOrEqual(0); - expect(e?.measurements.downloaded_bytes).toBe( - Buffer.byteLength(mockBinaryContent(TEST_VERSION)), - ); - }); + const prepare: Record void> = { + missing: () => {}, + version_mismatch: (t) => t.withExistingBinary("1.0.0"), + unreadable: (t) => t.withCorruptedBinary(), + }; + + it.each(["missing", "version_mismatch", "unreadable"])( + "emits cli.download with reason=%s", + async (reason) => { + const t = setupCliManager(); + prepare[reason](t); + t.withSuccessfulDownload(); + + await t.manager.fetchBinary(t.mockApi); + + t.expectProps("cli.download", { reason, result: "success" }); + expect(t.event("cli.download").measurements).toMatchObject({ + durationMs: expect.any(Number), + downloaded_bytes: Buffer.byteLength(mockBinaryContent(TEST_VERSION)), + }); + }, + ); it("emits cli.resolve cache-hit phases without cli.download", async () => { + const { + manager, + mockApi, + event, + noEvent, + expectProps, + withExistingBinary, + } = setupCliManager(); withExistingBinary(TEST_VERSION); + await manager.fetchBinary(mockApi); - expect(event("cli.download")).toBeUndefined(); + noEvent("cli.download"); expect(event("cli.resolve")).toMatchObject({ properties: { result: "success", @@ -220,62 +156,71 @@ describe("CliManager telemetry", () => { }, measurements: { durationMs: expect.any(Number) }, }); - expect(event("cli.resolve.cache_lookup")).toMatchObject({ - properties: { source: "directory", result: "success" }, + expectProps("cli.resolve.cache_lookup", { + source: "directory", + result: "success", }); - expect(event("cli.resolve.version_check")).toMatchObject({ - properties: { outcome: "match", result: "success" }, + expectProps("cli.resolve.version_check", { + outcome: "match", + result: "success", }); }); it("distinguishes disabled-download fallback", async () => { + const { + manager, + mockApi, + mockConfig, + noEvent, + expectProps, + withExistingBinary, + } = setupCliManager(); mockConfig.set("coder.enableDownloads", false); withExistingBinary("1.0.0"); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); - expect(event("cli.download")).toBeUndefined(); - expect(event("cli.resolve.download_decision")).toMatchObject({ - properties: { - reason: "version_mismatch", - outcome: "fallback", - result: "success", - }, + noEvent("cli.download"); + expectProps("cli.resolve.download_decision", { + reason: "version_mismatch", + outcome: "fallback", + result: "success", }); - expect(event("cli.resolve")).toMatchObject({ - properties: { - outcome: "download_disabled_fallback", - result: "success", - }, + expectProps("cli.resolve", { + outcome: "download_disabled_fallback", + result: "success", }); }); it("distinguishes disabled-download failure", async () => { + const { manager, mockApi, mockConfig, noEvent, expectProps } = + setupCliManager(); mockConfig.set("coder.enableDownloads", false); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download CLI because downloads are disabled", ); - expect(event("cli.download")).toBeUndefined(); - expect(event("cli.resolve")).toMatchObject({ - properties: { - failure_category: "downloads_disabled", - result: "error", - }, + noEvent("cli.download"); + expectProps("cli.resolve", { + failure_category: "downloads_disabled", + result: "error", }); }); it("distinguishes fallback declined from download failure", async () => { + const { + manager, + mockApi, + mockUI, + expectProps, + withBinaryVersion, + withExistingBinary, + withInterruptedDownload, + } = setupCliManager(); withExistingBinary("1.0.0"); - vi.mocked(cliExec.version).mockResolvedValueOnce("1.0.0"); - withHttpResponse( - 200, - { "content-length": "1024" }, - createMockStream("partial-binary", { - error: new Error("connection reset"), - }), - ); + withBinaryVersion("1.0.0"); // fallback re-reads the existing binary + withInterruptedDownload(); mockUI.setResponse( "Failed to update CLI binary. Run version 1.0.0 anyway?", undefined, @@ -285,33 +230,41 @@ describe("CliManager telemetry", () => { "Unable to download binary: connection reset", ); - expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ - properties: { failure_category: "fallback_declined", result: "error" }, + expectProps("cli.resolve.fallback_to_existing_binary", { + failure_category: "fallback_declined", + result: "error", }); }); it("omits downloaded_bytes when the server returns 304", async () => { + const { + manager, + mockApi, + event, + expectProps, + withExistingBinary, + withHttpResponse, + } = setupCliManager(); withExistingBinary("1.0.0"); withHttpResponse(304); + await manager.fetchBinary(mockApi); - const e = event("cli.download"); - expect(e).toMatchObject({ - properties: { reason: "version_mismatch", result: "success" }, - }); - expect(e?.measurements.downloaded_bytes).toBeUndefined(); - expect(event("cli.resolve")).toMatchObject({ - properties: { outcome: "downloaded", result: "success" }, + expectProps("cli.download", { + reason: "version_mismatch", + result: "success", }); + expect( + event("cli.download").measurements.downloaded_bytes, + ).toBeUndefined(); + expectProps("cli.resolve", { outcome: "downloaded", result: "success" }); }); it("emits downloaded_bytes when a download fails mid-stream", async () => { + const { manager, mockApi, event, expectProps, withInterruptedDownload } = + setupCliManager(); const partial = "partial-binary"; - withHttpResponse( - 200, - { "content-length": "1024" }, - createMockStream(partial, { error: new Error("connection reset") }), - ); + withInterruptedDownload(partial); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: connection reset", @@ -322,45 +275,49 @@ describe("CliManager telemetry", () => { error: { message: "Unable to download binary: connection reset" }, measurements: { downloaded_bytes: Buffer.byteLength(partial) }, }); - expect(event("cli.resolve.fallback_to_existing_binary")).toMatchObject({ - properties: { failure_category: "download", result: "error" }, - }); - expect(event("cli.resolve")).toMatchObject({ - properties: { result: "error" }, + expectProps("cli.resolve.fallback_to_existing_binary", { + failure_category: "download", + result: "error", }); + expectProps("cli.resolve", { result: "error" }); }); describe("cli.download.verify", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", false); - withSuccessfulDownload(); - }); + /** A signature-verifying download, ready for the first GET to be queued. */ + function setupVerify(): CliManagerHarness { + const t = setupCliManager(); + t.mockConfig.set("coder.disableSignatureVerification", false); + t.withSuccessfulDownload(); + return t; + } it("emits outcome=verified on valid signature", async () => { - withSignatureResponses([200]); - await manager.fetchBinary(mockApi); + const t = setupVerify(); + t.withSignatureResponses([200]); - const verify = event("cli.download.verify"); - const download = event("cli.download"); + await t.manager.fetchBinary(t.mockApi); + + const verify = t.event("cli.download.verify"); + const download = t.event("cli.download"); expect(verify).toMatchObject({ properties: { outcome: "verified", result: "success" }, }); - expect(verify?.measurements.durationMs).toBeGreaterThanOrEqual(0); - expect(verify?.traceId).toBe(download?.traceId); - expect(verify?.parentEventId).toBe(download?.eventId); + expect(verify.measurements.durationMs).toBeGreaterThanOrEqual(0); + expect(verify.traceId).toBe(download.traceId); + expect(verify.parentEventId).toBe(download.eventId); }); it("emits outcome=bypassed when user runs anyway despite invalid signature", async () => { - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", "Run anyway"); + const t = setupVerify(); + t.withSignatureResponses([200]); + t.withInvalidSignature(); + t.mockUI.setResponse("Signature does not match", "Run anyway"); - await manager.fetchBinary(mockApi); + await t.manager.fetchBinary(t.mockApi); - expect(event("cli.download.verify")).toMatchObject({ - properties: { outcome: "bypassed", result: "success" }, + t.expectProps("cli.download.verify", { + outcome: "bypassed", + result: "success", }); }); @@ -370,109 +327,35 @@ describe("CliManager telemetry", () => { ])( "emits outcome=sig_not_found with sig_status=$status when user runs without verification", async ({ status, message }) => { - withSignatureResponses([status, status]); - mockUI.setResponse(message, "Run without verification"); + const t = setupVerify(); + t.withSignatureResponses([status, status]); + t.mockUI.setResponse(message, "Run without verification"); - await manager.fetchBinary(mockApi); + await t.manager.fetchBinary(t.mockApi); - expect(event("cli.download.verify")).toMatchObject({ - properties: { - outcome: "sig_not_found", - sig_status: String(status), - result: "success", - }, + t.expectProps("cli.download.verify", { + outcome: "sig_not_found", + sig_status: String(status), + result: "success", }); }, ); it("emits error when verification is aborted", async () => { - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", undefined); + const t = setupVerify(); + t.withSignatureResponses([200]); + t.withInvalidSignature(); + t.mockUI.setResponse("Signature does not match", undefined); - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + await expect(t.manager.fetchBinary(t.mockApi)).rejects.toThrow( "Signature verification aborted", ); - expect(event("cli.download.verify")).toMatchObject({ + expect(t.event("cli.download.verify")).toMatchObject({ properties: { result: "error" }, error: { message: "Signature verification aborted" }, }); }); }); }); - - function createMockApi(version: string, url: string): Api { - const axios = { - defaults: { baseURL: url }, - get: vi.fn(), - } as unknown as AxiosInstance; - return { - getBuildInfo: vi.fn().mockResolvedValue({ version }), - getAxiosInstance: () => axios, - } as unknown as Api; - } - - function withExistingBinary(version: string, dir: string = BINARY_DIR) { - vol.mkdirSync(dir, { recursive: true }); - memfs.writeFileSync(`${dir}/${BINARY_NAME}`, mockBinaryContent(version), { - mode: 0o755, - }); - vi.mocked(cliExec.version).mockResolvedValueOnce(version); - } - - function withCorruptedBinary() { - vol.mkdirSync(BINARY_DIR, { recursive: true }); - memfs.writeFileSync(BINARY_PATH, "corrupted-binary-content", { - mode: 0o755, - }); - vi.mocked(cliExec.version).mockRejectedValueOnce(new Error("corrupted")); - } - - function withSuccessfulDownload(opts?: { - headers?: Record; - }) { - const stream = createMockStream(mockBinaryContent(TEST_VERSION)); - withHttpResponse( - 200, - opts?.headers ?? { "content-length": "1024" }, - stream, - ); - vi.mocked(cliExec.version).mockResolvedValue(TEST_VERSION); - } - - function withSignatureResponses(statuses: number[]): void { - for (const status of statuses) { - const data = - status === 200 ? createMockStream("mock-signature-content") : undefined; - withHttpResponse(status, {}, data); - } - } - - function withHttpResponse( - status: number, - headers: Record = {}, - data?: unknown, - ) { - vi.mocked(mockAxios.get).mockResolvedValueOnce({ - status, - headers, - data, - }); - } }); - -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}`; -} diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 3059e709d1..e28854ae47 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -1,32 +1,29 @@ -import globalAxios, { type AxiosInstance } from "axios"; -import { type Api } from "coder/site/src/api/api"; import { fs as memfs, vol } from "memfs"; -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 { afterEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; -import * as cliExec from "@/core/cliExec"; -import { CliManager } from "@/core/cliManager"; -import { PathResolver } from "@/core/pathResolver"; import * as pgp from "@/pgp"; import { isKeyringEnabled } from "@/settings/cli"; -import { createTestTelemetryService } from "../../mocks/telemetry"; -import { - createMockCliCredentialManager, - createMockLogger, - createMockStream, - MockConfigurationProvider, - MockProgressReporter, - MockUserInteraction, -} from "../../mocks/testHelpers"; import { expectPathsEqual } from "../../utils/platform"; -import type { CliCredentialManager } from "@/core/cliCredentialManager"; +import { + BINARY_DIR, + BINARY_NAME, + BINARY_PATH, + type CliManagerHarness, + expectFileInDir, + flushPendingIO, + makeAbortError, + mockBinaryContent, + readdir, + setupCliManager, + TEST_URL, + TEST_VERSION, +} from "./cliManagerHarness"; + +import type * as fs from "node:fs"; vi.mock("os"); vi.mock("axios"); @@ -38,21 +35,14 @@ vi.mock("@/settings/cli", async () => { vi.mock("fs", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); - return { - ...memfs.fs, - default: memfs.fs, - }; + return { ...memfs.fs, default: memfs.fs }; }); vi.mock("fs/promises", async () => { const memfs: { fs: typeof fs } = await vi.importActual("memfs"); - return { - ...memfs.fs.promises, - default: memfs.fs.promises, - }; + return { ...memfs.fs.promises, default: memfs.fs.promises }; }); -// Mock lockfile to bypass file locking in tests vi.mock("proper-lockfile", () => ({ lock: () => Promise.resolve(() => Promise.resolve()), check: () => Promise.resolve(false), @@ -63,86 +53,31 @@ vi.mock("@/pgp"); vi.mock("@/core/cliExec", async () => { const actual = await vi.importActual("@/core/cliExec"); - return { - ...actual, - version: vi.fn(), - }; + return { ...actual, version: vi.fn() }; }); describe("CliManager", () => { - let manager: CliManager; - let mockConfig: MockConfigurationProvider; - let mockProgress: MockProgressReporter; - let mockUI: MockUserInteraction; - let mockApi: Api; - let mockAxios: AxiosInstance; - let mockCredManager: CliCredentialManager; - - const TEST_VERSION = "1.2.3"; - const TEST_URL = "https://test.coder.com"; - const BASE_PATH = "/path/base"; - const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; - const PLATFORM = "linux"; - const ARCH = "amd64"; - const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; - const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; - beforeEach(() => { - vi.resetAllMocks(); - vol.reset(); - - // Core setup - mockApi = createMockApi(TEST_VERSION, TEST_URL); - mockAxios = mockApi.getAxiosInstance(); - vi.mocked(globalAxios.create).mockReturnValue(mockAxios); - mockConfig = new MockConfigurationProvider(); - mockProgress = new MockProgressReporter(); - mockUI = new MockUserInteraction(); - mockCredManager = createMockCliCredentialManager(); - manager = new CliManager( - createMockLogger(), - new PathResolver(BASE_PATH, "/code/log"), - mockCredManager, - createTestTelemetryService(), - ); - - // Mock only what's necessary - vi.mocked(os.platform).mockReturnValue(PLATFORM); - vi.mocked(os.arch).mockReturnValue(ARCH); - vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); - - // Most tests don't need signature verification. - mockConfig.set("coder.disableSignatureVerification", true); - }); - - afterEach(async () => { - mockProgress?.setCancellation(false); - vi.clearAllTimers(); - // memfs internally schedules some FS operations so we have to wait for them to finish - await new Promise((resolve) => setImmediate(resolve)); - vol.reset(); - }); + afterEach(flushPendingIO); describe("Configure CLI", () => { - const CONFIGURE_URL = "https://coder.example.com"; + const URL = "https://coder.example.com"; const TOKEN = "test-token"; - function configure(options?: { silent?: boolean }) { - return manager.configure(CONFIGURE_URL, TOKEN, options); - } - it("should store credentials with progress notification", async () => { - await configure(); + const { manager, mockCredManager } = setupCliManager(); + + await manager.configure(URL, TOKEN); expect(vscode.window.withProgress).toHaveBeenCalledWith( expect.objectContaining({ location: vscode.ProgressLocation.Notification, - title: `Storing credentials for ${CONFIGURE_URL}`, + title: `Storing credentials for ${URL}`, cancellable: true, }), expect.any(Function), ); expect(mockCredManager.storeToken).toHaveBeenCalledWith( - CONFIGURE_URL, + URL, TOKEN, expect.anything(), { signal: expect.any(AbortSignal) }, @@ -150,17 +85,21 @@ describe("CliManager", () => { }); it("should skip progress when silent", async () => { - await configure({ silent: true }); + const { manager, mockCredManager } = setupCliManager(); + + await manager.configure(URL, TOKEN, { silent: true }); expect(vscode.window.withProgress).not.toHaveBeenCalled(); expect(mockCredManager.storeToken).toHaveBeenCalledWith( - CONFIGURE_URL, + URL, TOKEN, expect.anything(), ); }); it("should throw when URL is empty", async () => { + const { manager } = setupCliManager(); + await expect(manager.configure("", TOKEN)).rejects.toThrow( "URL is required to configure the CLI", ); @@ -169,11 +108,14 @@ describe("CliManager", () => { it.each([{ silent: false }, { silent: true }])( "should throw and show error on failure (silent=$silent)", async (options) => { + const { manager, mockCredManager } = setupCliManager(); vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( new Error("keyring unavailable"), ); - await expect(configure(options)).rejects.toThrow("keyring unavailable"); + await expect(manager.configure(URL, TOKEN, options)).rejects.toThrow( + "keyring unavailable", + ); expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( expect.stringContaining("keyring unavailable"), "Open Settings", @@ -182,23 +124,25 @@ describe("CliManager", () => { ); it("should swallow AbortError when user cancels", async () => { + const { manager, mockCredManager } = setupCliManager(); vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( makeAbortError(), ); - await expect(configure()).resolves.not.toThrow(); + await expect(manager.configure(URL, TOKEN)).resolves.not.toThrow(); expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); }); }); describe("Locate Binary", () => { it("returns path when binary exists", async () => { + const { manager, withExistingBinary } = setupCliManager(); withExistingBinary(TEST_VERSION); - const result = await manager.locateBinary(TEST_URL); - expectPathsEqual(result, BINARY_PATH); + expectPathsEqual(await manager.locateBinary(TEST_URL), BINARY_PATH); }); it("throws when binary does not exist", async () => { + const { manager } = setupCliManager(); await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( "No CLI binary found at", ); @@ -206,11 +150,11 @@ describe("CliManager", () => { }); describe("Binary Resolution", () => { - /** Simulate a download failure with a usable fallback binary. */ - function withFailedDownload(fallbackVersion: string) { - withStreamError("write", "disk full"); - vi.mocked(cliExec.version).mockResolvedValueOnce(fallbackVersion); - mockUI.setResponse( + /** Simulate a write failure where the user accepts the existing fallback. */ + function withFailedDownload(t: CliManagerHarness, fallbackVersion: string) { + t.withStreamError("write", "disk full"); + t.withBinaryVersion(fallbackVersion); + t.mockUI.setResponse( `Failed to update CLI binary. Run version ${fallbackVersion} anyway?`, "Run", ); @@ -218,24 +162,29 @@ describe("CliManager", () => { describe("file destination", () => { const FILE_PATH = "/usr/local/bin/coder"; - const FILE_DIR = path.dirname(FILE_PATH); - const DOWNLOAD_PATH = path.join(FILE_DIR, BINARY_NAME); - - function withFileBinary(filePath: string, version: string) { - mockConfig.set("coder.binaryDestination", filePath); + const DOWNLOAD_PATH = path.join(path.dirname(FILE_PATH), BINARY_NAME); + + function withFileBinary( + t: CliManagerHarness, + filePath: string, + version: string, + ) { + t.mockConfig.set("coder.binaryDestination", filePath); vol.mkdirSync(path.dirname(filePath), { recursive: true }); memfs.writeFileSync(filePath, mockBinaryContent(version), { mode: 0o755, }); - vi.mocked(cliExec.version).mockResolvedValueOnce(version); + t.withBinaryVersion(version); } it("locateBinary returns file path directly", async () => { - withFileBinary(FILE_PATH, TEST_VERSION); - expectPathsEqual(await manager.locateBinary(TEST_URL), FILE_PATH); + const t = setupCliManager(); + withFileBinary(t, FILE_PATH, TEST_VERSION); + expectPathsEqual(await t.manager.locateBinary(TEST_URL), FILE_PATH); }); it("locateBinary throws when file does not exist", async () => { + const { manager, mockConfig } = setupCliManager(); mockConfig.set("coder.binaryDestination", "/nonexistent/coder"); await expect(manager.locateBinary(TEST_URL)).rejects.toThrow( "No CLI binary found at", @@ -243,16 +192,18 @@ describe("CliManager", () => { }); it("fetchBinary uses file when version matches", async () => { - withFileBinary(FILE_PATH, TEST_VERSION); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); - expect(mockAxios.get).not.toHaveBeenCalled(); + const t = setupCliManager(); + withFileBinary(t, FILE_PATH, TEST_VERSION); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), FILE_PATH); + expect(t.mockAxios.get).not.toHaveBeenCalled(); }); it("fetchBinary downloads to platform-specific name then renames", async () => { - withFileBinary(FILE_PATH, "0.0.1"); - withSuccessfulDownload(); + const t = setupCliManager(); + withFileBinary(t, FILE_PATH, "0.0.1"); + t.withSuccessfulDownload(); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), FILE_PATH); expect(memfs.existsSync(DOWNLOAD_PATH)).toBe(false); expect(memfs.readFileSync(FILE_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), @@ -260,38 +211,42 @@ describe("CliManager", () => { }); it("fetchBinary downloads in-place when file is already platform-specific", async () => { + const t = setupCliManager(); // User configured a path that matches the platform-specific name. - withFileBinary(DOWNLOAD_PATH, "0.0.1"); - withSuccessfulDownload(); + withFileBinary(t, DOWNLOAD_PATH, "0.0.1"); + t.withSuccessfulDownload(); - expectPathsEqual(await manager.fetchBinary(mockApi), DOWNLOAD_PATH); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), DOWNLOAD_PATH); expect(memfs.readFileSync(DOWNLOAD_PATH).toString()).toBe( mockBinaryContent(TEST_VERSION), ); }); it("fetchBinary keeps mismatched file when downloads disabled", async () => { - mockConfig.set("coder.enableDownloads", false); - withFileBinary(FILE_PATH, "0.0.1"); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); - expect(mockAxios.get).not.toHaveBeenCalled(); + const t = setupCliManager(); + t.mockConfig.set("coder.enableDownloads", false); + withFileBinary(t, FILE_PATH, "0.0.1"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), FILE_PATH); + expect(t.mockAxios.get).not.toHaveBeenCalled(); }); it("fetchBinary falls back to configured path on download failure", async () => { - withFileBinary(FILE_PATH, "0.0.1"); - withFailedDownload("0.0.1"); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + const t = setupCliManager(); + withFileBinary(t, FILE_PATH, "0.0.1"); + withFailedDownload(t, "0.0.1"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), FILE_PATH); }); it("fetchBinary renames fallback to configured path on download failure", async () => { - withFileBinary(FILE_PATH, "0.0.1"); + const t = setupCliManager(); + withFileBinary(t, FILE_PATH, "0.0.1"); // A previous download left a binary at the platform-specific path. memfs.writeFileSync(DOWNLOAD_PATH, mockBinaryContent("0.0.1"), { mode: 0o755, }); - withFailedDownload("0.0.1"); + withFailedDownload(t, "0.0.1"); - expectPathsEqual(await manager.fetchBinary(mockApi), FILE_PATH); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), FILE_PATH); expect(memfs.existsSync(DOWNLOAD_PATH)).toBe(false); }); }); @@ -299,38 +254,42 @@ describe("CliManager", () => { describe("simple name fallback", () => { const SIMPLE_PATH = `${BINARY_DIR}/coder`; - function withSimpleBinary(version: string) { + function withSimpleBinary(t: CliManagerHarness, version: string) { vol.mkdirSync(BINARY_DIR, { recursive: true }); memfs.writeFileSync(SIMPLE_PATH, mockBinaryContent(version), { mode: 0o755, }); - vi.mocked(cliExec.version).mockResolvedValueOnce(version); + t.withBinaryVersion(version); } it("locateBinary falls back to simple name", async () => { - withSimpleBinary(TEST_VERSION); - expectPathsEqual(await manager.locateBinary(TEST_URL), SIMPLE_PATH); + const t = setupCliManager(); + withSimpleBinary(t, TEST_VERSION); + expectPathsEqual(await t.manager.locateBinary(TEST_URL), SIMPLE_PATH); }); it("locateBinary prefers platform-specific name", async () => { - withSimpleBinary(TEST_VERSION); + const t = setupCliManager(); + withSimpleBinary(t, TEST_VERSION); memfs.writeFileSync(BINARY_PATH, mockBinaryContent(TEST_VERSION), { mode: 0o755, }); - expectPathsEqual(await manager.locateBinary(TEST_URL), BINARY_PATH); + expectPathsEqual(await t.manager.locateBinary(TEST_URL), BINARY_PATH); }); it("fetchBinary uses simple-named binary", async () => { - withSimpleBinary(TEST_VERSION); - expectPathsEqual(await manager.fetchBinary(mockApi), SIMPLE_PATH); - expect(mockAxios.get).not.toHaveBeenCalled(); + const t = setupCliManager(); + withSimpleBinary(t, TEST_VERSION); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), SIMPLE_PATH); + expect(t.mockAxios.get).not.toHaveBeenCalled(); }); it("fetchBinary downloads to platform-specific name (not simple name)", async () => { - withSimpleBinary("0.0.1"); - withSuccessfulDownload(); + const t = setupCliManager(); + withSimpleBinary(t, "0.0.1"); + t.withSuccessfulDownload(); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), BINARY_PATH); expect(memfs.readFileSync(SIMPLE_PATH).toString()).toBe( mockBinaryContent("0.0.1"), ); @@ -340,9 +299,10 @@ describe("CliManager", () => { }); it("fetchBinary falls back to simple name on download failure", async () => { - withSimpleBinary("0.0.1"); - withFailedDownload("0.0.1"); - expectPathsEqual(await manager.fetchBinary(mockApi), SIMPLE_PATH); + const t = setupCliManager(); + withSimpleBinary(t, "0.0.1"); + withFailedDownload(t, "0.0.1"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), SIMPLE_PATH); }); }); }); @@ -351,6 +311,8 @@ describe("CliManager", () => { const CLEAR_URL = "https://dev.coder.com"; it("should skip progress notification when keyring is disabled", async () => { + const { manager, mockCredManager } = setupCliManager(); + await manager.clearCredentials(CLEAR_URL); expect(vscode.window.withProgress).not.toHaveBeenCalled(); @@ -362,6 +324,7 @@ describe("CliManager", () => { }); it("should show progress notification when keyring is enabled", async () => { + const { manager } = setupCliManager(); vi.mocked(isKeyringEnabled).mockReturnValue(true); await manager.clearCredentials(CLEAR_URL); @@ -381,6 +344,7 @@ describe("CliManager", () => { { scenario: "fails", error: new Error("unexpected failure") }, { scenario: "is cancelled", error: makeAbortError() }, ])("should not throw when deleteToken $scenario", async ({ error }) => { + const { manager, mockCredManager } = setupCliManager(); if (error) { vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); } @@ -390,6 +354,7 @@ describe("CliManager", () => { describe("Binary Version Validation", () => { it("rejects invalid server versions", async () => { + const { manager, mockApi } = setupCliManager(); mockApi.getBuildInfo = vi.fn().mockResolvedValue({ version: "invalid" }); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Got invalid version from deployment", @@ -397,6 +362,7 @@ describe("CliManager", () => { }); it("accepts valid semver versions", async () => { + const { manager, mockApi, withExistingBinary } = setupCliManager(); withExistingBinary(TEST_VERSION); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); }); @@ -404,14 +370,17 @@ describe("CliManager", () => { describe("Existing Binary Handling", () => { it("reuses matching binary without downloading", async () => { + const { manager, mockApi, mockAxios, withExistingBinary } = + setupCliManager(); withExistingBinary(TEST_VERSION); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); expect(mockAxios.get).not.toHaveBeenCalled(); - // Verify binary still exists expect(memfs.existsSync(BINARY_PATH)).toBe(true); }); it("downloads when versions differ", async () => { + const { manager, mockApi, withExistingBinary, withSuccessfulDownload } = + setupCliManager(); withExistingBinary("1.0.0"); withSuccessfulDownload(); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); @@ -421,6 +390,8 @@ describe("CliManager", () => { }); it("keeps mismatched binary when downloads disabled", async () => { + const { manager, mockApi, mockAxios, mockConfig, withExistingBinary } = + setupCliManager(); mockConfig.set("coder.enableDownloads", false); withExistingBinary("1.0.0"); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); @@ -431,6 +402,8 @@ describe("CliManager", () => { }); it("downloads fresh binary when corrupted", async () => { + const { manager, mockApi, withCorruptedBinary, withSuccessfulDownload } = + setupCliManager(); withCorruptedBinary(); withSuccessfulDownload(); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); @@ -440,6 +413,7 @@ describe("CliManager", () => { }); it("downloads when no binary exists", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); @@ -451,6 +425,7 @@ describe("CliManager", () => { }); it("fails when downloads disabled and no binary", async () => { + const { manager, mockApi, mockConfig } = setupCliManager(); mockConfig.set("coder.enableDownloads", false); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download CLI because downloads are disabled", @@ -459,6 +434,8 @@ describe("CliManager", () => { }); it("restores old backup when replace fails", async () => { + const { manager, mockApi, withExistingBinary, withSuccessfulDownload } = + setupCliManager(); withExistingBinary("1.0.0"); withSuccessfulDownload(); @@ -486,6 +463,8 @@ describe("CliManager", () => { describe("Binary Download Behavior", () => { it("downloads with correct headers", async () => { + const { manager, mockApi, mockAxios, withSuccessfulDownload } = + setupCliManager(); withSuccessfulDownload(); await manager.fetchBinary(mockApi); expect(mockAxios.get).toHaveBeenCalledWith( @@ -501,6 +480,13 @@ describe("CliManager", () => { }); it("uses custom binary source", async () => { + const { + manager, + mockApi, + mockAxios, + mockConfig, + withSuccessfulDownload, + } = setupCliManager(); mockConfig.set("coder.binarySource", "/custom/path"); withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -515,6 +501,13 @@ describe("CliManager", () => { }); it("uses ETag for existing binaries", async () => { + const { + manager, + mockApi, + mockAxios, + withExistingBinary, + withSuccessfulDownload, + } = setupCliManager(); withExistingBinary("1.0.0"); withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -531,6 +524,7 @@ describe("CliManager", () => { }); it("cleans up old files before download", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); // Create old temporary files and signature files matching the binary name vol.mkdirSync(BINARY_DIR, { recursive: true }); memfs.writeFileSync( @@ -568,14 +562,15 @@ describe("CliManager", () => { }); it("moves existing binary to backup file before writing new version", async () => { + const { manager, mockApi, withExistingBinary, withSuccessfulDownload } = + setupCliManager(); withExistingBinary("1.0.0"); withSuccessfulDownload(); await manager.fetchBinary(mockApi); // Verify the old binary was backed up - const files = readdir(BINARY_DIR); - const backupFile = files.find( + const backupFile = readdir(BINARY_DIR).find( (f) => f.startsWith(BINARY_NAME) && /\.old-[a-z0-9]+$/.exec(f), ); expect(backupFile).toBeDefined(); @@ -584,6 +579,8 @@ describe("CliManager", () => { describe("Download HTTP Response Handling", () => { it("handles 304 Not Modified", async () => { + const { manager, mockApi, withExistingBinary, withHttpResponse } = + setupCliManager(); withExistingBinary("1.0.0"); withHttpResponse(304); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); @@ -593,6 +590,7 @@ describe("CliManager", () => { }); it("handles 404 platform not supported", async () => { + const { manager, mockApi, mockUI, withHttpResponse } = setupCliManager(); withHttpResponse(404); mockUI.setResponse( "Coder isn't supported for your platform. Please open an issue, we'd love to support it!", @@ -611,6 +609,7 @@ describe("CliManager", () => { }); it("handles server errors", async () => { + const { manager, mockApi, mockUI, withHttpResponse } = setupCliManager(); withHttpResponse(500); mockUI.setResponse( "Failed to download binary. Please open an issue.", @@ -631,6 +630,7 @@ describe("CliManager", () => { describe("Download Stream Handling", () => { it("handles write stream errors", async () => { + const { manager, mockApi, withStreamError } = setupCliManager(); withStreamError("write", "disk full"); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: disk full", @@ -639,6 +639,7 @@ describe("CliManager", () => { }); it("handles read stream errors", async () => { + const { manager, mockApi, withStreamError } = setupCliManager(); withStreamError("read", "network timeout"); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( "Unable to download binary: network timeout", @@ -647,6 +648,8 @@ describe("CliManager", () => { }); it("handles missing content-length", async () => { + const { manager, mockApi, mockProgress, withSuccessfulDownload } = + setupCliManager(); withSuccessfulDownload({ headers: {} }); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); const reports = mockProgress.getProgressReports(); @@ -659,6 +662,8 @@ describe("CliManager", () => { it.each(["content-length", "x-original-content-length"])( "reports progress with %s header", async (header) => { + const { manager, mockApi, mockProgress, withSuccessfulDownload } = + setupCliManager(); withSuccessfulDownload({ headers: { [header]: "1024" } }); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); const reports = mockProgress.getProgressReports(); @@ -672,6 +677,7 @@ describe("CliManager", () => { describe("Download Progress Tracking", () => { it("shows download progress", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); withSuccessfulDownload(); await manager.fetchBinary(mockApi); expect(vscode.window.withProgress).toHaveBeenCalledWith( @@ -683,6 +689,8 @@ describe("CliManager", () => { }); it("handles user cancellation", async () => { + const { manager, mockApi, mockProgress, withSuccessfulDownload } = + setupCliManager(); mockProgress.setCancellation(true); withSuccessfulDownload(); await expect(manager.fetchBinary(mockApi)).rejects.toThrow( @@ -693,50 +701,52 @@ describe("CliManager", () => { }); describe("Binary Signature Verification", () => { - beforeEach(() => { - mockConfig.set("coder.disableSignatureVerification", false); - }); + /** A download with signature verification enabled. */ + function setupVerify(): CliManagerHarness { + const t = setupCliManager(); + t.mockConfig.set("coder.disableSignatureVerification", false); + t.withSuccessfulDownload(); + return t; + } it("verifies valid signatures", async () => { - withSuccessfulDownload(); - withSignatureResponses([200]); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + const t = setupVerify(); + t.withSignatureResponses([200]); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), BINARY_PATH); expect(pgp.verifySignature).toHaveBeenCalled(); expect(expectFileInDir(BINARY_DIR, ".asc")).toBeDefined(); }); it("tries fallback signature on 404", async () => { - withSuccessfulDownload(); - withSignatureResponses([404, 200]); - mockUI.setResponse("Signature not found", "Download signature"); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); - expect(mockAxios.get).toHaveBeenCalledTimes(3); + const t = setupVerify(); + t.withSignatureResponses([404, 200]); + t.mockUI.setResponse("Signature not found", "Download signature"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), BINARY_PATH); + expect(t.mockAxios.get).toHaveBeenCalledTimes(3); expect(expectFileInDir(BINARY_DIR, ".asc")).toBeDefined(); }); it("allows running despite invalid signature", async () => { - withSuccessfulDownload(); - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", "Run anyway"); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + const t = setupVerify(); + t.withSignatureResponses([200]); + t.withInvalidSignature(); + t.mockUI.setResponse("Signature does not match", "Run anyway"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), BINARY_PATH); }); it("aborts on signature rejection", async () => { - withSuccessfulDownload(); - withSignatureResponses([200]); - vi.mocked(pgp.verifySignature).mockRejectedValueOnce( - createVerificationError("Invalid signature"), - ); - mockUI.setResponse("Signature does not match", undefined); - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + const t = setupVerify(); + t.withSignatureResponses([200]); + t.withInvalidSignature(); + t.mockUI.setResponse("Signature does not match", undefined); + await expect(t.manager.fetchBinary(t.mockApi)).rejects.toThrow( "Signature verification aborted", ); }); it("skips verification when disabled", async () => { + const { manager, mockApi, mockConfig, withSuccessfulDownload } = + setupCliManager(); mockConfig.set("coder.disableSignatureVerification", true); withSuccessfulDownload(); expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); @@ -748,10 +758,10 @@ describe("CliManager", () => { [404, "Signature not found"], [500, "Failed to download signature"], ])("allows skipping verification on %i", async (status, message) => { - withSuccessfulDownload(); - withHttpResponse(status); - mockUI.setResponse(message, "Run without verification"); - expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); + const t = setupVerify(); + t.withHttpResponse(status); + t.mockUI.setResponse(message, "Run without verification"); + expectPathsEqual(await t.manager.fetchBinary(t.mockApi), BINARY_PATH); expect(pgp.verifySignature).not.toHaveBeenCalled(); }); @@ -761,10 +771,10 @@ describe("CliManager", () => { ])( "aborts when user declines missing signature on %i", async (status, message) => { - withSuccessfulDownload(); - withHttpResponse(status); - mockUI.setResponse(message, undefined); // User cancels - await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + const t = setupVerify(); + t.withHttpResponse(status); + t.mockUI.setResponse(message, undefined); // User cancels + await expect(t.manager.fetchBinary(t.mockApi)).rejects.toThrow( "Signature download aborted", ); }, @@ -773,6 +783,7 @@ describe("CliManager", () => { describe("File System Operations", () => { it("creates binary directory", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -780,6 +791,7 @@ describe("CliManager", () => { }); it("validates downloaded binary version", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); withSuccessfulDownload(); await manager.fetchBinary(mockApi); expect(memfs.readFileSync(BINARY_PATH).toString()).toBe( @@ -788,6 +800,7 @@ describe("CliManager", () => { }); it("sets correct file permissions", async () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); withSuccessfulDownload(); await manager.fetchBinary(mockApi); expect(memfs.statSync(BINARY_PATH).mode & 0o777).toBe(0o755); @@ -796,13 +809,9 @@ describe("CliManager", () => { describe("Path Pecularities", () => { it("handles binary with spaces in path", async () => { - const pathWithSpaces = "/path with spaces/bin"; - const manager = new CliManager( - createMockLogger(), - new PathResolver(pathWithSpaces, "/log"), - createMockCliCredentialManager(), - createTestTelemetryService(), - ); + const pathWithSpaces = "/path with spaces"; + const { manager, mockApi, withSuccessfulDownload } = + setupCliManager(pathWithSpaces); withSuccessfulDownload(); expectPathsEqual( @@ -811,133 +820,4 @@ describe("CliManager", () => { ); }); }); - - function createMockApi(version: string, url: string): Api { - const axios = { - defaults: { baseURL: url }, - get: vi.fn(), - } as unknown as AxiosInstance; - return { - getBuildInfo: vi.fn().mockResolvedValue({ version }), - getAxiosInstance: () => axios, - } as unknown as Api; - } - - function withExistingBinary(version: string, dir: string = BINARY_DIR) { - vol.mkdirSync(dir, { recursive: true }); - memfs.writeFileSync(`${dir}/${BINARY_NAME}`, mockBinaryContent(version), { - mode: 0o755, - }); - - // Mock version to return the specified version - vi.mocked(cliExec.version).mockResolvedValueOnce(version); - } - - function withCorruptedBinary() { - vol.mkdirSync(BINARY_DIR, { recursive: true }); - memfs.writeFileSync(BINARY_PATH, "corrupted-binary-content", { - mode: 0o755, - }); - - // Mock version to fail - vi.mocked(cliExec.version).mockRejectedValueOnce(new Error("corrupted")); - } - - function withSuccessfulDownload(opts?: { - headers?: Record; - }) { - const stream = createMockStream(mockBinaryContent(TEST_VERSION)); - withHttpResponse( - 200, - opts?.headers ?? { "content-length": "1024" }, - stream, - ); - - // Mock version to return TEST_VERSION after download - vi.mocked(cliExec.version).mockResolvedValue(TEST_VERSION); - } - - function withSignatureResponses(statuses: number[]): void { - for (const status of statuses) { - const data = - status === 200 ? createMockStream("mock-signature-content") : undefined; - withHttpResponse(status, {}, data); - } - } - - function withHttpResponse( - status: number, - headers: Record = {}, - data?: unknown, - ) { - vi.mocked(mockAxios.get).mockResolvedValueOnce({ - status, - headers, - data, - }); - } - - function withStreamError(type: "read" | "write", message: string) { - if (type === "write") { - vi.spyOn(fs, "createWriteStream").mockImplementation(() => { - const stream = new EventEmitter(); - (stream as unknown as fs.WriteStream).write = vi.fn(); - (stream as unknown as fs.WriteStream).close = vi.fn(); - // Emit error on next tick after stream is returned - setImmediate(() => { - stream.emit("error", new Error(message)); - }); - - return stream as ReturnType; - }); - - // Provide a normal read stream - withHttpResponse( - 200, - { "content-length": "256" }, - createMockStream("data"), - ); - } else { - // Create a read stream that emits error - const errorStream = { - on: vi.fn((event: string, callback: (...args: unknown[]) => void) => { - if (event === "error") { - setImmediate(() => callback(new Error(message))); - } - return errorStream; - }), - destroy: vi.fn(), - } as unknown as IncomingMessage; - - withHttpResponse(200, { "content-length": "1024" }, errorStream); - } - } }); - -function makeAbortError(): Error { - const error = new Error("The operation was aborted"); - error.name = "AbortError"; - return error; -} - -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/cliManagerHarness.ts b/test/unit/core/cliManagerHarness.ts new file mode 100644 index 0000000000..a411bf1874 --- /dev/null +++ b/test/unit/core/cliManagerHarness.ts @@ -0,0 +1,237 @@ +import globalAxios, { type AxiosInstance } from "axios"; +import { type Api } from "coder/site/src/api/api"; +import { fs as memfs, vol } from "memfs"; +import EventEmitter from "node:events"; +import * as fs from "node:fs"; +import { type IncomingMessage } from "node:http"; +import * as os from "node:os"; +import { expect, vi } from "vitest"; + +import * as cliExec from "@/core/cliExec"; +import { CliManager } from "@/core/cliManager"; +import { PathResolver } from "@/core/pathResolver"; +import * as pgp from "@/pgp"; + +import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; +import { + createMockCliCredentialManager, + createMockLogger, + createMockStream, + MockConfigurationProvider, + MockProgressReporter, + MockUserInteraction, +} from "../../mocks/testHelpers"; + +export const TEST_VERSION = "1.2.3"; +export const TEST_URL = "https://test.coder.com"; +export const BASE_PATH = "/path/base"; +export const BINARY_DIR = `${BASE_PATH}/test.coder.com/bin`; +export const PLATFORM = "linux"; +export const ARCH = "amd64"; +export const BINARY_NAME = `coder-${PLATFORM}-${ARCH}`; +export const BINARY_PATH = `${BINARY_DIR}/${BINARY_NAME}`; + +export type CliManagerHarness = ReturnType; + +/** + * Build a fresh `CliManager` against memfs and mocked collaborators. Call once + * per test and destructure what you need; the builders close over this instance. + */ +export function setupCliManager(basePath: string = BASE_PATH) { + vi.resetAllMocks(); + vol.reset(); + + const telemetrySink = new TestSink(); + const mockApi = createMockApi(TEST_VERSION, TEST_URL); + const mockAxios = mockApi.getAxiosInstance(); + vi.mocked(globalAxios.create).mockReturnValue(mockAxios); + + const mockConfig = new MockConfigurationProvider(); + const mockProgress = new MockProgressReporter(); + const mockUI = new MockUserInteraction(); + const mockCredManager = createMockCliCredentialManager(); + const manager = new CliManager( + createMockLogger(), + new PathResolver(basePath, "/code/log"), + mockCredManager, + createTestTelemetryService(telemetrySink), + ); + + vi.mocked(os.platform).mockReturnValue(PLATFORM); + vi.mocked(os.arch).mockReturnValue(ARCH); + vi.mocked(pgp.readPublicKeys).mockResolvedValue([]); + mockConfig.set("coder.disableSignatureVerification", true); + + /** Queue the next axios GET to resolve with this response. */ + const withHttpResponse = ( + status: number, + headers: Record = {}, + data?: unknown, + ) => + vi.mocked(mockAxios.get).mockResolvedValueOnce({ status, headers, data }); + + /** Queue the next CLI version read to resolve `version`. */ + const withBinaryVersion = (version: string) => + vi.mocked(cliExec.version).mockResolvedValueOnce(version); + + /** Place a readable binary of `version` in `dir`. */ + const withExistingBinary = (version: string, dir: string = BINARY_DIR) => { + vol.mkdirSync(dir, { recursive: true }); + memfs.writeFileSync(`${dir}/${BINARY_NAME}`, mockBinaryContent(version), { + mode: 0o755, + }); + withBinaryVersion(version); + }; + + /** Place a cached binary whose version cannot be read. */ + const withCorruptedBinary = () => { + vol.mkdirSync(BINARY_DIR, { recursive: true }); + memfs.writeFileSync(BINARY_PATH, "corrupted-binary-content", { + mode: 0o755, + }); + vi.mocked(cliExec.version).mockRejectedValueOnce(new Error("corrupted")); + }; + + /** Serve a complete binary download that verifies as `TEST_VERSION`. */ + const withSuccessfulDownload = (opts?: { + headers?: Record; + }) => { + withHttpResponse( + 200, + opts?.headers ?? { "content-length": "1024" }, + createMockStream(mockBinaryContent(TEST_VERSION)), + ); + vi.mocked(cliExec.version).mockResolvedValue(TEST_VERSION); + }; + + /** Serve a download that errors mid-stream after `partial` bytes. */ + const withInterruptedDownload = (partial = "partial-binary") => + withHttpResponse( + 200, + { "content-length": "1024" }, + createMockStream(partial, { error: new Error("connection reset") }), + ); + + /** Queue one HTTP response per signature source status. */ + const withSignatureResponses = (statuses: number[]) => { + for (const status of statuses) { + const data = + status === 200 ? createMockStream("mock-signature-content") : undefined; + withHttpResponse(status, {}, data); + } + }; + + /** Make the next signature verification reject as invalid. */ + const withInvalidSignature = () => + vi + .mocked(pgp.verifySignature) + .mockRejectedValueOnce(createVerificationError("Invalid signature")); + + /** Fail the download via a read- or write-stream error. */ + const withStreamError = (type: "read" | "write", message: string) => { + if (type === "write") { + vi.spyOn(fs, "createWriteStream").mockImplementation(() => { + const stream = new EventEmitter(); + (stream as unknown as fs.WriteStream).write = vi.fn(); + (stream as unknown as fs.WriteStream).close = vi.fn(); + setImmediate(() => stream.emit("error", new Error(message))); + return stream as ReturnType; + }); + withHttpResponse( + 200, + { "content-length": "256" }, + createMockStream("data"), + ); + return; + } + const errorStream = { + on: vi.fn((event: string, callback: (...args: unknown[]) => void) => { + if (event === "error") { + setImmediate(() => callback(new Error(message))); + } + return errorStream; + }), + destroy: vi.fn(), + } as unknown as IncomingMessage; + withHttpResponse(200, { "content-length": "1024" }, errorStream); + }; + + /** The single event named `name`, asserting exactly one was emitted. */ + const event = (name: string) => telemetrySink.expectOne(name); + /** Asserts no event named `name` was emitted. */ + const noEvent = (name: string) => + expect(telemetrySink.eventsNamed(name)).toHaveLength(0); + /** Asserts the single event named `name` carries (at least) `properties`. */ + const expectProps = (name: string, properties: Record) => + expect(event(name).properties).toMatchObject(properties); + + return { + manager, + mockConfig, + mockProgress, + mockUI, + mockApi, + mockAxios, + mockCredManager, + telemetrySink, + withHttpResponse, + withBinaryVersion, + withExistingBinary, + withCorruptedBinary, + withSuccessfulDownload, + withInterruptedDownload, + withSignatureResponses, + withInvalidSignature, + withStreamError, + event, + noEvent, + expectProps, + }; +} + +/** Drain memfs's internally-scheduled FS operations between tests. */ +export async function flushPendingIO(): Promise { + vi.clearAllTimers(); + await new Promise((resolve) => setImmediate(resolve)); +} + +function createMockApi(version: string, url: string): Api { + const axios = { + defaults: { baseURL: url }, + get: vi.fn(), + } as unknown as AxiosInstance; + return { + getBuildInfo: vi.fn().mockResolvedValue({ version }), + getAxiosInstance: () => axios, + } as unknown as Api; +} + +export function mockBinaryContent(version: string): string { + return `mock-binary-v${version}`; +} + +export function makeAbortError(): Error { + const error = new Error("The operation was aborted"); + error.name = "AbortError"; + return error; +} + +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; +} + +export function readdir(dir: string): string[] { + return memfs.readdirSync(dir) as string[]; +} + +export function expectFileInDir( + dir: string, + pattern: string, +): string | undefined { + return readdir(dir).find((f) => f.includes(pattern)); +} From 092212baa3282192b88c768277dd9e90d0383e67 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 10 Jun 2026 15:03:12 +0300 Subject: [PATCH 6/7] refactor: align CLI telemetry naming with instrumentation conventions Rename CliTelemetry span wrappers to the trace form used across src/instrumentation (traceResolve, traceDownload, traceConfigure) and CliConfigureTrace outcome methods to the shared cancel/fail verb pair. --- src/core/cliManager.ts | 12 ++++++------ src/instrumentation/cli.ts | 12 +++++++----- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 474c8372f6..4b97d3e179 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -134,7 +134,7 @@ export class CliManager { * downloads being disabled. */ public fetchBinary(restClient: Api): Promise { - return this.cliTelemetry.resolve((trace) => + return this.cliTelemetry.traceResolve((trace) => this.resolveBinary(restClient, trace), ); } @@ -322,7 +322,7 @@ export class CliManager { latestVersion = waitResult.parsedVersion; } - const result = await this.cliTelemetry.download( + const result = await this.cliTelemetry.traceDownload( downloadReason, async (span) => { const downloadedBinPath = await this.performBinaryDownload( @@ -1013,7 +1013,7 @@ export class CliManager { } const silent = options?.silent === true; - return this.cliTelemetry.configure( + return this.cliTelemetry.traceConfigure( { silent, credentialSource: token === "" ? "empty_token" : "session_token", @@ -1033,7 +1033,7 @@ export class CliManager { try { await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { - trace.failed(error); + trace.fail(error); this.handleStoreError(error); } return; @@ -1053,10 +1053,10 @@ export class CliManager { } if (result.cancelled) { this.output.info("Credential storage cancelled by user"); - trace.cancelled(); + trace.cancel(); return; } - trace.failed(result.error); + trace.fail(result.error); this.handleStoreError(result.error); } diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index f80f4d5301..56c8eaea91 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -58,20 +58,22 @@ export class CliFallbackDeclinedError extends Error { export class CliTelemetry { public constructor(private readonly telemetry: TelemetryService) {} - public resolve(fn: (trace: CliResolveTrace) => Promise): Promise { + public traceResolve( + fn: (trace: CliResolveTrace) => Promise, + ): Promise { return this.telemetry.trace("cli.resolve", (span) => fn(new CliResolveTrace(span)), ); } - public download( + public traceDownload( reason: CliDownloadReason, fn: (span: Span) => Promise, ): Promise { return this.telemetry.trace("cli.download", fn, { reason }); } - public configure( + public traceConfigure( options: CliConfigureOptions, fn: (trace: CliConfigureTrace) => Promise, ): Promise { @@ -185,12 +187,12 @@ export class CliResolveTrace { export class CliConfigureTrace { public constructor(private readonly span: Span) {} - public cancelled(): void { + public cancel(): void { this.span.setProperty("failure_category", "cancelled"); this.span.markAborted(); } - public failed(error: unknown): void { + public fail(error: unknown): void { this.span.setProperty( "failure_category", categorizeConfigureFailure(error), From 409141eb49ca84b4af74ef6b46cff81b8357d974 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 11 Jun 2026 13:44:56 +0300 Subject: [PATCH 7/7] fix: address CLI telemetry review feedback - Record error.type via the shared recordError/recordAborted helpers instead of failure_category, using abort_stage for cancellation, to match the instrumentation conventions. - Replace the no-op download_decision phase with download_reason and download_action properties on cli.resolve. - Carry the fallback's error.type on the parent resolve span and record fallback_reason for why the fallback was triggered. --- src/core/cliManager.ts | 12 ++--- src/instrumentation/cli.ts | 58 ++++++++------------- test/unit/core/cliManager.telemetry.test.ts | 32 +++++++----- 3 files changed, 47 insertions(+), 55 deletions(-) diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 4b97d3e179..5d036e2a83 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -145,7 +145,7 @@ export class CliManager { ): Promise { const baseUrl = restClient.getAxiosInstance().defaults.baseURL; if (!baseUrl) { - trace.setFailure("unknown"); + trace.error("unknown"); throw new Error("REST client has no base URL configured"); } const safeHostname = toSafeHost(baseUrl); @@ -178,7 +178,7 @@ export class CliManager { } else { action = existingVersion !== null ? "fallback" : "blocked"; } - await trace.downloadDecision(downloadReason, action); + trace.setDownloadDecision(downloadReason, action); if (!enableDownloads) { if (existingVersion) { @@ -189,7 +189,7 @@ export class CliManager { return resolved.binPath; } this.output.warn("Unable to download CLI because downloads are disabled"); - trace.setFailure("downloads_disabled"); + trace.error("downloads_disabled"); throw new CliDownloadsDisabledError(); } @@ -1033,7 +1033,7 @@ export class CliManager { try { await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { - trace.fail(error); + trace.error(error); this.handleStoreError(error); } return; @@ -1053,10 +1053,10 @@ export class CliManager { } if (result.cancelled) { this.output.info("Credential storage cancelled by user"); - trace.cancel(); + trace.abort(); return; } - trace.fail(result.error); + trace.error(result.error); this.handleStoreError(result.error); } diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts index 56c8eaea91..c8c7d0a34d 100644 --- a/src/instrumentation/cli.ts +++ b/src/instrumentation/cli.ts @@ -1,6 +1,5 @@ -import { isAbortError } from "../error/errorUtils"; - import { CredentialFileError } from "./credentials"; +import { recordAborted, recordError } from "./outcomes"; import type { CallerPropertyValue } from "../telemetry/event"; import type { TelemetryService } from "../telemetry/service"; @@ -21,12 +20,11 @@ export type CliVersionCheckOutcome = | "match" | "mismatch" | "unreadable"; -export type CliConfigureFailureCategory = - | "cancelled" +export type CliConfigureErrorType = | "filesystem" | "credential_store" | "unknown"; -export type CliResolveFailureCategory = +export type CliResolveErrorType = | "downloads_disabled" | "download" | "fallback_declined"; @@ -92,8 +90,8 @@ export class CliResolveTrace { this.span.setProperty("outcome", outcome); } - public setFailure(category: CliResolveFailureCategory | "unknown"): void { - this.span.setProperty("failure_category", category); + public error(category: CliResolveErrorType | "unknown"): void { + recordError(this.span, category); } public cacheLookup( @@ -130,30 +128,27 @@ export class CliResolveTrace { }); } - public downloadDecision( + public setDownloadDecision( reason: CliDownloadReason, action: CliDownloadAction, - ): Promise { + ): void { this.span.setProperty("download_reason", reason); - return this.span.phase("download_decision", () => Promise.resolve(), { - reason, - outcome: action, - }); + this.span.setProperty("download_action", action); } public async fallback(error: unknown, fn: () => Promise): Promise { + // Why we fell back, recorded even when the fallback succeeds. + this.span.setProperty("fallback_reason", categorizeResolveError(error)); const result = await this.span.phase( "fallback_to_existing_binary", - async (span) => { + async (child) => { try { - const result = await fn(); - span.setProperty("failure_category", categorizeResolveFailure(error)); - return result; + return await fn(); } catch (fallbackError) { - span.setProperty( - "failure_category", - categorizeResolveFailure(fallbackError), - ); + // Fallback failed too: tag the phase and parent with its error.type. + const category = categorizeResolveError(fallbackError); + recordError(child, category); + this.error(category); throw fallbackError; } }, @@ -187,25 +182,16 @@ export class CliResolveTrace { export class CliConfigureTrace { public constructor(private readonly span: Span) {} - public cancel(): void { - this.span.setProperty("failure_category", "cancelled"); - this.span.markAborted(); + public abort(): void { + recordAborted(this.span, "credential_store"); } - public fail(error: unknown): void { - this.span.setProperty( - "failure_category", - categorizeConfigureFailure(error), - ); + public error(error: unknown): void { + recordError(this.span, categorizeConfigureError(error)); } } -function categorizeConfigureFailure( - error: unknown, -): CliConfigureFailureCategory { - if (isAbortError(error)) { - return "cancelled"; - } +function categorizeConfigureError(error: unknown): CliConfigureErrorType { // A CredentialFileError is a file-write failure; anything else is keyring/CLI. if (error instanceof CredentialFileError) { return "filesystem"; @@ -213,7 +199,7 @@ function categorizeConfigureFailure( return error instanceof Error ? "credential_store" : "unknown"; } -function categorizeResolveFailure(error: unknown): CliResolveFailureCategory { +function categorizeResolveError(error: unknown): CliResolveErrorType { if (error instanceof CliDownloadsDisabledError) { return "downloads_disabled"; } diff --git a/test/unit/core/cliManager.telemetry.test.ts b/test/unit/core/cliManager.telemetry.test.ts index cbfbc4851a..e9371ef3c8 100644 --- a/test/unit/core/cliManager.telemetry.test.ts +++ b/test/unit/core/cliManager.telemetry.test.ts @@ -78,7 +78,7 @@ describe("CliManager telemetry", () => { }); it.each([{ silent: false }, { silent: true }])( - "emits credential_store failure category on failure (silent=$silent)", + "emits credential_store error type on failure (silent=$silent)", async ({ silent }) => { const { manager, mockCredManager, event } = setupCliManager(); vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( @@ -89,13 +89,13 @@ describe("CliManager telemetry", () => { "keyring unavailable", ); expect(event("cli.configure")).toMatchObject({ - properties: { result: "error", failure_category: "credential_store" }, + properties: { result: "error", "error.type": "credential_store" }, error: { message: "keyring unavailable" }, }); }, ); - it("emits cancelled failure category when the user cancels", async () => { + it("aborts with a stage when the user cancels", async () => { const { manager, mockCredManager, expectProps } = setupCliManager(); vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( makeAbortError(), @@ -104,7 +104,7 @@ describe("CliManager telemetry", () => { await expect(manager.configure(URL, TOKEN)).resolves.not.toThrow(); expectProps("cli.configure", { result: "aborted", - failure_category: "cancelled", + abort_stage: "credential_store", }); }); }); @@ -181,12 +181,9 @@ describe("CliManager telemetry", () => { expectPathsEqual(await manager.fetchBinary(mockApi), BINARY_PATH); noEvent("cli.download"); - expectProps("cli.resolve.download_decision", { - reason: "version_mismatch", - outcome: "fallback", - result: "success", - }); expectProps("cli.resolve", { + download_reason: "version_mismatch", + download_action: "fallback", outcome: "download_disabled_fallback", result: "success", }); @@ -203,7 +200,7 @@ describe("CliManager telemetry", () => { noEvent("cli.download"); expectProps("cli.resolve", { - failure_category: "downloads_disabled", + "error.type": "downloads_disabled", result: "error", }); }); @@ -231,7 +228,12 @@ describe("CliManager telemetry", () => { ); expectProps("cli.resolve.fallback_to_existing_binary", { - failure_category: "fallback_declined", + "error.type": "fallback_declined", + result: "error", + }); + expectProps("cli.resolve", { + fallback_reason: "download", + "error.type": "fallback_declined", result: "error", }); }); @@ -276,10 +278,14 @@ describe("CliManager telemetry", () => { measurements: { downloaded_bytes: Buffer.byteLength(partial) }, }); expectProps("cli.resolve.fallback_to_existing_binary", { - failure_category: "download", + "error.type": "download", + result: "error", + }); + expectProps("cli.resolve", { + fallback_reason: "download", + "error.type": "download", result: "error", }); - expectProps("cli.resolve", { result: "error" }); }); describe("cli.download.verify", () => {