From d08a53027a3e6bdeea3d091af6a49ac2cdef5919 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 23:49:20 -0400 Subject: [PATCH] fix: dispatch the connect event after the listen stream is up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 2: opening the stream after `dispatchTypedEvent("connect")` left a real gap. The managed list states start their initial `refresh()` from that event, so `tools/list` could go out ahead of `subscriptions/listen` — and a list the server changed in that window would notify nobody, leaving the UI stale with no way to notice. The handler registration, the initial logging level, and the stream open now all run before the `connect` dispatch, so no consumer can act on the connection until the notification channel is established (or its retry armed). Costs one listen round-trip on a modern connect. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 --- .../inspectorClient-subscriptions-era.test.ts | 24 +++++++++++++++++++ core/mcp/inspectorClient.ts | 11 +++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts index 91daa638..9c61266f 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts @@ -373,6 +373,30 @@ describe("resource subscriptions era fork (#1630)", () => { ); }); + it("has the stream up before the connect event fires", async () => { + // The managed list states start their initial `refresh()` from the + // `connect` event, so the stream has to be established (or its retry + // armed) first: dispatching earlier would let `tools/list` go out ahead of + // `subscriptions/listen`, and a list the server changed in that window + // would notify nobody. + const started = await startToolsOnlyServer({ tools: true }); + const connected = new InspectorClient( + { type: "streamable-http", url: started.url }, + { + environment: { transport: createTransportNode }, + versionNegotiation: eraToVersionNegotiation("modern"), + }, + ); + let streamAtConnectEvent: boolean | undefined; + connected.addEventListener("connect", () => { + streamAtConnectEvent = internals(connected).modernSubscription !== null; + }); + await connected.connect(); + client = connected; + + expect(streamAtConnectEvent).toBe(true); + }); + it("connects anyway when the connect-time listen fails, and retries", async () => { // The handshake succeeded; every request-scoped feature works without the // stream, so the failure is handed to the reconnect machinery instead of diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index c9ec8207..4f9993a4 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1936,8 +1936,6 @@ export class InspectorClient extends InspectorClientEventTarget { // capabilities and wipe tools/prompts/resources to empty on every connect. await this.fetchServerInfo(); - this.dispatchTypedEvent("connect"); - // Set initial logging level if configured and server supports it if (this.initialLoggingLevel && this.capabilities?.logging) { await this.client.setLoggingLevel( @@ -2067,6 +2065,15 @@ export class InspectorClient extends InspectorClientEventTarget { // server with no resources would have no way in, since a subscribe click // was the only thing that ever opened the stream (#1920). await this.openModernListenStreamOnConnect(); + + // Last, so the notification channel is established (or its retry armed) + // before any consumer acts on the connection. The managed list states + // start their initial `refresh()` from this event, so dispatching earlier + // would let `tools/list` go out ahead of `subscriptions/listen` — and a + // list the server changes in that window would notify nobody, leaving the + // UI stale with no way to notice (#1920). The cost is one listen + // round-trip added to connect on the modern era. + this.dispatchTypedEvent("connect"); } catch (error) { if (!isConnectAuthRecoveryError(error)) { this.status = "error";