fix(client): opt-in graceful close drains in-flight requests before transport teardown - #2750
fix(client): opt-in graceful close drains in-flight requests before transport teardown#2750K4bain wants to merge 2 commits into
Conversation
…ransport teardown
close({ drainPendingRequests: true }) waits for in-flight requests to
settle before the transport closes. Without it, transport teardown
aborts in-flight HTTP requests the server had already answered, which
OpenTelemetry's undici instrumentation reports as UND_ERR_ABORTED on
200 OK responses (modelcontextprotocol#1231).
- Protocol tracks pending request ids alongside the response-handler
lifecycle and drains them before transport close when opted in
- Client.close({ drainPendingRequests }) and a ClientOptions.gracefulClose
constructor default expose the behavior; an explicit argument wins
- Requests outstanding after the drain timeout (default 2s) settle via
the normal close path; default close() behavior is unchanged
🦋 Changeset detectedLatest commit: 7e9700a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
The open brace in {@linkcode Protocol.close | close({ drainPendingRequests })}
is flagged by typedoc ('Encountered an open brace within an inline tag'),
which fails the docs:check CI gate. Use plain backticks for the call form.
|
Status check |
Ricky-7-Yan
left a comment
Ricky-7-Yan
left a comment
There was a problem hiding this comment.
I found one concurrency issue in the drain waiter bookkeeping; details are inline.
| resolve(drained); | ||
| }; | ||
| const timer = setTimeout(() => finish(false), timeoutMs); | ||
| this._drainNotify = () => finish(true); |
There was a problem hiding this comment.
[P1] Preserve every concurrent graceful-close waiternn_drainNotify is a single slot, so overlapping close({ drainPendingRequests: ... }) calls overwrite each other. For example, close A installs waiter A, close B installs waiter B, then A times out and its finish() sets _drainNotify = undefined; when the last request settles, B is no longer notified and waits until its own timeout despite the drain being complete. The inverse ordering also leaves A waiting unnecessarily. This can happen when signal handling and framework/finally teardown race. Please keep a set of waiters (removing only the finishing waiter), or share one drain promise while preserving each caller's timeout semantics, and add a regression with two concurrent graceful close() calls.
2 participants
Closes #1231.
What
Opt-in graceful close at the
Client.close()level — the exact direction proposed by @felixweinberger on #1692 ("I'd rather see that as an opt-in onClient.close()(which already has the pending-request registry in_responseHandlers) than a mandatory transport-level drain"):Why
Without draining, transport teardown aborts in-flight HTTP requests that the server may have already answered. OpenTelemetry's undici instrumentation then reports those as
UND_ERR_ABORTED— failed spans on 200 OK responses — the symptom in #1231. Draining lets the response finish reading so telemetry reflects the real outcome.This deliberately differs from the closed attempts #1569/#1692 (mandatory transport-level drain) and composes with the open #2028 (per-request controller decoupling at the transport layer) — the two are complementary, not competing.
How
Protocol(core-internal) tracks pending outbound request ids in a_pendingRequestIdsset alongside the existing response-handler lifecycle: registered before the send so a concurrent close sees the request, released on every exit path (response, timeout, cancel, caller abort, close) via the request funnel's existingfinallycleanup.Protocol.close(options)gainsdrainPendingRequests: waits for the pending set to empty (event-driven wake, never rejects), then closes the transport. Requests still outstanding after the drain timeout fall through to the normal close path and settle with the existing connection-closed error — stuck-request cleanup is preserved exactly as before.Client.close(ClientCloseOptions)resolves precedence: explicit argument >ClientOptions.gracefulCloseconstructor default > immediate close (unchanged historical behavior).subscriptions/listensends directly on the transport and does not enter the pending set, so long-lived subscriptions never stall a drain._oncloseclears the pending set, so a drain started before an unexpected transport drop resolves immediately rather than waiting out its timeout.Tests
7 new regression tests (
gracefulClose.test.ts): default close unchanged (in-flight request still settles with connection-closed), drain-then-close ordering (transport provably still open while the request is outstanding), multiple in-flight requests (close waits for the last one), drain-timeout fallback, immediate close when idle, constructor default applying to parameterlessclose(), and explicitfalseoverriding the constructor default.Verification: client suite 881/881 green; core-internal suite green except the 2 pre-existing
schemaTwinConformancebyte-identity failures that also fail on cleanmain; typecheck green; prettier clean on all touched files.A changeset is included (
@modelcontextprotocol/client+@modelcontextprotocol/core-internalpatch).