fix(client): bound initialized handshake timeout - #3079

🦋 Changeset detected

Latest commit: 1a45dc8

The changes in this PR will be included in the next version bump.

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

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/client@3079

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3079

npm i https://pkg.pr.new/@modelcontextprotocol/core@3079

npm i https://pkg.pr.new/@modelcontextprotocol/server@3079

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3079

npm i https://pkg.pr.new/@modelcontextprotocol/express@3079

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3079

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3079

npm i https://pkg.pr.new/@modelcontextprotocol/node@3079

commit: 1a45dc8

Read the diff and the two tests. The race is set up correctly (listener and timer both cleaned in finally, Promise.race keeps the losers from becoming unhandled rejections, and the first test covers a transport that ignores requestSignal). Three points:

1. The budget restarts. notificationTimeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC is a fresh full timeout for the last leg, while initialize already got its own. With connect(t, { timeout: 5_000 }) the handshake can now take up to ~10 s before failing. If timeout is documented as the bound for connect(), compute the remainder from a start time taken before initialize; if it is per-request, the changeset line and the comment should say so.
2. A caller abort is reported as RequestTimeout. toConnectAbortError wraps any non-SdkError reason in new SdkError(SdkErrorCode.RequestTimeout, String(reason)), and the second test pins it (controller.abort(new Error('connect cancelled')) expects code: RequestTimeout). Callers who branch on the code to decide whether to retry will treat a deliberate cancel as a timeout. Is there an abort/cancel code in SdkErrorCode to use, or can the original reason be passed as cause?
3. Edge: the options?.signal?.aborted check happens after this._instructions is set but before the notification, and the throw leaves no listener behind, which is fine; but I could not see in the diff whether connect() closes the transport on this new failure path. A test asserting transport.close was called (or not) would document it, since a half-open session that already received initialize is the state a retrying caller will hit.

@arhancanli Thanks for the review. Addressed the three points in d7a104f and 753157b:

1. Timeout budget: Kept a separate timeout for each legacy handshake stage, consistent with the existing per-request semantics. The connect() JSDoc, inline comment, and changeset now state this explicitly. Added a test verifying that initialized gets a fresh budget after initialize uses part of its timeout.

2. Caller cancellation: The initialized stage now rejects with the original abort reason and passes that same reason to the transport signal. initialize still follows the existing request() behavior, which wraps non-SdkError reasons as RequestTimeout. I kept that broader behavior unchanged to limit this PR’s scope, and documented the distinction. A cross-stage test records the current boundary using the same Error object.

3. Transport cleanup: The existing handshake failure handler already calls close(). Added assertions covering timeout, in-flight cancellation, and cancellation between stages. The last case also verifies that no initialized notification is sent.

Validation: all 1,085 client tests, typecheck, and lint pass. Temporarily restoring timeout wrapping in the initialized stage makes the new cross-stage test fail as expected.

Could you take another look when you have a chance?

Re-read 753157b against the three points.

1. Budget: the JSDoc, the inline comment and the changeset now all say the timeout is per stage, and the fake-timer test (initialize takes 80 ms of 100, initialized still gets the full 100) pins it. That settles it for me.
2. Abort reason: confirmed that requestSignal reaches the transport (_notificationViaCodec forwards options whole to transport.send), and the initialized stage now rejects with the caller's own reason, including the string and DOMException cases. The cross-stage test makes the asymmetry explicit, which is the honest way to leave it.
3. Cleanup: closeCalls is asserted on the timeout, in-flight abort and between-stages paths, and the last one also asserts no initialized was sent.

Two small things, neither blocking:

• timeoutError is built before the timer starts, so its stack trace points at the setup, not at the moment of the timeout. Constructing it inside the setTimeout callback gives the stack the same shape as other RequestTimeout errors.
• The asymmetry in (2) means a caller who branches on code === RequestTimeout sees a cancel during initialize as a timeout but a cancel during initialized as their own reason. Worth a follow-up issue so request() can stop wrapping non-SdkError reasons, if the maintainers agree.