From faf2e868ad29b32d7c81bae3ea6343c875c3256a Mon Sep 17 00:00:00 2001 From: Roman Kuznetsov Date: Thu, 21 May 2026 03:53:26 +0300 Subject: [PATCH] fix(wsdriver): provide connection error info --- src/browser/wsdriver/request.ts | 2 +- src/ws-connection/error.ts | 8 +- src/ws-connection/index.ts | 26 +++++- src/ws-connection/utils.ts | 2 +- test/src/browser/wsdriver/request.ts | 16 ++++ test/src/ws-connection/index.ts | 131 +++++++++++++++++++++++++++ 6 files changed, 177 insertions(+), 8 deletions(-) diff --git a/src/browser/wsdriver/request.ts b/src/browser/wsdriver/request.ts index 563baebfc..18a909ce8 100644 --- a/src/browser/wsdriver/request.ts +++ b/src/browser/wsdriver/request.ts @@ -63,7 +63,7 @@ export const constructWsDriverRequest = async ( throw new Error("Not session-related request"); } - const command = url.pathname.slice(commandStartIdx + connectionOptions.sessionPrefix.length); + const command = decodeURIComponent(url.pathname.slice(commandStartIdx + connectionOptions.sessionPrefix.length)); const bodyPayload = requestOptions.json ? Buffer.from(JSON.stringify(requestOptions.json)) : Buffer.alloc(0); const shouldCompress = connectionOptions.compressionType !== WsDriverCompression.None && diff --git a/src/ws-connection/error.ts b/src/ws-connection/error.ts index c45338945..62e26ba77 100644 --- a/src/ws-connection/error.ts +++ b/src/ws-connection/error.ts @@ -26,13 +26,17 @@ export abstract class WsError extends Error { } export class WsConnectionEstablishmentError extends WsError { - constructor({ message, requestId }: { message: string; requestId?: number }) { - super({ message, code: WS_ERROR_CODE.CONNECTION_ESTABLISHMENT, requestId }); + constructor({ message, requestId, statusCode }: { message: string; requestId?: number; statusCode?: number }) { + super({ message, code: statusCode || WS_ERROR_CODE.CONNECTION_ESTABLISHMENT, requestId }); this.name = this.constructor.name; } isRetryable(): boolean { + if (this.code && this.code >= 400 && this.code < 500) { + return false; + } + return true; } } diff --git a/src/ws-connection/index.ts b/src/ws-connection/index.ts index c5c100a49..fa05149de 100644 --- a/src/ws-connection/index.ts +++ b/src/ws-connection/index.ts @@ -1,5 +1,6 @@ /* eslint-disable new-cap */ import { IncomingMessage } from "node:http"; +import { text as consumeText } from "node:stream/consumers"; import { WebSocket, type RawData } from "ws"; import { WsTimeoutError, @@ -146,6 +147,19 @@ export class WsConnection< done(ws); }; + const onUnexpectedResponse = async (ws: WebSocket, res: IncomingMessage): Promise => { + closeWsConnection(ws); + + const reason = await consumeText(res).catch(() => "Unknown reason"); + + done( + new this._errors.ConnectionEstablishment({ + message: `Couldn't establish WS connection to "${endpoint}"\n\tReason: ${reason}`, + statusCode: res.statusCode, + }), + ); + }; + const onError = (error: unknown): void => { closeWsConnection(ws); done( @@ -169,6 +183,7 @@ export class WsConnection< }; ws.on("open", onOpen); + ws.on("unexpected-response", onUnexpectedResponse); ws.on("error", onError); ws.on("close", onClose); ws.on("upgrade", onUpgrade); @@ -183,6 +198,7 @@ export class WsConnection< isSettled = true; clearTimeout(timeoutId); ws.off("open", onOpen); + ws.off("unexpected-response", onUnexpectedResponse); ws.off("error", onError); ws.off("close", onClose); ws.off("upgrade", onUpgrade); @@ -276,7 +292,7 @@ export class WsConnection< throw result; } - this._debugFn(`⟳ ${result.message}; retries left: ${retriesLeft}`); + this._debugFn(`⟳ ${result.message}; retries left: ${retriesLeft}; endpoint: "${this._endpoint}"`); // Intentionally avoiding wait after timeout if (result instanceof WsError && !(result instanceof WsTimeoutError)) { @@ -383,7 +399,9 @@ export class WsConnection< provideResponseFor(requestId: number, data: ResponseMessageType | WsError): void { if (!this._pendingRequests[requestId]) { - this._debugFn(`! Received response to request ${requestId}, which is probably timed out already`); + this._debugFn( + `! Received response to request ${requestId}, which is probably timed out already; endpoint: "${this._endpoint}"`, + ); return; } @@ -508,7 +526,7 @@ export class WsConnection< if (isWaitingForPong && this._isWebSocketActive(ws)) { isWaitingForPong = false; - this._debugFn("< PONG"); + this._debugFn(`< PONG; endpoint: "${this._endpoint}"`); clearTimeout(pongTimeout); @@ -549,7 +567,7 @@ export class WsConnection< ws.ping(); - this._debugFn("> PING"); + this._debugFn(`> PING; endpoint: "${this._endpoint}"`); isWaitingForPong = true; }, WS_PING_INTERVAL).unref()); diff --git a/src/ws-connection/utils.ts b/src/ws-connection/utils.ts index db3dbc605..badcef9e9 100644 --- a/src/ws-connection/utils.ts +++ b/src/ws-connection/utils.ts @@ -6,5 +6,5 @@ export const exponentiallyWait = ({ }: { baseDelay?: number; attempt?: number; factor?: number; jitter?: number } = {}): Promise => { const delay = Math.round(baseDelay * factor ** attempt + Math.random() * jitter); - return new Promise(resolve => setTimeout(resolve, delay).unref()); + return new Promise(resolve => setTimeout(resolve, delay)); }; diff --git a/test/src/browser/wsdriver/request.ts b/test/src/browser/wsdriver/request.ts index db28c07ac..2017ddc0a 100644 --- a/test/src/browser/wsdriver/request.ts +++ b/test/src/browser/wsdriver/request.ts @@ -27,6 +27,22 @@ describe("wsdriver/request", () => { assert.equal(body, JSON.stringify(options.json)); }); + it("should decode percent-encoded characters in command", async () => { + const url = new URL("http://localhost/session/123/element/abc%3A123%2Fdef/value"); + const options = { method: "GET" }; + const connectionOptions = { + requestId: 1, + sessionPrefix: "/session/123/", + compressionType: WsDriverCompression.None, + }; + + const result = await constructWsDriverRequest(url, options as any, connectionOptions); + + const commandEnd = result.indexOf(0, 8); + const command = result.toString("utf8", 8, commandEnd); + assert.equal(command, "element/abc:123/def/value"); + }); + it("should construct request with compression if body is large enough", async () => { const url = new URL("http://localhost/session/123/element"); const largeString = "a".repeat(WSD_COMPRESSION_THRESHOLD_BYTES); diff --git a/test/src/ws-connection/index.ts b/test/src/ws-connection/index.ts index 19729f1c3..fa29dc3e3 100644 --- a/test/src/ws-connection/index.ts +++ b/test/src/ws-connection/index.ts @@ -1,3 +1,4 @@ +import * as http from "node:http"; import { WebSocket, WebSocketServer } from "ws"; import sinon, { SinonStub, SinonFakeTimers } from "sinon"; import proxyquire from "proxyquire"; @@ -13,6 +14,7 @@ import { import { WS_MAX_REQUEST_ID } from "src/ws-connection/constants"; const STUB_SERVER_PORT = 50123; +const STUB_HTTP_SERVER_PORT = 50124; type StubWebSocketServer = WebSocketServer & { waitForConnection: Promise; @@ -394,4 +396,133 @@ describe('"WsConnection"', () => { assert.calledOnce(exponentiallyWaitStub); }); }); + + describe("unexpected response handling", () => { + const httpEndpoint = `ws://localhost:${STUB_HTTP_SERVER_PORT}`; + let httpServer: http.Server | null = null; + let httpRequestsCount = 0; + let connection: TestWsConnection | null = null; + + const startHttpServer = ( + responder: (req: http.IncomingMessage, res: http.ServerResponse) => void, + ): Promise => { + httpRequestsCount = 0; + httpServer = http.createServer((req, res) => { + httpRequestsCount++; + responder(req, res); + }); + + return new Promise((resolve, reject) => { + httpServer!.once("error", reject); + httpServer!.listen(STUB_HTTP_SERVER_PORT, () => resolve()); + }); + }; + + afterEach(async () => { + connection?.close(); + connection = null; + + if (httpServer) { + await new Promise(resolve => httpServer!.close(() => resolve())); + httpServer = null; + } + }); + + it("should reject with 'WsConnectionEstablishmentError' when server returns non-upgrade response", async () => { + await startHttpServer((_req, res) => { + res.statusCode = 404; + res.end("not a websocket"); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + await assert.isRejected( + connection.request("Successful.Method"), + WsConnectionEstablishmentError, + `Couldn't establish WS connection to "${httpEndpoint}"`, + ); + }); + + it("should populate error with the response status code", async () => { + await startHttpServer((_req, res) => { + res.statusCode = 401; + res.end("unauthorized"); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + try { + await connection.request("Successful.Method"); + assert.fail("should have thrown WsConnectionEstablishmentError"); + } catch (err) { + assert.instanceOf(err, WsConnectionEstablishmentError); + assert.equal((err as WsConnectionEstablishmentError).code, 401); + } + }); + + it("should include the response body as the reason in the error message", async () => { + await startHttpServer((_req, res) => { + res.statusCode = 400; + res.end("bad request body from server"); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + try { + await connection.request("Successful.Method"); + assert.fail("should have thrown WsConnectionEstablishmentError"); + } catch (err) { + assert.instanceOf(err, WsConnectionEstablishmentError); + assert.match((err as Error).message, /Reason: bad request body from server/); + } + }); + + it("should not retry connection on a 4xx unexpected response", async () => { + await startHttpServer((_req, res) => { + res.statusCode = 403; + res.end("forbidden"); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + await assert.isRejected(connection.request("Successful.Method"), WsConnectionEstablishmentError); + + assert.equal(httpRequestsCount, 1); + }); + + it("should retry connection on a non-4xx unexpected response", async () => { + await startHttpServer((_req, res) => { + res.statusCode = 500; + res.end("internal server error"); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + await assert.isRejected(connection.request("Successful.Method"), WsConnectionEstablishmentError); + + // 1 initial attempt + 3 retries (retries.count = 3) + assert.equal(httpRequestsCount, 4); + }); + + it("should fall back to 'Unknown reason' when reading the response body fails", async () => { + await startHttpServer((_req, res) => { + // Promise a 100-byte body, flush the headers (so the WS client sees an + // unexpected response), then abruptly close the socket so reading the + // body rejects. + res.writeHead(403, { "Content-Length": "100" }); + res.flushHeaders(); + res.socket?.destroy(); + }); + + connection = new TestWsConnectionProxied(httpEndpoint); + + try { + await connection.request("Successful.Method"); + assert.fail("should have thrown WsConnectionEstablishmentError"); + } catch (err) { + assert.instanceOf(err, WsConnectionEstablishmentError); + assert.match((err as Error).message, /Reason: Unknown reason/); + } + }); + }); });