Repository navigation
Keep a terminated reserved connection out of the pool (fixes #1199) - #1240
Mariomarquezt wants to merge 1 commit into
Conversation
When the server terminates the backend of a reserved connection, onclose moves the connection to the closed queue, and may reconnect it at once for a waiting query. The ReservedSql handle still pointed at it: - later queries on the handle called execute() on a connection with a null socket, so nextWrite() threw an uncaught TypeError and the query never settled; - release() called onopen() and put the dead connection in the open queue, so the next pool query hung (porsager#1199). onclose now marks the handle closed. Its queued queries are rejected, later queries reject with the close error, and release() leaves the connection with the pool, which reconnects it as usual. Fixes porsager#1199
|
reserve() and begin(): a reserved connection keeps being used after the server ends its session Versions. postgres 3.4.9, Node 24.19.0, PostgreSQL 17.11 (the Related. #1199, #1208 and #1209, #1240, #1246, #1195 and #1229, #1216 and #1215, #1223, #1248. This is one more data point for this PR, #1246 and #1215 (#1248 reports the transaction case too), from a session-level advisory lock held on a reserved connection, with two consequences we did not see described: What happensA
Separately,
And
And
Every case is traced from the source above, and the output below is the script's own, run once on 3.4.9 and once with the change. ReproductionOnly docker run --rm -d --name pg-reserve-repro -p 127.0.0.1:55432:5432 -e POSTGRES_PASSWORD=repro postgres:17
node repro.mjsimport net from 'node:net'
import postgres from 'postgres'
const PORT = 55432
const admin = postgres({ host: '127.0.0.1', port: PORT, user: 'postgres', pass: 'repro', database: 'postgres', max: 1, onnotice() {} })
const sleep = ms => new Promise(r => setTimeout(r, ms))
const settle = (p, ms = 3000) => Promise.race([
Promise.resolve(p).then(x => 'resolved ' + JSON.stringify(x), e => 'rejected ' + (e.code || e.message)),
sleep(ms).then(() => `STILL PENDING after ${ ms } ms`)
])
process.on('uncaughtException', e => console.log(' uncaughtException:', e.message))
// A relay in front of the server, so case 5 can reset the client's socket.
const sockets = new Set()
const relay = net.createServer(client => {
const server = net.connect(PORT, '127.0.0.1')
client.on('error', () => server.destroy())
server.on('error', () => client.destroy())
client.on('close', () => (server.destroy(), sockets.delete(client)))
client.pipe(server).pipe(client)
sockets.add(client)
})
await new Promise(r => relay.listen(0, '127.0.0.1', r))
const pool = () => postgres({ host: '127.0.0.1', port: relay.address().port, user: 'postgres', pass: 'repro', database: 'postgres', max: 1, onnotice() {} })
const pid = async sql => (await sql`select pg_backend_pid() as pid`)[0].pid
const end = async p => { await admin`select pg_terminate_backend(${ p })`; await sleep(300) }
const waiting = async key => {
for (;;) {
const [row] = await admin`select pid from pg_locks where locktype = 'advisory' and not granted and objid = ${ key }`
if (row) return row.pid
await sleep(20)
}
}
console.log('1. The session ends while the reserved connection is idle; then a statement on the handle')
{
const sql = pool()
const conn = await sql.reserve()
await end(await pid(conn))
console.log(' statement on the handle:', await settle(conn`select ${ 1 }::int as x`))
console.log(' next pool query :', await settle(sql`select 1 as x`))
}
console.log('2. The session ends while reserved and idle; the pool reserves the connection again; then the old handle')
{
const sql = pool()
const stale = await sql.reserve()
await end(await pid(stale))
const next = await sql.reserve() // max: 1, so the same connection, reconnected
const nextPid = await pid(next)
console.log(' old handle on next\'s session :', await settle(stale`select pg_backend_pid() = ${ nextPid } as same`))
stale.release()
console.log(' pool query while next holds it:', await settle(sql`select pg_backend_pid() = ${ nextPid } as same`))
}
console.log('3. Two statements in flight on a reserved connection when its session ends')
{
const sql = pool()
await admin`select pg_advisory_lock(${ 3 })`
const conn = await sql.reserve()
const first = settle(conn`select pg_advisory_lock(${ 3 }) as waits`, 5000)
const second = settle(conn`select 1 as queued`, 5000)
await end(await waiting(3))
console.log(' first :', await first)
console.log(' second:', await second)
await admin`select pg_advisory_unlock(${ 3 })`
}
console.log('4. A session ends under a statement; then the next borrower of that connection')
{
const sql = pool()
await admin`select pg_advisory_lock(${ 4 })`
const conn = await sql.reserve()
const first = settle(conn`select pg_advisory_lock(${ 4 }) as waits_again`, 5000)
await end(await waiting(4))
console.log(' statement in flight:', await first)
await admin`select pg_advisory_unlock(${ 4 })`
console.log(' next reserve() :', await settle(sql.reserve().then(c => (c.release(), 'reserved'))))
console.log(' then a pool query :', await settle(sql`select 1 as x`))
}
console.log('5. A reset under a statement; a statement sent from the rejection handler; then two pool queries')
{
const sql = pool()
await admin`select pg_advisory_lock(${ 5 })`
const conn = await sql.reserve()
let fromHandler
const first = conn.unsafe('select pg_advisory_lock(5)').catch(e => {
fromHandler = settle(conn`select 1 as after_reset`) // after the socket's error, before its close
return 'rejected ' + e.code
})
await waiting(5)
for (const s of sockets) s.resetAndDestroy()
console.log(' statement in flight :', await first)
console.log(' sent from the handler:', await fromHandler)
await admin`select pg_advisory_unlock(${ 5 })`
console.log(' pool query 1 :', await settle(sql`select 1 as x`))
console.log(' pool query 2 :', await settle(sql`select 2 as x`))
}
console.log('6. A session ends under a statement in a transaction, with a second statement queued behind it; then two pool queries')
{
const sql = pool()
await admin`select pg_advisory_lock(${ 6 })`
let queued
const tx = settle(sql.begin(t => {
const first = t`select pg_advisory_lock(${ 6 }) as waits_in_transaction`.execute()
queued = settle(t`select 1 as queued`, 5000)
return first
}), 5000)
await end(await waiting(6))
console.log(' begin() :', await tx)
console.log(' queued :', await queued)
await admin`select pg_advisory_unlock(${ 6 })`
console.log(' pool query 1:', await settle(sql`select 1 as x`))
console.log(' pool query 2:', await settle(sql`select 2 as x`))
}
console.log('7. A statement handed to a connection in the turn its socket closes; then a pool query')
{
let socket
const sql = postgres({ host: '127.0.0.1', port: PORT, user: 'postgres', pass: 'repro', database: 'postgres', max: 1, connect_timeout: 3, onnotice() {}, socket: () => (socket = net.connect(PORT, '127.0.0.1')) })
await sql`select 1 as x`
const handed = new Promise(r => setImmediate(() => {
socket.destroy()
r(settle(sql`select 1 as handed`)) // its write waits for the next check phase, after the close
}))
console.log(' statement handed over:', await handed)
console.log(' pool query :', await settle(sql`select 2 as x`, 5000))
}
console.log('8. The session ends while a transaction\'s callback awaits a timer; the pool reserves the connection again; then the callback\'s next statement')
{
const sql = pool()
let nextPid, gotPid, resumed
const txPid = new Promise(r => gotPid = r)
const late = new Promise(r => resumed = r)
const tx = settle(sql.begin(async t => {
gotPid(await pid(t))
await sleep(1000)
resumed(settle(t`select pg_backend_pid() = ${ nextPid } as same`))
}), 5000)
await end(await txPid)
console.log(' begin() :', await tx)
const next = await sql.reserve()
nextPid = await pid(next)
console.log(' callback on next\'s session:', await late)
}
process.exit(0)Each case uses a pool of its own with On 3.4.9: With the change below: In case 2 the pool query still waits because Proposed changeThree hunks. The first is #1240's change (its hook named --- a/src/index.js
+++ b/src/index.js
@@ -210,14 +210,22 @@
closed.length && connect(closed.shift(), query)
})
+ let lost = null
move(c, reserved)
- c.reserved = () => queue.length
+ const own = c.reserved = () => queue.length
? c.execute(queue.shift())
: move(c, reserved)
- c.reserved.release = true
+ own.release = true
+ own.close = e => {
+ lost = e
+ while (queue.length)
+ queue.shift().reject(e)
+ }
const sql = Sql(handler)
sql.release = () => {
+ if (lost || c.reserved !== own)
+ return
c.reserved = null
onopen(c)
}
@@ -225,6 +233,8 @@
return sql
function handler(q) {
+ if (lost)
+ return q.reject(lost)
c.queue === full
? queue.push(q)
: c.execute(q) || move(c, full)
@@ -237,12 +247,18 @@
let savepoints = 0
, connection
, prepare = null
+ , lost = null
try {
await sql.unsafe('begin ' + options.replace(/[^a-z ]/ig, ''), [], { onexecute }).execute()
return await Promise.race([
scope(connection, fn),
- new Promise((_, reject) => connection.onclose = reject)
+ new Promise((_, reject) => connection.onclose = e => {
+ lost = e
+ while (queries.length)
+ queries.shift().reject(e)
+ reject(e)
+ })
])
} catch (error) {
throw error
@@ -290,6 +306,8 @@
function handler(q) {
q.catch(e => uncaughtError || (uncaughtError = e))
+ if (lost)
+ return q.reject(lost)
c.queue === full
? queries.push(q)
: c.execute(q) || move(c, full)
@@ -420,6 +438,7 @@
function onclose(c, e) {
move(c, closed)
+ c.reserved && c.reserved.close && c.reserved.close(e)
c.reserved = null
c.onclose && (c.onclose(e), c.onclose = null)
options.onclose && options.onclose(c.id)
--- a/src/connection.js
+++ b/src/connection.js
@@ -438,6 +438,7 @@
remaining = 0
incomings = null
clearImmediate(nextWriteTimer)
+ chunk = nextWriteTimer = null
socket.removeListener('data', data)
socket.removeListener('connect', connected)
idleTimer.cancel()
@@ -450,7 +451,9 @@
if (initial)
return reconnect()
- !hadError && (query || sent.length) && error(Errors.connection('CONNECTION_CLOSED', options, socket))
+ if (query || sent.length)
+ error(Errors.connection('CONNECTION_CLOSED', options, socket))
+ query = results = errorResponse = null
closedTime = performance.now()
hadError && options.shared.retries++
delay = (typeof backoff === 'function' ? backoff(options.shared.retries) : backoff) * 1000In the first hunk, In the second, the statement in flight on a reset has already been rejected by the socket's In the third, The diff has not been run against the package's own test suite. Not covered here#1195 (a |
|
The third bullet in the description, "a late query on the old handle could run on that caller's connection", is worth calling out on its own: it is a session-isolation issue, not only a hang or a crash. The stale handle reads and resets another caller's session state (settings, temp tables, an open transaction), and I measured it on master (411429e) and with this PR applied (7616069). Setup: Node 24.22, PostgreSQL 17 in Docker, Repro (standalone, import { pathToFileURL } from 'node:url'
const [entry, port, mode] = process.argv.slice(2)
const { default: postgres } = await import(pathToFileURL(entry).href)
const opts = { host: '127.0.0.1', port: Number(port), user: 'postgres', password: 'postgres', database: 'postgres', onnotice: () => {} }
let closed
const closedOnce = new Promise(r => (closed = r))
const sql = postgres({ ...opts, max: 1, onclose: () => closed() })
const admin = postgres({ ...opts, max: 1 })
const timeout = (p, ms, label) => Promise.race([p, new Promise((_, rej) => setTimeout(() => rej(new Error(label + ' pending after ' + ms + 'ms')), ms))])
process.on('uncaughtException', e => { console.log('UNCAUGHT', e.message); process.exit(1) })
// A reserves, sets session state, and its backend is terminated.
const a = await sql.reserve()
const [{ pid: pidA }] = await a`select pg_backend_pid() as pid, set_config('app.marker', 'A', false)`
await admin`select pg_terminate_backend(${ pidA })`
await timeout(closedOnce, 10000, 'onclose')
// B reserves: the pool reconnects the same Connection object for it.
const b = await sql.reserve()
const [{ pid: pidB }] = await b`select pg_backend_pid() as pid, set_config('app.marker', 'B', false)`
await b`begin`
await b`create temp table b_marker (x int)`
const bState = async () => {
const [s] = await b`select current_setting('app.marker', true) as marker, to_regclass('pg_temp.b_marker')::text as temp_table`
s.in_transaction = await b`savepoint probe`.then(() => true, e => e.code === '25P01' ? false : e.code)
return JSON.stringify(s)
}
try {
if (mode === 'read') {
const [r] = await timeout(a`select pg_backend_pid() as pid, current_setting('app.marker', true) as marker`, 3000, 'stale query')
console.log(`STALE_QUERY_RAN on pid=${ r.pid } (B is ${ pidB }) and read app.marker=${ r.marker }`)
} else if (mode === 'reset') {
await timeout(a.unsafe('rollback; reset all'), 3000, 'stale query')
console.log(`STALE_QUERY_RAN; B now sees ${ await bState() }`)
} else if (mode === 'release') {
a.release()
const r = await Promise.race([sql.reserve(), new Promise(res => setTimeout(() => res(null), 1000))])
if (r) {
const [x] = await r`select pg_backend_pid() as pid, current_setting('app.marker', true) as marker`
console.log(`STALE_RELEASE_REPOOLED: a third reserve() got pid=${ x.pid } (B is ${ pidB }) while B still holds it, app.marker=${ x.marker }`)
} else {
console.log('STALE_RELEASE_NOOP: a third reserve() stays pending while B holds the connection')
}
}
} catch (e) {
console.log(`STALE_HANDLE_REJECTED ${ e.code || e.name }: ${ e.message }`)
}
process.exit(0)Results
So on master, after a backend of a reserved connection is terminated (a server restart,
It takes a backend death while a handle is held, plus another This PR closes all three paths in my runs. Thank you for it. I'd suggest adding the (The crash variant of the same root cause is #1208 / #1209.) Disclaimer: this comment and the repro in it were drafted by @kloud-ai under human review and approval. Human in the loop: GravyaDev. |
## Issue Closes #154. Upstream: porsager/postgres#1208, #1199 (crash, dead connection released to the pool), #1226 (stale 57P01, `sql.end()` never settles); fix PR porsager/postgres#1240. This guard stays until postgres.js ships them. ## Why postgres.js throws an uncaught TypeError on a statement sent to a connection whose backend ended. The driver still sent `rollback` and the reset after such a failure, so `migrate` crashed the process. After that, `sql.end()` in the CLI never settled, so Node exited 13 without printing the error. ## What - When `fn` rejects with `CONNECTION_CLOSED` or `CONNECTION_DESTROYED`, the transaction and session send nothing more and rethrow. `backendAlive` can't judge this: it's false for any error `fn` throws itself. - A failed rollback now rejects with an AggregateError of it and `fn`'s error. - `Driver.session` notes that SQL `prepare` survives the reset, and that a pool kept open after such a failure may give a stale 57P01 (#1226). - The CLI's `run()` and `postgresJsConnect`'s `close` end with `end({ timeout: 0 })`: every statement has settled by then, and a graceful `end()` hangs (#1226). - A separate commit moves the client-option tests out of `driver.test.ts`, which hit 300 lines. ## Testing New tests end the backend mid-statement in a session, a transaction, `migrate` through `main()` (asserts exit 1 and CONNECTION_CLOSED on stderr), and a `connect()` close; each hangs or crashes without its fix. `pnpm check` passes.
…error (#359) ## Issue Closes #221. Stacked on #356. ## Why The driver judged a reserved connection dead by the error `fn` threw. After a socket reset (plain ECONNRESET), it sent `rollback` and the reset to the closed socket, which postgres.js 3.4.9 never answers. When `fn` rethrew another connection's `CONNECTION_CLOSED`, the driver skipped rollback and release and leaked a live connection. ## What - `reservedConnection` records the first statement that fails with `CONNECTION_CLOSED`, `CONNECTION_DESTROYED`, `ECONNRESET`, `EPIPE`, or `ETIMEDOUT`. Later statements reject with that error, unsent. - Session and transaction skip rollback, reset, and release only when their own connection recorded a failure. - Still open: the issue comment's third case, a backend that dies while `fn` sends nothing. That needs a close signal postgres.js doesn't expose (porsager/postgres#1240). Tracked in #364. ## Testing A TCP proxy (`tcp-proxy.test-helper.ts`) resets the socket mid-statement. Session, transaction, later-statement, and caught-error tests hung before the fix. A one-connection pool covers a self-made ECONNRESET and a foreign `CONNECTION_CLOSED`. Five mutations of the fix each fail a test. `pnpm check` passes.
Fixes #1199.
The defect
When the server terminates the backend of a reserved connection,
onclose()moves the connection to theclosedqueue and setsc.reserved = null. It can also reconnect it at once for a waiting query. TheReservedSqlhandle does not know any of this:c.execute()on a connection whose socket isnull.nextWrite()throws an uncaughtTypeError: Cannot read properties of null (reading 'write'), and the query never settles (the nextWrite() throws an uncaughtException when a reserved connection's backend dies #1208 symptom, reached throughreserve()).release()callsonopen(c)and puts the dead connection in theopenqueue. The next pool query goes to it and hangs (bug: pool never recovers after releasing a reserved connection whose backend was terminated. #1199).The fix
onclose()calls aclosedhook on the reservation before it clearsc.reserved. The hook records the close error and rejects the handle's own overflow queue. After that, the handle is dead for good:CONNECTION_CLOSED);release()does nothing, because the connection already belongs to the pool'sclosedqueue, which reconnects it as usual.Healthy reservations do not change.
Tests
Two new tests in
tests/index.jsagainst a real server:On a CI-like setup (PostgreSQL 17 with the workflow's
pg_hba.conf,ssl=on,wal_level=logical,max_prepared_transactions=100, and a second server on 5433; Node 20):node tests/index.jsdies with the uncaughtTypeErrorabove;test:esmand intest:cjs(264 existing + 2 new).Deno was not run locally.
Related open PRs
These fix nearby parts of the same close path and do not overlap with this change: #1209 (guard
nextWrite()itself), #1215 (reject transaction work after its connection closes), #1229 (a queuedreserve()stranded by a termination). This PR only covers the reserved handle andrelease().We have run this change as a local patch against 3.4.9 in production since 2026-09-30.
🤖 Generated with Claude Code