fix: end() hangs forever after ECONNRESET error

[ftonato]

Problem

When a query fails with ECONNRESET (or any connection error), calling pg.end() hangs forever. This happens because:

1. When a connection error occurs, the query variable in the connection is never cleared
2. In the normal path, ReadyForQuery clears query = null, but connection errors never reach that handler
3. When end() is called, it checks !query to determine if it can terminate immediately
4. Since query still references the rejected query object, the condition fails and end() creates a promise waiting for ended to be called
5. However, ended is only called in terminate(), which is only called when !query is true - creating a deadlock

Solution

I've made two changes to ensure the query variable is properly cleared when connection errors occur:

1. In errored() function: Clear query = null after rejecting the query
2. In closed() function: When hadError is true, clear the query before closing (since errored() might not always be called in all error paths)

Changes

• src/connection.js:
  • Modified errored() to clear query after queryError()
  • Modified closed() to clear query when hadError is true
• tests/index.js: Added test 'end() completes after ECONNRESET error' to verify the fix

Testing

The new test creates a real PostgreSQL connection, terminates the backend during a query execution, and verifies that end() completes quickly (within 1 second) instead of hanging forever.

Notes

I'm not entirely sure if this is the best approach - I spent a significant amount of time understanding the codebase and the problem. The connection lifecycle and query state management is quite complex, and I wanted to make sure I understood the flow before making changes. I also received help from AI to better understand the problem and explore the codebase, which was invaluable in identifying where the query variable needed to be cleared.

I'm open to feedback and alternative approaches if there's a better way to handle this!

Fixes #1130

[AceCodePt]

This condition is slightly weird to me. If (we had an error or sent.length isn't 0) and query is empty then call error which will clean the query regardless right?

[AceCodePt]

Wouldn't your code just mean that regardless if we had an error or not we should just clear the queue (which error function does) and show econreset?

[AceCodePt]

Suggested change

	if (hadError) {
	// Clear query when connection closes with error (e.g., ECONNRESET)
	query && (queryError(query, Errors.connection('CONNECTION_CLOSED', options, socket)), query = null)
	while (sent.length)
	queryError(sent.shift(), Errors.connection('CONNECTION_CLOSED', options, socket))
	} else {
	(query || sent.length) && error(Errors.connection('CONNECTION_CLOSED', options, socket))
	}
	(query || sent.length) && error(Errors.connection('CONNECTION_CLOSED', options, socket)) //and query=null somewhere

[Nitjsefnie]

Independent confirmation of this from a separate investigation, plus one observation about the second hunk and one about a sibling case this does not reach.

The errored() hunk is right, and it is sufficient on its own. We hit the same hang on 3.4.9 and traced it to the same place before finding this PR: errored() rejects the in-flight query but leaves query set, ReadyForQuery (the only other writer of query = null) never arrives once the backend is gone, so end() fails its !connection.reserved && !initial && !query && sent.length === 0 guard, returns ending = new Promise(r => ended = r), and nothing reaches the terminate() that would resolve it. sql.end() awaits every connection with no default timeout, so one stranded connection strands the client.

We are shipping exactly the one-line change:

query && (queryError(query, err), query = null)

pinned as a package patch, and running it in production. Reproduction is a real backend killed mid-query with pg_terminate_backend(pg_backend_pid()); the shutdown hangs indefinitely before the change and settles in a few hundred ms after it. The change is symmetric with the initial handling on the very next line, which is part of why it reads as the intended shape.

The closed() hunk looks unnecessary. socket.on('error', error) at connection.js line 291 means closed(true) is only ever reached after error() — and therefore errored() — has already run, so with the first hunk in place query is null by then and sent has already been drained by error(). We could not construct a path that reaches closed(hadError = true) without errored() having run first. We left that hunk out and everything we can exercise still passes. Not an objection to including it defensively, just a data point if its presence is part of what has held this up.

We also checked the other paths through errored() for the clearing being harmful rather than merely redundant. The ReadyForQuery call sites fall through to query = results = errorResponse = null immediately afterwards, and the retry() branch does not go through errored() at all, so prepared-statement retry is untouched. terminate()'s if (stream || query || initial || sent.length) guard simply stops re-erroring an already-rejected query. The one place worth naming in review: in data()'s catch the message loop keeps handling messages already in the buffer, and handlers that dereference query (ParseComplete, CommandComplete, DataRow) now throw TypeErrors the same catch swallows. The connection still recovers at the next ReadyForQuery, and operating on an already-rejected query was not better, but that is the behavior change a reviewer will find.

A sibling case survives this fix. The reverse interleaving — end() called while a query is in flight, backend dies afterwards — still never settles, with this patch applied. end() has already committed to the ending = new Promise(...) branch, and ended is invoked only from terminate(), whose reachable call sites are end()'s own fast path and three arms of ReadyForQuery. closed() calls error() and onclose() and never terminate(), so once the socket is gone the promise end() returned has no resolver left. Verified against a patched build: sql.end() called during pg_sleep(30), backend then terminated from a second connection, in-flight query rejects with CONNECTION_CLOSED, and the shutdown is still pending 20s later.

That is a separate fix — presumably closed() settling a pending end() — and it is not a reason to hold this one. Mentioning it because #1097's title says "after a server disconnection", and someone reading only that may assume this PR closes the whole shape.

[Nitjsefnie]

Correcting my earlier comment: this hunk has a silent-wrong-answer regression, and we no longer ship it in this form. I said the errored() change was sufficient on its own. It is sufficient for the hang, but it is not safe where it sits, and I would rather say so here than leave the endorsement standing.

errored() is not only reached when the connection is gone. execute()'s catch (connection.js lines 178-181) reaches it when build() throws — an undefined parameter (UNDEFINED_VALUE), MAX_PARAMETERS_EXCEEDED, NOT_TAGGED_CALL. That path writes a Sync and returns true, and the connection is still alive, so the server answers the Sync with a ReadyForQuery.

Stock keeps the rejected query pinned in the query slot until that ReadyForQuery (line 550), so a query pipelined meanwhile lands in sent (line 169) and is picked up correctly at line 573. With the slot cleared in errored(), the next query takes it (line 170), the Sync's ReadyForQuery resolves that query at line 544 with the empty result, and its real rows then arrive against query === null and are swallowed by the data() catch. The pool does dispatch this way: handler at index.js lines 329-341 routes to open then busy, and go() moves a connection to busy whenever execute() returns true, which it does at line 181 after the throw.

Minimal reproduction — one connection, max: 1, warm, then two statements back to back:

const a = sql`select ${undefined}`.catch(e => `rejected(${e.code})`)
const b = sql`select 42 as value`

stock   first=rejected(UNDEFINED_VALUE) second=[{"value":42}]
patched first=rejected(UNDEFINED_VALUE) second=[]

The first statement is an application bug that reports itself through its own rejection. The second is innocent and gets a wrong answer with no error anywhere. On a shared pool under a request path that is worse than the hang the change fixes, because the hang is at least visible.

What works instead: leave errored() stock and clear the slot in error(), immediately after its errored(err) call (line 385). error() is reached only when the connection is gone or being torn down — the socket error listeners (lines 139 and 291), closed() (453), terminate() (425), and connected()'s catch (377) — so no ReadyForQuery can follow it and clearing there is safe, while every live-connection path keeps its pin. Both behaviors then hold together:

first=rejected(UNDEFINED_VALUE) second=[{"value":42}]     # build-throw neighbour intact
kill=rejected(CONNECTION_CLOSED) end settled              # the hang this PR fixes

We have this pinned as a package patch and covered by two container tests, and each one fails on the placement the other requires — which is the part that convinced me the placement is load-bearing rather than a matter of taste.

One caveat if you take this route: error() returns early at lines 382-383 when connection.queue === queues.connecting && options.host[retries + 1], the multi-host fallback. On that branch errored() never runs, so a closed(true) can still be reached with query set. Single-host connections cannot reach it, and we have not tried to construct the multi-host case; it may want handling before this lands generally.

This does not change anything about the diagnosis in the PR description, which is correct and was ahead of everyone else — only where the assignment belongs.