refactor(logging): flush the flight recorder directly from sockets

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.
 

src/api/coderApi.ts

@@ -45,7 +45,6 @@ import {
 import {
 	ConnectionState,
 	ReconnectingWebSocket,
-	type ConnectionFailureReason,
 	type ReconnectingWebSocketOptions,
 	type SocketFactory,
 } from "../websocket/reconnectingWebSocket";
@@ -129,10 +128,6 @@ export class CoderApi extends Api implements vscode.Disposable {
 		private readonly telemetry: TelemetryReporter,
 		private readonly httpRequestsTelemetry: HttpRequestsTelemetry,
 		private readonly authConfigTracker: AuthConfigTracker,
-		private readonly onConnectionFailure?: (
-			reason: ConnectionFailureReason,
-			route: string,
-		) => void,
 	) {
 		super();
 		wrapWithValidation(this);
@@ -153,10 +148,6 @@ export class CoderApi extends Api implements vscode.Disposable {
 		token: string | undefined,
 		output: Logger,
 		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,
 		};
 

src/commands.ts

@@ -84,7 +84,6 @@ 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 { LoginCoordinator, LoginMethod } from "./login/loginCoordinator";
 import type { TelemetryService } from "./telemetry/service";
@@ -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,

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 { FlightRecorder } from "../logging/flightRecorder";
 import { prefixLogger } from "../logging/prefixLogger";
 import { shortId } from "../logging/utils";
 import { LoginCoordinator } from "../login/loginCoordinator";
@@ -38,7 +35,7 @@ import type { Logger } 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;
@@ -60,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(),
@@ -170,11 +167,6 @@ export class ServiceContainer implements vscode.Disposable {
 		return this.logger;
 	}
 
-	/** The connection log buffer that replays below-level entries on failure. */
-	getConnectionLogBuffer(): ConnectionLogBuffer {
-		return this.logger;
-	}
-
 	getCliManager(): CliManager {
 		return this.cliManager;
 	}

src/extension.ts

@@ -141,7 +141,6 @@ async function doActivate(
 		deploymentSessionAuth?.token,
 		output,
 		telemetryService,
-		serviceContainer.getConnectionLogBuffer().onConnectionFailure,
 	);
 	ctx.subscriptions.push(client);
 

src/logging/logBuffer.ts → src/logging/flightRecorder.ts

@@ -1,6 +1,6 @@
 import { safeStringify } from "./utils";
 
-import type { Logger } from "./logger";
+import type { LogSink, 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,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 Logger, ConnectionLogBuffer {
+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,
 	) {}
@@ -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
-	 * `<reason> <route>` 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();
 	}

src/logging/logger.ts

@@ -1,8 +1,14 @@
-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;
 	warn(message: string, ...args: unknown[]): void;
 	error(message: string, ...args: unknown[]): void;
 	show(): void;
 }
+
+/** 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;
+}

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),

src/remote/remote.ts

@@ -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");
 	}
 

src/websocket/reconnectingWebSocket.ts

@@ -111,11 +111,6 @@ function reduceState(
 
 export type SocketFactory<TData> = () => Promise<UnidirectionalStream<TData>>;
 
-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,14 +126,6 @@ export interface ReconnectingWebSocketOptions {
 	route: string;
 	/** Callback invoked when a refreshable certificate error is detected. Returns true if refresh succeeded. */
 	onCertificateRefreshNeeded: () => Promise<boolean>;
-	/**
-	 * 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<
@@ -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;
@@ -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);

test/mocks/testHelpers.ts

@@ -44,7 +44,6 @@ 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 { LoginCoordinator } from "@/login/loginCoordinator";
 import type { NetworkInfo } from "@/remote/sshProcess";
@@ -561,6 +560,7 @@ export function createMockLogger(): Logger {
 		warn: vi.fn(),
 		error: vi.fn(),
 		show: vi.fn(),
+		flush: vi.fn(),
 	};
 }
 
@@ -607,6 +607,8 @@ export class LogCollector implements Logger {
 
 	show(): void {}
 
+	flush(): void {}
+
 	private collect(
 		level: LogEntry["level"],
 		message: string,
@@ -669,10 +671,6 @@ export function createMockServiceContainer(
 	return {
 		getTelemetryService: () => telemetry,
 		getLogger: () => logger,
-		getConnectionLogBuffer: (): ConnectionLogBuffer => ({
-			flush: () => {},
-			onConnectionFailure: () => {},
-		}),
 		getSecretsManager: () =>
 			require("secretsManager", overrides.secretsManager),
 		getMementoManager: () =>