Skip to content

fix(patch): settle and space a connect-phase socket death - #222

Merged
Nitjsefnie merged 7 commits into
mainfrom
issue-164-connect-phase-end
Sep 7, 2026
Merged

fix(patch): settle and space a connect-phase socket death#222
Nitjsefnie merged 7 commits into
mainfrom
issue-164-connect-phase-end

Conversation

@Nitjsefnie

@Nitjsefnie Nitjsefnie commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

postgres.js's closed() handed the whole close to reconnect() while a connection was still opening, above both this repository's patched settle for a pending sql.end() and stock's own closedTime, retry counter and delay — so a connection whose socket died mid-connect left an awaited shutdown pending forever against a peer that accepts and closes without speaking the protocol, and issued its retries back to back. This settles and spaces that close, and closes the three further ways the resulting shutdown could still hand back a promise nothing would settle.

Related Issues and Pull Requests

Fixes #164
Fixes #223
Relates to #224
Relates to #226

Changes

  • patches/postgres@3.4.9.patch, the package's src/connection.js — a connect-phase close with a shutdown already pending rejects the startup query, which clears initial and lets the close continue down the same path an established connection takes; a connect-phase close with no shutdown pending records closedTime, advances the retry counter and computes delay before reconnecting, so attempts are spaced and a backoff function is asked for attempt 1, then 2, rather than attempt 0 forever.
  • patches/postgres@3.4.9.patch, the same file — reconnect() returns its timer handle, the connect-phase close keeps it and connect() spends it, so end() can cancel a retry that is merely scheduled instead of waiting for it to run.
  • patches/postgres@3.4.9.patch, the package's src/index.js — the pool's end() disposes of its queued work with the same CONNECTION_DESTROYED destroy() already uses, before the connections are told to end; and reserve() refuses with handler()'s own CONNECTION_ENDED once the client is ending.
  • pnpm-lock.yaml — regenerated patchedDependencies hash.
  • tests/db/connect-phase-death.test.ts — new suite covering the shutdown that overlaps the death, the retry schedule, the deliberate rejection when the server comes straight back, the retry that still serves the opening query when nothing is ending, the shutdown arriving between scheduled retries, and a live handshake that discriminates the cancel from a timeout.
  • tests/db/shutdown-backlog-drain.test.ts — new suite covering the drain and its ordering against a connection that cannot finish ending.
  • tests/db/reserve-contract.test.ts — a queued reservation is now refused by the shutdown rather than left pending, so the case that asserted it was still pending asserts the refusal.
  • patches/README.md — the sql.end() section rewritten to describe the shipped behaviour, with recomputed edit and hunk counts and the two remaining residues named by issue number.

Testing

The full CI order was run bare on the rebased head, in CI's own sequence — pnpm db:migrate && pnpm test --run && pnpm lint && pnpm typecheck && pnpm build — and exited 0. 113 test files, 2615 tests, all passed, zero skipped and zero todo, at a box load average of 2.5. Twenty-five of those files are testcontainers-backed and none of them skipped, so every one ran against a live postgres:17-alpine — including both new suites, all tests/db/closesql-*, and tests/db/reserve-contract.test.ts, whose known flake did not fire.

Three checks beyond the suite. The patch's sha256sum equals the hash: under patchedDependencies in pnpm-lock.yaml and the _patch_hash= of the resolved node_modules/postgres. All six commits carry the co-author trailer by exact string match. And the three installed module files were compared against the patch's own post-image blob ids, so the tree the suite ran against is byte-for-byte the tree this ships.

Each behaviour was pinned by watching it fail first, with the defect reintroduced into the real installed module rather than by reverting the patch, and the resulting cases were then attacked by mutation: the settle, the spacing, the retry counter, the cancellable timer, the drain and its ordering, and the reservation guard each have a mutant that kills the case named for them. Two coverage gaps found that way were repaired during the branch. One residue is disclosed rather than fixed — see Follow-ups.

Bugs Discovered

Breaking Changes

sql.end() now settles the pool's queued-but-undispatched work instead of leaving it pending, and this applies to healthy shutdowns as well as broken ones. Stock behaviour was worse than "abandons it": terminate() nulls the connection's ending, so the close behind it reached onclose and resurrected the connection to serve that backlog after sql.end() had already resolved. Queued work is now rejected with CONNECTION_DESTROYED, the same disposition destroy() gives it, and a shutdown opens no new sockets once it has resolved.

A reservation requested after the client is ending is likewise refused with CONNECTION_ENDED rather than queued, matching what an ordinary query in that position has always done.

