From eb6d2ed97c4fb8d95da994217c558c53861b8c07 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 8 Oct 2026 11:26:17 +0300 Subject: [PATCH 1/2] refactor(logging): flush the buffered logger directly from sockets Replace the onConnectionFailure callback threaded through CoderApi with a BufferedLogger type. ReconnectingWebSocket flushes the logger it already holds, and getLogger() returns the buffered logger so getConnectionLogBuffer() is removed. --- src/api/coderApi.ts | 17 +-- src/commands.ts | 9 +- src/core/container.ts | 14 +- src/deployment/deploymentManager.ts | 4 +- src/extension.ts | 1 - src/logging/logBuffer.ts | 22 +-- src/logging/logger.ts | 5 + src/login/loginCoordinator.ts | 4 +- src/oauth/authorizer.ts | 4 +- src/oauth/sessionManager.ts | 4 +- src/remote/remote.ts | 7 +- src/websocket/reconnectingWebSocket.ts | 27 +--- test/mocks/testHelpers.ts | 16 +-- test/unit/api/coderApi.test.ts | 39 ++---- test/unit/commands.netcheck.test.ts | 1 - test/unit/commands.supportBundle.test.ts | 10 +- test/unit/commands.telemetry.test.ts | 5 - test/unit/remote/remote.test.ts | 4 +- .../websocket/reconnectingWebSocket.test.ts | 129 ++++++++---------- 19 files changed, 109 insertions(+), 213 deletions(-) diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 3a36ce6070..46704d75eb 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -45,7 +45,6 @@ import { import { ConnectionState, ReconnectingWebSocket, - type ConnectionFailureReason, type ReconnectingWebSocketOptions, type SocketFactory, } from "../websocket/reconnectingWebSocket"; @@ -73,7 +72,7 @@ import type { } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger, Logger } from "../logging/logger"; import type { CloseEvent, ErrorEvent, @@ -125,14 +124,10 @@ export class CoderApi extends Api implements vscode.Disposable { private readonly configWatcher: vscode.Disposable; private constructor( - private readonly output: Logger, + private readonly output: BufferedLogger, private readonly telemetry: TelemetryReporter, private readonly httpRequestsTelemetry: HttpRequestsTelemetry, private readonly authConfigTracker: AuthConfigTracker, - private readonly onConnectionFailure?: ( - reason: ConnectionFailureReason, - route: string, - ) => void, ) { super(); wrapWithValidation(this); @@ -151,12 +146,8 @@ export class CoderApi extends Api implements vscode.Disposable { static create( baseUrl: string, token: string | undefined, - output: Logger, + output: BufferedLogger, telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, - onConnectionFailure?: ( - reason: ConnectionFailureReason, - route: string, - ) => void, ): CoderApi { const httpRequestsTelemetry = new HttpRequestsTelemetry(telemetry); const authConfigTracker = new AuthConfigTracker(); @@ -165,7 +156,6 @@ export class CoderApi extends Api implements vscode.Disposable { telemetry, httpRequestsTelemetry, authConfigTracker, - onConnectionFailure, ); client.getAxiosInstance().defaults.timeout = DEFAULT_REQUEST_TIMEOUT_MS; client.getAxiosInstance().defaults.headers.common[BAGGAGE_HEADER] = @@ -565,7 +555,6 @@ export class CoderApi extends Api implements vscode.Disposable { } return refreshCertificates(refreshCommand, this.output); }, - onConnectionFailure: this.onConnectionFailure, telemetry: this.telemetry, }; diff --git a/src/commands.ts b/src/commands.ts index 19b7017d5f..c33746f31d 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -84,8 +84,7 @@ import type { MementoManager } from "./core/mementoManager"; import type { PathResolver } from "./core/pathResolver"; import type { SecretsManager, SessionAuth } from "./core/secretsManager"; import type { DeploymentManager } from "./deployment/deploymentManager"; -import type { ConnectionLogBuffer } from "./logging/logBuffer"; -import type { Logger } from "./logging/logger"; +import type { BufferedLogger } from "./logging/logger"; import type { LoginCoordinator, LoginMethod } from "./login/loginCoordinator"; import type { TelemetryService } from "./telemetry/service"; import type { NetcheckPanelFactory } from "./webviews/netcheck/netcheckPanelFactory"; @@ -156,7 +155,7 @@ const openDefaults = { } as const satisfies Partial; export class Commands { - private readonly logger: Logger; + private readonly logger: BufferedLogger; private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; @@ -169,7 +168,6 @@ export class Commands { private readonly authTelemetry: AuthTelemetry; private readonly diagnosticTelemetry: DiagnosticTelemetry; private readonly workspaceOpenTelemetry: WorkspaceOpenTelemetry; - private readonly connectionLogBuffer: ConnectionLogBuffer; // These will only be populated when actively connected to a workspace and are // used in commands. Because commands can be executed by the user, it is not @@ -195,7 +193,6 @@ export class Commands { this.telemetryService, ); this.logger = serviceContainer.getLogger(); - this.connectionLogBuffer = serviceContainer.getConnectionLogBuffer(); this.pathResolver = serviceContainer.getPathResolver(); this.mementoManager = serviceContainer.getMementoManager(); this.secretsManager = serviceContainer.getSecretsManager(); @@ -493,7 +490,7 @@ export class Commands { // the channel has time to write them to disk; retain the ring so a // later failure flush still replays them. Best-effort: the channel // writes on its own schedule, so the tail may not land in this bundle. - this.connectionLogBuffer.flush("support_bundle", { retain: true }); + this.logger.flush("support_bundle", { retain: true }); await cliExec.supportBundle(env, workspaceId, { outputPath: outputUri.fsPath, agentName, diff --git a/src/core/container.ts b/src/core/container.ts index a39e26503b..dd381010c0 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -2,10 +2,7 @@ import * as vscode from "vscode"; import { watchConfigurationChanges } from "../configWatcher"; import { AuthTelemetry } from "../instrumentation/auth"; -import { - BufferingLogger, - type ConnectionLogBuffer, -} from "../logging/logBuffer"; +import { BufferingLogger } from "../logging/logBuffer"; import { prefixLogger } from "../logging/prefixLogger"; import { shortId } from "../logging/utils"; import { LoginCoordinator } from "../login/loginCoordinator"; @@ -30,7 +27,7 @@ import { PathResolver } from "./pathResolver"; import { SecretsManager } from "./secretsManager"; import { sessionId } from "./sessionId"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; /** * Service container for dependency injection. @@ -166,12 +163,7 @@ export class ServiceContainer implements vscode.Disposable { return this.secretsManager; } - getLogger(): Logger { - return this.logger; - } - - /** The connection log buffer that replays below-level entries on failure. */ - getConnectionLogBuffer(): ConnectionLogBuffer { + getLogger(): BufferedLogger { return this.logger; } diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 36e6fac94d..848fa5223d 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -13,7 +13,7 @@ import { type DeploymentRecoveryTrigger, type DeploymentSuspendReason, } from "../instrumentation/deployment"; -import { type Logger } from "../logging/logger"; +import { type BufferedLogger } from "../logging/logger"; import { type OAuthSessionManager } from "../oauth/sessionManager"; import { getAuthConfigWatchSettings } from "../settings/authConfig"; import { type TelemetryService } from "../telemetry/service"; @@ -47,7 +47,7 @@ export class DeploymentManager implements vscode.Disposable { private readonly secretsManager: SecretsManager; private readonly mementoManager: MementoManager; private readonly contextManager: ContextManager; - private readonly logger: Logger; + private readonly logger: BufferedLogger; private readonly telemetryService: TelemetryService; private readonly deploymentTelemetry: DeploymentTelemetry; diff --git a/src/extension.ts b/src/extension.ts index bb32b36c65..210ba68c28 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -141,7 +141,6 @@ async function doActivate( deploymentSessionAuth?.token, output, telemetryService, - serviceContainer.getConnectionLogBuffer().onConnectionFailure, ); ctx.subscriptions.push(client); diff --git a/src/logging/logBuffer.ts b/src/logging/logBuffer.ts index 31e3679641..e4181af1ac 100644 --- a/src/logging/logBuffer.ts +++ b/src/logging/logBuffer.ts @@ -1,6 +1,6 @@ import { safeStringify } from "./utils"; -import type { Logger } from "./logger"; +import type { BufferedLogger, Logger } from "./logger"; /** * Numeric severities matching `vscode.LogLevel` (Off=0, Trace=1, Debug=2, @@ -29,12 +29,6 @@ const MAX_BUFFERED_CHARS = 2_000_000; /** Entries replayed per channel call, so a flush is not one RPC per entry. */ const REPLAY_CHUNK = 100; -/** Replays buffered below-level log entries on a connection failure. */ -export interface ConnectionLogBuffer { - flush(reason: string, options?: { readonly retain?: boolean }): void; - readonly onConnectionFailure: (reason: string, route: string) => void; -} - interface LogEntry { readonly atMs: number; readonly level: Level; @@ -46,7 +40,7 @@ interface LogEntry { * Buffers entries below the current log level and replays them on failure at a * level the output channel persists. */ -export class BufferingLogger implements Logger, ConnectionLogBuffer { +export class BufferingLogger implements BufferedLogger { private entries: LogEntry[] = []; private chars = 0; @@ -62,18 +56,6 @@ export class BufferingLogger implements Logger, ConnectionLogBuffer { public readonly warn = this.wrap("warn"); public readonly error = this.wrap("error"); - /** - * Flush the buffer on a terminal socket failure, keyed by the - * ` ` string Support greps for. Arrow property so it can be - * passed by value as the socket's failure callback. - */ - public readonly onConnectionFailure = ( - reason: string, - route: string, - ): void => { - this.flush(`${reason} ${route}`); - }; - public show(): void { this.inner.show(); } diff --git a/src/logging/logger.ts b/src/logging/logger.ts index 0010d758c1..46b6dba2ca 100644 --- a/src/logging/logger.ts +++ b/src/logging/logger.ts @@ -6,3 +6,8 @@ export interface Logger { error(message: string, ...args: unknown[]): void; show(): void; } + +/** A logger that buffers below-level entries and can replay them. */ +export interface BufferedLogger extends Logger { + flush(reason: string, options?: { readonly retain?: boolean }): void; +} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 248d1fe2cf..380b2d9677 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -29,7 +29,7 @@ import type { AuthTelemetry, LoginPromptReason, } from "../instrumentation/auth"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; import type { OAuthCallback } from "../oauth/oauthCallback"; export type LoginMethod = @@ -86,7 +86,7 @@ export class LoginCoordinator implements vscode.Disposable { constructor( private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, - private readonly logger: Logger, + private readonly logger: BufferedLogger, private readonly cliCredentialManager: CliCredentialManager, private readonly authTelemetry: AuthTelemetry, oauthCallback: OAuthCallback, diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index b22bb90a70..27f946f4c1 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -35,7 +35,7 @@ import type { import type { SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; import type { OAuthCallback } from "./oauthCallback"; @@ -49,7 +49,7 @@ export class OAuthAuthorizer implements vscode.Disposable { constructor( private readonly secretsManager: SecretsManager, private readonly oauthCallback: OAuthCallback, - private readonly logger: Logger, + private readonly logger: BufferedLogger, private readonly extensionId: string, ) {} diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 5ccc122aa5..726dd5b51e 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -23,7 +23,7 @@ import type * as vscode from "vscode"; import type { ServiceContainer } from "../core/container"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; /** * Token refresh threshold: refresh when token expires in less than this time. @@ -79,7 +79,7 @@ export class OAuthSessionManager implements vscode.Disposable { private constructor( private deployment: Deployment | null, private readonly secretsManager: SecretsManager, - private readonly logger: Logger, + private readonly logger: BufferedLogger, private readonly onAuthRequired: () => Promise, private readonly authTelemetry: AuthTelemetry, ) {} diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 002f452d4e..7446d5108b 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -89,7 +89,7 @@ import type { ContextManager } from "../core/contextManager"; import type { StartupMode } from "../core/mementoManager"; import type { PathResolver } from "../core/pathResolver"; import type { SecretsManager } from "../core/secretsManager"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; export interface RemoteDetails extends vscode.Disposable { @@ -149,7 +149,7 @@ export function workspaceLabelSuffix( } export class Remote { - private readonly logger: Logger; + private readonly logger: BufferedLogger; private readonly pathResolver: PathResolver; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; @@ -297,7 +297,6 @@ export class Remote { token, this.logger, this.serviceContainer.getTelemetryService(), - this.serviceContainer.getConnectionLogBuffer().onConnectionFailure, ); disposables.push(workspaceClient); @@ -1060,7 +1059,7 @@ export class Remote { // closeRemote ends the current remote session. public async closeRemote() { - this.serviceContainer.getConnectionLogBuffer().flush("remote_closed"); + this.logger.flush("remote_closed"); await vscode.commands.executeCommand("workbench.action.remote.close"); } diff --git a/src/websocket/reconnectingWebSocket.ts b/src/websocket/reconnectingWebSocket.ts index 4d9b6f90e7..f392b42259 100644 --- a/src/websocket/reconnectingWebSocket.ts +++ b/src/websocket/reconnectingWebSocket.ts @@ -16,7 +16,7 @@ import { handshakeStatus } from "./utils"; import type { WebSocketEventType } from "coder/site/src/utils/OneWayWebSocket"; -import type { Logger } from "../logging/logger"; +import type { BufferedLogger } from "../logging/logger"; import type { TelemetryReporter } from "../telemetry/reporter"; import type { @@ -111,11 +111,6 @@ function reduceState( export type SocketFactory = () => Promise>; -export type ConnectionFailureReason = ConnectionStateReason | "unreachable"; - -/** Default failure callback for callers that do not observe connection failures. */ -const NOOP_CONNECTION_FAILURE = (): void => undefined; - /** * Consecutive failed reconnect attempts before the buffer is flushed once and * the server is treated as unreachable. @@ -131,21 +126,13 @@ export interface ReconnectingWebSocketOptions { route: string; /** Callback invoked when a refreshable certificate error is detected. Returns true if refresh succeeded. */ onCertificateRefreshNeeded: () => Promise; - /** - * Callback invoked on a terminal failure, or once per outage when the server - * stays unreachable. Retrying continues in the unreachable case. - */ - onConnectionFailure?: ( - reason: ConnectionFailureReason, - route: string, - ) => void; } export class ReconnectingWebSocket< TData = unknown, > implements UnidirectionalStream { readonly #socketFactory: SocketFactory; - readonly #logger: Logger; + readonly #logger: BufferedLogger; readonly #telemetry: WebSocketTelemetry; readonly #options: Required< Omit @@ -194,7 +181,7 @@ export class ReconnectingWebSocket< private constructor( socketFactory: SocketFactory, - logger: Logger, + logger: BufferedLogger, options: ReconnectingWebSocketOptions, onDispose?: () => void, ) { @@ -206,8 +193,6 @@ export class ReconnectingWebSocket< maxBackoffMs: options.maxBackoffMs ?? 30000, jitterFactor: options.jitterFactor ?? 0.1, onCertificateRefreshNeeded: options.onCertificateRefreshNeeded, - onConnectionFailure: - options.onConnectionFailure ?? NOOP_CONNECTION_FAILURE, }; this.#lastRoute = options.route; this.#backoffMs = this.#options.initialBackoffMs; @@ -216,7 +201,7 @@ export class ReconnectingWebSocket< public static async create( socketFactory: SocketFactory, - logger: Logger, + logger: BufferedLogger, options: ReconnectingWebSocketOptions, onDispose?: () => void, ): Promise> { @@ -332,7 +317,7 @@ export class ReconnectingWebSocket< }); this.clearCurrentSocket(options.code, options.closeReason); if (options.failure) { - this.#options.onConnectionFailure(reason, this.#route); + this.#logger.flush(`${reason} ${this.#route}`); } } @@ -477,7 +462,7 @@ export class ReconnectingWebSocket< this.#route, this.#consecutiveConnectFailures, ); - this.#options.onConnectionFailure("unreachable", this.#route); + this.#logger.flush(`unreachable ${this.#route}`); } const jitter = this.#backoffMs * this.#options.jitterFactor * (Math.random() * 2 - 1); diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 0d86635fa2..2ab14c099a 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -44,8 +44,7 @@ import type { MementoManager } from "@/core/mementoManager"; import type { PathResolver } from "@/core/pathResolver"; import type { SecretsManager } from "@/core/secretsManager"; import type { Deployment } from "@/deployment/types"; -import type { ConnectionLogBuffer } from "@/logging/logBuffer"; -import type { Logger } from "@/logging/logger"; +import type { BufferedLogger } from "@/logging/logger"; import type { LoginCoordinator } from "@/login/loginCoordinator"; import type { NetworkInfo } from "@/remote/sshProcess"; import type { TelemetryService } from "@/telemetry/service"; @@ -553,7 +552,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { } as unknown as CliCredentialManager; } -export function createMockLogger(): Logger { +export function createMockLogger(): BufferedLogger { return { trace: vi.fn(), debug: vi.fn(), @@ -561,6 +560,7 @@ export function createMockLogger(): Logger { warn: vi.fn(), error: vi.fn(), show: vi.fn(), + flush: vi.fn(), }; } @@ -574,7 +574,7 @@ export interface LogEntry { * Logger that records what was logged. Assert on `entries` for exact output, * or search `text` when checking that a secret never reached the log. */ -export class LogCollector implements Logger { +export class LogCollector implements BufferedLogger { readonly entries: LogEntry[] = []; /** Every message and argument logged, as one searchable string. */ @@ -607,6 +607,8 @@ export class LogCollector implements Logger { show(): void {} + flush(): void {} + private collect( level: LogEntry["level"], message: string, @@ -648,7 +650,7 @@ export async function setAge(filePath: string, daysAgo: number): Promise { export function createMockServiceContainer( overrides: { telemetry?: TelemetryService; - logger?: Logger; + logger?: BufferedLogger; secretsManager?: SecretsManager; mementoManager?: MementoManager; cliCredentialManager?: CliCredentialManager; @@ -669,10 +671,6 @@ export function createMockServiceContainer( return { getTelemetryService: () => telemetry, getLogger: () => logger, - getConnectionLogBuffer: (): ConnectionLogBuffer => ({ - flush: () => {}, - onConnectionFailure: () => {}, - }), getSecretsManager: () => require("secretsManager", overrides.secretsManager), getMementoManager: () => diff --git a/test/unit/api/coderApi.test.ts b/test/unit/api/coderApi.test.ts index 0cf3c10226..7bcedd47c7 100644 --- a/test/unit/api/coderApi.test.ts +++ b/test/unit/api/coderApi.test.ts @@ -117,15 +117,8 @@ describe("CoderApi", () => { url = CODER_URL, token = AXIOS_TOKEN, telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, - onConnectionFailure?: (reason: string) => void, ) => { - return CoderApi.create( - url, - token, - mockLogger, - telemetry, - onConnectionFailure, - ); + return CoderApi.create(url, token, mockLogger, telemetry); }; beforeEach(() => { @@ -577,15 +570,9 @@ describe("CoderApi", () => { }); }); - describe("connection failure callback", () => { - it("invokes onConnectionFailure on a terminal socket failure", async () => { - const onConnectionFailure = vi.fn(); - const failingApi = createApi( - CODER_URL, - AXIOS_TOKEN, - NOOP_TELEMETRY_REPORTER, - onConnectionFailure, - ); + describe("log flush on failure", () => { + it("flushes the logger on a terminal socket failure", async () => { + const failingApi = createApi(); const mockWs = createMockWebSocket( `wss://${CODER_URL.replace("https://", "")}/api/v2/workspaceagents/${AGENT_ID}/watch-metadata-ws`, ); @@ -600,9 +587,8 @@ describe("CoderApi", () => { wasClean: false, }); - expect(onConnectionFailure).toHaveBeenCalledWith( - "unrecoverable_close", - `/api/v2/workspaceagents/${AGENT_ID}/watch-metadata-ws`, + expect(mockLogger.flush).toHaveBeenCalledWith( + `unrecoverable_close /api/v2/workspaceagents/${AGENT_ID}/watch-metadata-ws`, ); connection.close(); }); @@ -675,13 +661,7 @@ describe("CoderApi", () => { }); it("treats an HTTP failure of the SSE fallback as unrecoverable", async () => { - const onConnectionFailure = vi.fn(); - api = createApi( - CODER_URL, - AXIOS_TOKEN, - NOOP_TELEMETRY_REPORTER, - onConnectionFailure, - ); + api = createApi(); setupWebSocketMock( createMockWebSocket("wss://test", { connectError: { @@ -700,9 +680,8 @@ describe("CoderApi", () => { const connection = await api.watchAgentMetadata(AGENT_ID); - expect(onConnectionFailure).toHaveBeenCalledWith( - "unrecoverable_http", - `/api/v2/workspaceagents/${AGENT_ID}/watch-metadata-ws`, + expect(mockLogger.flush).toHaveBeenCalledWith( + `unrecoverable_http /api/v2/workspaceagents/${AGENT_ID}/watch-metadata-ws`, ); connection.close(); }); diff --git a/test/unit/commands.netcheck.test.ts b/test/unit/commands.netcheck.test.ts index 793924879a..d18e9f988a 100644 --- a/test/unit/commands.netcheck.test.ts +++ b/test/unit/commands.netcheck.test.ts @@ -33,7 +33,6 @@ function setup(options: { extensionBaseUrl?: string } = {}) { getDuplicateWorkspaceIpc: () => ({}), getSpeedtestPanelFactory: () => ({}), getNetcheckPanelFactory: () => ({}) as NetcheckPanelFactory, - getConnectionLogBuffer: () => ({ flush: () => {} }), } as unknown as ServiceContainer; const commands = new Commands( diff --git a/test/unit/commands.supportBundle.test.ts b/test/unit/commands.supportBundle.test.ts index 97dd1f1b6c..313fbce0d6 100644 --- a/test/unit/commands.supportBundle.test.ts +++ b/test/unit/commands.supportBundle.test.ts @@ -80,11 +80,9 @@ function setup(options: { cliVersion?: string } = {}) { vi.mocked(appendVsCodeLogs).mockResolvedValue(undefined); const logger = createMockLogger(); - const connectionLogBufferFlush = vi.fn(); const serviceContainer = { getTelemetryService: () => service, getLogger: () => logger, - getConnectionLogBuffer: () => ({ flush: connectionLogBufferFlush }), getPathResolver: () => ({ getGlobalConfigDir: () => "/cfg", getProxyLogPath: () => "/logs/proxy", @@ -114,7 +112,7 @@ function setup(options: { cliVersion?: string } = {}) { {} as DeploymentManager, ); - return { commands, client, logger, interaction, connectionLogBufferFlush }; + return { commands, client, logger, interaction }; } function setRemoteAuthority(value: string | undefined): void { @@ -139,17 +137,17 @@ function connectToWorkspace( describe("Commands.supportBundle", () => { it("collects the selected agent's bundle with remote log globs", async () => { - const { commands, connectionLogBufferFlush } = setup(); + const { commands, logger } = setup(); await commands.supportBundle(agentItem("dev")); // The buffered below-level connection logs are replayed for the bundle, // keeping the ring so a later failure flush still has them, and before the // CLI runs so the channel has time to write them to disk. - expect(connectionLogBufferFlush).toHaveBeenCalledWith("support_bundle", { + expect(logger.flush).toHaveBeenCalledWith("support_bundle", { retain: true, }); - expect(connectionLogBufferFlush.mock.invocationCallOrder[0]).toBeLessThan( + expect(vi.mocked(logger.flush).mock.invocationCallOrder[0]).toBeLessThan( vi.mocked(cliExec.supportBundle).mock.invocationCallOrder[0], ); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 43921ee375..ef21501de7 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -18,7 +18,6 @@ import type { PathResolver } from "@/core/pathResolver"; import type { SecretsManager, SessionAuth } from "@/core/secretsManager"; import type { DeploymentManager } from "@/deployment/deploymentManager"; import type { Deployment } from "@/deployment/types"; -import type { ConnectionLogBuffer } from "@/logging/logBuffer"; import type { LoginCoordinator, LoginResult } from "@/login/loginCoordinator"; import type { NetcheckPanelFactory } from "@/webviews/netcheck/netcheckPanelFactory"; import type { SpeedtestPanelFactory } from "@/webviews/speedtest/speedtestPanelFactory"; @@ -121,10 +120,6 @@ function setup(options: SetupOptions = {}) { getDuplicateWorkspaceIpc: () => ({}) as DuplicateWorkspaceIpc, getSpeedtestPanelFactory: () => ({}) as SpeedtestPanelFactory, getNetcheckPanelFactory: () => ({}) as NetcheckPanelFactory, - getConnectionLogBuffer: (): ConnectionLogBuffer => ({ - flush: () => {}, - onConnectionFailure: () => {}, - }), } as ServiceContainer; const extensionClient = createTestCoderApi({ baseUrl: TEST_URL }); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index fe4d7cf6a5..3b418cc89b 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -31,7 +31,7 @@ import { import type { Commands } from "@/commands"; import type { CliManager } from "@/core/cliManager"; -import type { Logger } from "@/logging/logger"; +import type { BufferedLogger } from "@/logging/logger"; import type { CliAuth } from "@/settings/cli"; const mockWorkspace = vscode.workspace as typeof vscode.workspace & { @@ -71,7 +71,7 @@ const CLI_AUTH: CliAuth = { allowRedirects: false, }; -function createRemote(logger: Logger = createMockLogger()) { +function createRemote(logger: BufferedLogger = createMockLogger()) { new MockConfigurationProvider(); const userInteraction = new MockUserInteraction(); const pathResolver = new PathResolver("/mock/global", "/mock/log"); diff --git a/test/unit/websocket/reconnectingWebSocket.test.ts b/test/unit/websocket/reconnectingWebSocket.test.ts index 9ad55f71e5..a730e3ba54 100644 --- a/test/unit/websocket/reconnectingWebSocket.test.ts +++ b/test/unit/websocket/reconnectingWebSocket.test.ts @@ -12,7 +12,6 @@ import { import { ConnectionState, ReconnectingWebSocket, - type ConnectionFailureReason, type SocketFactory, } from "@/websocket/reconnectingWebSocket"; import { HandshakeError } from "@/websocket/utils"; @@ -40,8 +39,7 @@ describe("ReconnectingWebSocket", () => { describe("Reconnection Logic", () => { it("automatically reconnects on abnormal closure (1006)", async () => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + const { ws, sockets, flush } = await createReconnectingWebSocket(); sockets[0].fireOpen(); expect(ws.state).toBe(ConnectionState.CONNECTED); @@ -58,7 +56,7 @@ describe("ReconnectingWebSocket", () => { expect(ws.state).toBe(ConnectionState.CONNECTED); // A transient drop that reconnects is not a terminal failure. - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); ws.close(); }); @@ -68,8 +66,7 @@ describe("ReconnectingWebSocket", () => { ])( "reconnects on a server-initiated normal closure: $name ($code)", async ({ code }) => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + const { ws, sockets, flush } = await createReconnectingWebSocket(); sockets[0].fireOpen(); sockets[0].fireClose({ code, reason: "Normal" }); @@ -78,7 +75,7 @@ describe("ReconnectingWebSocket", () => { await vi.advanceTimersByTimeAsync(300); expect(sockets).toHaveLength(2); // A server-initiated close is not a terminal failure. - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); ws.close(); }, @@ -90,8 +87,7 @@ describe("ReconnectingWebSocket", () => { ])( "does not reconnect on unrecoverable WebSocket close code: %i", async (code) => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + const { ws, sockets, flush } = await createReconnectingWebSocket(); sockets[0].fireOpen(); expect(ws.state).toBe(ConnectionState.CONNECTED); @@ -101,9 +97,8 @@ describe("ReconnectingWebSocket", () => { await vi.advanceTimersByTimeAsync(10000); expect(sockets).toHaveLength(1); - expect(onConnectionFailure).toHaveBeenCalledWith( - "unrecoverable_close", - expect.any(String), + expect(flush).toHaveBeenCalledWith( + expect.stringMatching(/^unrecoverable_close /), ); ws.close(); @@ -116,18 +111,17 @@ describe("ReconnectingWebSocket", () => { const factory = vi .fn>() .mockRejectedValue(new HandshakeError(statusCode)); - const onConnectionFailure = vi.fn(); - const ws = await fromFactory(factory, { onConnectionFailure }); + const flush = vi.fn(); + const ws = await fromFactory(factory, { flush }); expect(ws.state).toBe(ConnectionState.DISCONNECTED); expect(vi.getTimerCount()).toBe(0); expect(factory).toHaveBeenCalledOnce(); if (statusCode === HttpStatusCode.UNAUTHORIZED) { - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); } else { - expect(onConnectionFailure).toHaveBeenCalledExactlyOnceWith( - "unrecoverable_http", - "/api/v2/test", + expect(flush).toHaveBeenCalledExactlyOnceWith( + "unrecoverable_http /api/v2/test", ); } @@ -138,8 +132,7 @@ describe("ReconnectingWebSocket", () => { it.each([...UNRECOVERABLE_HTTP_CODES])( "does not reconnect on unrecoverable HTTP error via error event: %i", async (statusCode) => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + const { ws, sockets, flush } = await createReconnectingWebSocket(); sockets[0].fireOpen(); sockets[0].fireError(new HandshakeError(statusCode)); expect(ws.state).toBe(ConnectionState.DISCONNECTED); @@ -151,11 +144,10 @@ describe("ReconnectingWebSocket", () => { expect(vi.getTimerCount()).toBe(0); if (statusCode === HttpStatusCode.UNAUTHORIZED) { - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); } else { - expect(onConnectionFailure).toHaveBeenCalledExactlyOnceWith( - "unrecoverable_http", - "/api/test", + expect(flush).toHaveBeenCalledExactlyOnceWith( + "unrecoverable_http /api/test", ); } ws.close(); @@ -169,8 +161,8 @@ describe("ReconnectingWebSocket", () => { .mockRejectedValueOnce(new HandshakeError(503)) .mockResolvedValueOnce(socket) .mockImplementation(() => Promise.resolve(createMockSocket())); - const onConnectionFailure = vi.fn(); - const ws = await fromFactory(factory, { onConnectionFailure }); + const flush = vi.fn(); + const ws = await fromFactory(factory, { flush }); expect(ws.state).toBe(ConnectionState.AWAITING_RETRY); expect(vi.getTimerCount()).toBe(1); @@ -183,18 +175,17 @@ describe("ReconnectingWebSocket", () => { expect(vi.getTimerCount()).toBe(1); await vi.advanceTimersToNextTimerAsync(); expect(factory).toHaveBeenCalledTimes(3); - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); ws.close(); }); it("does not read host/port digits as a status code", async () => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + const { ws, sockets, flush } = await createReconnectingWebSocket(); // A port ending in 404x must not be treated as HTTP 404. sockets[0].fireError(new Error("connect ECONNREFUSED 127.0.0.1:4040")); - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); // Generic connection errors retry rather than terminate. await vi.advanceTimersByTimeAsync(1000); expect(sockets.length).toBeGreaterThan(1); @@ -755,7 +746,7 @@ describe("ReconnectingWebSocket", () => { const setupRefreshTest = async (onRefresh: () => Promise) => { const sockets: MockSocket[] = []; const refreshCallback = vi.fn().mockImplementation(onRefresh); - const onConnectionFailure = vi.fn(); + const flush = vi.fn(); const factory = vi.fn(() => { const socket = createMockSocket(); sockets.push(socket); @@ -763,10 +754,10 @@ describe("ReconnectingWebSocket", () => { }); const ws = await fromFactory(factory, { onCertificateRefreshNeeded: refreshCallback, - onConnectionFailure, + flush, }); sockets[0].fireOpen(); - return { ws, sockets, refreshCallback, onConnectionFailure }; + return { ws, sockets, refreshCallback, flush }; }; it("reconnects after successful refresh", async () => { @@ -784,7 +775,7 @@ describe("ReconnectingWebSocket", () => { }); it("disconnects when refresh fails", async () => { - const { ws, sockets, onConnectionFailure } = await setupRefreshTest(() => + const { ws, sockets, flush } = await setupRefreshTest(() => Promise.resolve(false), ); @@ -794,9 +785,8 @@ describe("ReconnectingWebSocket", () => { ); expect(sockets).toHaveLength(1); - expect(onConnectionFailure).toHaveBeenCalledWith( - "certificate_error", - expect.any(String), + expect(flush).toHaveBeenCalledWith( + expect.stringMatching(/^certificate_error /), ); ws.close(); }); @@ -850,15 +840,14 @@ describe("ReconnectingWebSocket", () => { }); }); - describe("Connection failure callback", () => { - it("does not fire onConnectionFailure on a manual disconnect", async () => { - const { ws, sockets, onConnectionFailure } = - await createReconnectingWebSocket(); + describe("Log flush on failure", () => { + it("does not flush logs on a manual disconnect", async () => { + const { ws, sockets, flush } = await createReconnectingWebSocket(); sockets[0].fireOpen(); ws.disconnect(); - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); ws.close(); }); }); @@ -874,12 +863,11 @@ describe("ReconnectingWebSocket", () => { async function setupUnreachable( options: { telemetry?: TelemetryReporter } = {}, ) { - const onConnectionFailure = - vi.fn<(reason: ConnectionFailureReason, route: string) => void>(); + const flush = vi.fn<(reason: string) => void>(); const { ws, sockets, setFactoryError } = await createReconnectingWebSocketWithErrorControl({ ...options, - onConnectionFailure, + flush, initialBackoffMs: BACKOFF_MS, maxBackoffMs: BACKOFF_MS, jitterFactor: 0, @@ -913,7 +901,7 @@ describe("ReconnectingWebSocket", () => { return { ws, sockets, - onConnectionFailure, + flush, startOutage, failNextAttempt, failUntilFlush, @@ -922,58 +910,56 @@ describe("ReconnectingWebSocket", () => { } it("flushes once with the unreachable reason after N failed attempts", async () => { - const { ws, onConnectionFailure, startOutage, failNextAttempt } = + const { ws, flush, startOutage, failNextAttempt } = await setupUnreachable(); startOutage(); // attempt 1 for (let i = 0; i < FAILURES_BEFORE_FLUSH - 2; i++) { await failNextAttempt(); // through attempt N-1 } - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); await failNextAttempt(); // attempt N - expect(onConnectionFailure).toHaveBeenCalledTimes(1); - expect(onConnectionFailure).toHaveBeenCalledWith("unreachable", ROUTE); + expect(flush).toHaveBeenCalledTimes(1); + expect(flush).toHaveBeenCalledWith(`unreachable ${ROUTE}`); ws.close(); }); it("keeps retrying through a long outage without flushing again, then recovers", async () => { - const { ws, onConnectionFailure, failUntilFlush, recover } = - await setupUnreachable(); + const { ws, flush, failUntilFlush, recover } = await setupUnreachable(); await failUntilFlush(); - expect(onConnectionFailure).toHaveBeenCalledTimes(1); + expect(flush).toHaveBeenCalledTimes(1); // A long outage (sleep, network loss) never gives up or re-flushes. await vi.advanceTimersByTimeAsync(5 * 60_000); expect(ws.state).toBe(ConnectionState.AWAITING_RETRY); - expect(onConnectionFailure).toHaveBeenCalledTimes(1); + expect(flush).toHaveBeenCalledTimes(1); await recover(); expect(ws.state).toBe(ConnectionState.CONNECTED); - expect(onConnectionFailure).toHaveBeenCalledTimes(1); + expect(flush).toHaveBeenCalledTimes(1); ws.close(); }); it("flushes again after a successful open resets the counter", async () => { - const { ws, onConnectionFailure, failUntilFlush, recover } = - await setupUnreachable(); + const { ws, flush, failUntilFlush, recover } = await setupUnreachable(); await failUntilFlush(); - expect(onConnectionFailure).toHaveBeenCalledTimes(1); + expect(flush).toHaveBeenCalledTimes(1); await recover(); await failUntilFlush(); - expect(onConnectionFailure).toHaveBeenCalledTimes(2); + expect(flush).toHaveBeenCalledTimes(2); ws.close(); }); it("does not flush a transient outage that recovers before N", async () => { - const { ws, onConnectionFailure, startOutage, failNextAttempt, recover } = + const { ws, flush, startOutage, failNextAttempt, recover } = await setupUnreachable(); startOutage(); @@ -981,7 +967,7 @@ describe("ReconnectingWebSocket", () => { await failNextAttempt(); await recover(); - expect(onConnectionFailure).not.toHaveBeenCalled(); + expect(flush).not.toHaveBeenCalled(); ws.close(); }); @@ -1069,17 +1055,12 @@ function createMockSocket(): MockSocket { }; } -type ConnectionFailureSpy = ReturnType< - typeof vi.fn<(reason: ConnectionFailureReason, route: string) => void> ->; +type FlushSpy = ReturnType void>>; interface FactoryOptions { onDispose?: () => void; onCertificateRefreshNeeded?: () => Promise; - onConnectionFailure?: ( - reason: ConnectionFailureReason, - route: string, - ) => void; + flush?: (reason: string) => void; route?: string; telemetry?: TelemetryReporter; initialBackoffMs?: number; @@ -1092,19 +1073,18 @@ async function createReconnectingWebSocket( ): Promise<{ ws: ReconnectingWebSocket; sockets: MockSocket[]; - onConnectionFailure: ConnectionFailureSpy; + flush: FlushSpy; }> { const sockets: MockSocket[] = []; - const onConnectionFailure = - vi.fn<(reason: ConnectionFailureReason, route: string) => void>(); + const flush = vi.fn<(reason: string) => void>(); const factory = vi.fn(() => { const socket = createMockSocket(); sockets.push(socket); return Promise.resolve(socket); }); - const ws = await fromFactory(factory, { ...options, onConnectionFailure }); + const ws = await fromFactory(factory, { ...options, flush }); expect(sockets).toHaveLength(1); - return { ws, sockets, onConnectionFailure }; + return { ws, sockets, flush }; } async function createReconnectingWebSocketWithErrorControl( @@ -1144,13 +1124,12 @@ async function fromFactory( ): Promise> { return await ReconnectingWebSocket.create( factory, - createMockLogger(), + { ...createMockLogger(), flush: options.flush ?? vi.fn() }, { telemetry: options.telemetry ?? NOOP_TELEMETRY_REPORTER, route: options.route ?? "/api/v2/test", onCertificateRefreshNeeded: options.onCertificateRefreshNeeded ?? (() => Promise.resolve(false)), - onConnectionFailure: options.onConnectionFailure ?? vi.fn(), initialBackoffMs: options.initialBackoffMs, maxBackoffMs: options.maxBackoffMs, jitterFactor: options.jitterFactor, From 538f9e9f8ee93e8f4f859ca7a1c8ad4290d6125c Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sat, 10 Oct 2026 00:25:18 +0300 Subject: [PATCH 2/2] refactor(logging): rename the buffering logger to FlightRecorder Match the flight recorder naming from coder/slog. Logger now carries flush, so consumers take a plain Logger again; the write-only side (output channel, prefixLogger) becomes LogSink. BufferingLogger is renamed to FlightRecorder and is only named where it is constructed. --- CONTRIBUTING.md | 2 +- src/api/coderApi.ts | 6 +++--- src/commands.ts | 4 ++-- src/core/container.ts | 10 +++++----- src/deployment/deploymentManager.ts | 4 ++-- src/logging/{logBuffer.ts => flightRecorder.ts} | 6 +++--- src/logging/logger.ts | 7 ++++--- src/logging/prefixLogger.ts | 6 +++--- src/login/loginCoordinator.ts | 4 ++-- src/oauth/authorizer.ts | 4 ++-- src/oauth/sessionManager.ts | 4 ++-- src/remote/remote.ts | 4 ++-- src/websocket/reconnectingWebSocket.ts | 8 ++++---- test/mocks/testHelpers.ts | 8 ++++---- test/unit/error/serverCertificateError.test.ts | 1 + .../{logBuffer.test.ts => flightRecorder.test.ts} | 12 ++++++------ test/unit/remote/remote.test.ts | 4 ++-- 17 files changed, 48 insertions(+), 46 deletions(-) rename src/logging/{logBuffer.ts => flightRecorder.ts} (96%) rename test/unit/logging/{logBuffer.test.ts => flightRecorder.test.ts} (96%) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 18bf8c53fa..87b0db4ebe 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -172,7 +172,7 @@ next to the code: The extension logs to the "Coder" output channel, a `LogOutputChannel` that gates messages by the level chosen in its gear menu. To help Support diagnose connection failures without asking users to reproduce with debug logging enabled, -a `BufferingLogger` ([`src/logging/logBuffer.ts`](src/logging/logBuffer.ts)) +a `FlightRecorder` ([`src/logging/flightRecorder.ts`](src/logging/flightRecorder.ts)) wraps the channel and keeps a bounded, in-memory ring of the entries that sit **below** the current level, which the channel would otherwise drop. diff --git a/src/api/coderApi.ts b/src/api/coderApi.ts index 46704d75eb..6398e1086e 100644 --- a/src/api/coderApi.ts +++ b/src/api/coderApi.ts @@ -72,7 +72,7 @@ import type { } from "coder/site/src/api/typesGenerated"; import type { ClientOptions } from "ws"; -import type { BufferedLogger, Logger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; import type { CloseEvent, ErrorEvent, @@ -124,7 +124,7 @@ export class CoderApi extends Api implements vscode.Disposable { private readonly configWatcher: vscode.Disposable; private constructor( - private readonly output: BufferedLogger, + private readonly output: Logger, private readonly telemetry: TelemetryReporter, private readonly httpRequestsTelemetry: HttpRequestsTelemetry, private readonly authConfigTracker: AuthConfigTracker, @@ -146,7 +146,7 @@ export class CoderApi extends Api implements vscode.Disposable { static create( baseUrl: string, token: string | undefined, - output: BufferedLogger, + output: Logger, telemetry: TelemetryReporter = NOOP_TELEMETRY_REPORTER, ): CoderApi { const httpRequestsTelemetry = new HttpRequestsTelemetry(telemetry); diff --git a/src/commands.ts b/src/commands.ts index c33746f31d..759306ee8a 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -84,7 +84,7 @@ import type { MementoManager } from "./core/mementoManager"; import type { PathResolver } from "./core/pathResolver"; import type { SecretsManager, SessionAuth } from "./core/secretsManager"; import type { DeploymentManager } from "./deployment/deploymentManager"; -import type { BufferedLogger } from "./logging/logger"; +import type { Logger } from "./logging/logger"; import type { LoginCoordinator, LoginMethod } from "./login/loginCoordinator"; import type { TelemetryService } from "./telemetry/service"; import type { NetcheckPanelFactory } from "./webviews/netcheck/netcheckPanelFactory"; @@ -155,7 +155,7 @@ const openDefaults = { } as const satisfies Partial; export class Commands { - private readonly logger: BufferedLogger; + private readonly logger: Logger; private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; private readonly secretsManager: SecretsManager; diff --git a/src/core/container.ts b/src/core/container.ts index dd381010c0..cde147bd0c 100644 --- a/src/core/container.ts +++ b/src/core/container.ts @@ -2,7 +2,7 @@ import * as vscode from "vscode"; import { watchConfigurationChanges } from "../configWatcher"; import { AuthTelemetry } from "../instrumentation/auth"; -import { BufferingLogger } from "../logging/logBuffer"; +import { FlightRecorder } from "../logging/flightRecorder"; import { prefixLogger } from "../logging/prefixLogger"; import { shortId } from "../logging/utils"; import { LoginCoordinator } from "../login/loginCoordinator"; @@ -27,7 +27,7 @@ import { PathResolver } from "./pathResolver"; import { SecretsManager } from "./secretsManager"; import { sessionId } from "./sessionId"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; /** * Service container for dependency injection. @@ -35,7 +35,7 @@ import type { BufferedLogger } from "../logging/logger"; */ export class ServiceContainer implements vscode.Disposable { private readonly outputChannel: vscode.LogOutputChannel; - private readonly logger: BufferingLogger; + private readonly logger: FlightRecorder; private readonly connectionLogBufferConfigSubscription: vscode.Disposable; private readonly pathResolver: PathResolver; private readonly mementoManager: MementoManager; @@ -57,7 +57,7 @@ export class ServiceContainer implements vscode.Disposable { }); const readSize = () => readConnectionLogBufferSize(vscode.workspace.getConfiguration()); - this.logger = new BufferingLogger( + this.logger = new FlightRecorder( prefixLogger(this.outputChannel, `[session ${shortId(sessionId)}]`), this.outputChannel, readSize(), @@ -163,7 +163,7 @@ export class ServiceContainer implements vscode.Disposable { return this.secretsManager; } - getLogger(): BufferedLogger { + getLogger(): Logger { return this.logger; } diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 848fa5223d..36e6fac94d 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -13,7 +13,7 @@ import { type DeploymentRecoveryTrigger, type DeploymentSuspendReason, } from "../instrumentation/deployment"; -import { type BufferedLogger } from "../logging/logger"; +import { type Logger } from "../logging/logger"; import { type OAuthSessionManager } from "../oauth/sessionManager"; import { getAuthConfigWatchSettings } from "../settings/authConfig"; import { type TelemetryService } from "../telemetry/service"; @@ -47,7 +47,7 @@ export class DeploymentManager implements vscode.Disposable { private readonly secretsManager: SecretsManager; private readonly mementoManager: MementoManager; private readonly contextManager: ContextManager; - private readonly logger: BufferedLogger; + private readonly logger: Logger; private readonly telemetryService: TelemetryService; private readonly deploymentTelemetry: DeploymentTelemetry; diff --git a/src/logging/logBuffer.ts b/src/logging/flightRecorder.ts similarity index 96% rename from src/logging/logBuffer.ts rename to src/logging/flightRecorder.ts index e4181af1ac..32486c3519 100644 --- a/src/logging/logBuffer.ts +++ b/src/logging/flightRecorder.ts @@ -1,6 +1,6 @@ import { safeStringify } from "./utils"; -import type { BufferedLogger, Logger } from "./logger"; +import type { LogSink, Logger } from "./logger"; /** * Numeric severities matching `vscode.LogLevel` (Off=0, Trace=1, Debug=2, @@ -40,12 +40,12 @@ interface LogEntry { * Buffers entries below the current log level and replays them on failure at a * level the output channel persists. */ -export class BufferingLogger implements BufferedLogger { +export class FlightRecorder implements Logger { private entries: LogEntry[] = []; private chars = 0; public constructor( - private readonly inner: Logger, + private readonly inner: LogSink, private readonly channel: { readonly logLevel: number }, private capacity: number, ) {} diff --git a/src/logging/logger.ts b/src/logging/logger.ts index 46b6dba2ca..f42a02adf5 100644 --- a/src/logging/logger.ts +++ b/src/logging/logger.ts @@ -1,4 +1,5 @@ -export interface Logger { +/** A destination that writes log entries, such as the output channel. */ +export interface LogSink { trace(message: string, ...args: unknown[]): void; debug(message: string, ...args: unknown[]): void; info(message: string, ...args: unknown[]): void; @@ -7,7 +8,7 @@ export interface Logger { show(): void; } -/** A logger that buffers below-level entries and can replay them. */ -export interface BufferedLogger extends Logger { +/** A sink that also records below-level entries and can replay them. */ +export interface Logger extends LogSink { flush(reason: string, options?: { readonly retain?: boolean }): void; } diff --git a/src/logging/prefixLogger.ts b/src/logging/prefixLogger.ts index 1cf4d2ce12..c5b95f67c0 100644 --- a/src/logging/prefixLogger.ts +++ b/src/logging/prefixLogger.ts @@ -1,11 +1,11 @@ -import type { Logger } from "./logger"; +import type { LogSink } from "./logger"; /** - * Wraps a {@link Logger} so every message is prefixed, letting all lines that + * Wraps a {@link LogSink} so every message is prefixed, letting all lines that * share a prefix (a session ID, a workspace name) be found with one search. * Extra arguments are forwarded untouched. */ -export function prefixLogger(inner: Logger, prefix: string): Logger { +export function prefixLogger(inner: LogSink, prefix: string): LogSink { const tag = (message: string) => `${prefix} ${message}`; return { trace: (message, ...args) => inner.trace(tag(message), ...args), diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 380b2d9677..248d1fe2cf 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -29,7 +29,7 @@ import type { AuthTelemetry, LoginPromptReason, } from "../instrumentation/auth"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; import type { OAuthCallback } from "../oauth/oauthCallback"; export type LoginMethod = @@ -86,7 +86,7 @@ export class LoginCoordinator implements vscode.Disposable { constructor( private readonly secretsManager: SecretsManager, private readonly mementoManager: MementoManager, - private readonly logger: BufferedLogger, + private readonly logger: Logger, private readonly cliCredentialManager: CliCredentialManager, private readonly authTelemetry: AuthTelemetry, oauthCallback: OAuthCallback, diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index 27f946f4c1..b22bb90a70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -35,7 +35,7 @@ import type { import type { SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; import type { OAuthCallback } from "./oauthCallback"; @@ -49,7 +49,7 @@ export class OAuthAuthorizer implements vscode.Disposable { constructor( private readonly secretsManager: SecretsManager, private readonly oauthCallback: OAuthCallback, - private readonly logger: BufferedLogger, + private readonly logger: Logger, private readonly extensionId: string, ) {} diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 726dd5b51e..5ccc122aa5 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -23,7 +23,7 @@ import type * as vscode from "vscode"; import type { ServiceContainer } from "../core/container"; import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; /** * Token refresh threshold: refresh when token expires in less than this time. @@ -79,7 +79,7 @@ export class OAuthSessionManager implements vscode.Disposable { private constructor( private deployment: Deployment | null, private readonly secretsManager: SecretsManager, - private readonly logger: BufferedLogger, + private readonly logger: Logger, private readonly onAuthRequired: () => Promise, private readonly authTelemetry: AuthTelemetry, ) {} diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 7446d5108b..88277d3501 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -89,7 +89,7 @@ import type { ContextManager } from "../core/contextManager"; import type { StartupMode } from "../core/mementoManager"; import type { PathResolver } from "../core/pathResolver"; import type { SecretsManager } from "../core/secretsManager"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; export interface RemoteDetails extends vscode.Disposable { @@ -149,7 +149,7 @@ export function workspaceLabelSuffix( } export class Remote { - private readonly logger: BufferedLogger; + private readonly logger: Logger; private readonly pathResolver: PathResolver; private readonly cliManager: CliManager; private readonly contextManager: ContextManager; diff --git a/src/websocket/reconnectingWebSocket.ts b/src/websocket/reconnectingWebSocket.ts index f392b42259..ff8a154ce9 100644 --- a/src/websocket/reconnectingWebSocket.ts +++ b/src/websocket/reconnectingWebSocket.ts @@ -16,7 +16,7 @@ import { handshakeStatus } from "./utils"; import type { WebSocketEventType } from "coder/site/src/utils/OneWayWebSocket"; -import type { BufferedLogger } from "../logging/logger"; +import type { Logger } from "../logging/logger"; import type { TelemetryReporter } from "../telemetry/reporter"; import type { @@ -132,7 +132,7 @@ export class ReconnectingWebSocket< TData = unknown, > implements UnidirectionalStream { readonly #socketFactory: SocketFactory; - readonly #logger: BufferedLogger; + readonly #logger: Logger; readonly #telemetry: WebSocketTelemetry; readonly #options: Required< Omit @@ -181,7 +181,7 @@ export class ReconnectingWebSocket< private constructor( socketFactory: SocketFactory, - logger: BufferedLogger, + logger: Logger, options: ReconnectingWebSocketOptions, onDispose?: () => void, ) { @@ -201,7 +201,7 @@ export class ReconnectingWebSocket< public static async create( socketFactory: SocketFactory, - logger: BufferedLogger, + logger: Logger, options: ReconnectingWebSocketOptions, onDispose?: () => void, ): Promise> { diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 2ab14c099a..858fd31956 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -44,7 +44,7 @@ import type { MementoManager } from "@/core/mementoManager"; import type { PathResolver } from "@/core/pathResolver"; import type { SecretsManager } from "@/core/secretsManager"; import type { Deployment } from "@/deployment/types"; -import type { BufferedLogger } from "@/logging/logger"; +import type { Logger } from "@/logging/logger"; import type { LoginCoordinator } from "@/login/loginCoordinator"; import type { NetworkInfo } from "@/remote/sshProcess"; import type { TelemetryService } from "@/telemetry/service"; @@ -552,7 +552,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { } as unknown as CliCredentialManager; } -export function createMockLogger(): BufferedLogger { +export function createMockLogger(): Logger { return { trace: vi.fn(), debug: vi.fn(), @@ -574,7 +574,7 @@ export interface LogEntry { * Logger that records what was logged. Assert on `entries` for exact output, * or search `text` when checking that a secret never reached the log. */ -export class LogCollector implements BufferedLogger { +export class LogCollector implements Logger { readonly entries: LogEntry[] = []; /** Every message and argument logged, as one searchable string. */ @@ -650,7 +650,7 @@ export async function setAge(filePath: string, daysAgo: number): Promise { export function createMockServiceContainer( overrides: { telemetry?: TelemetryService; - logger?: BufferedLogger; + logger?: Logger; secretsManager?: SecretsManager; mementoManager?: MementoManager; cliCredentialManager?: CliCredentialManager; diff --git a/test/unit/error/serverCertificateError.test.ts b/test/unit/error/serverCertificateError.test.ts index 3ded52128c..c639f8de52 100644 --- a/test/unit/error/serverCertificateError.test.ts +++ b/test/unit/error/serverCertificateError.test.ts @@ -42,6 +42,7 @@ describe("Certificate errors", () => { warn: throwingLog, error: throwingLog, show: () => {}, + flush: () => {}, }; const disposers: Array<() => void> = []; diff --git a/test/unit/logging/logBuffer.test.ts b/test/unit/logging/flightRecorder.test.ts similarity index 96% rename from test/unit/logging/logBuffer.test.ts rename to test/unit/logging/flightRecorder.test.ts index 323fc2e8a0..4c0b822698 100644 --- a/test/unit/logging/logBuffer.test.ts +++ b/test/unit/logging/flightRecorder.test.ts @@ -1,8 +1,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { BufferingLogger } from "@/logging/logBuffer"; +import { FlightRecorder } from "@/logging/flightRecorder"; -import type { Logger } from "@/logging/logger"; +import type { LogSink } from "@/logging/logger"; // Numeric levels matching vscode.LogLevel. const OFF = 0; @@ -11,7 +11,7 @@ const INFO = 3; const WARNING = 4; const ERROR = 5; -type LogMethod = Exclude; +type LogMethod = Exclude; interface Call { level: LogMethod; @@ -25,7 +25,7 @@ function setup(level: number, capacity: number) { (method: LogMethod) => (message: string, ...args: unknown[]) => calls.push({ level: method, message, args }); - const logger: Logger = { + const logger: LogSink = { trace: push("trace"), debug: push("debug"), info: push("info"), @@ -34,7 +34,7 @@ function setup(level: number, capacity: number) { show: vi.fn(), }; const channel = { logLevel: level }; - const buffer = new BufferingLogger(logger, channel, capacity); + const buffer = new FlightRecorder(logger, channel, capacity); // Ignore the pass-through calls, then return only what the flush replayed. const flush = ( reason = "r", @@ -51,7 +51,7 @@ afterEach(() => { vi.restoreAllMocks(); }); -describe("BufferingLogger", () => { +describe("FlightRecorder", () => { it("forwards every call to the inner logger", () => { const { buffer, calls } = setup(INFO, 10); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 3b418cc89b..fe4d7cf6a5 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -31,7 +31,7 @@ import { import type { Commands } from "@/commands"; import type { CliManager } from "@/core/cliManager"; -import type { BufferedLogger } from "@/logging/logger"; +import type { Logger } from "@/logging/logger"; import type { CliAuth } from "@/settings/cli"; const mockWorkspace = vscode.workspace as typeof vscode.workspace & { @@ -71,7 +71,7 @@ const CLI_AUTH: CliAuth = { allowRedirects: false, }; -function createRemote(logger: BufferedLogger = createMockLogger()) { +function createRemote(logger: Logger = createMockLogger()) { new MockConfigurationProvider(); const userInteraction = new MockUserInteraction(); const pathResolver = new PathResolver("/mock/global", "/mock/log");