Skip to content

sql: implement Query.cancel() for postgres - #33370

Open
robobun wants to merge 42 commits into
mainfrom
farm/4be26dfa/postgres-query-cancel
Open

robobun wants to merge 42 commits into
mainfrom
farm/4be26dfa/postgres-query-cancel

Conversation

@robobun

@robobun robobun commented Jul 5, 2026 •

Copy link
Copy Markdown
Collaborator

Merge #32149 first (Downsides). Open question for a maintainer: Downsides.

Problem

Fix

  • PostgresSQLConnection::open, split from createConnection, lets native code open a connection.
  • For the running query (the queue head), PostgresSQLConnection::cancel opens a second connection to the session's peer address, encrypted when the session is. It sends a CancelRequest unless the session runs another query. The query rejects with 57014.
  • A query that is not written yet, or written behind the head, rejects with ERR_POSTGRES_QUERY_CANCELLED (reject_in_flight, sql(postgres): reject only the query whose row the client cannot decode #43187).
  • Verified: test/js/sql/postgres-query-cancel.test.ts (31 tests, 29 fail without it), PostgreSQL 17.11 plain and TLS, test/js/sql/. Self-reviewed: 26 concerns raised, 21 addressed (Notes).

Background

  • A backend reads nothing while it runs a query. A CancelRequest names the backend process, so it only stops the queue head.
  • Considered a node:net socket: plaintext on a TLS session, no route to [::1], cancel key in JavaScript.

Downsides

  • Without sql: make transaction.close({ timeout }) roll back when pending queries drain before the timeout #32149, tx.close({timeout}) leaves an unhandled 57014 rejection when it cancels a running query (Notes).
  • A CancelRequest that the server handles after its query ended stops that connection's next query: 45 to 149 of 300 rounds on a loaded server, and the ROLLBACK of tx.close() in 2 of 400 (Notes). Open question: should Bun wait for the server to close the cancel connection, like libpq, pgx, pgjdbc and asyncpg?
  • Cost: 8 bytes and 2 writes per query, 32 bytes per connection. Release text: +7,424 bytes at 950a49c (Notes).
Notes

Repro

const q = sql`select pg_sleep(2)`.execute();
q.cancel();
await q; // main: resolves after 2 s, with rows. This PR: rejects with SQLSTATE 57014.

What cancel() does, by request state

do_cancel finds the connection that the query was dispatched to and calls PostgresSQLConnection::cancel. The connection owns the FIFO and the request counters, so it decides. A CancelRequest stops whatever the backend process runs now, and that is the request at the head of the FIFO.

  • Pending: no Bind, Execute or Query of the request is written. finish_request takes it out of pending_requests, the count that gates pipelining at enqueue time. on_js_error marks it Fail, and advance() discards it before it writes anything.
  • Written and the head of the FIFO (Binding, Running, PartialResponse): send_cancel_request opens the cancel connection. The backend answers the session with 57014, and the existing ErrorResponse path rejects the query. If that error arrives while the statement is still in Parsing, the statement goes back to Pending and is not failed (see below).
  • Written behind the head (pipelined): a CancelRequest would stop the query ahead of it. Each pipelined unit has its own Flush and Sync, so the backend answers it anyway. reject_in_flight rejects the promise, sets discard_response, and leaves the FIFO entry in place. The connection takes the replies of that request, rows or an error, and ReadyForQuery retires it. The error has the hint The server already received this query and still runs it. Bun discards the result., the way sql(postgres): reject only the query whose row the client cannot decode #43187 marks an undecodable row, so a caller can tell it from a query that was never sent and does not retry a write blindly.
  • No connection on the query yet (it is not dispatched, or do_run is on the stack): do_cancel marks the query, and do_run does not send it (see the next section).

reject_in_flight is main's on_undecodable_row from #43187 under a name that fits both callers.

A cancel() from the code of a parameter

The connection turns a parameter into its wire form while it encodes the Bind. An object does that with its own code (toString, toJSON), and that code can call cancel() on the query that it belongs to. main is safe from this only because cancel() does nothing there. Three changes make it safe here:

  • RequestCounter::Pending records on the request that it is in pending_requests, as Nonpipelinable and Pipelined already do for the other two counters. finish_request reads that record, so a second call does nothing. Before, advance() took the request out of the count ahead of the encode and cancel() took it out again.
  • encode_request, the one caller of the batch writers, fails the batch when the request was rejected while its parameters were converted. atomically then takes the batch out of the write buffer, so nothing of the request reaches the wire.
  • A cancel() that arrives while do_run is on the stack finds no connection on the query. do_cancel marks the query, and do_run throws ERR_POSTGRES_QUERY_CANCELLED and enqueues nothing.

Against PostgreSQL 17.11, debug builds, with a parameter whose toString() calls cancel() on its query or close() on its reserved connection:

case 950a49c now
cancel(), first run of the statement (advance() encodes the Bind) panic: pending_requests underflow rejects with ERR_POSTGRES_QUERY_CANCELLED, and the connection runs the next query
cancel(), prepared statement (do_run encodes the Bind) the cancel is lost: the query runs and resolves rejects with ERR_POSTGRES_QUERY_CANCELLED
reserved.close(), first run panic: pending_requests underflow rejects, and the process exits
reserved.close(), prepared statement the query rejects, and the process never exits rejects, and the process exits

The last row is also a hang on the released build 367d939 (killed after 60 s): do_run enqueues the query on the connection that its own parameter closed. Here reserved.close() cancels the query first, so do_run stops. A parameter that closes the connection in another way than through close() is not covered by this PR.

The cancel connection

PostgresSQLConnection::send_cancel_request runs on the session. It builds a ConnectParams and calls PostgresSQLConnection::open (see the split below), so the cancel connection is an ordinary native connection with the flag IS_CANCEL_REQUEST. It differs from a session in five places, each behind that flag:

  • send_startup_message writes the CancelRequest in place of the StartupMessage. First it asks its CancelTarget, which holds the session and the query, whether the packet can still stop the right query (see the late CancelRequest below). It reads the cancel key from the session at that time.
  • setup_tls does not call start() for it. on_handshake does. So the question above is asked when the connection can write, not when the handshake starts.
  • start does not restart the connection timer, so there is one budget from dial to hang-up. The budget is the connection timeout of the session, and at most 5 s.
  • on_data after the write fails the connection. A server answers a CancelRequest by hanging up, so a server that answers must not turn this into a session.
  • It is not counted as a postgres_connections use.

It always ends through fail. So ref_and_close closes it by main's rule for a failed connection (#44327): it sends its close_notify and FIN, and does not wait for the peer. An earlier revision of this PR had a close rule of its own for it.

The cancel key is an Option: a server that sent no BackendKeyData has none, and then cancel() on a running query does nothing. A process id of 0 is a key like any other.

What it takes from the session:

  • The target is the peer address and port of the session's socket (getpeername), or its unix socket path. A host name can resolve to another server on a second lookup. libpq's PQcancel also dials the stored address.
  • The certificate is checked against the host name of the session (tls_config.server_name), not against the dialed address. With a certificate that names only localhost, a session to localhost connected through ::1, and its cancel connection completed TLS with SNI localhost and delivered the CancelRequest. A session addressed as 127.0.0.1 was refused with that certificate.
  • sslmode is derived from what the session negotiated. A session with TLS makes a cancel connection that requires TLS, also under prefer. A session without TLS makes one with disable. The SSL_CTX is shared (SSL_CTX_up_ref) and the TLS options are copied, so SNI and certificate verification are the session's.
  • The socket group is the one of the session's socket, so the connection belongs to the same owner.

The cancel key never becomes a JavaScript value. handle.cancel() returns nothing, and the adapter has no cancel method. Measured with net.connect, net.createConnection, tls.connect, Bun.connect and net.Socket.prototype.{connect,write,end} patched before new SQL(): 0 patched calls during cancel(), and the query rejects with 57014.

An earlier revision sent the packet from the adapter on a node:net socket. On that revision postgres://postgres@[::1]:5432 connected but cancel() did nothing (select pg_sleep(3) resolved after 3019 ms), because URL parsing keeps the brackets and only the native connect strips them. On a TLS session it sent the 16 bytes in the clear.

The split of createConnection (the former #43942)

This PR was a stack of two. #43942 held the split alone, and its review is there. Its commits are in this branch unchanged (819cd9c).

call, the host function of createConnection, parses its JavaScript arguments and calls PostgresSQLConnection::open(global, group, ConnectParams). open allocates the connection, dials it and wraps it for JS. It returns the dial error of uws, and call throws it as the same exception as before. ConnectionStrings::new builds the one buffer that holds user, password, database, options and path, and the five slices into it. It is the only way to make them, so a second caller cannot pass slices that point elsewhere.

Three statements change for createConnection:

  • call looks up the socket group before open allocates the connection. It did so after the allocation. No JavaScript runs between the two.
  • call checks user, password, database and path for null bytes before it copies them into the buffer. It did so after the copy and then freed the buffer. The error is the same.
  • ConnectionStrings::new writes the terminator after each string (count_z, append_z). main reserves that byte and does not write it, so its buffer ends with 5 bytes that were never written. The allocation has the same size. The slices do not include the terminator, so the startup message has the same bytes.

Considered a second constructor for the cancel connection. It copies the 50-line struct initialiser, and the two copies drift.

Time bound

The cancel connection has one timer from its dial to its end. The budget is the connection timeout of the session, and at most 5 s.

  • A server that takes the CancelRequest and then neither answers nor hangs up: with connectionTimeout: 1 the client closes the cancel connection after 1.1 s (test the cancel connection ends when the server stays silent after the CancelRequest, debug build).
  • A server that drops the SYN of the cancel connection (a listener with a full backlog), while the session ends right after cancel(): with connectionTimeout: 1 the process exits 0.97 s after the cancel (2 runs, debug build). Since usockets: tell a socket's holder it is gone exactly once, whoever closes it and whether or not it ever opened #44327 uSockets reports the close of a socket that never opened, so this case needs no code in this PR.

A statement whose Parse is cancelled

The first run of a statement with no parameters sends Parse, Bind, Execute and Sync together, and the request is the running one. A backend that waits for a lock waits inside Parse. A cancel then gives an ErrorResponse with no ParseComplete before it. main fails the statement on every error in Parsing, and each query that shares the statement rejects with that error.

An error with SQLSTATE 57014 in Parsing now sets the statement back to Pending. The next query that shares it sends the Parse again. Against PostgreSQL 17.11, with another session holding ACCESS EXCLUSIVE on the table, two select v from t on one connection and cancel() on the first:

first query second query
before 57014 57014
now 57014 its row, when the lock is released

The rule reads the SQLSTATE only, so a statement_timeout inside Parse also stops failing the queries that share the statement.

Known limit: a CancelRequest that arrives before its query

The server ignores a cancel while it still reads the request. Bun sends the CancelRequest at once, also when bytes of the request are still in its write buffer. PostgreSQL 17.11 on loopback, release build, select pg_sleep(1), length($2) on an idle connection with a warm statement, execute() and cancel() in the same tick, 5 runs each:

size of the parameter result
64 KB 5 rejected with 57014
256 KB 5 rejected with 57014
1 MB 4 rejected, 1 lost
2 MB 2 rejected, 3 lost
4 MB 5 lost
8 MB 5 lost
8 MB, cancel() 300 ms later 5 rejected with 57014

"Lost" means that the query ran its full second and resolved. The 1 MB and 2 MB rows are a race, so their split is not a stable number. A simple query and a prepared query with a small parameter gave 20 of 20 rejected, on the debug and on the release build.

This PR does not fix it. A wait for the write buffer to drain is wrong: the buffer also holds the bytes of pipelined queries behind the head, and the server does not read those while it runs the head. A fix has to record when the bytes of one request have left the buffer, at every place that writes a request.

A late CancelRequest, and the open question

A CancelRequest names the backend process. The server acts on it after the postmaster has accepted the cancel connection and started a process for it. If the query has ended by then, the signal stops the query that the backend runs at that moment. A signal that meets an idle backend does nothing. Bun cannot take the packet back after the write.

The numbers in this section are from release builds of 950a49c and earlier revisions, as named. The code of this path has not changed since.

What this PR does. The cancel connection holds the session and the query (CancelTarget). When it can write, which on a TLS connection is the end of the handshake, it asks the session. It writes the packet unless the session is still connected and its backend no longer runs that query. In that case it hangs up and sends nothing. A session that closed gets the packet (see the next section).

What this PR does not do. The session writes its next request as soon as the backend is ready, also while a cancel connection is open. The open question at the end of this section is about that.

The size of the window on this machine. PostgreSQL 17.11 on loopback, release builds, 16 cores, load average 450 to 550 from other work. A raw protocol client on node:net, with no use of Bun.SQL, 200 cancels of a running pg_sleep(5), 2 runs:

step median p90 max
dial of the cancel connection 0.4 ms 0.6 ms 109 ms
from the dial until the ErrorResponse on the session 54 ms, 52 ms 248 ms, 216 ms 668 ms, 416 ms
from the dial until the server closes the cancel connection 132 ms, 117 ms 347 ms, 292 ms 759 ms, 689 ms
for comparison: pg_cancel_backend() on a second session, until the ErrorResponse 0.45 ms 0.5 ms 22 ms
for comparison: a new session, until ReadyForQuery 18 ms 265 ms 1449 ms

cancel() of this PR in the same measurement: a median of 46 to 95 ms in 5 runs, and 1.7 ms in the best case. So the time is the server's. It goes into the new connection and not into the signal. A server without this load has a smaller window.

How often the wrong query is stopped. A loop of 300 rounds per run on a pool of 1: A = select 1, A.cancel() in the same tick, await A, then B = select pg_sleep(0.01). The number is how often B, which nobody cancelled, rejected with 57014:

client B rejected, of 300
main e70cca7, where cancel() does nothing 0
358721d, before the check at write time 46, 181, 165
this PR, 950a49c 94, 148, 149, 45
raw protocol client, B written when A has ended 62, 57, 103
raw protocol client, B written when the server has closed the cancel connection 0, 0, 0

With B already queued behind A (select pg_sleep(0.03), simple protocol), 300 rounds per run:

first query 358721d 950a49c
select 1, cancel() in the same tick 129, 300, 296 68 to 287 in 13 runs
pg_sleep(0.002), cancel() in the same tick 149, 87, 44 62, 22, 88
pg_sleep(0.005), cancel() 3 ms later 155, 63, 78 56, 78, 80

What the numbers say:

  • The check at write time changes nothing that these loops can show. The dial takes 0.4 ms, so the packet is nearly always written while A runs. The server then needs tens of milliseconds, and A has ended. On the debug build, which is slower than the server, the check cut the first row of the second table from 115 and 110 to 15, 10 and 12.
  • The check decides the case of a slow dial. PostgreSQL 17.11 with ssl = on, a TCP hop that holds the TLS ClientHello of the cancel connection for 300 ms, A = pg_sleep(0.15), B = pg_sleep(1) behind it, cancel() on A after 30 ms. On c55aa7a, which asked the session before the handshake, A resolved and B rejected with 57014. On 950a49c both resolve. With A = pg_sleep(3), A rejects with 57014 and B resolves, on both builds.
  • The loops cancel faster than this server accepts connections. Without the wait, the raw client had up to 236 cancel connections open at once, and they lived for a median of 2.2 to 4.7 s. With this PR the accept queue of the server reached 85 to 201 entries, and its limit is 200. 4 of 17 runs of the first row of the second table then failed at their first connect with ERR_POSTGRES_CONNECTION_TIMEOUT after 5 s, behind the cancel connections of the run before. 40 connects with no cancel before them took 7 to 21 ms.
  • With the wait, the raw client never had more than 1 cancel connection open. B started later by a median of 92, 80 and 5 ms in the three runs (p99 0.8 to 2.8 s).

tx.close(). It calls cancel() on the queries of the transaction and then sends ROLLBACK on the same connection, with nothing between the two. 200 rounds each of sql.begin(async tx => { tx.unsafe("select pg_sleep(...)").execute(); await tx.close(); }) with a query of 20 ms and of 5 ms, debug build of a46f129, PostgreSQL 17.11: in 1 round of each 200 the CancelRequest stopped the ROLLBACK, and tx.close() rejected with 57014. sql.begin then sent COMMIT, the server answered it with the ROLLBACK tag because the transaction was aborted, and the next query on the connection ran in all 400 rounds. On the released build 367d939, where cancel() does nothing, tx.close() resolved in 100 of 100 rounds.

What other clients do, as read in their sources:

  • libpq PQcancel (fe-cancel.c), pgx PgConn.CancelRequest (pgconn/pgconn.go) and pgjdbc QueryExecutorBase.sendQueryCancel write the packet and then read until the server closes the connection. Only then does the caller continue. The comment in libpq: "Without this delay, we might issue another command only to find that our cancel zaps that command instead of the one we thought we were canceling."
  • asyncpg keeps a connection out of its pool while a cancel for it is in progress (PoolConnectionHolder.release in asyncpg/pool.py waits for it), and it ends a connection whose CancelRequest failed.
  • node-postgres Client.cancel and postgres.js cancel write the packet and do not hold the session. postgres.js returns a promise that settles when the cancel connection closes. Its README says that the race can cancel another query, and that this is fine for long queries.
  • On Bun.SQL: add ability to cancel query using AbortSignal #23175 the maintainer of Bun.SQL names this hazard ("other queries that are also executing in the same connection could also be canceled"), points to the postgres.js way, and says that he wants to look for a better solution.

The open question. Should a session write no new request while one of its cancel connections is open?

  • For: a cancel for one query then never stops a request that was not yet written. That includes the ROLLBACK of tx.close(). A session has at most 1 cancel connection open, so cancels cannot queue up at the server.
  • Against: each cancel() of a running query delays the next request of that connection until the server has closed the cancel connection. On this machine that was a median of 5 to 92 ms. The limit is the budget of the cancel connection: the connection timeout of the session, and at most 5 s. A request that is already written behind the head cannot be held back and stays exposed.
  • This PR does not build it. It changes when a session sends, so a maintainer should decide it first. It needs a count of open cancel connections on the session, a test of that count in advance() and in the writes at enqueue time, and a release on every end of the cancel connection.

A session that closes while its cancel connection dials

reserved.close() calls cancel() on each query of the reserved connection and then closes the connection. The backend still runs the query, so the CancelRequest has to go out after the session is gone. The CancelTarget holds the session itself. A session that is not connected any more always gets the packet: no other query of this client can follow on it.

A reserved connection runs pg_sleep(6), then reserved.close(). Time until pg_stat_activity shows that the backend stopped the query, release builds, 2 runs each:

build the backend stops after
main e70cca7 5987 ms, 6041 ms (the query runs to its end)
358721d, before the check at write time 160 ms, 131 ms
c55aa7a, which asked through the query 5994 ms, 6069 ms
950a49c 125 ms, 110 ms

c55aa7a reached the session through the link from the query to its connection. The close clears that link, so it found no session and sent nothing. 100 rounds of reserve, query and close on the debug build of 950a49c: 0 backends left in pg_sleep, connections 3 to 3, open file descriptors 11 to 11.

Known limit: cancel while only the Parse is on the wire

The first run of a statement with parameters writes Parse, Describe and Sync and waits for the parameter types before it writes Bind and Execute. The request is Pending in that window. cancel() rejects it locally and sends no CancelRequest: Parse normally ends within one round trip, so a cancel would mostly arrive late and stop the next query on that backend. A backend that is blocked inside Parse (a lock wait) stays blocked until the lock clears, as on main. The connection serves the next request as soon as the backend answers.

Known limit: a link-local IPv6 peer

us_socket_remote_address returns the peer address without its scope. For a session to fe80::1%eth0 the cancel connection dials fe80::1, which does not reach the peer, and cancel() does nothing, as on main. Not tested: the machine has no link-local address.

A query that is cancelled before it runs

Not in this PR. #41492 makes that change in query.ts for every adapter, with its tests. Until it merges, a query that is cancelled before it was started never settles, as on main. An earlier revision of this PR carried the same query.ts hunk. query.ts is main's again.

close() and close({ timeout }) with a running query

close({ timeout }) in src/js/bun/sql.ts waits on the queries in its scope with Promise.all(...).finally(...). A query that rejects during the wait leaves a rejected promise with no handler. On main that needs a query that fails by itself. With this PR the cancel() that the timeout calls makes a running query reject. #32149 handles that rejection for a transaction.

Against PostgreSQL 17.11, max: 1, a running pg_sleep(3) whose own promise has a handler. The first column is the released build 367d939, the second is the debug build of a46f129:

case without this PR this PR
reserved, close({ timeout: 0.3 }) query rejects with ERR_POSTGRES_CONNECTION_CLOSED, 1 unhandled rejection the same
reserved, close() query rejects with ERR_POSTGRES_CONNECTION_CLOSED, 0 unhandled the same, and the backend stops the query
transaction, close({ timeout: 0.3 }) cancel() does nothing, close waits 3 s for the query, 0 unhandled query rejects with 57014, 1 unhandled rejection
transaction, close() the query runs its 3 s and resolves, sql.begin rejects with ERR_POSTGRES_CONNECTION_CLOSED the query rejects with 57014, sql.begin rejects the same way

In all 8 runs the pool ran its next query. The third row is the reason for the first line of this description. On an earlier revision of this branch, with the sql.ts diff of #32149 applied, that case had no unhandled rejection and rolled back.

tx.close() cancels every query in the scope of the transaction, and the SAVEPOINT, RELEASE and ROLLBACK TO statements are in that scope too (run_internal_transaction_sql). One that waits behind a running query is rejected and never sent. One that the backend runs gets a CancelRequest. On main they ran to their end.

GC edge

do_cancel needs the connection, so the query wrapper caches the connection it was dispatched to. That slot is a strong edge. allow_gc, reject and on_write_fail clear it, so a settled Query that user code keeps does not pin the connection. The test a settled query does not keep its connection alive holds 40 settled queries of 10 pools. Without the clear in reject it finds 11 connections alive after GC.

A cancel connection holds a reference to its session and to the query (CancelTarget) from cancel() until it writes the packet or fails. send_startup_message and fail_with_js_value drop it. The longest hold is the budget of the cancel connection.

Merges with main

  • A dead-code sweep on main emptied BackendKeyData and removed the field on PostgresSQLConnection. This PR restores process_id, secret_key and the field, because cancel_request() needs them.
  • wire-frames.ts: main has the Parse, Bind and ParameterDescription helpers. This PR adds only pgCancelRequest and pgBackendKeyData.
  • main faac63e has usockets: tell a socket's holder it is gone exactly once, whoever closes it and whether or not it ever opened #44327: uSockets tells the holder of a socket that never opened when that socket closes. An earlier revision of this PR released the event loop for that case in ref_and_close, and had a session test for it. Both are gone: the merge takes main's ref_and_close, and main has the test.

Open PRs that touch the same code

Checked with git merge-tree against a46f129, which contains main faac63e. Each conflict is for the PR that lands second.

Tests

The server is a scripted wire server. The tests assert the exact CancelRequest bytes, and assert that none are sent where none may be sent. That needs a server that answers on demand. The last test is the exception: it runs against a real PostgreSQL server (describeWithContainer), which is the judge of the bytes.

In postgres-query-cancel.test.ts, 31 tests:

  • The running query: cancel() on an extended and on a simple query sends Int32(16) Int32(80877102) Int32(pid) Int32(secret) on a second connection and rejects with 57014 (2). It reaches a server at an IPv6 literal and on a unix socket (2).
  • TLS: on a session under verify-full, verify-ca, require and prefer, both connections send only the 8-byte SSLRequest in the clear, and the CancelRequest arrives inside TLS (4). A cancel connection that is told N sends nothing, under prefer and under require (2). One that meets a certificate the session's CA did not sign sends nothing (1). The certificate is checked against the host name of the session (1).
  • The end of the cancel connection: it closes when the server answers it like a session (1), and when the server stays silent after the packet (1). The process exits when its dial never completes (1, not on Windows and musl: the fixture loads glibc or libSystem for a raw listener).
  • The cancel key: no connection is opened when the server sent no BackendKeyData (1). A process id of 0 is sent like any other (1).
  • The query ended or the session closed during the dial: nothing is sent when the query ended, with the server holding its answer to the SSLRequest or holding the TLS handshake (2). The packet is still sent when the session closed (1).
  • The statement: a cancel during Parse does not fail the queries that share the statement (1). A cancel while only the Parse of the query is on the wire rejects it at once, and the statement is prepared for the next run (1).
  • Not the head: a queued query is rejected with no hint and the running one keeps its rows (1). The connection still pipelines afterwards (1). A pipelined query is rejected with the hint, and both replies are consumed (1). A pipelined query that the server answers with an error leaves the connection in sync (1). The queries that are pipelined behind a cancelled head get their rows (1).
  • The code of a parameter calls cancel(): the query rejects and no Bind of it reaches the server, for a first run, for a prepared statement, and with prepare: false (2).
  • GC: a settled query does not keep its connection alive (1).
  • A real server: cancel() stops a running pg_sleep, seen in pg_stat_activity, and the connection runs the next query (1).

On the released build 367d939, 29 of the 31 tests fail. The other 2 pass there because that build opens no cancel connection: the one for a server with no cancel key, and the one for the dial that never completes. The scenario of the first test for the code of a parameter stops the debug build of 950a49c with panic: pending_requests underflow. With this PR all 31 pass on the debug build (3570e33).

End to end, PostgreSQL 17.11

Debug build of a46f129. The head cancel is pg_sleep(5), cancelled after 300 ms. The times are the server's under a load average of 800:

[::1] head cancel:      rejected:57014 after 220 ms | next: 42
127.0.0.1 head cancel:  rejected:57014 after 1826 ms | next: 42
localhost head cancel:  rejected:57014 after 711 ms | next: 42
pipelined: b = rejected:ERR_POSTGRES_QUERY_CANCELLED (hint) | a = resolved:[{"x":1,"pg_sleep":""}]
queued: rejected:ERR_POSTGRES_QUERY_CANCELLED | running: kept

Release build of 950a49c, the same server with ssl = on and a certificate for localhost. ssl and the version are what pg_stat_ssl has for the session:

verify-full, localhost: session ssl=true TLSv1.3 | cancel -> 57014 after 54 ms | next query -> 42
require, 127.0.0.1:     session ssl=true TLSv1.3 | cancel -> 57014 after 312 ms | next query -> 42
require, [::1]:         session ssl=true TLSv1.3 | cancel -> 57014 after 493 ms | next query -> 42

Cost

For a query that is never cancelled: one cached slot on the query wrapper (8 bytes), written at dispatch and cleared when the query settles. There is no new allocation, syscall, promise or host function per query. encode_request reads the status of the request once per Bind. size_of::<PostgresSQLQuery>() is 64 bytes. This PR adds no field to it.

what number
release linux-x64 text 80,668,872 on main e70cca7, 80,676,296 on 950a49c (+7,424). Not measured for the commits after it: a release build did not finish on the machine, which had a load average above 1000
release linux-x64 data and bss 110,424 and 1,822,992 on both of those builds
size_of::<PostgresSQLConnection>() 672 bytes on main, 704 bytes here: 12 for the cancel key, 16 for the CancelTarget of a cancel connection, and padding
host functions of the postgres binding 3, unchanged
runtime hooks (SqlRuntimeHooks) +1 (ssl_config_clone)
per cancel() of a running query one connection to the server, one PostgresSQLConnection, and on a TLS session a copy of the TLS options
after 1000 head cancels against PostgreSQL 17.11 and a full GC (950a49c) PostgresSQLConnection objects 2 to 2, PostgresSQLQuery 2 to 2, open file descriptors 10 to 10
patched net/tls/Bun.connect calls during one cancel() 0

Not measured: instructions per query and syscalls per cancel. valgrind, perf and strace are not on the machine and cannot be installed there.

MySQL

MySQL cancel() is unchanged: it sends nothing and rejects nothing (JSMySQLQuery::do_cancel is a stub). The docs and the JSDoc of cancel() describe PostgreSQL only, and make no claim for the other adapters.

Other suites

All of test/js/sql/ on a46f129 with the debug build and a 30 s test timeout: 957 pass, 2 skip, 47 fail. 46 failures are MySQL tests that get Access denied for user 'root'@'localhost' from the local MariaDB. The other one is json/jsonb bind parameter does not leak the stringified payload, which timed out at 30 s under a load average of 800. Alone it passes in 10 s, 2 of 2 runs. rustfmt and prettier are clean. The two commits after 3570e33 change two comments and the cleanup of one test. They ran in CI only: the machine lost its build.

Self-review

A review of 950a49c raised 26 concerns. 21 are addressed in a46f129 and the commits after it:

Not changed, with the reason:

  • An explicit type for the wire position of a request in place of status. It would rewrite the state machine of advance() on main. The decision is now one match in the connection, and the three counters are recorded on the request.
  • tx.close() cancels control statements and sends ROLLBACK right behind a cancel. Measured above. The fix is the hold of the open question.
  • A pipelined query that cancel() rejects gives its pool slot back while the server still runs it, so the pool can hand that connection to the next caller, who then waits. main has the same for an undecodable row (sql(postgres): reject only the query whose row the client cannot decode #43187).
  • The cancel connection ends through fail, also after a delivery, and builds an error that nobody reads. It is one object per cancel.
  • The cancel connection copies the TLS options of the session. It is one copy per cancel.

Alternatives that were built or weighed

  • Document the plaintext node:net socket and change nothing. cancel() stays a no-op for [::1] and for servers that only accept TLS.
  • Add node:tls to the JS socket. The adapter then has a second copy of the sslmode rules that the native connection implements.
  • Pass the packet from JS to createConnection as an extra argument. This was built and measured first (11 tests passed). The cancel key is a JS value on the way, the host name is resolved a second time, and a TLS socket stayed open when the peer went silent.
  • Open the connection from the session in native code. This is what the PR does.

no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/sql/postgres-query-cancel.test.ts

The native cancel hook was an empty stub, so cancel() never wrote
anything: an in-flight query ran to completion and resolved with its
rows, and a query cancelled before it was dispatched never settled.

A backend running a query reads nothing from its connection until the
query finishes, so cancel() now opens a second connection and sends a
CancelRequest built from the BackendKeyData the server already handed
out. The backend answers the original connection with SQLSTATE 57014,
which rejects the query through the existing ErrorResponse path.

A query that is queued on a connection but whose bytes are not on the
wire is failed locally instead: a CancelRequest there would stop
whatever the backend is actually running. A query that was never
dispatched is rejected with ERR_POSTGRES_QUERY_CANCELLED.
@robobun
robobun requested a review from alii as a code owner July 5, 2026 10:35
@coderabbitai

coderabbitai Bot commented Jul 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 3a792dcb-8c8c-4fe5-9300-6ff713b5d69e

📥 Commits

Reviewing files that changed from the base of the PR and between 74fb430 and 3b664c6.

📒 Files selected for processing (11)
  • docs/runtime/sql.mdx
  • packages/bun-types/sql.d.ts
  • src/runtime/hw_exports.rs
  • src/sql/postgres/protocol/ErrorResponse.rs
  • src/sql/shared/ConnectionFlags.rs
  • src/sql_jsc/jsc.rs
  • src/sql_jsc/postgres/PostgresSQLConnection.rs
  • src/sql_jsc/postgres/PostgresSQLQuery.rs
  • test/js/sql/postgres-pending-dial-fixture.ts
  • test/js/sql/postgres-query-cancel.test.ts
  • test/js/sql/sql-connect-error-reporting.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


Walkthrough

The change adds PostgreSQL query cancellation through a separate connection that sends a CancelRequest. It updates query lifecycle handling, records PostgreSQL backend key data, adds cancellation tests, and expands the SQL cancellation documentation.

Changes

PostgreSQL cancellation flow

Layer / File(s) Summary
Cancellation protocol and runtime support
src/sql/postgres/protocol/BackendKeyData.rs, src/sql/postgres/protocol/ErrorResponse.rs, src/sql/postgres/AnyPostgresError.rs, src/sql/shared/ConnectionFlags.rs, src/sql_jsc/postgres/error_jsc.rs, src/sql_jsc/jsc.rs, src/runtime/hw_exports.rs
Backend key data is retained and encoded for CancelRequest packets. Error handling recognizes SQLSTATE 57014 and maps query cancellation to ERR_POSTGRES_QUERY_CANCELLED. Runtime hooks clone SSLConfig.
CancelRequest connection lifecycle
src/sql_jsc/postgres/PostgresSQLConnection.rs
PostgreSQL connections retain backend key data and can open a separate connection to send a CancelRequest. The cancel connection uses the session’s address and TLS state and has distinct timeout and close handling.
Query cancellation handling
src/js/internal/sql/query.ts, src/runtime/api/sql.classes.ts, src/sql_jsc/jsc.rs, src/sql_jsc/postgres/PostgresSQLQuery.rs
Query handles can return a cancellation payload for adapter delivery. PostgreSQL queries cache their connection and handle cancellation differently for pending, current, and queued in-flight requests.
Cancellation tests and documentation
test/js/sql/postgres-query-cancel.test.ts, test/js/sql/postgres-pending-dial-fixture.ts, test/js/sql/sql-connect-error-reporting.test.ts, test/js/sql/wire-frames.ts, docs/runtime/sql.mdx, packages/bun-types/sql.d.ts
Tests cover query states, protocols, TLS settings, connection types, and pending dials. The documentation describes cancellation outcomes and errors.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 3b664

PostgreSQL Query.cancel() now sends a best-effort CancelRequest over a separate connection. That connection uses the same TLS protection as the session, so cancellation credentials are no longer sent in plaintext. The documentation explains the best-effort limits, including that a late cancel can affect the next query. No outstanding issues block merging.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: implementing PostgreSQL support for Query.cancel().
Description check ✅ Passed The description explains the problem, implementation, behavior, limitations, and verification. It does not use the template headings, but it provides the required information in detail.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 5, 2026
@robobun

robobun commented Jul 5, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:14 PM PT - Oct 2nd, 2026

❌ @robobun, your commit 52e8500 has 2 failures in Build #123045 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33370

That installs a local version of the PR into your bun-33370 executable, so you can run:

bun-33370 --bun

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Bun.SQL: add ability to cancel query using AbortSignal #23175 - This PR implements the underlying Query.cancel() mechanism that this feature request depends on; the issue's "alternatives considered" sample even shows query.cancel()

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #23175

🤖 Generated with Claude Code

@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Not adding Fixes #23175. That issue asks for .timeout(ms) / .signal(AbortSignal) on a query, and this PR adds neither.

What it does change for that issue is the workaround its author already wrote under "alternatives considered":

const query = sql`SELECT pg_sleep(100)`;
request.signal.addEventListener("abort", () => query.cancel());
const result = await query.execute();

On main that cancel() is a no-op for Postgres, so the query runs to completion anyway. With this PR it actually stops the query on the server. A real .signal() / .timeout() API can then be built on top, so #23175 stays open.

Comment thread src/sql_jsc/postgres/PostgresSQLQuery.rs Outdated
Comment thread packages/bun-types/sql.d.ts
…running

A CancelRequest names the backend process, not a statement, so it stops
whatever that backend is executing: the request at the head of the
connection's FIFO. do_run()'s Prepared arm writes a query's Bind/Execute
as soon as can_pipeline() allows, which happens while an earlier query is
still running, so cancelling the pipelined query sent a CancelRequest
that killed the query ahead of it instead.

Check that the request is the head of the FIFO before asking the server
to stop it. A request behind the head is settled locally, but the two
cases differ: a Pending request has nothing on the wire and is failed, so
advance() discards it instead of writing it, while a pipelined request is
already on the socket and the backend will answer it no matter what, so
its promise is rejected in place and the FIFO entry stays to consume those
answers in order. Marking it failed would make advance() discard it and
the next BindComplete would land on the wrong request.
@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

CI status

The one hard failure on both builds so far is infrastructure, not this diff.

:darwin: 26 aarch64 - test-bun exits before running a single test:

2026-07-05 05:02:19 INFO   Searching for artifacts: "**" within step: "darwin-aarch64-build-bun"
2026-07-05 05:02:19 INFO   Found 2 artifacts. Starting to download to: .../bun/bun/release

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).
    at getExecPathFromBuildKite (scripts/runner.node.mjs:2182:13)

Why it is not this change:

  • The step it is downloading from, darwin-aarch64-build-bun, passed. So did every other build lane (darwin/linux/windows/freebsd x cpp/rust/bun), plus Format, Lint JavaScript, TypeScript types and cargo clippy.
  • The lane dies inside buildkite-agent artifact download, before runTests ever starts. No test of any kind ran on it.
  • It reproduced identically on two independent builds: #68532 (acec2d8) and #68546 (d920eab), same lane, same 120s timeout. The second build was a fresh push, so it was already effectively a re-roll.
  • This diff touches the Postgres wire protocol, src/js/internal/sql/* and a cached value on the query handle. None of that can affect a macOS artifact download.

The only other annotation on each build is a different test that CI itself tags flaky and retries (bun-install.test.ts on #68532, bake/dev-and-prod.test.ts on #68546). Neither touches SQL.

I have not pushed a ci: retrigger commit: a new push cancels the in-flight build, and #68546 still has ~235 jobs pending, including the shards that run test/js/sql/postgres-query-cancel.test.ts. Losing that signal to chase a green checkmark seemed like the wrong trade, especially since the previous push already demonstrated a re-roll does not clear this.

If someone with write access could re-run just the darwin 26 aarch64 - test-bun job, that should be enough. Happy to push a retrigger instead if that is preferred.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both prior concerns are addressed (the head-of-FIFO check + reject_in_place for pipelined queries, and the generalized JSDoc), and I found nothing new — deferring only because this touches native SQL/wire-protocol code and warrants a human look.

Extended reasoning...

Overview

Implements Query.cancel() for Postgres end to end: encodes the CancelRequest packet from stored BackendKeyData, opens a second plaintext connection via Bun.connect to deliver it, and settles never-dispatched queries locally. Touches 14 files across the shared JS query layer, the Postgres adapter, native Rust (PostgresSQLQuery, PostgresSQLConnection, BackendKeyData), the .classes.ts codegen input (adds a connection cached-value slot on the Postgres query wrapper), types/docs, and a new hermetic wire-server test suite.

Prior feedback

I previously flagged (🔴) that a pipelined Status::Binding query behind the FIFO head would cancel the wrong query, and (🟡) that the shared SQL.Query.cancel() JSDoc described only Postgres. Both are fixed in this revision: do_cancel() now checks is_current_request() and routes non-head on-wire requests through the new reject_in_place() (which leaves the FIFO entry so the backend's replies are still consumed in order), a pipelined-query test was added, and the JSDoc was generalized. Nothing outstanding.

Security risks

Low. The CancelRequest carries only the pid/secret the server itself issued in BackendKeyData; no credentials cross the plaintext side-connection (matches libpq's PQcancel). connectionInfo.hostname/port/path come from the user's own SQL config, not attacker input.

Level of scrutiny

High. This is production database code: it changes user-visible semantics (a former no-op now aborts server-side queries), adds a GC-visited JS value slot on a native class, and introduces reject_in_place() whose correctness depends on FIFO-ordering invariants and on Query.resolve() being a no-op on an already-rejected promise. The pipelined case in particular is subtle enough that a maintainer familiar with advance() / the request FIFO should confirm the desync reasoning.

Other factors

Test coverage is good and hermetic (scripted wire server asserting exact bytes and connection counts, plus the new pipelined variant). Not approving solely on scope/complexity — the change looks correct to me.

@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks. Since the one thing you flagged for a human is the advance() / FIFO desync reasoning behind reject_in_place(), here it is laid out so it can be checked against the code rather than taken on trust.

Why a pipelined request cannot just be marked Fail

bind_and_execute() ends every request with Flush + Sync (PostgresRequest.rs), so each pipelined query is its own sync point and the backend answers it even if the one ahead of it errored. Concretely, with a at the head and b pipelined behind it:

  1. b.cancel() marks b as Fail.
  2. a finishes. ReadyForQuery calls advance().
  3. advance()'s defer_cleanup! pops Success/Fail entries off the head, so it pops a, then sees b is Fail and pops b too. The FIFO is now empty.
  4. b's replies arrive anyway. MessageType::BindComplete does self.current().ok_or(AnyPostgresError::ExpectedRequest)?, current() is None, and the whole pooled connection fails.

What reject_in_place() does instead

It rejects the promise and touches nothing the connection still needs: status stays Binding, target stays set, and this_value stays strong. So:

  • advance() sees a non-terminal status at the head and leaves the entry alone (the Running | Binding | PartialResponse arm just advances offset), so b's replies still land on b.
  • b then finishes down the ordinary path. on_result(is_last = true) fires on_query_resolve_fn, which reaches Query.resolve(), which calls this[_resolve](x) on an already rejected promise: a no-op per the promise spec. The same path runs allow_gc() + downgrade(), so there is no new GC state to reason about and no early downgrade.

This is also why it does not reuse on_js_error(): that one sets Status::Fail, clears target, and downgrades this_value, which is exactly right for a Pending request (nothing on the wire, so advance() should discard it instead of writing it) and exactly wrong for a pipelined one.

The new test covers the desync directly: it cancels the pipelined query, feeds both queries' replies, and then runs one more query, which only succeeds if the FIFO and the protocol stayed in sync.

CI

test/js/sql/postgres-query-cancel.test.ts is now green on every platform that ran it in #68546, 5 pass / 0 fail each:

Linux x64 debian 13, ubuntu 25.04, x64-baseline, x64-asan
Linux aarch64 debian 13, ubuntu 25.04
musl alpine 3.23 x64, x64-baseline, aarch64
Windows 2019 x64, 11 aarch64

The two red lanes are unrelated:

  • darwin 26 aarch64 - test-bun dies in buildkite-agent artifact download with a 120s timeout before runTests starts, so it never ran a test. Reproduced identically on #68532 and #68546.
  • windows 2019 x64-baseline - test-bun fails napi > napi_wrap > has the right lifetime with Condition was not met after 100 GC attempts, a GC-timing assertion in a suite that never constructs a SQL object. The other annotation on that lane, bake/dev-and-prod.test.ts, CI already tags flaky and retries.

@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

CI: final state

#68546 has settled at 282 passed, 2 failed. Neither failure is this diff, and I have not pushed a ci: retrigger because a rebuild would not clear either one.

1. darwin 26 aarch64 - test-bun exits inside buildkite-agent artifact download with a 120s timeout, before runTests starts, so it never runs a test. The step it downloads from, darwin-aarch64-build-bun, passed.

Both builds that hit this ran on the same agent:

build sha agent hostname
#68532 acec2d8d darwin-aarch64-26.5.1-1 bingus
#68546 d920eaba darwin-aarch64-26.5.1-1 bingus

So this looks like one macOS agent that cannot pull artifacts, rather than a transient blip. A fresh build would most likely be scheduled straight back onto it, which is why I would rather not burn a full 287-job rebuild on it. Re-running just that job (once bingus is healthy) should be enough.

2. windows 2019 x64-baseline - test-bun fails napi > napi_wrap > has the right lifetime with Condition was not met after 100 GC attempts. That is a GC-timing assertion in a suite that never constructs a SQL object. The lane's other annotation, bake/dev-and-prod.test.ts, CI already tags flaky and retried.

This diff is green. test/js/sql/postgres-query-cancel.test.ts ran 5 pass / 0 fail on every platform that reached it:

Linux x64 debian 13, ubuntu 25.04, x64-baseline, x64-asan
Linux aarch64 debian 13, ubuntu 25.04
musl alpine 3.23 x64, x64-baseline, aarch64
Windows 2019 x64, 11 aarch64

Happy to push a retrigger if you would rather roll the dice on it, but my read is that a one-job re-run is the cheaper move.

The `connection` cached value a query keeps so that cancel() can reach the
connection it ran on is a strong GC edge. `allow_gc` clears it for a request
that completed, but `on_js_error` and `on_write_fail` did not, so holding a
settled Query kept its whole connection reachable: socket buffers, the
prepared-statement map and all.

Clear it on the failure paths too, through one helper shared with `allow_gc`
so the next slot added here has an obvious place to be released. `do_cancel`
reads the value before it hands off to `on_js_error`, so nothing loses its
connection early.
@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 771c008, a small follow-up that tightens the lifetime of the connection cached value this PR adds to the query handle.

That slot is a strong GC edge from the query's JS wrapper to the connection's. allow_gc() clears it for a request that completed, but on_js_error() and on_write_fail() did not, so holding on to a settled (rejected) Query kept the whole connection reachable: socket buffers, the prepared-statement map, all of it. All three sites now go through one release_connection() helper, so the next cached value added here has an obvious place to be released. do_cancel() reads the slot before it hands off to on_js_error(), so nothing loses its connection early.

Being straight about the evidence: I could not make this show up as an observable leak, because a PostgresSQLConnection already survives sql.close() plus a forced GC even when no query was ever created, so heapStats() cannot see the incremental edge. The fix is on the strength of the asymmetry (success path clears it, failure paths did not) rather than a reproduction, which is why it is hygiene rather than a behavior change and ships with no new test.

The five cancel tests still pass, the full test/js/sql/ run is unchanged (586 pass, same 29 pre-existing failures from MariaDB root auth and two debug+ASAN-slow sqlite fuzz tests), and both end-to-end checks against PostgreSQL 17 still behave: the head query dies with 57014 sub-second, and a cancelled pipelined query rejects while its neighbour runs its full pg_sleep(30).

This push also starts a fresh build, which re-rolls the two unrelated red lanes from #68546 (the bingus artifact-download timeout and the napi GC flake) without me spending a throwaway retrigger commit on them.

@robobun

robobun commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator Author

CI: three distinct failures, none from this diff

#68564 (771c008) is red on three things. I looked at each rather than assuming, and none of them is reachable from what this PR changes. I have still not pushed a ci: retrigger, because it cannot fix the main one.

1. darwin 26 aarch64 - test-bun — a single sick agent.

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

It exits before runTests starts, so it never runs a test, and the step it downloads from (darwin-aarch64-build-bun) passed. All three builds that hit it ran on the same agent:

build sha agent hostname
#68532 acec2d8d darwin-aarch64-26.5.1-1 bingus
#68546 d920eaba darwin-aarch64-26.5.1-1 bingus
#68564 771c008 darwin-aarch64-26.5.1-1 bingus

Three for three on bingus. A rebuild gets scheduled straight back onto it, which is exactly why I have not burned a retrigger: it would be a fourth identical failure plus a junk commit. Re-running that one job (or taking bingus out) is the fix.

2. debian 13 x64-asan - test-bun, 1 shard of ~20 — a JSC termination race.

test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT
ASSERTION FAILED: !scope.exception() || !hasSlot
JSCJSValuePropertyInlines.h(51) : JSValue JSC::JSValue::get(JSGlobalObject *, PropertyName, PropertySlot &)

A property get running with a pending exception, in a test that terminates a worker mid message-port transfer. I did not wave this one off:

  • It ran 0/30 aborts locally under the same debug+ASAN build.
  • Build #68546 completed with two of this PR's three commits and that test passed there. The only delta into #68564 is 771c008, ten lines that clear a cached GC slot on PostgresSQLQuery's failure paths.
  • The test never constructs a SQL object and never requires internal/sql/*, so neither the new cached value nor the protocol code is reachable from it.
  • The other 19 asan shards passed.

For what it is worth, !scope.exception() || !hasSlot firing during worker termination has the shape of a missing exception check in the termination path. Probably worth its own look, but it is not this PR.

3. test/bake/dev-and-prod.test.ts is annotated flaky by CI itself and retried.

This diff is green

From #68546, the build that ran to completion, test/js/sql/postgres-query-cancel.test.ts was 5 pass / 0 fail on every platform that reached it:

Linux x64 debian 13, ubuntu 25.04, x64-baseline, x64-asan
Linux aarch64 debian 13, ubuntu 25.04
musl alpine 3.23 x64, x64-baseline, aarch64
Windows 2019 x64, 11 aarch64

Locally: the five cancel tests pass, test/js/sql/ is unchanged at 586 pass (same 29 pre-existing failures from MariaDB root auth and two debug+ASAN-slow sqlite fuzz tests), and both end-to-end checks against PostgreSQL 17 behave: the head query dies with 57014 in well under a second, and a cancelled pipelined query rejects while its neighbour runs its full pg_sleep(30).

Happy to push a retrigger if you would rather roll the dice, but with bingus at three for three I do not think it buys anything.

A query that is cancelled before it is dispatched now rejects from
#run() and #runAsync(), the same placement as #41492. cancel() no longer
rejects the promise itself. An eager reject inside cancel() makes a
cancelled query that nobody awaits an unhandled rejection, which exits
the process with code 1.

Carry #41492's sqlite tests so the sqlite and mysql adapters are covered
in this branch too. After #41492 merges, the query.ts hunks for #run()
and #runAsync() resolve as identical on rebase.
Comment thread src/js/internal/sql/postgres.ts Outdated
Comment thread src/js/internal/sql/postgres.ts Outdated
Comment thread src/js/internal/sql/postgres.ts Outdated
Comment thread src/js/internal/sql/postgres.ts Outdated
Comment thread src/js/internal/sql/postgres.ts Outdated
Comment thread src/js/internal/sql/query.ts Outdated
Comment thread src/js/internal/sql/shared.ts Outdated
Comment thread src/runtime/api/sql.classes.ts Outdated
Comment thread src/sql/postgres/protocol/BackendKeyData.rs Outdated
Comment thread src/sql/postgres/protocol/BackendKeyData.rs Outdated
Comment thread src/sql_jsc/postgres/PostgresSQLConnection.rs Outdated
Comment thread src/sql_jsc/postgres/PostgresSQLQuery.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

CI result for e8b81f9383

Buildkite #120531: 180 of 181 jobs passed. No failure comes from this PR.

  • The one red job is a debian 13 x64-asan shard, on test/js/bun/spawn/spawn.test.ts (stdout reader of an unref'd child and process lifetime > an idle reader stopped at the highwater ...). CI marks that test as also failing on main. It failed the same way on the earlier build of this PR, #120382. This PR does not touch it.
  • Two tests passed on a retry: test-fs-read-stream-pos.js and serve-http2-lifecycle.test.ts.
  • The GitHub checks passed: Format, Lint JavaScript, Source lints, TypeScript types, clippy and miri.

The two test files of this PR ran in jobs that passed on every platform: Linux glibc, Linux ASAN, musl, Windows and macOS, each on x64 and aarch64 where CI has both. The two tests that use the raw listener do not run on Windows and musl.

For a maintainer

There is no open review thread. I do not plan another push unless a review asks for a change.

@robobun
robobun changed the base branch from robobun/4be26dfa/postgres-connection-open to main September 30, 2026 01:50
@robobun

robobun commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

This is now one pull request against main

Checked after the merge, on the debug build:

  • The 36 tests of postgres-query-cancel.test.ts and sql-connect-error-reporting.test.ts pass. On the release build 367d939, which does not have this change, 17 of them fail.
  • All of test/js/sql/ with a 30 s test timeout: 936 pass, 2 skip, 46 fail. The 46 are MySQL tests that the local MariaDB refuses with Access denied.
  • Against PostgreSQL 17.11, the end-to-end results are unchanged: a head cancel through [::1], 127.0.0.1 and localhost, a pipelined query, a cancel before dispatch, and a cancel inside Parse.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked the cancel connection's TLS handling: the ssl_mode derivation in send_cancel_request (src/sql_jsc/postgres/PostgresSQLConnection.rs:1712-1716) fails closed (a TLS session always yields Require or stricter, a plaintext one Disable), and the new ssl_config_clone hook goes through the existing deep Clone of bun_runtime::socket::SSLConfig (src/http/ssl_config.rs:457), so the session and cancel connection do not share owned C strings and there is no double free on drop.

Extended reasoning...

The change implements Query.cancel() for the Postgres client across ~1,300 lines, adding a second native connection that carries the backend cancel key, an SSLConfig clone hook across the runtime/sql_jsc FFI boundary, and a refactor of createConnection into open/ConnectionStrings. The security-sensitive surface is the cancel dial's TLS mode and certificate settings relative to the session; those were checked and found fail-closed and deep-copied. Two confirmed findings are posted inline and a third verified finding was dropped before posting, so the PR is not approvable this run; this note only records what else was examined.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/sql_jsc/postgres/PostgresSQLConnection.rs
Comment thread test/js/sql/postgres-query-cancel.test.ts
…e dial

The cancel connection now holds the query that it stops. When it is ready
to write, it looks at the query again: the packet goes out only while the
query is still the head of its session and the backend has not finished
it. Before, a query that ended during the TCP and TLS handshake of the
cancel connection left a CancelRequest that stopped the next query of
that backend.

The cancel connection reads the cancel key from the session at that time
and no longer keeps a copy.

Tests: a cancel connection whose SSLRequest the server answers only after
the query ended sends nothing, and cancel() stops a query on a real
PostgreSQL server.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/sql/postgres-query-cancel.test.ts
…adline

The loop now fails after 4 s with the rows that pg_stat_activity has for
the query, in place of a test timeout with no message.
@robobun

robobun commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

76a436ef4c and c55aa7ab13: the findings of the last review

  • A cancel for a query that ended during the dial. Fixed in 76a436e. The cancel connection now holds the query that it stops and looks at it again when it is ready to write. It sends the CancelRequest only while the query is still the one the backend runs. With select 1 cancelled in the same tick and a second query behind it, the second query was stopped in 115 and 110 of 300 rounds before, and in 15, 10 and 12 of 300 now.
  • What remains. A query that ends after the packet is written and before the server delivers the signal. In two setups with a query that is cancelled in its last milliseconds, the next query was still stopped in 39 to 75 of 300 rounds. The runs differ a lot on this machine, so I do not claim a change for those. The description has the table, and the docs keep the sentence about the next query.
  • A test against a real server. cancel() stops a query that a real server is running uses describeWithContainer. Its poll has a deadline (c55aa7a).

Checked on the debug build:

  • The two test files: 38 pass with a 30 s test timeout. On the release build 367d939, which does not have this change, 19 of them fail.
  • All of test/js/sql/: 937 pass, 2 skip, 47 fail. 46 are MySQL tests that the local MariaDB refuses. One is a retry-count test with a 0.5 s budget that saw 1 attempt under a load average above 600. Its file passes alone, 3 of 3 runs.
  • 1000 cancels and a full GC: no connection object, query object or file descriptor is left over.

size_of::<PostgresSQLConnection>() is now 688 bytes (672 on main). The release size in the description is from 3b664c6, and I have not measured it again for these two commits.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/sql_jsc/postgres/PostgresSQLConnection.rs Outdated
… end of the TLS handshake

The cancel connection now holds its session and its query. It sends no
CancelRequest only when the session is still connected and its backend
runs another query by now. A session that is closed or failed has nothing
left that the packet could stop by mistake. Before, reserved.close(),
which cancels its queries and closes the connection in the same tick, got
no CancelRequest, and the backend ran the query to its end.

For a TLS session the cancel connection now decides when the handshake has
ended. Before, it decided when the handshake started, so a query that
ended during the handshake still got a CancelRequest, which stopped the
next query of that backend.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/sql/postgres-query-cancel.test.ts
@robobun

robobun commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

950a49cb4a, release-build numbers, and one open question

  • TLS. The cancel connection asked the session whether its query still runs when the TLS handshake started. It now asks when the handshake has ended (on_handshake). Checked against PostgreSQL 17.11 with ssl = on and a TCP hop that holds the ClientHello of the cancel connection for 300 ms. On c55aa7a a query that nobody cancelled rejected with 57014. On 950a49c it resolves.
  • reserved.close(). c55aa7a sent no CancelRequest when the session closed during the dial, so the backend ran pg_sleep(6) to its end (5994 ms). The cancel connection now holds the session itself, and the backend stops after 110 to 125 ms.
  • Release numbers. The PR body had debug-build numbers for a CancelRequest that the server handles after its query ended. It now has release-build numbers. A loop cancels select 1 and then runs a second query. The second query, which nobody cancelled, rejected with 57014 in 45 to 149 of 300 rounds. A raw protocol client on node:net, with no use of Bun.SQL, gives 57 to 103 of 300. So the cause is the protocol on this server, and not this client: the machine is loaded, and the server needs a median of 52 ms to act on a cancel connection.
  • Open question for a maintainer. When the same raw client waits for the server to close the cancel connection before it writes the next query, the result is 0 of 300. libpq, pgx and pgjdbc wait like that. node-postgres and postgres.js do not. Should a Bun session write no new request while its cancel connection is open? The price is a delay of the next request on that connection after each cancel() of a running query, at most the budget of the cancel connection (5 s). I did not build it, because it changes when a session sends. The numbers and both sides of the trade-off are in the Notes of the PR body, section "A late CancelRequest, and the open question".

robobun added a commit that referenced this pull request Sep 30, 2026
Two other open pull requests add a ConnectionFlags member at bit 6:
IS_CANCEL_REQUEST in #33370 and ON_CONNECT_PENDING in #40913. Each pull
request inserts its line after KEEP_ALIVE_REQUESTED, so the second one
to merge gets a textual conflict there. With LIFETIME_EXCEEDED at bit 7,
the result of keeping both lines has no two members on one bit.
main closes a socket that never opened with on_connect_error (#44327), so
ref_and_close is main's, and the session test for that case is main's too.
…er cancels it

The connection encodes the Bind of a query when it converts the parameters,
and an object parameter is converted by its own code. That code can call
cancel() on the query.

- advance() takes a request out of `pending_requests` before it encodes
  the Bind, and cancel() took it out again: "pending_requests underflow".
  `RequestCounter::Pending` now records that a request is in that counter,
  so `finish_request` is idempotent for all three counters.
- `encode_request` stops when the request was rejected while its
  parameters were encoded, and the batch leaves the write buffer.
- A cancel() that arrives while `do_run` is on the stack marks the query,
  and `do_run` does not send it.
- `PostgresSQLConnection::cancel` decides what cancel() does. The query
  only finds its connection.

Also:

- The cancel key is an `Option`. A process id of 0 is a key, and a server
  that sent no BackendKeyData has none.
- `query.ts` is main's again. The query that is cancelled before it runs
  is #41492.
- The docs say what cancel() does on PostgreSQL and make no claim for the
  other adapters.
Comment thread src/sql_jsc/postgres/PostgresRequest.rs Outdated
Comment thread src/sql_jsc/postgres/PostgresSQLQuery.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline nit, I also checked that the cancel connection's CancelTarget (the RefPtrs to the session and query) is released on every terminal path: it is taken at write time in send_startup_message, cleared in fail_with_js_value for the timeout/connect-error/close paths, and the struct's own Drop covers the rest, so the refcounts balance.

Extended reasoning...

The only finding this run is a test-cleanup nit; the native cancel-connection lifecycle in src/sql_jsc/postgres/PostgresSQLConnection.rs was read for a refcount leak on the timeout and failure paths and none was found.

Comment thread test/js/sql/postgres-query-cancel.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

@robobun

robobun commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author

52e8500d7c: merged main, fixed a panic, and made the PR smaller

  • Merged main. main has usockets: tell a socket's holder it is gone exactly once, whoever closes it and whether or not it ever opened #44327, which reports the close of a socket that never opened. The release of the event loop that this PR had for that case is gone, with its session test. The cancel connection now closes by main's rule for a failed connection.
  • Fixed: cancel() from the code of a parameter. A parameter whose toString() calls cancel() on its own query stopped a debug build with panic: pending_requests underflow. The request now records which counter it is in (RequestCounter::Pending), encode_request takes a rejected request out of the write buffer, and do_run does not send a query that was cancelled during its dispatch. PostgresSQLConnection::cancel makes the decision now.
  • Smaller. The query.ts change left this PR, because sql: reject a query that is cancelled before it runs #41492 has it. Only sql: make transaction.close({ timeout }) roll back when pending queries drain before the timeout #32149 has to merge first now. The first line of the description says why.
  • Cancel key. It is an Option. A server that sent no BackendKeyData gets no cancel connection, and a process id of 0 is sent like any other.
  • Tests. 31, up from 21: four TLS modes, prepare: false, a cancelled head with pipelined queries behind it, an error for a cancelled pipelined query, a silent server after the packet, a cancel while only the Parse is on the wire, and the GC edge. 29 of the 31 fail on the released build.
  • Docs. The cancel() text describes PostgreSQL only.
  • Not measured again: the release text size. A release build did not finish on my machine. The last number (+7,424 bytes) is from 950a49c.

New data for the open question. tx.close() cancels its running query and sends ROLLBACK right behind it. In 2 of 400 rounds the CancelRequest stopped that ROLLBACK, and tx.close() rejected with 57014. The transaction still ended rolled back. asyncpg, like libpq, pgx and pgjdbc, does not reuse a connection while a cancel for it is in progress.

@cirospaciari this is the hazard that you named on #23175. Should a session write nothing new while its cancel connection is open? I have not built it. The numbers and both sides are in the Notes of the description, section "A late CancelRequest, and the open question".

CI for 52e8500d7c. Buildkite #123045: 170 jobs passed, and every test lane that ran is green. 1 job failed: windows aarch64 - build-bun could not download the WebKit archive (HTTP 500 from the GitHub release), so its 8 test jobs did not run. No failure comes from this PR. The build before it, #122966, had no build failure, and this head differs from that one by one test file.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants