diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index b65f0f0a6..5d036e2a8 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -10,6 +10,16 @@ import * as semver from "semver"; import * as vscode from "vscode"; import { errToStr } from "../api/api-helper"; +import { + CliDownloadsDisabledError, + CliFallbackDeclinedError, + CliTelemetry, + type CliConfigureTrace, + type CliDownloadAction, + type CliDownloadReason, + type CliVersionCheckOutcome, + type CliResolveTrace, +} from "../instrumentation/cli"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; @@ -36,8 +46,6 @@ type ResolvedBinary = | { binPath: string; stat: Stats; source: "file-path" | "directory" } | { binPath: string; source: "not-found" }; -type CliDownloadReason = "missing" | "version_mismatch" | "unreadable"; - type CliVerifyResult = | { kind: "verified" } | { kind: "bypassed" } @@ -50,14 +58,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); } /** @@ -123,74 +133,64 @@ export class CliManager { * unable to download a working binary, whether because of network issues or * downloads being disabled. */ - public async fetchBinary(restClient: Api): Promise { + public fetchBinary(restClient: Api): Promise { + return this.cliTelemetry.traceResolve((trace) => + this.resolveBinary(restClient, trace), + ); + } + + private async resolveBinary( + restClient: Api, + trace: CliResolveTrace, + ): Promise { const baseUrl = restClient.getAxiosInstance().defaults.baseURL; if (!baseUrl) { + trace.error("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; 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 this.resolveBinaryPath(safeHostname); - this.output.debug( - `Resolved binary: ${resolved.binPath} (${resolved.source})`, + const resolved = await trace.cacheLookup(() => + this.lookupBinary(safeHostname), ); - - 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), + const { buildInfo, parsedVersion, existingVersion, downloadReason } = + await trace.versionCheck(() => + this.checkResolvedBinary(restClient, resolved), ); - try { - existingVersion = await cliVersion(resolved.binPath); - this.output.debug("Existing binary version is", existingVersion); - downloadReason = "version_mismatch"; - } catch (error) { - this.output.warn( - "Unable to get version of existing binary, downloading instead", - error, - ); - downloadReason = "unreadable"; - } - } if (existingVersion === buildInfo.version) { this.output.debug("Existing binary matches server version"); + trace.setOutcome("cache_hit"); return resolved.binPath; } + let action: CliDownloadAction; + if (enableDownloads) { + action = "download"; + } else { + action = existingVersion !== null ? "fallback" : "blocked"; + } + trace.setDownloadDecision(downloadReason, action); + 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"); - throw new Error("Unable to download CLI because downloads are disabled"); + trace.error("downloads_disabled"); + throw new CliDownloadsDisabledError(); } if (existingVersion) { @@ -199,12 +199,103 @@ export class CliManager { ); } + return this.downloadBinary( + restClient, + { + resolved, + parsedVersion, + serverVersion: buildInfo.version, + downloadReason, + }, + trace, + ); + } + + private async lookupBinary(safeHostname: string): Promise { + const resolved = await this.resolveBinaryPath(safeHostname); + this.output.debug( + `Resolved binary: ${resolved.binPath} (${resolved.source})`, + ); + return resolved; + } + + 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}`, + ); + } + + if (resolved.source === "not-found") { + this.output.info("No existing binary found, starting download"); + return { + buildInfo, + parsedVersion, + existingVersion: null, + downloadReason: "missing", + outcome: "missing", + }; + } + + 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, + options: { + resolved: ResolvedBinary; + parsedVersion: semver.SemVer; + serverVersion: string; + downloadReason: CliDownloadReason; + }, + trace: CliResolveTrace, + ): Promise { + const { resolved, parsedVersion, serverVersion, downloadReason } = options; // 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"; @@ -214,61 +305,49 @@ export class CliManager { | undefined; let latestVersion = parsedVersion; try { - lockResult = await this.binaryLock.acquireLockOrWait( - downloadBinPath, - progressLogPath, + lockResult = await trace.lockWait(() => + 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); - - const recheckAfterWait = await this.checkBinaryVersion( - downloadBinPath, - latestBuildInfo.version, + const waitResult = await trace.lockRecheck(() => + this.recheckBinaryAfterWait(restClient, downloadBinPath), ); - if (recheckAfterWait.matches) { + 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); } - - const latestParsedVersion = semver.parse(latestBuildInfo.version); - if (!latestParsedVersion) { - throw new Error( - `Got invalid version from deployment: ${latestBuildInfo.version}`, - ); - } - latestVersion = latestParsedVersion; + latestVersion = waitResult.parsedVersion; } - return await this.telemetry.trace( - "cli.download", + const result = await this.cliTelemetry.traceDownload( + downloadReason, async (span) => { const downloadedBinPath = await this.performBinaryDownload( restClient, - latestVersion, - downloadBinPath, - progressLogPath, + { + parsedVersion: latestVersion, + binPath: downloadBinPath, + progressLogPath, + }, span, ); return this.renameToFinalPath(resolved, downloadedBinPath); }, - { reason: downloadReason }, ); + trace.setOutcome("downloaded"); + return result; } catch (error) { - const fallback = await this.handleAnyBinaryFailure( - error, - downloadBinPath, - buildInfo.version, - resolved.binPath !== downloadBinPath ? resolved.binPath : undefined, + return await trace.fallback(error, () => + this.fallbackToExistingBinary( + error, + downloadBinPath, + serverVersion, + resolved, + ), ); - // 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(); @@ -277,6 +356,60 @@ export class CliManager { } } + 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; + } + /** * Check if a binary exists and matches the expected version. */ @@ -405,7 +538,7 @@ export class CliManager { !check.matches && !(await this.promptUseExistingBinary(check.version, message)) ) { - throw error; + throw new CliFallbackDeclinedError(error); } return candidate; }; @@ -436,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"); @@ -529,7 +665,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)); } }); } @@ -590,7 +726,7 @@ export class CliManager { } } finally { if (bytesWritten > 0) { - downloadSpan.setMeasurement("downloadedBytes", bytesWritten); + downloadSpan.setMeasurement("downloaded_bytes", bytesWritten); } await downloadProgress.clearProgress(progressLogPath); } @@ -876,12 +1012,28 @@ export class CliManager { throw new Error("URL is required to configure the CLI"); } + const silent = options?.silent === true; + return this.cliTelemetry.traceConfigure( + { + silent, + credentialSource: token === "" ? "empty_token" : "session_token", + }, + (trace) => this.storeCredentials({ url, token, silent }, trace), + ); + } + + private async storeCredentials( + options: { url: string; token: string; silent: boolean }, + trace: CliConfigureTrace, + ): Promise { + const { url, token, silent } = options; const configs = vscode.workspace.getConfiguration(); - if (options?.silent) { + if (silent) { try { await this.cliCredentialManager.storeToken(url, token, configs); } catch (error) { + trace.error(error); this.handleStoreError(error); } return; @@ -901,8 +1053,10 @@ export class CliManager { } if (result.cancelled) { this.output.info("Credential storage cancelled by user"); + trace.abort(); return; } + trace.error(result.error); this.handleStoreError(result.error); } diff --git a/src/instrumentation/cli.ts b/src/instrumentation/cli.ts new file mode 100644 index 000000000..c8c7d0a34 --- /dev/null +++ b/src/instrumentation/cli.ts @@ -0,0 +1,210 @@ +import { CredentialFileError } from "./credentials"; +import { recordAborted, recordError } from "./outcomes"; + +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 CliDownloadAction = "download" | "fallback" | "blocked"; +export type CliCredentialSource = "session_token" | "empty_token"; +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 CliConfigureErrorType = + | "filesystem" + | "credential_store" + | "unknown"; +export type CliResolveErrorType = + | "downloads_disabled" + | "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) {} + + public traceResolve( + fn: (trace: CliResolveTrace) => Promise, + ): Promise { + return this.telemetry.trace("cli.resolve", (span) => + fn(new CliResolveTrace(span)), + ); + } + + public traceDownload( + reason: CliDownloadReason, + fn: (span: Span) => Promise, + ): Promise { + return this.telemetry.trace("cli.download", fn, { reason }); + } + + public traceConfigure( + options: CliConfigureOptions, + fn: (trace: CliConfigureTrace) => Promise, + ): Promise { + return this.telemetry.trace("cli.configure", (span) => { + span.setProperty("silent", options.silent); + span.setProperty("credential_source", options.credentialSource); + return fn(new CliConfigureTrace(span)); + }); + } +} + +export class CliResolveTrace { + public constructor(private readonly span: Span) {} + + public setOutcome(outcome: CliResolveOutcome): void { + this.span.setProperty("outcome", outcome); + } + + public error(category: CliResolveErrorType | "unknown"): void { + recordError(this.span, category); + } + + public cacheLookup( + fn: () => Promise, + ): Promise { + return this.tracedPhase("cache_lookup", fn, (r) => r.source, { + child: "source", + parent: "cache_source", + }); + } + + public versionCheck( + fn: () => Promise, + ): Promise { + return this.tracedPhase("version_check", fn, (r) => r.outcome, { + child: "outcome", + parent: "version_check", + }); + } + + public lockWait( + fn: () => Promise, + ): Promise { + return this.tracedPhase("lock_wait", fn, (r) => r.waited, { + child: "waited", + }); + } + + public lockRecheck( + fn: () => Promise, + ): Promise { + return this.tracedPhase("lock_wait_recheck", fn, (r) => r.outcome, { + child: "outcome", + }); + } + + public setDownloadDecision( + reason: CliDownloadReason, + action: CliDownloadAction, + ): void { + this.span.setProperty("download_reason", reason); + 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 (child) => { + try { + return await fn(); + } catch (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; + } + }, + ); + this.setOutcome("fallback_to_existing_binary"); + return result; + } + + /** + * 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; + } +} + +export class CliConfigureTrace { + public constructor(private readonly span: Span) {} + + public abort(): void { + recordAborted(this.span, "credential_store"); + } + + public error(error: unknown): void { + recordError(this.span, categorizeConfigureError(error)); + } +} + +function categorizeConfigureError(error: unknown): CliConfigureErrorType { + // 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"; +} + +function categorizeResolveError(error: unknown): CliResolveErrorType { + if (error instanceof CliDownloadsDisabledError) { + return "downloads_disabled"; + } + if (error instanceof CliFallbackDeclinedError) { + return "fallback_declined"; + } + return "download"; +} diff --git a/src/instrumentation/remoteSetup.ts b/src/instrumentation/remoteSetup.ts index c3b49e46b..6a0d4a6e5 100644 --- a/src/instrumentation/remoteSetup.ts +++ b/src/instrumentation/remoteSetup.ts @@ -1,16 +1,23 @@ 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"; + | "agent_resolve" + | "ssh_config_write" + | "ssh_monitor_setup" + | "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 0db376053..d0f3307a3 100644 --- a/src/instrumentation/websocket.ts +++ b/src/instrumentation/websocket.ts @@ -39,6 +39,7 @@ interface ReconnectCycle { readonly startMs: number; readonly reason: ConnectionStateReason; attempts: number; + maxBackoffMs: number; } interface DropOptions { @@ -85,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" }); } @@ -104,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); @@ -136,6 +137,7 @@ export class WebSocketTelemetry { startMs: performance.now(), reason, attempts: 0, + maxBackoffMs: 0, }; } @@ -146,9 +148,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 { @@ -163,11 +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, - totalDurationMs: performance.now() - cycle.startMs, + max_backoff_ms: cycle.maxBackoffMs, + total_duration_ms: performance.now() - cycle.startMs, }); } } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 1e8012f24..129cf3db1 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -238,35 +238,29 @@ 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 = featureSetForVersion(version); - const configDir = this.pathResolver.getGlobalConfigDir( - parts.safeHostname, - ); - const cliAuth = resolveCliAuth( - vscode.workspace.getConfiguration(), - featureSet, - baseUrl, - configDir, + const { featureSet, cliAuth } = await tracer.phase( + "compatibility_check", + () => + this.checkCompatibility({ + workspaceClient, + binaryPath, + baseUrl, + safeHostname: parts.safeHostname, + }), ); // Server versions before v0.14.1 don't support the vscodessh command! @@ -312,10 +306,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, @@ -349,7 +345,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), ); @@ -391,15 +387,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 +466,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("connection_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( @@ -691,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/src/websocket/reconnectingWebSocket.ts b/src/websocket/reconnectingWebSocket.ts index 6e49e830f..a88159937 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/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 91799edf9..854e43ada 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -178,7 +178,9 @@ 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.toBeUndefined(); expect(execFile).not.toHaveBeenCalled(); expect(memfs.readFileSync(URL_FILE, "utf8")).toBe(TEST_URL); @@ -197,7 +199,9 @@ 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.toBeUndefined(); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); @@ -220,7 +224,9 @@ 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.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 000000000..e9371ef3c --- /dev/null +++ b/test/unit/core/cliManager.telemetry.test.ts @@ -0,0 +1,367 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { expectPathsEqual } from "../../utils/platform"; + +import { + BINARY_PATH, + type CliManagerHarness, + flushPendingIO, + makeAbortError, + mockBinaryContent, + setupCliManager, + TEST_VERSION, +} from "./cliManagerHarness"; + +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 }; +}); + +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", () => { + afterEach(flushPendingIO); + + describe("cli.configure", () => { + const URL = "https://coder.example.com"; + const TOKEN = "test-token"; + + 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(); + + await manager.configure(URL, token, options); + + expect(event("cli.configure")).toMatchObject({ + properties: { result: "success", ...props }, + measurements: { durationMs: expect.any(Number) }, + }); + }); + + it.each([{ silent: false }, { silent: true }])( + "emits credential_store error type on failure (silent=$silent)", + async ({ silent }) => { + const { manager, mockCredManager, event } = setupCliManager(); + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + new Error("keyring unavailable"), + ); + + await expect(manager.configure(URL, TOKEN, { silent })).rejects.toThrow( + "keyring unavailable", + ); + expect(event("cli.configure")).toMatchObject({ + properties: { result: "error", "error.type": "credential_store" }, + error: { message: "keyring unavailable" }, + }); + }, + ); + + it("aborts with a stage when the user cancels", async () => { + const { manager, mockCredManager, expectProps } = setupCliManager(); + vi.mocked(mockCredManager.storeToken).mockRejectedValueOnce( + makeAbortError(), + ); + + await expect(manager.configure(URL, TOKEN)).resolves.not.toThrow(); + expectProps("cli.configure", { + result: "aborted", + abort_stage: "credential_store", + }); + }); + }); + + describe("cli.resolve and cli.download", () => { + 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); + + noEvent("cli.download"); + expect(event("cli.resolve")).toMatchObject({ + properties: { + result: "success", + outcome: "cache_hit", + cache_source: "directory", + version_check: "match", + }, + measurements: { durationMs: expect.any(Number) }, + }); + expectProps("cli.resolve.cache_lookup", { + source: "directory", + 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); + + noEvent("cli.download"); + expectProps("cli.resolve", { + download_reason: "version_mismatch", + download_action: "fallback", + 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", + ); + + noEvent("cli.download"); + expectProps("cli.resolve", { + "error.type": "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"); + 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, + ); + + await expect(manager.fetchBinary(mockApi)).rejects.toThrow( + "Unable to download binary: connection reset", + ); + + expectProps("cli.resolve.fallback_to_existing_binary", { + "error.type": "fallback_declined", + result: "error", + }); + expectProps("cli.resolve", { + fallback_reason: "download", + "error.type": "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); + + 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"; + withInterruptedDownload(partial); + + 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) }, + }); + expectProps("cli.resolve.fallback_to_existing_binary", { + "error.type": "download", + result: "error", + }); + expectProps("cli.resolve", { + fallback_reason: "download", + "error.type": "download", + result: "error", + }); + }); + + describe("cli.download.verify", () => { + /** 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 () => { + const t = setupVerify(); + t.withSignatureResponses([200]); + + 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); + }); + + it("emits outcome=bypassed when user runs anyway despite invalid signature", async () => { + const t = setupVerify(); + t.withSignatureResponses([200]); + t.withInvalidSignature(); + t.mockUI.setResponse("Signature does not match", "Run anyway"); + + await t.manager.fetchBinary(t.mockApi); + + t.expectProps("cli.download.verify", { + 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 }) => { + const t = setupVerify(); + t.withSignatureResponses([status, status]); + t.mockUI.setResponse(message, "Run without verification"); + + await t.manager.fetchBinary(t.mockApi); + + t.expectProps("cli.download.verify", { + outcome: "sig_not_found", + sig_status: String(status), + result: "success", + }); + }, + ); + + it("emits error when verification is aborted", async () => { + 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", + ); + + expect(t.event("cli.download.verify")).toMatchObject({ + properties: { result: "error" }, + error: { message: "Signature verification aborted" }, + }); + }); + }); + }); +}); diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index 4e0c185d1..e28854ae4 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, TestSink } 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,88 +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; - 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(); - - // 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(); - telemetrySink = new TestSink(); - manager = new CliManager( - createMockLogger(), - new PathResolver(BASE_PATH, "/code/log"), - mockCredManager, - createTestTelemetryService(telemetrySink), - ); - - // 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) }, @@ -152,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", ); @@ -171,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", @@ -184,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", ); @@ -208,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", ); @@ -220,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", @@ -245,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), @@ -262,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); }); }); @@ -301,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"), ); @@ -342,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); }); }); }); @@ -353,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(); @@ -364,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); @@ -383,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); } @@ -392,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", @@ -399,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); }); @@ -406,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); @@ -423,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); @@ -433,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); @@ -442,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(); @@ -453,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", @@ -461,6 +434,8 @@ describe("CliManager", () => { }); it("restores old backup when replace fails", async () => { + const { manager, mockApi, withExistingBinary, withSuccessfulDownload } = + setupCliManager(); withExistingBinary("1.0.0"); withSuccessfulDownload(); @@ -488,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( @@ -503,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); @@ -517,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); @@ -533,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( @@ -570,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(); @@ -586,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); @@ -595,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!", @@ -613,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.", @@ -633,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", @@ -641,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", @@ -649,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(); @@ -661,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(); @@ -674,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( @@ -685,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( @@ -695,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); @@ -750,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(); }); @@ -763,153 +771,19 @@ 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", ); }, ); }); - 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.downloadedBytes).toBe( - Buffer.byteLength(mockBinaryContent(TEST_VERSION)), - ); - }); - - it("does not emit cli.download when a cached binary already matches", async () => { - withExistingBinary(TEST_VERSION); - await manager.fetchBinary(mockApi); - - expect(event("cli.download")).toBeUndefined(); - }); - - it("omits downloadedBytes 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.downloadedBytes).toBeUndefined(); - }); - - it("emits downloadedBytes 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: { downloadedBytes: Buffer.byteLength(partial) }, - }); - }); - - 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 sigStatus=$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", - sigStatus: 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 () => { + const { manager, mockApi, withSuccessfulDownload } = setupCliManager(); expect(memfs.existsSync(BINARY_DIR)).toBe(false); withSuccessfulDownload(); await manager.fetchBinary(mockApi); @@ -917,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( @@ -925,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); @@ -933,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( @@ -948,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 000000000..a411bf187 --- /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)); +} diff --git a/test/unit/instrumentation/remoteSetup.test.ts b/test/unit/instrumentation/remoteSetup.test.ts index cd388acf2..4dec34433 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", + "agent_resolve", + "ssh_config_write", + "ssh_monitor_setup", + "connection_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 0af7a2a7b..2809c097e 100644 --- a/test/unit/instrumentation/websocket.test.ts +++ b/test/unit/instrumentation/websocket.test.ts @@ -40,17 +40,28 @@ 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) }, }); }); + 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(); ws.opened("/api/test"); const [event] = sink.eventsNamed("connection.opened"); - expect(event.measurements.connectDurationMs).toBe(0); + expect(event.measurements.connect_duration_ms).toBe(0); }); }); @@ -72,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), ); }); @@ -134,7 +145,11 @@ describe("WebSocketTelemetry", () => { const [event] = sink.eventsNamed("connection.reconnect_resolved"); expect(event).toMatchObject({ properties: { result: "success", reason: "manual_reconnect" }, - measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, + measurements: { + attempts: 1, + max_backoff_ms: 0, + total_duration_ms: expect.any(Number), + }, }); }); @@ -148,7 +163,7 @@ describe("WebSocketTelemetry", () => { expect(event.properties).toEqual({ result: "error", reason: "manual_reconnect", - terminationReason: "unrecoverable_http", + termination_reason: "unrecoverable_http", }); }); @@ -191,15 +206,21 @@ 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([ + { measurements: { attempts: 0, max_backoff_ms: 250 } }, + ]); }); }); }); diff --git a/test/unit/websocket/reconnectingWebSocket.test.ts b/test/unit/websocket/reconnectingWebSocket.test.ts index 769625ab4..250332f22 100644 --- a/test/unit/websocket/reconnectingWebSocket.test.ts +++ b/test/unit/websocket/reconnectingWebSocket.test.ts @@ -639,7 +639,11 @@ describe("ReconnectingWebSocket", () => { expect(sink.eventsNamed("connection.reconnect_resolved")).toMatchObject([ { properties: { result: "success", reason: "unexpected_close" }, - measurements: { attempts: 1, totalDurationMs: expect.any(Number) }, + measurements: { + attempts: 1, + max_backoff_ms: expect.any(Number), + total_duration_ms: expect.any(Number), + }, }, ]); }); @@ -660,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