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 3a36ce6070..6398e1086e 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"; @@ -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, }; diff --git a/src/commands.ts b/src/commands.ts index 19b7017d5f..759306ee8a 100644 --- a/src/commands.ts +++ b/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, diff --git a/src/core/container.ts b/src/core/container.ts index a39e26503b..cde147bd0c 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 { 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; } 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/flightRecorder.ts similarity index 85% rename from src/logging/logBuffer.ts rename to src/logging/flightRecorder.ts index 31e3679641..32486c3519 100644 --- a/src/logging/logBuffer.ts +++ b/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 - * ` ` 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..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; @@ -6,3 +7,8 @@ export interface Logger { 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; +} 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/remote/remote.ts b/src/remote/remote.ts index 002f452d4e..88277d3501 100644 --- a/src/remote/remote.ts +++ b/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"); } diff --git a/src/websocket/reconnectingWebSocket.ts b/src/websocket/reconnectingWebSocket.ts index 4d9b6f90e7..ff8a154ce9 100644 --- a/src/websocket/reconnectingWebSocket.ts +++ b/src/websocket/reconnectingWebSocket.ts @@ -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,14 +126,6 @@ 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< @@ -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); diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 0d86635fa2..858fd31956 100644 --- a/test/mocks/testHelpers.ts +++ b/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: () => 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/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/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,