From b038083f0f17331ccaec3fa3b83e3383c8c328ca Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Wed, 22 Jul 2026 05:26:05 +1000 Subject: [PATCH 1/2] Tear down the connection when session setup fails in Connection.open() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the TCP connection is established (createConnection + createClient) and its 'error'/'close' listeners are registered, if openSession() or requestStatementId() rejects (bad credentials, timeout, or a non-200 status), the catch only logged the error and rethrew — leaking the open socket and its listeners. In the pool this propagates through init() with no close(), so each failed connect attempt leaks a socket. Mirror close()'s teardown (removeAllListeners + destroy/end + null the refs + isConnected=false) in the catch before rethrowing, and add a regression test. Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com> --- src/connection/Connection.ts | 14 ++++++++++++++ tests/unit/Connection.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/connection/Connection.ts b/src/connection/Connection.ts index 54d0b2b..f17de81 100644 --- a/src/connection/Connection.ts +++ b/src/connection/Connection.ts @@ -92,6 +92,20 @@ export class Connection { this.isConnected = true; } catch (error) { logger.error("Failed to connect:", error); + // Tear down the half-open connection so its socket and event listeners + // don't leak when session setup (openSession/requestStatementId) fails + // after the TCP connection was already established. Mirrors close(). + if (this.connection) { + this.connection.removeAllListeners(); + if (typeof this.connection.destroy === "function") { + this.connection.destroy(); + } else { + this.connection.end(); + } + this.connection = null; + } + this.client = null; + this.isConnected = false; throw error; } } diff --git a/tests/unit/Connection.test.ts b/tests/unit/Connection.test.ts index 26eca2d..fe4f21c 100644 --- a/tests/unit/Connection.test.ts +++ b/tests/unit/Connection.test.ts @@ -129,4 +129,36 @@ describe("Connection", () => { await connection.close(); }); + + test("Should tear down the socket when session setup fails", async () => { + // openSession rejects after the TCP connection was established. + thriftMock.createClient.mockReturnValueOnce({ + openSession: jest.fn((_req: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(new Error("auth failed"), null), + ), + requestStatementId: jest.fn((_sid: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(null, 456), + ), + closeSession: jest.fn((_req: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(null, { status: { code: 200 } }), + ), + }); + + const config: InternalConfig = { + host: "localhost", + port: 6667, + username: "root", + password: "bad", + enableSSL: false, + sqlDialect: "tree", + }; + const connection = new Connection(config); + + await expect(connection.open()).rejects.toThrow(); + + // The half-open connection must be torn down (mirrors close()); the buggy + // catch only logged + rethrew, leaking the socket and its listeners. + expect(thriftMock.__mockConnection.removeAllListeners).toHaveBeenCalled(); + expect(thriftMock.__mockConnection.destroy).toHaveBeenCalled(); + }); }); From fcf865d23e921546744a5abefc0fbc40c11d7ccd Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:25:41 +1000 Subject: [PATCH 2/2] Reset session ids and guard teardown in Connection.open() error path Address review feedback on #20: on a failed session setup the open() catch now (1) clears sessionId/statementId so getSessionId() cannot return a stale id after openSession succeeds but requestStatementId fails, and (2) wraps the socket teardown in its own try/catch so a cleanup failure cannot mask the original error that is rethrown below. Strengthen the regression tests: assert the original setup error surfaces rather than a teardown error, and add a case asserting that a failure after openSession clears the session id. Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com> --- src/connection/Connection.ts | 24 +++++++++++++++++------- tests/unit/Connection.test.ts | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/connection/Connection.ts b/src/connection/Connection.ts index f17de81..38969d0 100644 --- a/src/connection/Connection.ts +++ b/src/connection/Connection.ts @@ -95,16 +95,26 @@ export class Connection { // Tear down the half-open connection so its socket and event listeners // don't leak when session setup (openSession/requestStatementId) fails // after the TCP connection was already established. Mirrors close(). - if (this.connection) { - this.connection.removeAllListeners(); - if (typeof this.connection.destroy === "function") { - this.connection.destroy(); - } else { - this.connection.end(); + // Guard the teardown itself so a cleanup failure can't mask the + // original error that we rethrow below. + try { + if (this.connection) { + this.connection.removeAllListeners(); + if (typeof this.connection.destroy === "function") { + this.connection.destroy(); + } else { + this.connection.end(); + } + this.connection = null; } - this.connection = null; + } catch (cleanupError) { + logger.warn("Error during connection teardown:", cleanupError); } this.client = null; + // Mirror close(): clear session/statement ids so a failed setup does + // not leave a stale sessionId reachable via getSessionId(). + this.sessionId = null; + this.statementId = null; this.isConnected = false; throw error; } diff --git a/tests/unit/Connection.test.ts b/tests/unit/Connection.test.ts index fe4f21c..7d5b01b 100644 --- a/tests/unit/Connection.test.ts +++ b/tests/unit/Connection.test.ts @@ -154,11 +154,43 @@ describe("Connection", () => { }; const connection = new Connection(config); - await expect(connection.open()).rejects.toThrow(); + // The original setup error must surface, not be masked by the teardown. + await expect(connection.open()).rejects.toThrow("auth failed"); // The half-open connection must be torn down (mirrors close()); the buggy // catch only logged + rethrew, leaking the socket and its listeners. expect(thriftMock.__mockConnection.removeAllListeners).toHaveBeenCalled(); expect(thriftMock.__mockConnection.destroy).toHaveBeenCalled(); }); + + test("Should clear sessionId when statement setup fails after openSession", async () => { + // openSession succeeds (sets sessionId), then requestStatementId rejects. + thriftMock.createClient.mockReturnValueOnce({ + openSession: jest.fn((_req: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(null, { status: { code: 200 }, sessionId: 123 }), + ), + requestStatementId: jest.fn((_sid: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(new Error("statement setup failed"), null), + ), + closeSession: jest.fn((_req: unknown, callback: (e: Error | null, r: unknown) => void) => + callback(null, { status: { code: 200 } }), + ), + }); + + const config: InternalConfig = { + host: "localhost", + port: 6667, + username: "root", + password: "root", + enableSSL: false, + sqlDialect: "tree", + }; + const connection = new Connection(config); + + await expect(connection.open()).rejects.toThrow("statement setup failed"); + + // The failed setup must not leave a stale sessionId reachable (mirrors + // close()); getSessionId() throws once the id is cleared. + expect(() => connection.getSessionId()).toThrow("Session is not open"); + }); });