Follow-ups / Known Limitations

  • The cancel covers a retry this connection scheduled, not one the pool scheduled for it: connection.connect(query) discards the handle reconnect() returns, so a shutdown inside that window still waits for the attempt to run. That window exists on main too and is short there only because main schedules every reconnect at delay zero — the same storm this pull request removes — so spacing the attempts lengthens it rather than creating it. Measured 43 ms on main against 17,099 ms here. Tracked as Closing the database clients waits for a scheduled reconnect that the shutdown cannot cancel #224, deliberately not fixed here: closing it by keeping that handle would reject a freshly dispatched query that both builds serve today, which the boundary case in tests/db/connect-phase-death.test.ts exists to protect.
  • Two reservation orderings still leave sql.end() itself pending — a reserve() in the same tick as end(), which wins end()'s own await 1 before ending is assigned, and a reservation held across end() and released afterwards, where release() returns the connection to the pool instead of closing it. Both reproduce identically on main, so they are stock rather than introduced here. Tracked as Closing the database clients never completes when a reservation is taken or released around the shutdown #226.
  • The drain's shape is held by construction rather than by an assertion: a mutation that never shifts the queue makes it an unterminating synchronous loop, which blocks the event loop so completely that no per-test timeout can fire either, leaving only an unnamed job timeout as the signal. That is a detection with a bad signal rather than no detection, and patches/README.md records it rather than leaving it implied. Making the loop terminate by construction — shifting into a local array before rejecting — would retire it, and is deliberately left out of a branch that is already reviewed and verified.
  • One guard's failure signal got worse, and it is the branch's strongest liveness guard. tests/db/reserve-contract.test.ts's shutting-down case used to name its defect in an assertion; now that the drain refuses the reservation first, the mutant that case exists to catch is caught only by the unbounded await sql.end() hanging to the suite timeout. A five-second poll was tried and reverted, because asserting that something did happen inside a window is the wrong direction on a suite this box is documented to stall for two minutes. The mutant and the timeout signature are named in the case's comment.
  • The patch now edits four functions across two dependency files. A future postgres upgrade is a real merge rather than a reapply, and patches/README.md carries the per-section edit counts and the drop-the-patch checklist for whoever does it.

Footer

Generated by Claude Opus 5 (brief, implementation, review, testing)

This was referenced Sep 7, 2026
Closed
Open
Open
Nitjsefnie and others added 6 commits September 7, 2026 15:51
@Nitjsefnie @claude
fix(patch): settle and space a connect-phase socket death
1118b35 
postgres.js's closed() handed the whole close to reconnect() as soon as the
connection was still opening, above everything that makes a close observable:
the settle this repository's patch added for a pending end(), and stock's own
closedTime, retry counter and delay. A pooled connection whose socket died
mid-connect therefore left an awaited sql.end() pending forever whenever every
retry was then accepted and closed with no error event and no protocol message
-- what a pooler, a TCP load balancer or a service address does while its
backend is gone -- and issued those retries back to back, 32,605 connections in
90 seconds on the report.

A close that finds a shutdown already pending now rejects the startup query,
which is also what clears `initial`, so the close continues down the same path
an established connection takes: settle the shutdown, then hand the connection
back. The query is rejected rather than replayed on a retry even when the
server is reachable again, matching what an established connection does with an
in-flight query whose socket died, and leaving nothing armed for a
shutting-down process to wait on. A close with no shutdown pending still
reconnects, now scheduled through closedTime and delay with the retry counter
advanced, so a backoff function is asked for attempt 1, then 2, rather than for
attempt 0 forever.

The regression test drives the library client through a local TCP proxy in
front of a container and asserts on records -- the proxy's accept count, the
attempt numbers backoff was asked for, the order the two settlements happened
in -- rather than on elapsed time. Its four cases cover the shutdown that
overlaps the death, the retry schedule, the deliberate rejection when the
server comes straight back, and the retry that still serves the opening query
when nothing is ending.

Overflow issue 164.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie @claude
fix(patch): settle a shutdown that arrives between scheduled retries
6c225aa 
Spacing connect-phase retries made the settle they share an edit site with
conditional on a wait it had just lengthened: a pending end() rode the next
scheduled attempt, so a shutdown issued while the client was merely waiting
blocked for a whole backoff interval. Measured at the production pool size,
against a peer that accepts and closes, with the shutdown between retries:
16,859 ms with the cancel removed and 2 ms with it, where the same shape
before this branch took 17-20 ms.

reconnect() now hands its timer back, the connect-phase close keeps the
handle, and end() cancels a scheduled attempt when nothing else is in flight
-- which turns that state into the same nothing-in-flight case terminate()
already settles at once, with the same rejection for the startup query it
strands. The handle is only ever armed by a close, never by a first connect,
so a query the pool has just handed to a fresh connection still runs:
recognising the state by the absence of a socket instead would reject that
one too, and a new case pins the boundary.

