fix(client): destroy pool clients that fail to close - #3437
Conversation
2732fa9 to
e10da39
Compare
close() awaited Promise.all() over each idle client's close() inside a try/catch with an empty catch block. One client failing to close skipped everything after it: #idleClients and #clientsInUse were never reset, and the rejection never reached the caller. Switched to Promise.allSettled and moved cleanup into a finally block, so every client gets processed and the pool resets regardless of individual failures. A client whose close() rejects is destroyed, and close() rethrows the first rejection instead of discarding it. Also documents this in docs/pool.md.
e10da39 to
97ed810
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97ed810868
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this._self.#drainResolve = undefined; | ||
| this._self.#isClosing = false; | ||
| this._self.#isOpen = false; | ||
| this._self.#clientSideCache?.onPoolClose(); |
There was a problem hiding this comment.
When a pooled client's close() rejects and a custom PooledClientSideCacheProvider.onPoolClose() also throws, this call in the finally block replaces the original client-close rejection. That contradicts the documented behavior that pool.close() rejects with the client close error and defeats the nearby attempt to preserve that error when forced destruction fails; retain the first close failure while still attempting cache cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in 51a84e5. onPoolClose() is now wrapped in its own try/catch so a cleanup failure can't replace the client close error, same as the destroy() fallback right above it.
| for (const result of results) { | ||
| if (result.status === 'rejected') throw result.reason; |
There was a problem hiding this comment.
When two callers invoke pool.close() while shutdown is in progress, only the first caller can receive the newly propagated client-close error here; the second hits the existing #isClosing early return and immediately receives a fulfilled promise, potentially before shutdown finishes. This makes identical concurrent shutdown requests report contradictory outcomes, so callers should share the active close promise (including its rejection) rather than treating an in-progress close as successful.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — fixed in 4ea2a0e. close() now stores its in-flight promise and returns it to a concurrent caller instead of resolving early.
A second caller invoking close() while the first was still in progress hit the #isClosing guard and resolved immediately, independent of how the first call's close() turned out. Once close() could reject, this meant concurrent callers could see contradictory outcomes for the same shutdown: one rejected, the other resolved before the pool had actually finished closing. close() now stores its in-flight promise and returns that to a concurrent caller instead of resolving early.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
Hi @GiHoon1123, this looks great, thanks!
one small nit:
#isClosing and #closePromise now change together: both are set in close(), both cleared in the finally. They can never disagree, so the flag is redundant. The isClosing getter can just return #closePromise !== undefined, and the field can go (the four internal reads route through the getter)
51a84e5 to
11b19c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11b19c605e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| this._self.#isClosing = true; | ||
| clearTimeout(this._self.cleanupTimeout); | ||
| this._self.#closePromise = this._self.#doClose(); |
There was a problem hiding this comment.
When an idle client's synchronous shutdown hook re-enters the pool—for example, an end listener emitted by client.close() calls pool.execute()—the RHS starts #doClose() before this assignment stores the promise. During that hook, isClosing is therefore false while #isOpen remains true, so the new task is accepted even though shutdown has started; #doClose() has already passed its drain check and can reset the client lists and settle while that task remains active. The fresh evidence in the current code is this pre-assignment execution window, so the closing state needs to be published before any client-close hooks run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a8f5a8. close() publishes the in-flight promise before #doClose() can run client shutdown hooks, and the regression test covers re-entrant work from a close hook.
11b19c6 to
6d3e3d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3e3d6028
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this._self.#isOpen = false; | ||
| this._self.#closePromise = undefined; |
There was a problem hiding this comment.
When a custom PooledClientSideCacheProvider.onPoolClose() synchronously calls pool.connect(), these assignments have already made both isOpen and isClosing false, so connect() accepts the request and starts new clients before the original close() has settled. Since no state reset occurs after the hook, close() can return with the pool open again; run the hook before clearing the closing state and only mark the pool closed afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a8f5a8. onPoolClose() now runs while the pool is still closing; the pool state is reset only after the hook returns. Added a regression test for a hook that calls connect().
6d3e3d6 to
5d50fcd
Compare
onPoolClose() ran directly inside the finally block. A throw from a custom PooledClientSideCacheProvider there replaced any client close error already in flight, since a throw inside finally overrides the exception it was handling. close() would then reject with the cache cleanup error instead of the real client close failure.\n\nonPoolClose() is now wrapped in its own try/catch, matching the existing destroy() fallback above it: attempt cleanup, but never let its failure hide the original close error. The pool stays in its closing state until the hook returns, so a synchronous hook cannot reopen it with connect().
5d50fcd to
2a8f5a8
Compare
|
Thanks for the careful review. I’ve addressed the review feedback and the maintainer’s suggestion: the close promise is published before shutdown hooks run, the pool remains in its closing state during cache cleanup, and the redundant #isClosing field has been removed. I also added regression coverage for the re-entrant close and connect paths. |
2 participants
close() awaited Promise.all() over each idle client's close() inside a try/catch with an empty catch block. One client failing to close skipped everything after it: #idleClients and #clientsInUse were never reset, and the rejection never reached the caller.
Switched to Promise.allSettled and moved cleanup into a finally block, so every client gets processed and the pool resets regardless of individual failures. A client whose close() rejects is destroyed, and close() rethrows the first rejection instead of discarding it.
Two follow-ups from review: a concurrent close() call while one was already in progress used to resolve immediately regardless of the outcome, so close() now shares its in-flight promise with concurrent callers. And a cache cleanup failure in the finally block could replace a pending client close error (a throw in finally overrides the exception it's handling), so cache cleanup is now wrapped in its own try/catch.
Tests: one client already closed makes close() reject while the pool still resets; a second client's close() is delayed to check it still gets waited on; concurrent close() calls observe the same outcome; a failing cache cleanup doesn't hide the real close error.
Note
Medium Risk
Changes lifecycle teardown for pooled connections (close/drain/cache), which can affect shutdown ordering and error propagation in apps using credentials providers or client-side cache.
Overview
pool.close()no longer swallows per-client close failures or leaves the pool half-torn-down. Client shutdown usesPromise.allSettledso every idle client is closed (or cleaned up) before the pool resets; a rejectingclose()triggersdestroy()on that client and the promise rejects with the first close error instead of resolving after an empty catch.Concurrent
close()calls now share one in-flight promise (via#closePromise, replacing a separate#isClosingflag) so later callers get the same resolve/reject outcome. Pool teardown (idle/in-use lists,isOpen, client-side cacheonPoolClose) runs in afinallyblock so it always runs; cache cleanup errors are caught so they do not mask the underlying client close error.docs/pool.mddocuments graceful shutdown,ClientClosedErrorfor new work while closing, and handling close rejections.pool.spec.tsadds coverage for partial failures, concurrent close, credential disposal errors, cache cleanup, and reentrantexecuteduring close.Reviewed by Cursor Bugbot for commit 2a8f5a8. Bugbot is set up for automated code reviews on this repo. Configure here.