From 308c7c465ef65bbeba4d7704eda0648108e245f1 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Tue, 22 Sep 2026 18:12:26 -0400 Subject: [PATCH] fix(mcp): notify only the session whose compatibility shim transitioned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under DynamicToolCompatibilityMode.DiscoverAndCallShim, each initialize-era session answers its first tools/list with the discover_tools/call_tool pair, then signals tools/list_changed so it re-lists and gets the real catalog. The transition is connection-local — each session claims its own intro — but the signal cleared _toolListChanged, a handler-wide collection whose SDK fan-out notifies every attached session. Every other session got a tools/list_changed for a catalog that had not changed. The notification now goes to request.Server, the server of the request that made the transition. SignalToolListChanged lost its only caller and is removed. Its comment, about Clear() raising Changed on an empty collection, moves to SignalDiscoveryChanged, which still relies on that. TDD: the new test is red 3/3 before the fix — session B receives the notification session A's transition caused. A ping on B, sent after A's own notification arrives, is the ordering barrier: the SDK starts every session's send synchronously inside that one Clear(), so a notification bound for B is already queued ahead of the ping's response. B's own transition is the positive control that the handler observes one when it should. Refs #102 --- src/Repl.Mcp/McpServerHandler.cs | 23 +++++---- .../Given_McpConcurrentSessions.cs | 48 +++++++++++++++++++ 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index 4f7bf72..2c3e530 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -311,7 +311,14 @@ private async ValueTask ListToolsAsync( && !IsSessionlessRequest() && context.TryClaimCompatibilityIntro()) { - SignalToolListChanged(); + // Sent to this request's own server, not through _toolListChanged: that collection's SDK + // fan-out reaches every attached session, and only this one's catalog is about to change. + if (request.Server is { } server) + { + await server.SendNotificationAsync(NotificationMethods.ToolListChangedNotification, cancellationToken) + .ConfigureAwait(false); + } + return McpCacheHints.MarkPrivateToThisClient(request, new ListToolsResult { Tools = @@ -849,20 +856,18 @@ private void OnRoutingInvalidated(bool isVisibilityRetraction) } } - private void SignalDiscoveryChanged() - { - _toolListChanged.Clear(); - _resourceListChanged.Clear(); - _promptListChanged.Clear(); - } - // Clearing an already-empty primitive collection raises its Changed event without mutating // anything, which is what lets an empty collection act as a pure signal. That the event fires // unconditionally is NOT documented on Clear(), so it is pinned by // Given_McpSubscriptions.When_ClearingAnEmptyCollection_Then_ChangedStillFires: if a future SDK // turns Clear() into a no-op, that test fails loudly instead of discovery notifications silently // disappearing. - private void SignalToolListChanged() => _toolListChanged.Clear(); + private void SignalDiscoveryChanged() + { + _toolListChanged.Clear(); + _resourceListChanged.Clear(); + _promptListChanged.Clear(); + } private void UnsubscribeFromRoutingChanges() { diff --git a/src/Repl.McpTests/Given_McpConcurrentSessions.cs b/src/Repl.McpTests/Given_McpConcurrentSessions.cs index f97a746..f9e0f1b 100644 --- a/src/Repl.McpTests/Given_McpConcurrentSessions.cs +++ b/src/Repl.McpTests/Given_McpConcurrentSessions.cs @@ -705,6 +705,54 @@ public async Task When_ShimEnabledAndTwoLegacySessionsList_Then_EachSessionGetsT firstListB.Select(static tool => tool.Name).Should().BeEquivalentTo(["discover_tools", "call_tool"]); } + [TestMethod] + [Description("The compatibility-shim transition is connection-local — each session claims its own intro — so its tools/list_changed must reach that session alone. It used to clear a handler-wide collection whose SDK fan-out notifies every attached session, sending a spurious list_changed to sessions whose catalog had not changed. A ping on the other session after the first session's own notification arrives is the ordering barrier: the SDK starts every session's send synchronously inside that one clear, so any notification bound for the other session is already queued ahead of the ping's response.")] + public async Task When_OneLegacySessionConsumesItsShimIntro_Then_NoOtherSessionIsNotified() + { + var app = ReplApp.Create(); + app.UseMcpServer(); + app.Map("alpha", () => "a"); + var handler = CreateHandler(app, DynamicToolCompatibilityMode.DiscoverAndCallShim); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + var legacy = new McpClientOptions { ProtocolVersion = McpProtocolRevisions.LastWithSessions }; + var sessionA = await StartSessionAsync(handler, legacy, cts.Token).ConfigureAwait(false); + await using var scopeA = sessionA.ConfigureAwait(false); + var sessionB = await StartSessionAsync(handler, legacy, cts.Token).ConfigureAwait(false); + await using var scopeB = sessionB.ConfigureAwait(false); + + var notifiedA = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var registrationA = sessionA.Client.RegisterNotificationHandler( + NotificationMethods.ToolListChangedNotification, + (_, _) => + { + notifiedA.TrySetResult(); + return ValueTask.CompletedTask; + }).ConfigureAwait(false); + var notificationsToB = 0; + var notifiedB = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var registrationB = sessionB.Client.RegisterNotificationHandler( + NotificationMethods.ToolListChangedNotification, + (_, _) => + { + Interlocked.Increment(ref notificationsToB); + notifiedB.TrySetResult(); + return ValueTask.CompletedTask; + }).ConfigureAwait(false); + + (await sessionA.Client.ListToolsAsync(cancellationToken: cts.Token).ConfigureAwait(false)) + .Select(static tool => tool.Name).Should().BeEquivalentTo(["discover_tools", "call_tool"]); + await notifiedA.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + await sessionB.Client.PingAsync(cancellationToken: cts.Token).ConfigureAwait(false); + + Volatile.Read(ref notificationsToB).Should().Be(0, "session A's transition changed nothing session B can see"); + + // Positive control: B's own transition does reach B, so the handler above can observe one. + await sessionB.Client.ListToolsAsync(cancellationToken: cts.Token).ConfigureAwait(false); + await notifiedB.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + Volatile.Read(ref notificationsToB).Should().Be(1); + } + [TestMethod] [Description("Regression guard: the compatibility bootstrap must not run on 2026-07-28. Serving discover_tools/call_tool on the first tools/list and the real catalog on the next is a connection-local change caused by another request on that connection — the second half of the MUST NOT, and observable with a single connection. A modern client gets the real catalog immediately, and twice in a row it gets the same one.")] public async Task When_ShimEnabledAndAModernSessionLists_Then_TheCatalogIsTheSameEveryTime()