From ce942d63470ea90971c83bf8e988180b33120191 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Tue, 22 Sep 2026 18:28:20 -0400 Subject: [PATCH 1/2] test(mcp): pin the one-way era order that makes one snapshot slot enough Issue #101 reports that one snapshot slot per session lets publishing one era's catalog evict the other's: alternating requests would rebuild on every switch, and a legacy build that failed transiently would find only a modern entry and lose its availability fallback. Both consequences need a modern catalog to be published after a legacy one on the same connection, and the SDK does not allow that. McpServerImpl (2.2.0) sets a session's protocol version on its first request that carries one. initialize supersedes a version set earlier, and after that SetNegotiatedProtocolVersion rejects any request naming a different version with InvalidRequest. So one connection goes through modern requests, then an initialize, then legacy requests only. Nothing can evict the legacy entry the fallback reads, there is one switch at most, and a legacy build that fails before any legacy success has nothing to fall back to with one slot or two. The new test, written as #101's regression, could not reproduce it: the modern request after initialize came back as InvalidRequest (-32600). It now pins that premise instead. If the SDK ever lets a connection return to the modern era, the test fails and the cache needs one slot per era. Pointing the fourth request at no version at all turns it red, so the assertion is not swallowed by its null-conditional chain. SnapshotCacheEntry's documentation now says why one slot is enough. Refs #101 --- src/Repl.Mcp/McpSessionContext.cs | 7 +++ .../Given_McpConcurrentSessions.cs | 47 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/Repl.Mcp/McpSessionContext.cs b/src/Repl.Mcp/McpSessionContext.cs index e90aacb..230e34d 100644 --- a/src/Repl.Mcp/McpSessionContext.cs +++ b/src/Repl.Mcp/McpSessionContext.cs @@ -113,6 +113,13 @@ public async ValueTask DisposeAsync() /// pipe, which is what the specification means by a dual-era server. Without the era in the key the /// second request is served the first one's catalog — see /// When_OneConnectionIsServedBothEras_Then_EachGetsItsOwnCatalog. + /// + /// One slot is still enough, because the order is one-way. initialize supersedes the version + /// modern requests established, and after that the SDK rejects any modern request with + /// InvalidRequest, so no later modern publication can evict the legacy entry that the + /// availability fallback reads. When_ALegacySessionIsNegotiated_Then_TheConnectionCannotReturnToTheModernEra + /// pins that premise. If it ever fails, this cache needs one slot per era. + /// /// internal sealed record SnapshotCacheEntry( McpServerHandler.McpGeneratedSnapshot Snapshot, diff --git a/src/Repl.McpTests/Given_McpConcurrentSessions.cs b/src/Repl.McpTests/Given_McpConcurrentSessions.cs index f97a746..5c2d6ca 100644 --- a/src/Repl.McpTests/Given_McpConcurrentSessions.cs +++ b/src/Repl.McpTests/Given_McpConcurrentSessions.cs @@ -881,6 +881,53 @@ public async Task When_OneConnectionIsServedBothEras_Then_EachGetsItsOwnCatalog( } } + [TestMethod] + [Description("Pins the premise that makes one snapshot slot per session enough. A connection can be served both eras, but only in one direction: modern requests, then an initialize handshake that supersedes their version, then legacy requests only — the SDK rejects a modern request once a session has negotiated its version. So no legacy entry is ever evicted by a later modern one, and neither alternating rebuilds nor a lost legacy fallback can happen. If the SDK ever lets one connection go back to the modern era, this fails, and the cache needs one slot per era (#101).")] + public async Task When_ALegacySessionIsNegotiated_Then_TheConnectionCannotReturnToTheModernEra() + { + var app = ReplApp.Create(); + app.UseMcpServer(); + app.Map("always", () => "ok"); + var handler = CreateHandler(app); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var clientToServer = new Pipe(); + var serverToClient = new Pipe(); + var io = new McpRawIo(clientToServer, serverToClient); + var serverTask = handler.RunAsync( + new McpTestFixture.PipeIoContext( + clientToServer.Reader.AsStream(), + serverToClient.Writer.AsStream()), + cts.Token); + var modernMeta = new JsonObject + { + ["io.modelcontextprotocol/protocolVersion"] = McpProtocolRevisions.Sessionless, + ["io.modelcontextprotocol/clientCapabilities"] = new JsonObject(), + }; + + try + { + ToolNames(await io.CallAsync(id: 1, method: "tools/list", meta: modernMeta.DeepClone().AsObject(), cancellationToken: cts.Token).ConfigureAwait(false)) + .Should().Contain("always", "the connection starts in the modern era"); + await io.InitializeLegacyAsync(id: 2, cts.Token).ConfigureAwait(false); + ToolNames(await io.CallAsync(id: 3, method: "tools/list", meta: null, cancellationToken: cts.Token).ConfigureAwait(false)) + .Should().Contain("always", "initialize supersedes the modern version"); + + var backToModern = await io.CallAsync(id: 4, method: "tools/list", meta: modernMeta.DeepClone().AsObject(), cancellationToken: cts.Token).ConfigureAwait(false); + + // Read into a local first: a null-conditional chain ending in .Should() would skip the assertion + // entirely when there is no error at all — the one outcome this test exists to catch. + var errorCode = backToModern["error"]?["code"]?.GetValue(); + errorCode.Should().Be( + (int)McpErrorCode.InvalidRequest, + "a negotiated session must not change protocol versions"); + } + finally + { + await StopRawServerAsync(cts, io, serverTask).ConfigureAwait(false); + } + } + private static async Task StopRawServerAsync(CancellationTokenSource cts, McpRawIo io, Task serverTask) { await cts.CancelAsync().ConfigureAwait(false); From e84eef7c1ef01d594f5320bc366c1e45456b94dd Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Tue, 22 Sep 2026 18:37:17 -0400 Subject: [PATCH 2/2] fix(mcp): keep one cached catalog per protocol era MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit closed #101 as unreachable: after initialize, the SDK rejects a new modern request, so it looked as if no modern catalog could ever be published after a legacy one. Review on #109 showed the hole in that. A modern request accepted before initialize can still be in flight, since the SDK dispatches one connection's requests concurrently, and its build then publishes after the legacy one. With one slot, that replaces the legacy entry the availability fallback reads, which is exactly what #101 describes. The window is narrow but real. McpSessionContext now keeps one entry per era, reached through GetSnapshotCache(sessionless), and publishing writes only its own era's slot. The era checks at each read are gone, since the slot already is the era. ThrowSanitizedIfAClientAlreadyHasASchema asks HasServedSnapshot, meaning either era, so a client that has seen any schema still gets the sanitised message. The test that pinned the one-way premise is removed, along with the documentation paragraph built on it. TDD: the new When_BothErasPublishOnOneSession_Then_EachKeepsItsOwnEntry was written against the new lookup first implemented over the single slot, and was red there — the modern publication evicted the legacy entry — before the slots were split. Refs #101 --- src/Repl.Mcp/McpServerHandler.cs | 23 ++++---- src/Repl.Mcp/McpSessionContext.cs | 31 +++++----- .../Given_McpConcurrentSessions.cs | 57 +++++-------------- 3 files changed, 43 insertions(+), 68 deletions(-) diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index 4f7bf72..738afca 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -454,9 +454,8 @@ private async ValueTask GetSnapshotAsync( { var sessionless = IsSessionlessRequest(); var snapshotVersion = Volatile.Read(ref _snapshotState).Version; - if (context.SnapshotCache is { IsStale: false } cached - && cached.Version == snapshotVersion - && cached.Sessionless == sessionless) + if (context.GetSnapshotCache(sessionless) is { IsStale: false } cached + && cached.Version == snapshotVersion) { return cached.Snapshot; } @@ -465,9 +464,8 @@ private async ValueTask GetSnapshotAsync( try { snapshotVersion = Volatile.Read(ref _snapshotState).Version; - if (context.SnapshotCache is { IsStale: false } refreshed - && refreshed.Version == snapshotVersion - && refreshed.Sessionless == sessionless) + if (context.GetSnapshotCache(sessionless) is { IsStale: false } refreshed + && refreshed.Version == snapshotVersion) { return refreshed.Snapshot; } @@ -495,7 +493,7 @@ private async ValueTask BuildOrServePreviousAsync( bool sessionless, CancellationToken cancellationToken) { - var previousSnapshot = context.SnapshotCache?.Snapshot; + var clientHasSchema = context.HasServedSnapshot; try { var built = await BuildCurrentSnapshotAsync(context, snapshotVersion, sessionless, cancellationToken) @@ -512,7 +510,7 @@ private async ValueTask BuildOrServePreviousAsync( } catch (HiddenRequiredOptionException) { - ThrowSanitizedIfAClientAlreadyHasASchema(previousSnapshot); + ThrowSanitizedIfAClientAlreadyHasASchema(clientHasSchema); throw; } catch (Exception) when (IsFallbackEligible(context, sessionless)) @@ -525,7 +523,7 @@ private async ValueTask BuildOrServePreviousAsync( // Re-read rather than reuse the filter's value: nothing holds the retraction watermark // still between the two, and a retraction that lands in between must fail closed with the // original failure rather than serve a catalog it has just withdrawn. - if (context.SnapshotCache is not { } fallback || !IsFallbackEligible(context, sessionless)) + if (context.GetSnapshotCache(sessionless) is not { } fallback || !IsFallbackEligible(context, sessionless)) { throw; } @@ -545,9 +543,9 @@ private async ValueTask BuildOrServePreviousAsync( // was meant to withhold — so it must not reach the client verbatim. A generic McpException (the // pattern this handler already uses for other client-facing failures) reports the failure without // disclosing what triggered it. - private static void ThrowSanitizedIfAClientAlreadyHasASchema(McpGeneratedSnapshot? previousSnapshot) + private static void ThrowSanitizedIfAClientAlreadyHasASchema(bool clientHasSchema) { - if (previousSnapshot is not null) + if (clientHasSchema) { throw new McpException("Tool discovery is temporarily unavailable due to a server configuration error."); } @@ -731,8 +729,7 @@ private void AttachSession(McpSessionContext context, McpServer server) /// private bool IsFallbackEligible(McpSessionContext context, bool sessionless) => !sessionless - && context.SnapshotCache is { } candidate - && candidate.Sessionless == sessionless + && context.GetSnapshotCache(sessionless) is { } candidate && Volatile.Read(ref _snapshotState).LastVisibilityRetractionVersion <= candidate.Version; internal sealed record SnapshotVersionState( diff --git a/src/Repl.Mcp/McpSessionContext.cs b/src/Repl.Mcp/McpSessionContext.cs index 230e34d..f34fac7 100644 --- a/src/Repl.Mcp/McpSessionContext.cs +++ b/src/Repl.Mcp/McpSessionContext.cs @@ -20,7 +20,12 @@ namespace Repl.Mcp; /// internal sealed class McpSessionContext : IAsyncDisposable { - private SnapshotCacheEntry? _snapshotCache; + // One entry per protocol era. A connection is served modern requests, then an initialize and legacy + // ones, and a modern request accepted before initialize can still publish after the legacy one — + // the SDK dispatches a connection's requests concurrently — so one shared slot would let it evict the + // legacy entry the availability fallback reads. + private SnapshotCacheEntry? _legacySnapshot; + private SnapshotCacheEntry? _sessionlessSnapshot; private int _compatibilityIntroServed; private readonly AsyncServiceScope? _scope; @@ -45,17 +50,21 @@ public McpSessionContext( public SemaphoreSlim SnapshotGate { get; } = new(initialCount: 1, maxCount: 1); /// - /// Cached snapshot paired with the routing version it was built at, or - /// before this session's first build. + /// This session's cached snapshot for the era, paired with the routing + /// version it was built at, or before that era's first build. /// - public SnapshotCacheEntry? SnapshotCache => Volatile.Read(ref _snapshotCache); + public SnapshotCacheEntry? GetSnapshotCache(bool sessionless) => Volatile.Read(ref Slot(sessionless)); + + /// Whether any catalog, of either era, has been published on this session. + public bool HasServedSnapshot => + Volatile.Read(ref _legacySnapshot) is not null || Volatile.Read(ref _sessionlessSnapshot) is not null; /// Publishes as current for . public void PublishSnapshot( McpServerHandler.McpGeneratedSnapshot snapshot, long version, bool sessionless) => - Volatile.Write(ref _snapshotCache, new SnapshotCacheEntry(snapshot, version, IsStale: false, sessionless)); + Volatile.Write(ref Slot(sessionless), new SnapshotCacheEntry(snapshot, version, IsStale: false, sessionless)); /// /// Publishes , built at , as serve-able but @@ -65,7 +74,10 @@ public void PublishStaleSnapshot( McpServerHandler.McpGeneratedSnapshot snapshot, long version, bool sessionless) => - Volatile.Write(ref _snapshotCache, new SnapshotCacheEntry(snapshot, version, IsStale: true, sessionless)); + Volatile.Write(ref Slot(sessionless), new SnapshotCacheEntry(snapshot, version, IsStale: true, sessionless)); + + private ref SnapshotCacheEntry? Slot(bool sessionless) => + ref sessionless ? ref _sessionlessSnapshot : ref _legacySnapshot; /// /// Claims this session's one-time compatibility-shim intro; for the first @@ -113,13 +125,6 @@ public async ValueTask DisposeAsync() /// pipe, which is what the specification means by a dual-era server. Without the era in the key the /// second request is served the first one's catalog — see /// When_OneConnectionIsServedBothEras_Then_EachGetsItsOwnCatalog. - /// - /// One slot is still enough, because the order is one-way. initialize supersedes the version - /// modern requests established, and after that the SDK rejects any modern request with - /// InvalidRequest, so no later modern publication can evict the legacy entry that the - /// availability fallback reads. When_ALegacySessionIsNegotiated_Then_TheConnectionCannotReturnToTheModernEra - /// pins that premise. If it ever fails, this cache needs one slot per era. - /// /// internal sealed record SnapshotCacheEntry( McpServerHandler.McpGeneratedSnapshot Snapshot, diff --git a/src/Repl.McpTests/Given_McpConcurrentSessions.cs b/src/Repl.McpTests/Given_McpConcurrentSessions.cs index 5c2d6ca..d34f7cc 100644 --- a/src/Repl.McpTests/Given_McpConcurrentSessions.cs +++ b/src/Repl.McpTests/Given_McpConcurrentSessions.cs @@ -882,50 +882,23 @@ public async Task When_OneConnectionIsServedBothEras_Then_EachGetsItsOwnCatalog( } [TestMethod] - [Description("Pins the premise that makes one snapshot slot per session enough. A connection can be served both eras, but only in one direction: modern requests, then an initialize handshake that supersedes their version, then legacy requests only — the SDK rejects a modern request once a session has negotiated its version. So no legacy entry is ever evicted by a later modern one, and neither alternating rebuilds nor a lost legacy fallback can happen. If the SDK ever lets one connection go back to the modern era, this fails, and the cache needs one slot per era (#101).")] - public async Task When_ALegacySessionIsNegotiated_Then_TheConnectionCannotReturnToTheModernEra() + [Description("Publishing one era's catalog must not evict the other's. A connection is served modern requests, then an initialize and legacy ones — and a modern request accepted before initialize can still be in flight, since the SDK dispatches one connection's requests concurrently. With one slot, that late modern publication replaced the legacy entry the availability fallback reads, so a legacy build that then failed transiently found only an opposite-era catalog and failed closed.")] + public async Task When_BothErasPublishOnOneSession_Then_EachKeepsItsOwnEntry() { var app = ReplApp.Create(); - app.UseMcpServer(); - app.Map("always", () => "ok"); - var handler = CreateHandler(app); - - using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); - var clientToServer = new Pipe(); - var serverToClient = new Pipe(); - var io = new McpRawIo(clientToServer, serverToClient); - var serverTask = handler.RunAsync( - new McpTestFixture.PipeIoContext( - clientToServer.Reader.AsStream(), - serverToClient.Writer.AsStream()), - cts.Token); - var modernMeta = new JsonObject - { - ["io.modelcontextprotocol/protocolVersion"] = McpProtocolRevisions.Sessionless, - ["io.modelcontextprotocol/clientCapabilities"] = new JsonObject(), - }; - - try - { - ToolNames(await io.CallAsync(id: 1, method: "tools/list", meta: modernMeta.DeepClone().AsObject(), cancellationToken: cts.Token).ConfigureAwait(false)) - .Should().Contain("always", "the connection starts in the modern era"); - await io.InitializeLegacyAsync(id: 2, cts.Token).ConfigureAwait(false); - ToolNames(await io.CallAsync(id: 3, method: "tools/list", meta: null, cancellationToken: cts.Token).ConfigureAwait(false)) - .Should().Contain("always", "initialize supersedes the modern version"); - - var backToModern = await io.CallAsync(id: 4, method: "tools/list", meta: modernMeta.DeepClone().AsObject(), cancellationToken: cts.Token).ConfigureAwait(false); - - // Read into a local first: a null-conditional chain ending in .Should() would skip the assertion - // entirely when there is no error at all — the one outcome this test exists to catch. - var errorCode = backToModern["error"]?["code"]?.GetValue(); - errorCode.Should().Be( - (int)McpErrorCode.InvalidRequest, - "a negotiated session must not change protocol versions"); - } - finally - { - await StopRawServerAsync(cts, io, serverTask).ConfigureAwait(false); - } + var roots = new McpClientRootsService(app.Core, new McpRequestServerAccessor(), McpRootsScope.Connection); + var context = new McpSessionContext(roots, McpTestFixture.EmptyServices, scope: null); + await using var owner = context.ConfigureAwait(false); + var legacy = new McpServerHandler.McpGeneratedSnapshot(null!, [], [], []); + var modern = new McpServerHandler.McpGeneratedSnapshot(null!, [], [], []); + + context.PublishSnapshot(legacy, version: 1, sessionless: false); + context.PublishSnapshot(modern, version: 1, sessionless: true); + + var legacyEntry = context.GetSnapshotCache(sessionless: false); + legacyEntry.Should().NotBeNull("the modern publication must leave the legacy entry in place"); + legacyEntry!.Snapshot.Should().BeSameAs(legacy); + context.GetSnapshotCache(sessionless: true)!.Snapshot.Should().BeSameAs(modern); } private static async Task StopRawServerAsync(CancellationTokenSource cts, McpRawIo io, Task serverTask)