diff --git a/src/sql_jsc/mysql/JSMySQLConnection.rs b/src/sql_jsc/mysql/JSMySQLConnection.rs index 26cbd93b1360..9aa95c93dd13 100644 --- a/src/sql_jsc/mysql/JSMySQLConnection.rs +++ b/src/sql_jsc/mysql/JSMySQLConnection.rs @@ -638,7 +638,8 @@ impl JSMySQLConnection { } S::Connected | S::Disconnected | S::Failed => { let queries = this.get_queries_array(); - this.connection_mut().clean_queue_and_close(None, queries); + this.connection_mut() + .clean_queue_and_close(None, queries, uws::CloseCode::Normal); } } Ok(JSValue::UNDEFINED) @@ -707,7 +708,8 @@ impl JSMySQLConnection { // `_ref` has not yet dropped, so `*p` is still live; `ParentRef` // yields a fresh `&Self` per access (R-2: every callee is `&self`). let queries = p.get_queries_array(); - p.connection_mut().clean_queue_and_close(Some(value), queries); + p.connection_mut() + .clean_queue_and_close(Some(value), queries, uws::CloseCode::Failure); p.update_reference_type(); } self.stop_timers(); diff --git a/src/sql_jsc/mysql/MySQLConnection.rs b/src/sql_jsc/mysql/MySQLConnection.rs index 423bd998bb80..53748e2f31f2 100644 --- a/src/sql_jsc/mysql/MySQLConnection.rs +++ b/src/sql_jsc/mysql/MySQLConnection.rs @@ -291,15 +291,17 @@ impl MySQLConnection { } } - pub(crate) fn close(&mut self) { - self.socket.close(uws::CloseKind::Normal); + pub(crate) fn close(&mut self, code: uws::CloseCode) { + self.socket.close(code); self.write_buffer = OffsetByteList::default(); } + /// `fail_with_js_value` passes `Failure`, the one code a TLS socket never defers (see `CloseCode`). pub(crate) fn clean_queue_and_close( &mut self, js_reason: Option, js_queries_array: JSValue, + code: uws::CloseCode, ) { // cleanup requests self.queue.clean( @@ -311,7 +313,7 @@ impl MySQLConnection { }, ); - self.close(); + self.close(code); } pub(crate) fn cleanup(&mut self) { diff --git a/src/sql_jsc/postgres/PostgresSQLConnection.rs b/src/sql_jsc/postgres/PostgresSQLConnection.rs index f575ab14d4f8..700ce8d9ee53 100644 --- a/src/sql_jsc/postgres/PostgresSQLConnection.rs +++ b/src/sql_jsc/postgres/PostgresSQLConnection.rs @@ -731,7 +731,7 @@ impl PostgresSQLConnection { &[js_error, queries], ); } - self.ref_and_close(Some(value)); + self.ref_and_close(Some(value), uws::CloseCode::Failure); // SAFETY: `self` is a live Box-allocated connection; this releases one ref. unsafe { Self::deref(self.as_ctx_ptr()) }; self.update_has_pending_activity(); @@ -1510,14 +1510,15 @@ impl PostgresSQLConnection { } } - fn ref_and_close(&self, js_reason: Option) { + /// `fail_with_js_value` passes `Failure`, the one code a TLS socket never defers (see `CloseCode`). + fn ref_and_close(&self, js_reason: Option, code: uws::CloseCode) { // refAndClose is always called when we wanna to disconnect or when we are closed if !self.socket.get().is_closed() { // event loop need to be alive to close the socket self.poll_ref.with_mut(|r| r.ref_(self.vm_ctx())); // will unref on socket close - self.socket.get().close(uws::CloseKind::Normal); + self.socket.get().close(code); } // cleanup requests @@ -1529,7 +1530,7 @@ impl PostgresSQLConnection { self.unregister_auto_flusher(); if self.status.get() == Status::Connected { self.status.set(Status::Disconnected); - self.ref_and_close(None); + self.ref_and_close(None, uws::CloseCode::Normal); } } diff --git a/test/js/sql/sql-close-pending-connection.test.ts b/test/js/sql/sql-close-pending-connection.test.ts index df35ab760a25..11d1d24e5d5f 100644 --- a/test/js/sql/sql-close-pending-connection.test.ts +++ b/test/js/sql/sql-close-pending-connection.test.ts @@ -18,6 +18,8 @@ import { SQL } from "bun"; import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tls } from "harness"; +import path from "node:path"; import { neverAnsweringServer } from "./wire-frames"; const drivers = [ @@ -101,3 +103,50 @@ test("pool scans tolerate unassigned connection slots during pool start", async server.close(); } }); + +// When the client itself gives up on a connection (connection timeout, protocol +// violation, forced close while still connecting, ...) it has to close the +// socket without waiting for the peer. Over TLS the graceful close both drivers +// used to issue from fail() sends a close_notify and keeps the socket open until +// the peer answers it, which a peer that has stopped responding never does: the +// failure was reported, but the socket stayed open and, with it, the process +// stayed alive. The fixture runs both drivers against mocks that complete the +// TLS handshake and then stop responding, and has to exit by itself once each +// client has reported the failure and each mock has seen its connection close. +// +// Test timeout: the fixture is a second debug build doing four TLS handshakes, +// and without the fix it only ends when it runs into the spawn timeout. +test("a TLS connection the client gives up on is closed at once even though the peer stopped responding", async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "sql-tls-peer-stopped-responding.fixture.ts")], + env: { ...bunEnv, MOCK_TLS_KEY: tls.key, MOCK_TLS_CERT: tls.cert }, + stdout: "pipe", + stderr: "pipe", + // Without the fix the fixture never exits; this turns that into a failure + // that shows which lines never got printed. + timeout: 20_000, + }); + const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ + stdout: stdout.trim().split(/\r?\n/).sort(), + exitCode: proc.exitCode, + signalCode: proc.signalCode, + stderr, + }).toEqual({ + stdout: [ + "mysql close: rejected with ERR_MYSQL_CONNECTION_CLOSED", + "mysql close: the mock saw the connection close", + "mysql unexpected: rejected with ERR_MYSQL_UNEXPECTED_PACKET", + "mysql unexpected: the mock saw the connection close", + "postgres close: rejected with ERR_POSTGRES_CONNECTION_CLOSED", + "postgres close: the mock saw the connection close", + "postgres unexpected: rejected with ERR_POSTGRES_UNEXPECTED_MESSAGE", + "postgres unexpected: the mock saw the connection close", + ], + exitCode: 0, + signalCode: null, + // not asserted; included so that whatever the fixture died of shows up + // in the diff + stderr: expect.any(String), + }); +}, 30_000); diff --git a/test/js/sql/sql-tls-peer-stopped-responding.fixture.ts b/test/js/sql/sql-tls-peer-stopped-responding.fixture.ts new file mode 100644 index 000000000000..19a7b595ed44 --- /dev/null +++ b/test/js/sql/sql-tls-peer-stopped-responding.fixture.ts @@ -0,0 +1,127 @@ +// Fixture for the "TLS peer stopped responding" test in +// sql-close-pending-connection.test.ts. Every driver x trigger combination +// below runs concurrently against its own mock. Each prints one line when the +// client reports the failure and one more when the mock's TCP socket sees the +// connection go away; the process then has to exit by itself, since a +// connection the client has given up on must not keep it alive. +// +// Each mock completes the STARTTLS upgrade, takes the client's startup message +// and then stops responding for good (see startTlsServerSide), so the client's +// close_notify is never answered. The client then gives the connection up +// either because the pool is force-closed while the connection is still +// waiting for the startup reply ("close") or because what the mock answered +// was a protocol violation ("unexpected"). Both triggers happen strictly after +// the mock has stopped responding, and both go through the driver's fail(), +// the same path a connection or idle timeout takes; a timeout itself is not +// used as a trigger because on a slow debug build it can fire before the TLS +// handshake is even done, at which point there is nothing to wait for. +// +// The pools in the "unexpected" scenarios are deliberately not closed: on +// mysql a close() issued after the failure closes the socket a second time, +// which happens to complete a graceful TLS close that is still waiting for +// the peer, and would hide exactly what this fixture observes. + +import { SQL } from "bun"; +import type net from "node:net"; +import { + listeningServer, + MYSQL_CLIENT_SSL, + MYSQL_DEFAULT_CAPABILITIES, + mysqlHandshakeV10, + mysqlRawPacket, + pgReadyForQuery, + pgSSLRequest, + pgSSLResponse, + startTlsServerSide, +} from "./wire-frames"; + +type Driver = "postgres" | "mysql"; +type Trigger = "close" | "unexpected"; + +// The harness certificate for 127.0.0.1, handed over by the test: importing +// "harness" here would cost this debug-build subprocess several seconds. +const { MOCK_TLS_KEY: key, MOCK_TLS_CERT: cert } = process.env; +if (!key || !cert) throw new Error("MOCK_TLS_KEY and MOCK_TLS_CERT must be set by the test that spawns this fixture"); +const tlsCredentials = { key, cert }; + +// Answers to the startup message that the client rejects: a ReadyForQuery before +// any Authentication message, resp. an auth reply whose header byte no auth +// packet uses. +const unexpectedReply: Record = { + postgres: pgReadyForQuery(), + mysql: mysqlRawPacket(3, Buffer.from([0x42])), +}; + +// Length of the plaintext the client sends before TLS once `buffered` holds all +// of it: postgres sends the 8-byte SSLRequest, mysql one packet (its SSLRequest) +// in reply to our greeting. undefined while more bytes are needed. +function preludeLength(driver: Driver, buffered: Buffer): number | undefined { + if (driver === "postgres") { + return buffered.length >= pgSSLRequest().length ? pgSSLRequest().length : undefined; + } + if (buffered.length < 4) return undefined; + const length = 4 + (buffered[0] | (buffered[1] << 8) | (buffered[2] << 16)); + return buffered.length >= length ? length : undefined; +} + +/** Resolves once the mock has taken the client's startup message and stopped responding. */ +function mockThatStopsResponding(driver: Driver, trigger: Trigger, raw: net.Socket): Promise { + const stoppedResponding = Promise.withResolvers(); + // Once the client closes for real, the RST it sends surfaces here. + raw.on("error", () => {}); + if (driver === "mysql") { + raw.write(mysqlHandshakeV10({ capabilities: MYSQL_DEFAULT_CAPABILITIES | MYSQL_CLIENT_SSL })); + } + let buffered = Buffer.alloc(0); + const onPlaintext = (chunk: Buffer) => { + buffered = Buffer.concat([buffered, chunk]); + const length = preludeLength(driver, buffered); + if (length === undefined) return; + raw.removeListener("data", onPlaintext); + if (driver === "postgres") raw.write(pgSSLResponse("S")); + const peer = startTlsServerSide(raw, buffered.subarray(length), tlsCredentials); + // The first decrypted bytes are the StartupMessage / HandshakeResponse. + peer.secure.once("data", () => { + peer.stopReading(); + if (trigger === "unexpected") peer.secure.write(unexpectedReply[driver]); + stoppedResponding.resolve(); + }); + }; + raw.on("data", onPlaintext); + return stoppedResponding.promise; +} + +async function scenario(driver: Driver, trigger: Trigger) { + const mockStoppedResponding = Promise.withResolvers(); + const mockSawClose = Promise.withResolvers(); + const { port, server } = await listeningServer(raw => { + raw.on("close", () => mockSawClose.resolve()); + mockThatStopsResponding(driver, trigger, raw).then(mockStoppedResponding.resolve); + }); + const sql = new SQL({ + url: `${driver}://user:password@127.0.0.1:${port}/db`, + max: 1, + tls: { ca: tlsCredentials.cert }, + }); + const query = sql`select 1`.then( + () => "resolved", + (err: any) => `rejected with ${err?.code ?? err}`, + ); + if (trigger === "close") { + await mockStoppedResponding.promise; + // The string form closes at once even though a query is waiting; a numeric + // 0 currently waits for it (https://github.com/oven-sh/bun/issues/32038). + await sql.close({ timeout: "0" }); + } + console.log(`${driver} ${trigger}: ${await query}`); + await mockSawClose.promise; + console.log(`${driver} ${trigger}: the mock saw the connection close`); + server.close(); +} + +await Promise.all([ + scenario("postgres", "close"), + scenario("postgres", "unexpected"), + scenario("mysql", "close"), + scenario("mysql", "unexpected"), +]); diff --git a/test/js/sql/wire-frames.ts b/test/js/sql/wire-frames.ts index 3903b1c7364f..8051be14fe5a 100644 --- a/test/js/sql/wire-frames.ts +++ b/test/js/sql/wire-frames.ts @@ -5,6 +5,8 @@ // Buffer.alloc / writeInt32BE sequences. import net from "node:net"; +import { Duplex } from "node:stream"; +import tls from "node:tls"; // --------------------------------------------------------------------------- // Server helpers shared by every fault-injection test. @@ -43,6 +45,46 @@ export async function neverAnsweringServer(): Promise<{ port: number; server: ne return { port, server, accepted: first.promise }; } +/** + * Server side of a STARTTLS-style upgrade (Postgres once the SSLRequest has been + * answered with 'S', MySQL once the client's SSLRequest packet has arrived) for + * a mock that is going to stop responding later on. The TLS engine runs over a + * Duplex that this helper feeds with the bytes read from `raw`, so after + * `stopReading()` everything the client sends is dropped unread while the TCP + * connection stays open: in particular the client's close_notify is never + * answered, which is what a peer that has hung looks like. (A TLS socket + * wrapping the connection itself would answer it natively, and pause() does + * not prevent that.) `leftover` is whatever arrived behind the plaintext + * prelude, i.e. the start of the client's ClientHello. + */ +export function startTlsServerSide( + raw: net.Socket, + leftover: Buffer, + credentials: { key: string; cert: string }, +): { secure: tls.TLSSocket; stopReading(): void } { + let reading = true; + const bridge = new Duplex({ + read() {}, + // Errors on `raw` are the caller's to observe; the bridge itself never fails. + write(chunk, _encoding, callback) { + raw.write(chunk); + callback(); + }, + }); + raw.on("data", chunk => { + if (reading) bridge.push(chunk); + }); + if (leftover.length) bridge.push(leftover); + const secure = new tls.TLSSocket(bridge, { isServer: true, ...credentials }); + secure.on("error", () => {}); + return { + secure, + stopReading() { + reading = false; + }, + }; +} + // --------------------------------------------------------------------------- // PostgreSQL frontend/backend protocol — https://www.postgresql.org/docs/current/protocol-message-formats.html // ---------------------------------------------------------------------------