Two claims in the neighbouring comments were false and are rewritten. error()
does not always reach errored(): it returns ahead of it on the multi-host
route, where `initial` survives a socket error and the close then arrives with
hadError set. The ending arm is safe for a different reason -- only end()
assigns `ending`, and it moves the connection out of queues.connecting before
it does -- and the two retry-counter forms agree wherever hadError is true.

The case asserting that no retry follows a shutdown against a healthy peer now
holds its quiet window, so a build that settled, rejected and still rearmed
cannot pass it on a snapshot taken before that retry fires.

Overflow issue 164.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie @claude
fix(patch): dispose of queued work when a client is ended
a1ae2d6 
Two subjects in one patch file, one per issue.

Overflow issue 223: the pool's end() now settles the work it accepted and never
dispatched, with the CONNECTION_DESTROYED that destroy() already uses, before
the connections are told to end. Stock left that queue alone, which is not the
same as leaving it pending -- terminate() nulls a connection's `ending`, so the
close behind it reaches onclose with the backlog still in the queue and
resurrects the connection to serve it after sql.end() has resolved; against an
unreachable server the same work is never settled at all. Both were observed on
the previous build: the new suite's first case was served by a resurrected
connection, its second timed out. A caller cannot tell those outcomes apart,
and neither of them is a shutdown being over.

Cancelling the retry is what exposed it. That cancel removed the last close,
and the last close was what had been incidentally disposing of the backlog: at
the production pool size, five queries of fifteen were left permanently
unsettled.

The reserve-contract case that asserted a queued reservation was still pending
after its shutdown resolved now asserts that it is refused. What that case
exists to catch is unchanged and still caught: with the startup arm mutated to
hand an ending pool's connection to the reserve, it times out on end() exactly
as it did before.

Overflow issue 164: the cancelled retry gets the assertions it was missing.
Five mutations of the cancellable-timer hunk passed the previous suite --
deleting the clearTimeout, nulling the handle without cancelling, reconnect()
returning nothing, rearming right after cancelling, and connect() no longer
spending the handle -- because the case watched a 300 ms window against a
30 second delay, which cannot tell a cancelled retry from one that was merely
outrun. The window is now longer than the delay rather than a fraction of it,
the attempt numbers backoff was asked for are asserted beside the accept count,
and a new case holds a live handshake open so that a spent handle cannot be
mistaken for a scheduled one. All five fail now.

The comment claiming the cancel reaches every scheduled retry is corrected: the
pool's own connection.connect() discards the handle reconnect() hands back, so
a shutdown arriving inside that window still waits it out.

Overflow issues 164 and 223.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie @claude
fix(patch): refuse a reservation once the client is ending
a97d4b2 
Overflow issue 223, second half. handler() consults the pool's `ending` flag
and reserve() did not, so a reservation requested after the shutdown was pushed
into a queue nothing would dispatch and nothing would drain -- end() drains
once, and before that flag is set. Against an unreachable server that left a
promise no build settles; against a reachable one it was worse, because the
pool opened a fresh socket for it after the shutdown had already resolved.
reserve() now refuses with the CONNECTION_ENDED an ordinary query in the same
position already gets, which was verified rather than assumed.

The drain's ORDERING is now pinned as well as its operation. Moving it after
Promise.all(connections.map(c => c.end())) survived every case, because they
all used a reachable client whose connections finish ending at once. The new
case gives it a subject that cannot: a listener that accepts the handshake and
never answers, so the connection's own end() cannot settle until its
connect_timeout, and the backlog has to be disposed of before that rather than
behind it.

The reserve-contract case that guards the startup arm gets its named failure
back. Since the drain refuses the queued reservation either way, the reservation
is no longer the discriminator, and the case had been left resting on an
unbounded await that fails as a bare timeout. It now bounds the absence itself
and asserts on the record: with the arm mutated to hand an ending pool's
connection to the reserve, it reports `expected false to be true` in five
seconds instead of timing out at two minutes.

Three test comments corrected: the drain case now says which mutations it
catches and that it catches them by hanging, the handshake case has the window
its no-rearm claim needs, and the residue the cancel deliberately does not
reach is attributed to Overflow issue 224 in both the patch and the suite.

Overflow issues 223 and 224.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie @claude
test(db): restore the unbounded shutdown await and correct three comm…
2abe907 
…ents

