Tear down the connection when session setup fails in Connection.open() (#20)

* Tear down the connection when session setup fails in Connection.open()

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>

* 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>

---------

Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>

Author: PDGGK

src/connection/Connection.ts

@@ -92,6 +92,30 @@ 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().
+      // 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;
+        }
+      } 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;
     }
   }

tests/unit/Connection.test.ts

@@ -129,4 +129,68 @@ 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);
+
+    // 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");
+  });
 });