From 1881e5c73f551080f3509fbc22e933efe4bf31ce Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 17:02:05 -0400 Subject: [PATCH 1/4] fix: settle the deliberately in-flight tool calls in the progress tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two progress tests fire `client.callTool()` without holding the promise so they can assert on the notifications the call streams while it is still in flight. `disconnect()` then closes the SDK client, which rejects every pending request with "Connection closed" — with nothing holding the promise that lands as an unhandled rejection, which vitest counts as a run error and fails `npm run ci` at `coverage` even though all tests pass. Add a `settleInFlight()` helper that attaches the handler at call time (not after the assertions, which would leave a window the rejection can escape through) and returns a promise the test awaits after `disconnect()` so teardown stays ordered. Neither outcome is asserted: the call may legitimately complete before the teardown or reject with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --- .../integration/mcp/inspectorClient.test.ts | 67 +++++++++++++------ 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 7937b219..3586e51f 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -95,6 +95,29 @@ async function getTool(client: InspectorClient, name: string): Promise { throw new Error(`Tool ${name} not found`); } +/** + * Hold a deliberately un-awaited in-flight call so its rejection is handled. + * + * A few tests start a tool call, assert on the notifications it streams, and + * then tear the connection down while the call is still in flight. That is a + * legitimate thing to exercise, but `disconnect()` closes the SDK client, which + * rejects every pending request with "Connection closed" — and a floating + * promise makes that an *unhandled* rejection, which vitest counts as a run + * error and fails `npm run ci` even though every test passes (#1947). + * + * Attach the handler at call time (not after the assertions) so there is no + * window in which the rejection can escape, and `await` the returned promise + * after `disconnect()` so teardown stays ordered. The call may legitimately + * either settle before the teardown or reject with it, so neither outcome is + * asserted here. + */ +function settleInFlight(call: Promise): Promise { + return call.then( + () => undefined, + () => undefined, + ); +} + /** Get all resources from the client via listResources() (paginates if needed). */ async function getAllResources( client: InspectorClient, @@ -2221,16 +2244,18 @@ describe("InspectorClient", () => { const progressToken = 12345; const sendProgressTool = await getTool(client, "send_progress"); - client.callTool( - sendProgressTool, - { - units: 3, - delayMs: 50, - total: 3, - message: "Test progress", - }, - undefined, // generalMetadata - { progressToken: progressToken.toString() }, // toolSpecificMetadata + const inFlight = settleInFlight( + client.callTool( + sendProgressTool, + { + units: 3, + delayMs: 50, + total: 3, + message: "Test progress", + }, + undefined, // generalMetadata + { progressToken: progressToken.toString() }, // toolSpecificMetadata + ), ); const progressEvents = await waitForProgressCount(client, 3, { @@ -2262,6 +2287,7 @@ describe("InspectorClient", () => { }); await client!.disconnect(); + await inFlight; await server.stop(); }); @@ -2349,15 +2375,17 @@ describe("InspectorClient", () => { const progressToken = 67890; const sendProgressTool2 = await getTool(client, "send_progress"); - client.callTool( - sendProgressTool2, - { - units: 2, - delayMs: 50, - message: "Indeterminate progress", - }, - undefined, // generalMetadata - { progressToken: progressToken.toString() }, // toolSpecificMetadata + const inFlight = settleInFlight( + client.callTool( + sendProgressTool2, + { + units: 2, + delayMs: 50, + message: "Indeterminate progress", + }, + undefined, // generalMetadata + { progressToken: progressToken.toString() }, // toolSpecificMetadata + ), ); const progressEvents = await waitForProgressCount(client, 2, { @@ -2380,6 +2408,7 @@ describe("InspectorClient", () => { expect((progressEvents[1] as { total?: number }).total).toBeUndefined(); await client!.disconnect(); + await inFlight; await server.stop(); }); From 0b9aee11a313e4d9afe056575766730913c9f5a9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 18:01:27 -0400 Subject: [PATCH 2/4] fix: only absorb the teardown's own rejection in settleInFlight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: the blanket rejection handler also converted a genuine `callTool` failure into a pass. That matters here because these tests assert on the progress notifications, which arrive before the result — so a call that emitted all three progress events and then failed would have satisfied every assertion and been swallowed. Narrow the handler to absorb only `SdkError` / `CONNECTION_CLOSED` and re-throw anything else. The handler is still attached at call time, so an unexpected rejection stays *handled* and surfaces as an ordinary test failure at the `await inFlight` after `disconnect()` rather than as another unhandled rejection. Fulfillment is still accepted: whether the call beats the teardown is a race, so asserting either outcome would reintroduce the original bug as a flake. Verified the narrowed predicate is actually exercised rather than dead code — both call sites reject with SdkError/CONNECTION_CLOSED on every run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --- .../integration/mcp/inspectorClient.test.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 3586e51f..9f5379a7 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -107,14 +107,27 @@ async function getTool(client: InspectorClient, name: string): Promise { * * Attach the handler at call time (not after the assertions) so there is no * window in which the rejection can escape, and `await` the returned promise - * after `disconnect()` so teardown stays ordered. The call may legitimately - * either settle before the teardown or reject with it, so neither outcome is - * asserted here. + * after `disconnect()` so teardown stays ordered. + * + * Only the teardown's own `CONNECTION_CLOSED` rejection is absorbed. Plain + * fulfillment is fine too — whether the call beats the teardown is a race, so + * asserting either outcome would turn this straight back into a flake. Any + * *other* rejection is re-thrown, so a call that fails for a real reason still + * fails the test instead of passing on the strength of the progress + * notifications it managed to emit first. */ function settleInFlight(call: Promise): Promise { return call.then( () => undefined, - () => undefined, + (error: unknown) => { + if ( + error instanceof SdkError && + error.code === SdkErrorCode.ConnectionClosed + ) { + return; + } + throw error; + }, ); } From c60563b7d647303ce15b34690836f3ffd8a7f127 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 18:09:52 -0400 Subject: [PATCH 3/4] fix: observe the derived promise so an unexpected rejection can't go unhandled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback (round 2). Narrowing the handler in the previous commit fixed the silent-pass but reintroduced #1947's own failure class on the unexpected path: `then` returns a *derived* promise, and the re-throw rejects that one rather than `call`. The caller doesn't await it until after `disconnect()`, so a rejection arriving while the test is still waiting on progress notifications sits unobserved for seconds and Node reports it as an unhandled rejection. Attach an observer to the derived promise the moment it exists. Nothing is swallowed — it stays rejected, so the caller's `await` still fails the test. Verified both directions with a probe rejecting with a non-ConnectionClosed error and awaiting after a 1.5s gap: without the observer the run reports "Unhandled Rejection" + "Errors 1 error"; with it, the test fails and the run stays clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --- .../src/test/integration/mcp/inspectorClient.test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 9f5379a7..9a3a690c 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -117,7 +117,7 @@ async function getTool(client: InspectorClient, name: string): Promise { * notifications it managed to emit first. */ function settleInFlight(call: Promise): Promise { - return call.then( + const settled = call.then( () => undefined, (error: unknown) => { if ( @@ -129,6 +129,15 @@ function settleInFlight(call: Promise): Promise { throw error; }, ); + // `then` returns a *derived* promise, and the re-throw above rejects that one + // — not `call`. The caller does not await it until after `disconnect()`, so an + // unexpected rejection arriving while the test is still waiting on progress + // notifications would sit unobserved for seconds and be reported as an + // unhandled rejection: precisely the failure this helper exists to prevent. + // Observe it the moment it exists. This does not swallow anything — `settled` + // stays rejected, so the caller's `await` still fails the test. + settled.catch(() => undefined); + return settled; } /** Get all resources from the client via listResources() (paginates if needed). */ From 347173fab7363fa25f0a50b1b4b246c72028757f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 18:19:33 -0400 Subject: [PATCH 4/4] fix: absorb only the teardown's own close, not any ConnectionClosed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback (round 3). The predicate absorbed *any* CONNECTION_CLOSED, while the doc comment claimed it absorbed only the teardown's — so a transport dropping on its own after the progress notifications but before `disconnect()` would have been swallowed. These tests assert only on the notifications, so that regression would have passed green. Gate the absorption on a teardown flag owned by the helper, which now returns an `InFlightCall` exposing `disconnectAndSettle()`. Keeping the flag inside means it can't be raised too early (which would silently reopen the hole) and the settle can't be forgotten; call sites also collapse from two statements to one. Verified: a pre-teardown ConnectionClosed now fails the test and stays observed (no run-level error), while a teardown-time one is still absorbed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --- .../integration/mcp/inspectorClient.test.ts | 44 +++++++++++++------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 9a3a690c..cc208835 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -106,21 +106,33 @@ async function getTool(client: InspectorClient, name: string): Promise { * error and fails `npm run ci` even though every test passes (#1947). * * Attach the handler at call time (not after the assertions) so there is no - * window in which the rejection can escape, and `await` the returned promise - * after `disconnect()` so teardown stays ordered. + * window in which the rejection can escape, then finish through + * `disconnectAndSettle()`, which tears down and awaits the call in one step. * - * Only the teardown's own `CONNECTION_CLOSED` rejection is absorbed. Plain + * Only a `CONNECTION_CLOSED` raised *by that teardown* is absorbed. Plain * fulfillment is fine too — whether the call beats the teardown is a race, so - * asserting either outcome would turn this straight back into a flake. Any - * *other* rejection is re-thrown, so a call that fails for a real reason still - * fails the test instead of passing on the strength of the progress - * notifications it managed to emit first. + * asserting either outcome would turn this straight back into a flake. Every + * other rejection is re-thrown, including a `CONNECTION_CLOSED` that arrives + * before teardown begins: a transport that drops on its own after emitting the + * progress notifications is a real regression, and absorbing it would let these + * tests pass on the strength of the notifications alone. + * + * The teardown flag is owned by the helper and set inside `disconnectAndSettle` + * rather than by the caller, so the flag cannot be raised too early (which would + * reopen the hole) and the await cannot be forgotten. */ -function settleInFlight(call: Promise): Promise { +interface InFlightCall { + /** Disconnect, then await the call — absorbing only this teardown's close. */ + disconnectAndSettle(client: InspectorClient): Promise; +} + +function settleInFlight(call: Promise): InFlightCall { + let tearingDown = false; const settled = call.then( () => undefined, (error: unknown) => { if ( + tearingDown && error instanceof SdkError && error.code === SdkErrorCode.ConnectionClosed ) { @@ -135,9 +147,15 @@ function settleInFlight(call: Promise): Promise { // notifications would sit unobserved for seconds and be reported as an // unhandled rejection: precisely the failure this helper exists to prevent. // Observe it the moment it exists. This does not swallow anything — `settled` - // stays rejected, so the caller's `await` still fails the test. + // stays rejected, so the caller's `await` below still fails the test. settled.catch(() => undefined); - return settled; + return { + async disconnectAndSettle(client: InspectorClient): Promise { + tearingDown = true; + await client.disconnect(); + await settled; + }, + }; } /** Get all resources from the client via listResources() (paginates if needed). */ @@ -2308,8 +2326,7 @@ describe("InspectorClient", () => { progressToken: progressToken.toString(), }); - await client!.disconnect(); - await inFlight; + await inFlight.disconnectAndSettle(client!); await server.stop(); }); @@ -2429,8 +2446,7 @@ describe("InspectorClient", () => { }); expect((progressEvents[1] as { total?: number }).total).toBeUndefined(); - await client!.disconnect(); - await inFlight; + await inFlight.disconnectAndSettle(client!); await server.stop(); });