The reserve-contract case that guards the startup arm goes back to an unbounded
`await sql.end()`. Bounding it and asserting the record read the right way round
but the wrong direction: this repository forbids asserting a wall-clock margin
and allows only the negative form, and a poll asserting the shutdown DID settle
within five seconds is the positive one, on a suite this box is documented to
stall. The code assertion stays; the comment now names the mutation the case
exists to catch and says its signature is the per-test timeout rather than an
assertion, which is the form `closesql-shutdown-before-death.test.ts` already
chose for a shutdown that never settles.

The drain case's comment claimed the assertion-shaped failure for a drain that
rejects without shifting comes from `reserve-contract.test.ts`. It does not.
Driving the dependency's own `Queue` shows the loop terminates only while every
entry removes itself: a queue holding one self-removing reserve empties in a
single pass and stays green, and this file's queue -- a reserve and an ordinary
query behind it -- spins synchronously and forever, taking the event loop with
it, so not even the per-test timeout fires. The comment now says what the case
does catch, which is a wrong entry, a wrong count or a wrong code, and that the
shape of the drain is held by construction rather than by the case.

The ordering case's comment claimed its list assertion pins the ordering. Every
drain mutation dies on the await above it, and all of them print the same text;
what pins the ordering is which cases fail -- the moved drain fails that case
alone, the deleted drain fails four. The comment now says that instead.

Overflow issues 164 and 223.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie @claude
docs(patches): describe the shutdown behaviour the patch now ships
b8a6ebe 
The postgres patch's README described the connect-phase case as an open gap and
stated counts that four commits have moved. It now describes what ships.

Counts recomputed from the patch rather than carried over: sixteen edits
rendered as twelve hunks, with the two hunks that carry three edits each named,
and the per-section counts adjusted to sum to sixteen. Housekeeping's "all eight
hunks land in src/" follows, along with the list of what the unpatched cjs and cf
builds still do.

The `sql.end()` section gains three subsections -- the connect-phase death, the
delay-zero reconnect loop, and the shutdown that arrives between two scheduled
attempts -- each naming the test file that holds it. The upstream comparison
moves below them and says what pull request 1142 does not carry. A new section
covers the sixth defect: a shutdown that reports itself finished while work the
pool accepted is neither run nor refused, its drain, and the reservation refused
once the client is ending.

What remains uncovered is described rather than dropped: the pool's own
first-connect window, which the cancel deliberately does not reach (Overflow
issue 224), and two reserve orderings that leave `sql.end()` pending and
reproduce identically on `main` (Overflow issue 226).

One claim is strengthened rather than softened. The multi-host route the caveat
records has since been measured with a shutdown pending, settling in 2 to 3
milliseconds in both orderings, which is what the connect-phase hunk predicts.
The paragraph now records the measurement and says what it does not establish:
the uncovered multi-host case, with no shutdown pending, is still read off the
package's source rather than run, and the route stays unfiled.

Overflow issues 164, 223, 224 and 226.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie
Nitjsefnie force-pushed the issue-164-connect-phase-end branch from eb614a5 to b8a6ebe Compare September 7, 2026 13:51
@Nitjsefnie @claude
docs(patches): say the retry counter is pool-wide and name the drain'…
e2df2ff 
…s signal

Two accuracy gaps from the whole-branch review, in the README only.

The retry counter's sharing was never stated. `options.shared` is one object per
client, so `options.shared.retries` is advanced by any connection's close and
read by every connection's next backoff(): the spacing one connection earns is
the spacing they all get. The sharing is stock's, not this patch's — stock
already advances it on an errored close from any connection — but this patch
widens what advances it to the clean FIN a connect-phase close is, so the
consequence now scales with max. A pool of ten failing to open reaches the
default backoff's 10-to-20-second cap inside its first round, and the section
says so, along with why that trade is the one the storm asks for and how a
caller changes it.

The drain's residue said rejecting without shifting spins the event loop, but
not what a reader would actually see. It is detected — as a run that never
finishes, because the timers a per-test timeout needs are on the loop the spin
has taken, leaving the workflow's own 45-minute job limit as the signal. That is
a detection with a bad signal rather than a gap, and the difference is what a
future reader needs: shifting into a local array before rejecting would retire
the question instead of renaming the failure, and the paragraph says why the
form that matches destroy() line for line was kept instead.

Overflow issues 164 and 223.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nitjsefnie
Nitjsefnie marked this pull request as ready for review September 7, 2026 14:10
@Nitjsefnie
Nitjsefnie merged commit 55e5c7f into main Sep 7, 2026
2 checks passed
@Nitjsefnie
Nitjsefnie deleted the issue-164-connect-phase-end branch September 7, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

1 participant

@Nitjsefnie