From 6d9bb2440f1fab359f044781dde967333df09352 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:06:36 -0400 Subject: [PATCH 01/24] fix(di): open one DI scope per session so Scoped services match the documented lifetime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repl created no IServiceScope anywhere. Every registration is a singleton and per-session services were faked with overlay decorators over one app-lifetime root, so a Scoped registration — which the documentation recommends for auth context and carts — was a de-facto singleton shared by every concurrent Telnet, WebSocket and MCP client. One Run* call is now one session and owns an AsyncServiceScope. The scope wraps the two _core.RunOutcomeWithServicesAsync call sites rather than the enclosing RunOutcomeAsync, so HostedServiceLifecycleCoordinator.StartAsync keeps starting hosted services from the unscoped provider: they are app-level, not session-level. That placement also covers the standalone signal-owning path, which reaches the core through RunOutcomeAsync directly and would be missed by scoping RunWithServicesAsync alone. SessionScopeBehavior.CallerOwned opts out for a provider that already represents the session scope — a Blazor circuit, an ASP.NET request scope, or a session owner spanning several one-shot runs. Repl.Testing is the last of those: its handle owns the scope and runs each command as a separate one-shot call. Two guards were added beyond the original branch, because falsifying the suite showed only one of its six tests went red without the scope: the hosted-service path (which pins that hosted services start OUTSIDE the session scope) and the automatic signal path (the entry the original placement would have missed). Refs #70 --- docs/comparison.md | 2 +- src/Repl.Defaults/ReplApp.cs | 66 +++++- src/Repl.Defaults/ReplRunOptions.cs | 7 +- src/Repl.Defaults/SessionScopeBehavior.cs | 20 ++ .../Given_SessionScopedServices.cs | 199 ++++++++++++++++++ src/Repl.Testing/ReplSessionHandle.cs | 64 +++++- 6 files changed, 335 insertions(+), 23 deletions(-) create mode 100644 src/Repl.Defaults/SessionScopeBehavior.cs create mode 100644 src/Repl.IntegrationTests/Given_SessionScopedServices.cs diff --git a/docs/comparison.md b/docs/comparison.md index 832e883d..1c45697a 100644 --- a/docs/comparison.md +++ b/docs/comparison.md @@ -73,7 +73,7 @@ Repl Toolkit is a command-surface framework — not just a CLI parser. It builds | WebSocket session hosting | ❌ | ❌ | ✅ `Repl.WebSocket` | | Telnet session hosting | ❌ | ❌ | ✅ `Repl.Telnet` | | Terminal metadata negotiation | ❌ | ❌ | ✅ NAWS, TTYPE, DTTERM | -| Per-session DI & state | ❌ | ❌ | ✅ `IReplSessionState` | +| Per-session DI & state | ❌ | ❌ | ✅ DI scope per session + `IReplSessionState` | | Window size detection | ❌ | ✅ `Console` only | ✅ Local + remote | | Transport-agnostic host | ❌ | ❌ | ✅ `StreamedReplHost` | diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 68471db2..e49e2719 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -1,4 +1,4 @@ -using System.Diagnostics.CodeAnalysis; +using System.Diagnostics.CodeAnalysis; using System.Linq.Expressions; using System.Reflection; using System.Runtime.ExceptionServices; @@ -18,8 +18,10 @@ public sealed class ReplApp : IReplApp private readonly IServiceCollection _services; // Lazily built provider shared between MapModule() and Run(). - // Ensures modules resolved via DI share the same service instances - // as handler parameters resolved at runtime. + // Singleton services resolved by modules are the same instances handler parameters + // see at runtime; Scoped services intentionally differ — handlers resolve them from + // the per-session scope opened by Run* (see RunInSessionScopeAsync), while module + // resolution at registration time happens outside any session. private ServiceProvider? _sharedProvider; private ProcessSignalHandlingMode _defaultProcessSignalHandling = ProcessSignalHandlingMode.None; @@ -396,8 +398,11 @@ private async ValueTask RunOutcomeAsync( { if (runOptions.HostedServiceLifecycle is HostedServiceLifecycleMode.None or HostedServiceLifecycleMode.Guest) { - return await _core.RunOutcomeWithServicesAsync(args, services, cancellationToken) - .ConfigureAwait(false); + return await RunInSessionScopeAsync( + services, + runOptions.SessionScope, + (sessionServices, token) => _core.RunOutcomeWithServicesAsync(args, sessionServices, token), + cancellationToken).ConfigureAwait(false); } // Routed through the policy before starting hosted services, so an already-cancelled caller token @@ -407,7 +412,8 @@ private async ValueTask RunOutcomeAsync( return cancelled; } - return await RunHostedLifecycleOutcomeAsync(args, services, cancellationToken).ConfigureAwait(false); + return await RunHostedLifecycleOutcomeAsync(args, services, runOptions.SessionScope, cancellationToken) + .ConfigureAwait(false); } /// @@ -418,6 +424,7 @@ private async ValueTask RunOutcomeAsync( private async ValueTask RunHostedLifecycleOutcomeAsync( string[] args, IServiceProvider services, + SessionScopeBehavior sessionScope, CancellationToken cancellationToken) { IReadOnlyList started = []; @@ -430,8 +437,13 @@ private async ValueTask RunHostedLifecycleOutcomeAsync( // rollback survives unreported, unlike one that fails in the normal shutdown below. started = await HostedServiceLifecycleCoordinator.StartAsync(services, cancellationToken) .ConfigureAwait(false); - outcome = await _core.RunOutcomeWithServicesAsync(args, services, cancellationToken) - .ConfigureAwait(false); + // The scope wraps the pipeline only: hosted services are app-level and keep starting from + // the unscoped provider above, so their lifetime is not tied to this session. + outcome = await RunInSessionScopeAsync( + services, + sessionScope, + (sessionServices, token) => _core.RunOutcomeWithServicesAsync(args, sessionServices, token), + cancellationToken).ConfigureAwait(false); } catch (HostedServiceLifecycleException ex) { @@ -662,9 +674,41 @@ public async ValueTask RunAsync( return _core.ResolveProcessExitCode(cancelled); } - var sessionProvider = CreateSessionOverlay(services); - return await _core.RunWithServicesAsync(args, sessionProvider, cancellationToken) - .ConfigureAwait(false); + // The session overlay composes on top of the session scope, so overlay lookups that fall + // through to the caller provider observe this session Scoped instances. + return await RunInSessionScopeAsync( + services, + runOptions.SessionScope, + (sessionServices, token) => + _core.RunWithServicesAsync(args, CreateSessionOverlay(sessionServices), token), + cancellationToken).ConfigureAwait(false); + } + } + /// + /// Runs one session inside its own DI scope, so Scoped services resolve per session and scoped + /// disposables are released when the session ends. + /// + /// + /// opts out for a provider that already represents + /// the session scope — a Blazor circuit, an ASP.NET request scope, or a session owner spanning + /// several one-shot runs. A provider without scope support runs unscoped, as before. + /// + private static async ValueTask RunInSessionScopeAsync( + IServiceProvider services, + SessionScopeBehavior sessionScope, + Func> run, + CancellationToken cancellationToken) + { + if (sessionScope == SessionScopeBehavior.CallerOwned + || services.GetService(typeof(IServiceScopeFactory)) is not IServiceScopeFactory scopeFactory) + { + return await run(services, cancellationToken).ConfigureAwait(false); + } + + var scope = scopeFactory.CreateAsyncScope(); + await using (scope.ConfigureAwait(false)) + { + return await run(scope.ServiceProvider, cancellationToken).ConfigureAwait(false); } } diff --git a/src/Repl.Defaults/ReplRunOptions.cs b/src/Repl.Defaults/ReplRunOptions.cs index 432a9959..382607d9 100644 --- a/src/Repl.Defaults/ReplRunOptions.cs +++ b/src/Repl.Defaults/ReplRunOptions.cs @@ -1,4 +1,4 @@ -namespace Repl; +namespace Repl; /// /// Runtime execution options for a single REPL run. @@ -16,6 +16,11 @@ public sealed record ReplRunOptions /// public HostedServiceLifecycleMode HostedServiceLifecycle { get; init; } = HostedServiceLifecycleMode.None; + /// + /// Gets or sets how this run manages the session dependency-injection scope. + /// + public SessionScopeBehavior SessionScope { get; init; } = SessionScopeBehavior.PerRun; + /// /// Gets or sets the ANSI support mode for the session. /// diff --git a/src/Repl.Defaults/SessionScopeBehavior.cs b/src/Repl.Defaults/SessionScopeBehavior.cs new file mode 100644 index 00000000..1dc6c310 --- /dev/null +++ b/src/Repl.Defaults/SessionScopeBehavior.cs @@ -0,0 +1,20 @@ +namespace Repl; + +/// +/// How a Run* call manages the session's dependency-injection scope. +/// +public enum SessionScopeBehavior +{ + /// + /// The run opens one DI scope for the session (the default): Scoped services resolve + /// per session and scoped disposables are released when the session ends. + /// + PerRun = 0, + + /// + /// The caller's service provider already represents the session's scope (for example a + /// Blazor circuit or per-request scope, or a session owner spanning several one-shot + /// runs): the run resolves from it directly and never opens a nested scope. + /// + CallerOwned = 1, +} diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs new file mode 100644 index 00000000..4e646f8e --- /dev/null +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -0,0 +1,199 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Repl.Testing; + +namespace Repl.IntegrationTests; + +[TestClass] +[DoNotParallelize] +public sealed class Given_SessionScopedServices +{ + private sealed class ScopedProbe + { + public Guid Id { get; } = Guid.NewGuid(); + } + + private sealed class TransientProbe + { + public Guid Id { get; } = Guid.NewGuid(); + } + + private sealed class DisposableProbe(List disposed) : IDisposable + { + public Guid Id { get; } = Guid.NewGuid(); + + public void Dispose() => disposed.Add(Id); + } + + private static ReplTestHost CreateProbeHost(List? disposedTracker = null) => + ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => + { + services.AddScoped(); + services.AddTransient(); + services.AddScoped(_ => new DisposableProbe(disposedTracker ?? [])); + }).UseDefaultInteractive(); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + app.Map("transient", (TransientProbe probe) => probe.Id.ToString()); + app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()); + return app; + }); + + private static async Task RunAndCaptureAsync(ReplSessionHandle session, string command) + { + var result = await session.RunCommandAsync($"{command} --no-logo").ConfigureAwait(false); + result.ExitCode.Should().Be(0, "command output was: {0}", result.OutputText); + return result.OutputText.Trim(); + } + + [TestMethod] + [Description("Guards the core per-run session scope on ONE shared app: two Run* calls against the same ReplApp (the exact shape of two hosted connections sharing one app) must resolve two distinct Scoped instances from the app's own root provider — this exercises the scope-creation branch itself, independently of Repl.Testing's session plumbing.")] + public void When_TwoRunsShareOneApp_Then_ScopedInstancesDiffer() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + + var first = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"])); + var second = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"])); + + first.ExitCode.Should().Be(0); + second.ExitCode.Should().Be(0); + first.Text.Trim().Should().NotBe(second.Text.Trim()); + } + + [TestMethod] + [Description("Guards the CallerOwned opt-out: a caller whose provider already represents the session scope (Blazor circuit, per-request scope, multi-run session owners) must keep its own Scoped instances across runs — the entry points must not open a nested sibling scope that would hide them.")] + public async Task When_CallerOwnsTheSessionScope_Then_RunsShareTheCallerScopedInstances() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var scopeFactory = (IServiceScopeFactory)sut.Services.GetService(typeof(IServiceScopeFactory))!; + var callerScope = scopeFactory.CreateAsyncScope(); + await using (callerScope.ConfigureAwait(false)) + { + var options = new ReplRunOptions { SessionScope = SessionScopeBehavior.CallerOwned }; + + var first = ConsoleCaptureHelper.Capture( + () => sut.Run(["scoped", "--no-logo"], callerScope.ServiceProvider, options)); + var second = ConsoleCaptureHelper.Capture( + () => sut.Run(["scoped", "--no-logo"], callerScope.ServiceProvider, options)); + + first.ExitCode.Should().Be(0); + first.Text.Trim().Should().Be(second.Text.Trim()); + } + } + + [TestMethod] + [Description("Guards the documented Scoped lifetime for hosted sessions: two distinct hosted sessions must resolve two distinct Scoped instances — sharing one instance across sessions leaks per-user state (auth context, carts) between concurrent clients.")] + public async Task When_TwoHostedSessionsResolveScopedService_Then_InstancesDiffer() + { + await using var host = CreateProbeHost(); + await using var sessionA = await host.OpenSessionAsync(); + await using var sessionB = await host.OpenSessionAsync(); + + var idA = await RunAndCaptureAsync(sessionA, "scoped"); + var idB = await RunAndCaptureAsync(sessionB, "scoped"); + + idA.Should().NotBe(idB); + } + + [TestMethod] + [Description("Guards the session-scope boundary from the other side: two commands within the SAME hosted session share the same Scoped instance — the scope is per session, not per command invocation.")] + public async Task When_SameSessionRunsTwoCommands_Then_ScopedInstanceIsShared() + { + await using var host = CreateProbeHost(); + await using var session = await host.OpenSessionAsync(); + + var first = await RunAndCaptureAsync(session, "scoped"); + var second = await RunAndCaptureAsync(session, "scoped"); + + first.Should().Be(second); + } + + [TestMethod] + [Description("Guards Transient semantics under the session scope: every resolution yields a fresh instance, including across commands of the same session.")] + public async Task When_TransientResolvedInTwoCommands_Then_InstancesDiffer() + { + await using var host = CreateProbeHost(); + await using var session = await host.OpenSessionAsync(); + + var first = await RunAndCaptureAsync(session, "transient"); + var second = await RunAndCaptureAsync(session, "transient"); + + first.Should().NotBe(second); + } + + [TestMethod] + [Description("Guards scoped-disposable lifetime: a Scoped IDisposable resolved during a hosted session is disposed when that session ends, not deferred to app shutdown — session resources (connections, per-user stores) must not accumulate for the host process lifetime.")] + public async Task When_SessionEnds_Then_ScopedDisposablesAreDisposed() + { + var disposed = new List(); + await using var host = CreateProbeHost(disposed); + + string id; + { + await using var session = await host.OpenSessionAsync(); + id = await RunAndCaptureAsync(session, "disposable"); + disposed.Should().NotContain(Guid.Parse(id)); + } + + disposed.Should().Contain(Guid.Parse(id)); + } + + private sealed class ScopeObservingHostedService(IServiceProvider services, List observed) : IHostedService + { + public Task StartAsync(CancellationToken cancellationToken) + { + observed.Add(services.GetRequiredService().Id); + return Task.CompletedTask; + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + } + + [TestMethod] + [Description("Guards the scope boundary on the hosted-service path: the run pipeline is wrapped in the session scope but HostedServiceLifecycleCoordinator.StartAsync is not, so a hosted service resolves from the unscoped provider and must NOT see the command Scoped instance. Placing the scope around the whole hosted branch instead would tie app-level services to a session lifetime.")] + public void When_HostedServicesRun_Then_TheyStartOutsideTheSessionScope() + { + var observed = new List(); + var sut = ReplApp.Create(services => + { + services.AddScoped(); + services.AddSingleton(observed); + services.AddSingleton(); + }); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var options = new ReplRunOptions { HostedServiceLifecycle = HostedServiceLifecycleMode.Head }; + + var first = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"], options)); + var second = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"], options)); + + first.ExitCode.Should().Be(0, "run output was: {0}", first.Text); + second.ExitCode.Should().Be(0, "run output was: {0}", second.Text); + + var firstCommandId = first.Text.Trim(); + var secondCommandId = second.Text.Trim(); + firstCommandId.Should().NotBe(secondCommandId, "each hosted run owns its session scope"); + + observed.Should().HaveCount(2); + observed.Select(id => id.ToString()).Should().NotContain(firstCommandId); + observed.Select(id => id.ToString()).Should().NotContain(secondCommandId); + } + + [TestMethod] + [Description("Guards the fourth run path: ProcessSignalHandlingMode.Automatic reaches the core through RunOutcomeAsync directly, bypassing RunWithServicesAsync. Scoping at that bypassed method would leave standalone signal-owning runs sharing one Scoped instance across runs.")] + public void When_AStandaloneRunOwnsProcessSignals_Then_ItStillGetsItsOwnSessionScope() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var options = new ReplRunOptions { ProcessSignalHandling = ProcessSignalHandlingMode.Automatic }; + + var first = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"], options)); + var second = ConsoleCaptureHelper.Capture(() => sut.Run(["scoped", "--no-logo"], options)); + + first.ExitCode.Should().Be(0, "run output was: {0}", first.Text); + second.ExitCode.Should().Be(0, "run output was: {0}", second.Text); + first.Text.Trim().Should().NotBe(second.Text.Trim()); + } +} diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 7d1bdd05..d57e420b 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection; namespace Repl.Testing; @@ -161,23 +161,32 @@ public SessionSnapshot GetSnapshot() /// /// Ends the session and removes it from its host. Disposing twice is a no-op. /// - public ValueTask DisposeAsync() + public async ValueTask DisposeAsync() { if (_disposed) { - return ValueTask.CompletedTask; + return; } _disposed = true; _owner.RemoveSession(SessionId); ReplSessionIO.RemoveSession(SessionId); - if (_services is IDisposable disposable) + + // Take the command gate before disposing the session scope so an in-flight command + // (for example an orphaned timed-out run) cannot race scoped-service disposal. + await _commandGate.WaitAsync().ConfigureAwait(false); + switch (_services) { - disposable.Dispose(); + case IAsyncDisposable asyncDisposable: + // Releases the session DI scope, disposing its Scoped services. + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; } _commandGate.Dispose(); - return ValueTask.CompletedTask; } internal static ValueTask StartAsync( @@ -196,10 +205,10 @@ internal static ValueTask StartAsync( var sessionId = $"session-{Guid.NewGuid():N}"; ReplSessionIO.EnsureSession(sessionId); var app = appFactory(); - var runOptions = descriptor.BuildRunOptions(options); - var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(); - var services = serviceCollection.BuildServiceProvider(); + // The handle owns the session DI scope (each command is a separate one-shot run), + // so every run opts out of per-run scoping. + var runOptions = descriptor.BuildRunOptions(options) with { SessionScope = SessionScopeBehavior.CallerOwned }; + var services = SessionScopedTestServices.Create(app); var handle = new ReplSessionHandle(owner, app, options, services, runOptions, descriptor.Answers, sessionId); return ValueTask.FromResult(handle); } @@ -317,6 +326,41 @@ public void OnInteractionEvent(ReplInteractionEvent evt) } } + // One logical test session spans several one-shot Run* calls, so the handle owns the session + // DI scope itself: the provider is built over a scope of the APP services (user registrations + // become visible to session commands, and Scoped instances are shared across the session + // commands) and every run uses SessionScopeBehavior.CallerOwned so the Run* entry points do not + // open a fresh scope — and fresh Scoped instances — per command. + private sealed class SessionScopedTestServices : IServiceProvider, IAsyncDisposable + { + private readonly AsyncServiceScope? _scope; + private readonly IServiceProvider _inner; + private readonly InMemorySessionState _sessionState = new(); + + private SessionScopedTestServices(AsyncServiceScope? scope, IServiceProvider inner) + { + _scope = scope; + _inner = inner; + } + + public static SessionScopedTestServices Create(ReplApp app) + { + var root = app.Services; + if (root.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory) + { + var scope = scopeFactory.CreateAsyncScope(); + return new SessionScopedTestServices(scope, scope.ServiceProvider); + } + + return new SessionScopedTestServices(scope: null, root); + } + + public object? GetService(Type serviceType) => + serviceType == typeof(IReplSessionState) ? _sessionState : _inner.GetService(serviceType); + + public ValueTask DisposeAsync() => _scope?.DisposeAsync() ?? ValueTask.CompletedTask; + } + private sealed class InMemorySessionState : IReplSessionState { private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); From 5f0474e195db8185a2e63082151b6ddad489068f Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:10:52 -0400 Subject: [PATCH 02/24] fix(testing): stop session disposal from waiting on a command that is still running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking the command gate before disposing the session scope, as the previous commit does, made DisposeAsync wait on it unbounded — so a still-running command turned an await using into a hang, and an orphaned timed-out run never released it at all. Disposing the semaphore afterwards compounded it: a caller that passed the disposal check and was queued on WaitAsync is never woken by Dispose, so it waits forever. Disposal now takes the gate with a zero timeout and releases what it took. Holding it still keeps a command that starts between two runs from racing scoped disposal, which is the reason it was taken, but nothing waits for a run already in flight. The gate is no longer disposed. It owns no unmanaged resource here — AvailableWaitHandle is never used — and disposing it is precisely what strands a queued caller. A queued caller is instead stopped by a second disposal check, taken inside the gate: passing the check before queueing no longer implies the session is still alive by the time the gate is granted, and such a caller must not run against a disposed scope. Refs #70 --- .../Given_SessionScopedServices.cs | 40 ++++++++++++++++++ src/Repl.Testing/ReplSessionHandle.cs | 42 +++++++++++++------ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 4e646f8e..1f86ac8c 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -196,4 +196,44 @@ public void When_AStandaloneRunOwnsProcessSignals_Then_ItStillGetsItsOwnSessionS second.ExitCode.Should().Be(0, "run output was: {0}", second.Text); first.Text.Trim().Should().NotBe(second.Text.Trim()); } + + [TestMethod] + [Description("Regression guard for the session command gate: taking the gate before disposing the session scope must not let a still-running command block disposal indefinitely. DisposeAsync waits on the gate, so an unbounded wait turns an orphaned command into a hung await using, and disposing the semaphore while a caller is queued on it strands that caller forever.")] + public async Task When_ACommandIsStillRunning_Then_DisposingTheSessionDoesNotHang() + { + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var host = ReplTestHost.Create( + () => + { + var app = ReplApp.Create().UseDefaultInteractive(); + app.Map("block", async () => + { + started.TrySetResult(); + + // VSTHRD003: the test owns both sources; this command exists to hold the session command + // gate open until the test releases it. +#pragma warning disable VSTHRD003 + await release.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + return "done"; + }); + return app; + }, + options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var running = session.RunCommandAsync("block --no-logo").AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + var dispose = session.DisposeAsync().AsTask(); + var disposedInTime = await Task.WhenAny(dispose, Task.Delay(TimeSpan.FromSeconds(5))) == dispose; + + release.TrySetResult(); + await Task.WhenAny(running, Task.Delay(TimeSpan.FromSeconds(30))); + await dispose.WaitAsync(TimeSpan.FromSeconds(30)); + + disposedInTime.Should().BeTrue( + "disposing a session must not wait unbounded on a command that is still running"); + } } diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index d57e420b..8becb4c3 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -86,6 +86,10 @@ private async ValueTask ExecuteCommandCoreAsync( await _commandGate.WaitAsync(cancellationToken).ConfigureAwait(false); try { + // Re-checked inside the gate: a caller that passed the check above can be queued here while + // the session is disposed, and must not go on to run against a disposed scope. + ThrowIfDisposed(); + var startedAt = DateTimeOffset.UtcNow; using var output = new StringWriter(); var host = new TestSessionHost(_sessionId, output); @@ -172,21 +176,35 @@ public async ValueTask DisposeAsync() _owner.RemoveSession(SessionId); ReplSessionIO.RemoveSession(SessionId); - // Take the command gate before disposing the session scope so an in-flight command - // (for example an orphaned timed-out run) cannot race scoped-service disposal. - await _commandGate.WaitAsync().ConfigureAwait(false); - switch (_services) + // Best-effort: holding the gate keeps a command that starts between two runs from racing + // scoped disposal. Never WAIT for one that is already running — an orphaned timed-out run + // holds the gate until it finishes on its own, and waiting would turn an await using into a + // hang. Commands queued behind it stop at the re-check inside the gate instead. + var held = await _commandGate.WaitAsync(TimeSpan.Zero).ConfigureAwait(false); + try { - case IAsyncDisposable asyncDisposable: - // Releases the session DI scope, disposing its Scoped services. - await asyncDisposable.DisposeAsync().ConfigureAwait(false); - break; - case IDisposable disposable: - disposable.Dispose(); - break; + switch (_services) + { + case IAsyncDisposable asyncDisposable: + // Releases the session DI scope, disposing its Scoped services. + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; + } + } + finally + { + if (held) + { + _commandGate.Release(); + } } - _commandGate.Dispose(); + // The semaphore is deliberately not disposed: disposing it while a caller is queued on + // WaitAsync strands that caller forever, and it owns no unmanaged resource here because + // AvailableWaitHandle is never used. } internal static ValueTask StartAsync( From bb06810146e1ed335093694f9d0c538499a19d3e Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:12:37 -0400 Subject: [PATCH 03/24] fix(core): make the default session state safe for concurrent sessions IReplSessionState is resolved by every concurrent Telnet, WebSocket and MCP session from one shared instance, and both default implementations stored their values in an unsynchronised Dictionary. Concurrent writes do not merely interleave: the runtime's own guard reports "a concurrent update was performed on this collection and corrupted its state", and a read racing a resize can fault or miss an entry that was written. Both implementations now use a ConcurrentDictionary. Remove becomes TryRemove; every other member keeps its signature and behaviour. This is the corruption half of the problem only. Sharing one instance across sessions is a lifetime question, answered separately. Refs #70 --- .../Session/InMemoryReplSessionState.cs | 8 +- src/Repl.Defaults/DefaultsSessionState.cs | 8 +- src/Repl.Tests/Given_SessionState.cs | 75 +++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 src/Repl.Tests/Given_SessionState.cs diff --git a/src/Repl.Core/Session/InMemoryReplSessionState.cs b/src/Repl.Core/Session/InMemoryReplSessionState.cs index 40ccc91a..245b2e83 100644 --- a/src/Repl.Core/Session/InMemoryReplSessionState.cs +++ b/src/Repl.Core/Session/InMemoryReplSessionState.cs @@ -1,8 +1,12 @@ +using System.Collections.Concurrent; + namespace Repl; internal sealed class InMemoryReplSessionState : IReplSessionState { - private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); + // Every concurrent Telnet, WebSocket and MCP session can resolve this state, so writes from + // two sessions race. An unsynchronised Dictionary corrupts its bucket table under that. + private readonly ConcurrentDictionary _values = new(StringComparer.OrdinalIgnoreCase); public bool TryGet(string key, out T? value) { @@ -33,7 +37,7 @@ public void Set(string key, T value) public bool Remove(string key) { ArgumentException.ThrowIfNullOrWhiteSpace(key); - return _values.Remove(key); + return _values.TryRemove(key, out _); } public void Clear() => _values.Clear(); diff --git a/src/Repl.Defaults/DefaultsSessionState.cs b/src/Repl.Defaults/DefaultsSessionState.cs index 8a404e59..2214658c 100644 --- a/src/Repl.Defaults/DefaultsSessionState.cs +++ b/src/Repl.Defaults/DefaultsSessionState.cs @@ -1,8 +1,12 @@ +using System.Collections.Concurrent; + namespace Repl; internal sealed class DefaultsSessionState : IReplSessionState { - private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); + // Every concurrent Telnet, WebSocket and MCP session can resolve this state, so writes from + // two sessions race. An unsynchronised Dictionary corrupts its bucket table under that. + private readonly ConcurrentDictionary _values = new(StringComparer.OrdinalIgnoreCase); public bool TryGet(string key, out T? value) { @@ -33,7 +37,7 @@ public void Set(string key, T value) public bool Remove(string key) { ArgumentException.ThrowIfNullOrWhiteSpace(key); - return _values.Remove(key); + return _values.TryRemove(key, out _); } public void Clear() => _values.Clear(); diff --git a/src/Repl.Tests/Given_SessionState.cs b/src/Repl.Tests/Given_SessionState.cs new file mode 100644 index 00000000..7f0a7a29 --- /dev/null +++ b/src/Repl.Tests/Given_SessionState.cs @@ -0,0 +1,75 @@ +using System.Collections.Concurrent; + +namespace Repl.Tests; + +[TestClass] +public sealed class Given_SessionState +{ + private const int Writers = 8; + private const int WritesPerWriter = 500; + + [TestMethod] + [Description("Regression guard: IReplSessionState is resolved by every concurrent Telnet, WebSocket and MCP session, so its default implementation must tolerate concurrent writers. An unsynchronised Dictionary does not: concurrent Set can lose entries or corrupt the bucket table during a resize.")] + public async Task When_SessionsWriteStateConcurrently_Then_EveryWriteIsReadableAfterwards() + { + var sut = new DefaultsSessionState(); + + await Task.WhenAll(Enumerable.Range(0, Writers).Select(writer => Task.Run(() => + { + for (var i = 0; i < WritesPerWriter; i++) + { + sut.Set(Key(writer, i), i); + } + }))); + + for (var writer = 0; writer < Writers; writer++) + { + for (var i = 0; i < WritesPerWriter; i++) + { + sut.TryGet(Key(writer, i), out var value).Should().BeTrue( + "write {0} of writer {1} was lost", i, writer); + value.Should().Be(i); + } + } + } + + [TestMethod] + [Description("Regression guard: reading session state while another session writes it must neither throw nor observe a torn entry. Enumeration-free readers still fault on a Dictionary being resized underneath them.")] + public async Task When_OneSessionReadsWhileAnotherWrites_Then_NoReaderFaults() + { + var sut = new DefaultsSessionState(); + var faults = new ConcurrentQueue(); + using var done = new CancellationTokenSource(); + + var readers = Enumerable.Range(0, 4).Select(_ => Task.Run(() => + { + try + { + while (!done.Token.IsCancellationRequested) + { + sut.TryGet(Key(0, 0), out _); + } + } + catch (Exception exception) + { + faults.Enqueue(exception); + } + })).ToArray(); + + var writer = Task.Run(() => + { + for (var i = 0; i < Writers * WritesPerWriter; i++) + { + sut.Set(Key(0, i), i); + } + }); + + await writer; + await done.CancelAsync(); + await Task.WhenAll(readers); + + faults.Should().BeEmpty("a reader faulted while another session was writing"); + } + + private static string Key(int writer, int index) => $"key-{writer}-{index}"; +} From ed4282ea79d84080fff1eac636063a368337a2bc Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:16:08 -0400 Subject: [PATCH 04/24] fix(di): resolve IReplSessionState per session instead of per process The glossary calls IReplSessionState a "per-session state container" and the feature matrix advertises per-session state, but it was registered as a singleton: one bag shared by every concurrent Telnet, WebSocket and MCP client, so a later session read what an earlier one wrote. Now that Run* opens a scope per session, the registration can say what the documentation already claims. A provider that cannot scope resolves it from the root, exactly as before. One consequence is deliberate and now pinned by a test rather than left to be discovered: a SINGLETON that injects IReplSessionState captures whichever session scope first resolved it, and keeps it for the life of the app. The provider is built without ValidateScopes, so this is silent. Inject the state into the handler, or register the holder scoped. Refs #70 --- src/Repl.Defaults/ReplApp.cs | 5 +- .../Given_SessionScopedServices.cs | 51 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index e49e2719..0c12468d 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -1038,7 +1038,10 @@ private static void EnsureDefaultServices(IServiceCollection services, CoreReplA services.AddReplLogging(); services.TryAddSingleton(core); services.TryAddSingleton(core); - services.TryAddSingleton(); + // Scoped, not singleton: Run* opens one scope per session, so this is the framework own + // per-session service and a second session no longer reads what the first one stored. With a + // provider that cannot scope, it resolves from the root exactly as it did before. + services.TryAddScoped(); services.TryAddSingleton(); services.TryAddSingleton(TimeProvider.System); services.TryAdd(ServiceDescriptor.Singleton(sp => diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 1f86ac8c..d9acfe60 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -236,4 +236,55 @@ public async Task When_ACommandIsStillRunning_Then_DisposingTheSessionDoesNotHan disposedInTime.Should().BeTrue( "disposing a session must not wait unbounded on a command that is still running"); } + + [TestMethod] + [Description("Guards the framework own per-session service: IReplSessionState is documented as a per-session state container, so two sessions of one app must not read each other writes. Registered as a singleton it was one process-wide bag shared by every concurrent Telnet, WebSocket and MCP client.")] + public void When_TwoRunsShareOneApp_Then_SessionStateIsNotShared() + { + var sut = ReplApp.Create(); + sut.Map("remember {value}", (IReplSessionState state, string value) => + { + state.Set("carried", value); + return "stored"; + }); + sut.Map("recall", (IReplSessionState state) => state.Get("carried") ?? "(none)"); + + var write = ConsoleCaptureHelper.Capture(() => sut.Run(["remember", "alpha", "--no-logo"])); + var read = ConsoleCaptureHelper.Capture(() => sut.Run(["recall", "--no-logo"])); + + write.ExitCode.Should().Be(0, "run output was: {0}", write.Text); + read.ExitCode.Should().Be(0, "run output was: {0}", read.Text); + read.Text.Trim().Should().Be("(none)", "a later session must not read an earlier session state"); + } + + private sealed class SessionStateCapturingSingleton(IReplSessionState state) + { + public IReplSessionState Captured { get; } = state; + } + + [TestMethod] + [Description("Documents the known consequence of making IReplSessionState scoped: a SINGLETON that injects it captures, for the life of the app, whichever session scope first resolved it, silently, because the provider is built without ValidateScopes. This test exists so the trap is discoverable and so enabling scope validation later is a deliberate change rather than an accident. Inject IReplSessionState into the handler, or register the holder scoped.")] + public void When_ASingletonInjectsSessionState_Then_ItCapturesTheFirstSessionInstance() + { + var sut = ReplApp.Create(services => services.AddSingleton()); + + // Resolves the singleton inside the FIRST session, which is what pins that session scope to it. + sut.Map("write {value}", (SessionStateCapturingSingleton holder, string value) => + { + holder.Captured.Set("carried", value); + return "stored"; + }); + sut.Map( + "captured", + (SessionStateCapturingSingleton holder) => holder.Captured.Get("carried") ?? "(none)"); + + var write = ConsoleCaptureHelper.Capture(() => sut.Run(["write", "alpha", "--no-logo"])); + var captured = ConsoleCaptureHelper.Capture(() => sut.Run(["captured", "--no-logo"])); + + write.ExitCode.Should().Be(0, "run output was: {0}", write.Text); + captured.ExitCode.Should().Be(0, "run output was: {0}", captured.Text); + captured.Text.Trim().Should().Be( + "alpha", + "a singleton holds the scope it was first resolved in, so it still sees that session"); + } } From d5eb1721b25b1bbc8317088eb41f0ab23622f0a4 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:25:12 -0400 Subject: [PATCH 05/24] fix(mcp): run each invocation in the scope the SDK opened for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MCP execution never reaches ReplApp.Run*, so the per-session scope the earlier commits added did not cover it: a Scoped service resolved from the application root and every tool call on every connection shared one instance. The SDK was already doing the work. McpServerOptions.ScopeRequests defaults to true, so it opens an AsyncServiceScope per request handler invocation and exposes it as MessageContext.Services. Repl discarded it on every path: ReplMcpServerTool passed only request.Server, the adapter built its overlay over the snapshot-time session provider, and McpExplicitPrompt assigned over request.Services outright. Each path now layers the session OVER that scope instead. The order is what matters and is stated once, in McpSessionContext.Compose: the scope descends from the application root and carries none of the session's services, so re-applying them on top is what keeps a handler injecting IMcpClientRoots resolvable — and, because the overlay also answers IServiceProviderIsService, what keeps a prompt declaring one from being reclassified as taking a client-supplied argument and becoming uninvocable. IReplSessionState moves into the session's own services rather than riding the invocation scope. It is Repl's session abstraction, not a user lifetime: on the initialize era a command writes it and calls InvalidateRouting(), and the next tools/list on that connection has to see what it wrote. Holding it on the session also stops it falling through to the application root, where one bag was shared by every connection at once — the MCP half of the defect this series fixes. ExecuteThroughPipelineAsync was at the 60-line analyzer cap, so the session registration and sub-invocation moved into RunSubInvocationAsync before anything was added to it. Fixes #70 --- src/Repl.Mcp/McpAppResource.cs | 9 +- src/Repl.Mcp/McpExplicitPrompt.cs | 16 ++- src/Repl.Mcp/McpServerHandler.cs | 11 +- src/Repl.Mcp/McpSessionContext.cs | 41 +++++- src/Repl.Mcp/McpToolAdapter.cs | 47 +++++-- src/Repl.McpTests/Given_McpSessionScopes.cs | 131 ++++++++++++++++++++ 6 files changed, 233 insertions(+), 22 deletions(-) create mode 100644 src/Repl.McpTests/Given_McpSessionScopes.cs diff --git a/src/Repl.Mcp/McpAppResource.cs b/src/Repl.Mcp/McpAppResource.cs index 015e9c62..cf047a0c 100644 --- a/src/Repl.Mcp/McpAppResource.cs +++ b/src/Repl.Mcp/McpAppResource.cs @@ -84,8 +84,11 @@ public override async ValueTask ReadAsync( // every command-backed path gets has to be repeated here: roots primed so a handler reading // Current sees them, and a buffer open so feedback the client cannot receive as a notification // is not simply lost. - await McpClientRootsService.PrimeFromServicesAsync(_services, cancellationToken).ConfigureAwait(false); - var feedbackService = _services.GetService(typeof(IMcpFeedback)) as McpFeedbackService; + // The session layered over the scope the SDK opened for this read, so a Scoped registration + // resolves per invocation while the session's own capability services keep answering. + var invocationServices = McpSessionContext.Compose(_services, request.Services); + await McpClientRootsService.PrimeFromServicesAsync(invocationServices, cancellationToken).ConfigureAwait(false); + var feedbackService = invocationServices.GetService(typeof(IMcpFeedback)) as McpFeedbackService; using var undelivered = feedbackService?.PushUndeliveredMessages(); string html; @@ -94,7 +97,7 @@ public override async ValueTask ReadAsync( html = await McpAppResourceInvoker .InvokeAsync( _registration.Handler, - _services, + invocationServices, new McpAppResourceContext(request.Params.Uri), request, cancellationToken) diff --git a/src/Repl.Mcp/McpExplicitPrompt.cs b/src/Repl.Mcp/McpExplicitPrompt.cs index 3f57e2d3..fb426417 100644 --- a/src/Repl.Mcp/McpExplicitPrompt.cs +++ b/src/Repl.Mcp/McpExplicitPrompt.cs @@ -38,13 +38,17 @@ public override async ValueTask GetAsync( // incapable. servers.BindRequest(request); - // The SDK resolves this handler's parameters from the request's own provider, and nothing on - // this path sets one — so a prompt injecting a capability service could not be invoked at all. - // Every other execution path reaches the handler through the adapter, which supplies them. - request.Services = services; + // The SDK resolves this handler's parameters from the request's own provider: the scope it + // opened for this invocation, which descends from the application root and so carries none of + // the session's services — a prompt injecting a capability service could not be invoked at all. + // The session is layered OVER that scope rather than replacing it, so a Scoped registration + // still resolves per invocation. Every other execution path composes the same way through the + // adapter. + var invocationServices = McpSessionContext.Compose(services, request.Services); + request.Services = invocationServices; - await McpClientRootsService.PrimeFromServicesAsync(services, cancellationToken).ConfigureAwait(false); - var feedbackService = services.GetService(typeof(IMcpFeedback)) as McpFeedbackService; + await McpClientRootsService.PrimeFromServicesAsync(invocationServices, cancellationToken).ConfigureAwait(false); + var feedbackService = invocationServices.GetService(typeof(IMcpFeedback)) as McpFeedbackService; using var undelivered = feedbackService?.PushUndeliveredMessages(); GetPromptResult result; diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index 3c0f8212..081c9eae 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -88,8 +88,17 @@ private McpSessionContext CreateSessionContext(McpRootsScope rootsScope) [typeof(IMcpSampling)] = _sampling, [typeof(IMcpElicitation)] = _elicitation, [typeof(IMcpFeedback)] = _feedback, + // Session state belongs to the SESSION, not to the per-invocation scope the SDK opens and + // disposes around each request: on the initialize era a command writes it and calls + // InvalidateRouting(), and the next tools/list has to see what it wrote. Holding it here also + // stops it falling through to the application root, where one bag was shared by every + // connection at once. + [typeof(IReplSessionState)] = new InMemoryReplSessionState(), }; - var context = new McpSessionContext(roots, new McpServiceProviderOverlay(_services, overlayServices)); + var context = new McpSessionContext( + roots, + overlayServices, + new McpServiceProviderOverlay(_services, overlayServices)); // The context rides in its own overlay so request handlers can recover their // originating session through the server's provider (the dictionary is captured by // reference, making this two-phase registration safe). diff --git a/src/Repl.Mcp/McpSessionContext.cs b/src/Repl.Mcp/McpSessionContext.cs index 6e10ab8e..e009cbab 100644 --- a/src/Repl.Mcp/McpSessionContext.cs +++ b/src/Repl.Mcp/McpSessionContext.cs @@ -21,9 +21,13 @@ internal sealed class McpSessionContext : IDisposable private SnapshotCacheEntry? _snapshotCache; private int _compatibilityIntroServed; - public McpSessionContext(McpClientRootsService roots, IServiceProvider services) + public McpSessionContext( + McpClientRootsService roots, + IReadOnlyDictionary overrides, + IServiceProvider services) { Roots = roots; + Overrides = overrides; Services = services; } @@ -33,6 +37,41 @@ public McpSessionContext(McpClientRootsService roots, IServiceProvider services) /// Per-session service overlay handed to McpServer.Create. public IServiceProvider Services { get; } + /// The session-owned services layered over whatever provider sits beneath them. + public IReadOnlyDictionary Overrides { get; } + + /// + /// Layers this session services over the provider the SDK opened for one request, so a Scoped + /// registration resolves per invocation while this session capability services keep answering. + /// + /// + /// The SDK creates an AsyncServiceScope per request handler invocation — + /// McpServerOptions.ScopeRequests defaults to — and exposes it as + /// MessageContext.Services. That scope descends from the APPLICATION root, so it carries + /// none of this session services: re-applying them on top is what keeps a handler injecting + /// resolvable. Falls back to the session provider when there is no + /// request scope to layer over, which is every path the SDK does not dispatch. + /// + public IServiceProvider ComposeOver(IServiceProvider? requestServices) => + requestServices is null || ReferenceEquals(requestServices, Services) + ? Services + : new McpServiceProviderOverlay(requestServices, Overrides); + + /// + /// Layers the session that belongs to over + /// , or returns it unchanged when it belongs to no session. + /// + /// + /// Every execution path resolves its provider through here, so the layering order — session + /// services above, the SDK per-request scope below — is stated once. A provider that carries no + /// is one of the prebuilt paths that never had a session to begin + /// with, and is handed back untouched. + /// + public static IServiceProvider Compose(IServiceProvider sessionServices, IServiceProvider? requestServices) => + sessionServices.GetService(typeof(McpSessionContext)) is McpSessionContext session + ? session.ComposeOver(requestServices) + : sessionServices; + /// Serializes snapshot builds for this session. public SemaphoreSlim SnapshotGate { get; } = new(initialCount: 1, maxCount: 1); diff --git a/src/Repl.Mcp/McpToolAdapter.cs b/src/Repl.Mcp/McpToolAdapter.cs index 756f57bc..6ab1724a 100644 --- a/src/Repl.Mcp/McpToolAdapter.cs +++ b/src/Repl.Mcp/McpToolAdapter.cs @@ -210,16 +210,18 @@ private async Task ExecuteThroughPipelineAsync( var invocableApp = _app as ISubInvocableReplApp ?? throw new InvalidOperationException("MCP tool adapter requires an app that supports sub-invocation."); - await McpClientRootsService.PrimeFromServicesAsync(_services, ct).ConfigureAwait(false); + // This session's services, layered over the scope the SDK opened for THIS request, so a Scoped + // registration resolves per invocation instead of once for the whole application. + var invocationServices = ResolveInvocationServices(); + await McpClientRootsService.PrimeFromServicesAsync(invocationServices, ct).ConfigureAwait(false); var outputWriter = new StringWriter(); var errorWriter = captureCommandOutput ? outputWriter : new StringWriter(); - var inputReader = new StringReader(string.Empty); - var feedback = _services.GetService(typeof(IMcpFeedback)) as IMcpFeedback; + var feedback = invocationServices.GetService(typeof(IMcpFeedback)) as IMcpFeedback; var interactionChannel = new McpInteractionChannel( prefills, _options.InteractivityMode, server, progressToken, feedback); var mcpServices = new McpServiceProviderOverlay( - _services, + invocationServices, new Dictionary { [typeof(IReplInteractionChannel)] = interactionChannel }); var feedbackService = feedback as McpFeedbackService; using var feedbackScope = feedbackService?.PushProgressToken(progressToken); @@ -231,6 +233,35 @@ private async Task ExecuteThroughPipelineAsync( var effectiveTokens = new List(tokens.Count + 1) { $"--output:{ForcedOutputFormat}" }; effectiveTokens.AddRange(tokens); + var completed = await RunSubInvocationAsync( + invocableApp, effectiveTokens, mcpServices, outputWriter, errorWriter, captureCommandOutput, ct) + .ConfigureAwait(false); + + var output = outputWriter.ToString().Trim(); + var error = captureCommandOutput ? string.Empty : errorWriter.ToString().Trim(); + var undelivered = undeliveredScope?.Messages.Drain() ?? []; + return new McpPipelineInvocation( + output, error, completed.ExitCode, completed.Kind, completed.Failure, undelivered); + } + + /// + /// Resolves the provider one invocation runs against: this session's services, layered over the + /// per-invocation scope the SDK opened for the flowing request. + /// + private IServiceProvider ResolveInvocationServices() => + McpSessionContext.Compose(_services, _requestServers.Current?.Services); + + /// Runs the command with the captured I/O and throwaway session registration of one call. + private async Task RunSubInvocationAsync( + ISubInvocableReplApp invocableApp, + List effectiveTokens, + IServiceProvider mcpServices, + StringWriter outputWriter, + StringWriter errorWriter, + bool captureCommandOutput, + CancellationToken ct) + { + var inputReader = new StringReader(string.Empty); // Command-backed resources expose the rendered return value as the resource body. // Low-level handler writes to IReplIoContext.Output/Error are side-channel output, not resource content. var commandOutput = captureCommandOutput ? outputWriter : TextWriter.Null; @@ -256,14 +287,8 @@ private async Task ExecuteThroughPipelineAsync( mcpServices, McpDiscoveryCapabilities.CreateSessionScopedOverrides(_options.InteractivityMode)) : null; - var completed = await invocableApp.RunSubInvocationWithOutcomeAsync( + return await invocableApp.RunSubInvocationWithOutcomeAsync( effectiveTokens.ToArray(), mcpServices, presenceServices, ct).ConfigureAwait(false); - - var output = outputWriter.ToString().Trim(); - var error = captureCommandOutput ? string.Empty : errorWriter.ToString().Trim(); - var undelivered = undeliveredScope?.Messages.Drain() ?? []; - return new McpPipelineInvocation( - output, error, completed.ExitCode, completed.Kind, completed.Failure, undelivered); } } diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs new file mode 100644 index 00000000..0f6e5f8f --- /dev/null +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -0,0 +1,131 @@ +using Microsoft.Extensions.DependencyInjection; +using ModelContextProtocol.Protocol; +using Repl.Mcp; + +namespace Repl.McpTests; + +[TestClass] +public sealed class Given_McpSessionScopes +{ + private sealed class ScopedProbe + { + public Guid Id { get; } = Guid.NewGuid(); + } + + private sealed class DisposableProbe(List disposed) : IDisposable + { + public Guid Id { get; } = Guid.NewGuid(); + + public void Dispose() => disposed.Add(Id); + } + + [TestMethod] + [Description("Probe for the MCP half of per-session DI: the SDK creates an AsyncServiceScope per request handler invocation (McpServerOptions.ScopeRequests defaults to true) and exposes it as RequestContext.Services. Repl discarded it, so a Scoped service resolved from the application root and every tool call on every connection shared one instance.")] + public async Task When_TwoToolCallsShareAConnection_Then_EachResolvesItsOwnScopedInstance() + { + await using var fixture = await McpTestFixture.CreateAsync( + app => app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()), + services => services.AddScoped()).ConfigureAwait(false); + + var first = await CallAsync(fixture, "scoped").ConfigureAwait(false); + var second = await CallAsync(fixture, "scoped").ConfigureAwait(false); + + first.Should().NotBeNullOrWhiteSpace(); + second.Should().NotBeNullOrWhiteSpace(); + first.Should().NotBe(second, "each MCP invocation is its own scope"); + } + + [TestMethod] + [Description("Guards that the per-invocation scope is DISPOSED, not merely created: a Scoped IDisposable resolved by a tool call must be released when that call ends rather than accumulating for the life of the server process. The SDK disposes the scope it opened in a finally, so this holds only while Repl resolves from that scope instead of from the application root.")] + public async Task When_AToolCallResolvesAScopedDisposable_Then_ItIsDisposedWhenTheCallEnds() + { + var disposed = new List(); + await using var fixture = await McpTestFixture.CreateAsync( + app => app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()), + services => services.AddScoped(_ => new DisposableProbe(disposed))).ConfigureAwait(false); + + var reported = await CallAsync(fixture, "disposable").ConfigureAwait(false); + + reported.Should().NotBeNullOrWhiteSpace(); + disposed.Should().ContainSingle( + "the scope the SDK opened for the call is disposed when the call returns"); + reported.Should().Contain( + disposed[0].ToString(), + "the instance disposed must be the one the command was handed"); + } + + [TestMethod] + [Description("Guards the composition ORDER, which is the invariant this change could break silently. The per-request scope descends from the application root and carries none of the session's services, so they are re-applied on top of it. Get that backwards and a command injecting IMcpClientRoots resolves nothing — and, because McpServiceProviderOverlay also answers IServiceProviderIsService, a prompt declaring one is reclassified as taking a client-supplied argument and stops being invocable at all.")] + public async Task When_ACommandInjectsACapabilityService_Then_ItStillResolvesThroughTheRequestScope() + { + await using var fixture = await McpTestFixture.CreateAsync( + app => app.Map( + "capability", + (IMcpClientRoots roots, IMcpFeedback feedback, ScopedProbe probe) => + $"{roots is not null}|{feedback is not null}|{probe.Id != Guid.Empty}"), + services => services.AddScoped()).ConfigureAwait(false); + + var reported = await CallAsync(fixture, "capability").ConfigureAwait(false); + + reported.Should().Contain("True|True|True"); + } + + [TestMethod] + [Description("Guards the MCP half of issue #70: IReplSessionState is documented as per-session, but on MCP it fell through the session overlay to the application root, so two concurrent connections shared one bag. It is now session-owned, which is also what keeps the initialize-era dynamic-tools story working — a command writes it and the next tools/list on THAT connection sees it.")] + public async Task When_TwoConnectionsShareOneServer_Then_NeitherReadsTheOtherSessionState() + { + var app = ReplApp.Create(); + app.UseMcpServer(); + app.Map("remember {value}", (IReplSessionState state, string value) => + { + state.Set("carried", value); + return "stored"; + }); + app.Map("recall", (IReplSessionState state) => state.Get("carried") ?? "(none)"); + + var options = new ReplMcpServerOptions { TransportFactory = McpTestFixture.PipeTransportFactory }; + var handler = new McpServerHandler(app.Core, options, app.Services); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + var first = await McpPipeSession + .StartAsync(handler.RunAsync, clientOptions: null, cts.Token).ConfigureAwait(false); + await using var firstScope = first.ConfigureAwait(false); + var second = await McpPipeSession + .StartAsync(handler.RunAsync, clientOptions: null, cts.Token).ConfigureAwait(false); + await using var secondScope = second.ConfigureAwait(false); + + await CallAsync(first, "remember", new Dictionary(StringComparer.Ordinal) + { + ["value"] = "alpha", + }, cts.Token).ConfigureAwait(false); + + var carried = await CallAsync(first, "recall", cancellationToken: cts.Token).ConfigureAwait(false); + var other = await CallAsync(second, "recall", cancellationToken: cts.Token).ConfigureAwait(false); + + carried.Should().Contain("alpha", "session state survives across calls on its own connection"); + other.Should().Contain("(none)", "another connection is another session"); + } + + private static Task CallAsync(McpTestFixture fixture, string tool) => + CallAsync(fixture.Client, tool, arguments: null, CancellationToken.None); + + private static Task CallAsync( + McpPipeSession session, + string tool, + Dictionary? arguments = null, + CancellationToken cancellationToken = default) => + CallAsync(session.Client, tool, arguments, cancellationToken); + + private static async Task CallAsync( + ModelContextProtocol.Client.McpClient client, + string tool, + Dictionary? arguments, + CancellationToken cancellationToken) + { + var result = await client.CallToolAsync( + toolName: tool, + arguments: arguments ?? new Dictionary(StringComparer.Ordinal), + cancellationToken: cancellationToken).ConfigureAwait(false); + return result.Content.OfType().First().Text; + } +} From 0378d04cbb6e96c1d7f92b80bc4c3f24507908cd Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 00:27:04 -0400 Subject: [PATCH 06/24] docs: state the DI lifetimes that now exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feature matrix already advertised per-session DI and the glossary already called IReplSessionState a per-session container; neither was true. Now that both are, the pages that describe lifetimes say which boundary each one resolves at. best-practices gains the lifetime table, the CallerOwned opt-out, and the one trap this change creates: a singleton injecting a scoped service captures whichever scope first resolved it, silently, because the provider is built without ValidateScopes. mcp-transports gains the DI row in the isolation table. The unit there is the invocation, not the connection, because 2026-07-28 has no connection to scope to — and because the scope is the SDK's own, which is what will make an ASP.NET Core host correct without further work. IReplSessionState stays on the connection, and the page says why. Refs #70 --- docs/architecture.md | 5 +++++ docs/best-practices.md | 21 +++++++++++++++++++++ docs/glossary.md | 3 ++- docs/mcp-transports.md | 17 +++++++++++++++-- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 617702f6..155273da 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -115,6 +115,11 @@ The toolkit provides two application entry points for different scenarios. - Wraps `CoreReplApp` with `Microsoft.Extensions.DependencyInjection` - Provides `UseDefaultInteractive()`, `UseCliProfile()`, and other composition profiles - Lazily builds a shared `ServiceProvider` for module resolution and handler injection +- Opens one DI scope per session: each `Run*` call is a session, so `Scoped` services resolve per + session and their disposables are released when it ends. `ReplRunOptions.SessionScope` set to + `SessionScopeBehavior.CallerOwned` opts out when the caller's provider already *is* the session scope + (a Blazor circuit, an ASP.NET request scope, a session owner spanning several one-shot runs). + Hosted services start outside that scope, because their lifetime is the application's - Best for: standalone CLI/REPL applications with service layers **`InvalidateRouting()`** — call this when module presence conditions may have changed at runtime (e.g., feature flags toggled, dynamic module discovery). Increments the routing cache version so the next execution re-evaluates all module presence predicates. diff --git a/docs/best-practices.md b/docs/best-practices.md index 3357651c..17d60a45 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -107,6 +107,27 @@ services.AddSingleton(sp => Note: DI singleton factories are resolved lazily, so the values are available after global option parsing completes. However, singleton factories capture values once — in interactive mode, global options can change between commands. If your service needs to see updated values per command, inject `IGlobalOptionsAccessor` directly and read values at call time instead of capturing them in a factory. See [Commands — Accessing global options](commands.md#accessing-global-options-outside-handlers). +## Pick the lifetime that matches the boundary + +| Lifetime | Resolves once per | Use it for | +|---|---|---| +| `Singleton` | application | caches, clients, anything shared by every session | +| `Scoped` | **session** — one `Run*` call: a CLI invocation, an interactive run, a Telnet or WebSocket connection; under MCP, one tool call | per-user state: auth context, a cart, a unit of work | +| `Transient` | resolution | cheap stateless helpers | + +A `Scoped` disposable is disposed when its session ends, so session resources do not accumulate for the +life of a long-running host. + +The trap worth naming: a **singleton** that injects a `Scoped` service captures whichever scope first +resolved it, and keeps it for the life of the application — silently, because the provider is built +without `ValidateScopes`. Inject the scoped service into the handler instead, or register the holder +`Scoped` too. + +When your caller's provider already represents the session — a Blazor circuit, an ASP.NET request scope, +or a session owner running several one-shot calls — set +`ReplRunOptions.SessionScope = SessionScopeBehavior.CallerOwned` so the run resolves from it directly +instead of nesting a second scope inside it, which would hide the caller's own scoped instances. + ## Group related options with `[ReplOptionsGroup]` When a command has many options, group them into a class instead of listing them all as handler parameters. This keeps handlers clean and makes option sets reusable across commands. diff --git a/docs/glossary.md b/docs/glossary.md index 4c862cde..07fafb15 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -56,7 +56,8 @@ Interface for packaging reusable command groups. ### IReplSessionState -Per-session state container for interactive and hosted sessions. +Per-session state container for interactive and hosted sessions. Registered `Scoped`, so each session +gets its own; under MCP it belongs to the connection rather than to the individual tool call. ### Literal segment diff --git a/docs/mcp-transports.md b/docs/mcp-transports.md index 08334cdf..48df9b24 100644 --- a/docs/mcp-transports.md +++ b/docs/mcp-transports.md @@ -101,8 +101,21 @@ every request rather than once per connection. So the boundaries are not all the | Isolated per | What | |---|---| | Request | Client capabilities, the requested log level, the destination for sampling, elicitation and progress, and — on a reused `BuildMcpServerOptions()` result — native roots, resolved once per request and never cached across connections | -| Invocation | I/O capture — each tool call gets its own capture scope, not one per connection | -| Connection (`mcp serve` only) | The MCP session object, the native roots cache, soft roots, and session-aware routing state | +| Invocation | I/O capture — each tool call gets its own capture scope, not one per connection — **and the DI scope**: a `Scoped` registration resolves once per tool call, prompt fetch or resource read, and its disposables are released when that call returns | +| Connection (`mcp serve` only) | The MCP session object, the native roots cache, soft roots, session-aware routing state, and `IReplSessionState` | + +The DI scope is the SDK's own: `McpServerOptions.ScopeRequests` defaults to `true`, so it opens an +`AsyncServiceScope` per request handler invocation and disposes it when the handler returns. Repl runs +the command inside that scope with the session's services layered over it. That is why the unit is the +invocation and not the connection — `2026-07-28` has no connection to scope to — and it is also what +makes a future ASP.NET Core host behave correctly for free, since there the scope becomes the HTTP +request's own. + +`IReplSessionState` is the exception, and deliberately so: it is Repl's session abstraction rather than +a user service lifetime. On the initialize era a command writes it and calls `InvalidateRouting()`, and +the next `tools/list` on that connection has to see what it wrote. What that means for the advertised +tool set on each revision — and what to reach for instead on `2026-07-28` — is in +[Conformance](mcp-conformance.md#tool-list-invariance-on-2026-07-28). A server created from a reused `BuildMcpServerOptions()` result has everything above except the connection row; see the known limitation above. That matters especially when using dynamic tools, From 137664634061703397ede57ea4a8b8e394f45e23 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 07:02:41 -0400 Subject: [PATCH 07/24] fix(mcp): give the connection its own DI scope instead of adopting the SDK's per-request one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopting the scope the SDK opens per request handler invocation put MCP on a different unit from every other transport, and three consequences followed. Issue #70's first criterion — two commands of one session share their Scoped instance — became false on MCP alone: a cart or an auth context could not survive from one tool call to the next. IReplSessionState had to be seeded as an override to keep the initialize-era dynamic-tools story working, which made it "the exception" in the docs and, worse, made a consumer's own IReplSessionState unreachable under MCP because an override always wins over the container. And discovery kept resolving user services from the serve run's provider while execution resolved them from a sibling scope rooted at the application root — so a module presence predicate gating on one would advertise a tool and then find it absent when called, which is exactly the invariant PresenceServiceProvider exists to hold, and exactly the pattern docs/module-presence.md recommends. An MCP connection is a session, so it now opens one AsyncServiceScope, held and disposed by its McpSessionContext. Discovery and execution both resolve from it. IReplSessionState needs no override: the scoped registration already gives each connection its own, and a consumer's implementation reaches MCP like every other transport. The per-request composition and its two helpers are gone with it. They also had a defect of their own: layering the session over request.Services dropped the handler's base provider entirely, so a server hosted under ASP.NET — where that provider belongs to a different container — lost every ReplApp registration during command binding. Refs #70 --- src/Repl.Defaults/DefaultsSessionState.cs | 44 ---- src/Repl.Mcp/McpAppResource.cs | 9 +- src/Repl.Mcp/McpExplicitPrompt.cs | 19 +- src/Repl.Mcp/McpServerHandler.cs | 22 +- src/Repl.Mcp/McpSessionContext.cs | 58 ++--- src/Repl.Mcp/McpToolAdapter.cs | 18 +- src/Repl.McpTests/Given_McpSessionScopes.cs | 221 +++++++++++++++----- 7 files changed, 222 insertions(+), 169 deletions(-) delete mode 100644 src/Repl.Defaults/DefaultsSessionState.cs diff --git a/src/Repl.Defaults/DefaultsSessionState.cs b/src/Repl.Defaults/DefaultsSessionState.cs deleted file mode 100644 index 2214658c..00000000 --- a/src/Repl.Defaults/DefaultsSessionState.cs +++ /dev/null @@ -1,44 +0,0 @@ -using System.Collections.Concurrent; - -namespace Repl; - -internal sealed class DefaultsSessionState : IReplSessionState -{ - // Every concurrent Telnet, WebSocket and MCP session can resolve this state, so writes from - // two sessions race. An unsynchronised Dictionary corrupts its bucket table under that. - private readonly ConcurrentDictionary _values = new(StringComparer.OrdinalIgnoreCase); - - public bool TryGet(string key, out T? value) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - - if (_values.TryGetValue(key, out var existing) && existing is T typed) - { - value = typed; - return true; - } - - value = default; - return false; - } - - public T? Get(string key) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - return TryGet(key, out var value) ? value : default; - } - - public void Set(string key, T value) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - _values[key] = value; - } - - public bool Remove(string key) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - return _values.TryRemove(key, out _); - } - - public void Clear() => _values.Clear(); -} diff --git a/src/Repl.Mcp/McpAppResource.cs b/src/Repl.Mcp/McpAppResource.cs index cf047a0c..015e9c62 100644 --- a/src/Repl.Mcp/McpAppResource.cs +++ b/src/Repl.Mcp/McpAppResource.cs @@ -84,11 +84,8 @@ public override async ValueTask ReadAsync( // every command-backed path gets has to be repeated here: roots primed so a handler reading // Current sees them, and a buffer open so feedback the client cannot receive as a notification // is not simply lost. - // The session layered over the scope the SDK opened for this read, so a Scoped registration - // resolves per invocation while the session's own capability services keep answering. - var invocationServices = McpSessionContext.Compose(_services, request.Services); - await McpClientRootsService.PrimeFromServicesAsync(invocationServices, cancellationToken).ConfigureAwait(false); - var feedbackService = invocationServices.GetService(typeof(IMcpFeedback)) as McpFeedbackService; + await McpClientRootsService.PrimeFromServicesAsync(_services, cancellationToken).ConfigureAwait(false); + var feedbackService = _services.GetService(typeof(IMcpFeedback)) as McpFeedbackService; using var undelivered = feedbackService?.PushUndeliveredMessages(); string html; @@ -97,7 +94,7 @@ public override async ValueTask ReadAsync( html = await McpAppResourceInvoker .InvokeAsync( _registration.Handler, - invocationServices, + _services, new McpAppResourceContext(request.Params.Uri), request, cancellationToken) diff --git a/src/Repl.Mcp/McpExplicitPrompt.cs b/src/Repl.Mcp/McpExplicitPrompt.cs index fb426417..a35911c9 100644 --- a/src/Repl.Mcp/McpExplicitPrompt.cs +++ b/src/Repl.Mcp/McpExplicitPrompt.cs @@ -38,17 +38,16 @@ public override async ValueTask GetAsync( // incapable. servers.BindRequest(request); - // The SDK resolves this handler's parameters from the request's own provider: the scope it - // opened for this invocation, which descends from the application root and so carries none of - // the session's services — a prompt injecting a capability service could not be invoked at all. - // The session is layered OVER that scope rather than replacing it, so a Scoped registration - // still resolves per invocation. Every other execution path composes the same way through the - // adapter. - var invocationServices = McpSessionContext.Compose(services, request.Services); - request.Services = invocationServices; + // The SDK resolves this handler's parameters from the request's own provider — a scope it opens + // per invocation, descending from the application root and carrying none of this session's + // services, so a prompt injecting a capability service could not be invoked at all. Replaced + // rather than layered over, because the session's provider already carries the session's own DI + // scope: resolving from both would give this prompt different instances from the command graph + // that advertised it. + request.Services = services; - await McpClientRootsService.PrimeFromServicesAsync(invocationServices, cancellationToken).ConfigureAwait(false); - var feedbackService = invocationServices.GetService(typeof(IMcpFeedback)) as McpFeedbackService; + await McpClientRootsService.PrimeFromServicesAsync(services, cancellationToken).ConfigureAwait(false); + var feedbackService = services.GetService(typeof(IMcpFeedback)) as McpFeedbackService; using var undelivered = feedbackService?.PushUndeliveredMessages(); GetPromptResult result; diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index 081c9eae..050621bf 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -1,6 +1,7 @@ using System.Diagnostics.CodeAnalysis; using System.Text.Json; using System.Text.Json.Nodes; +using Microsoft.Extensions.DependencyInjection; using ModelContextProtocol; using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; @@ -88,17 +89,22 @@ private McpSessionContext CreateSessionContext(McpRootsScope rootsScope) [typeof(IMcpSampling)] = _sampling, [typeof(IMcpElicitation)] = _elicitation, [typeof(IMcpFeedback)] = _feedback, - // Session state belongs to the SESSION, not to the per-invocation scope the SDK opens and - // disposes around each request: on the initialize era a command writes it and calls - // InvalidateRouting(), and the next tools/list has to see what it wrote. Holding it here also - // stops it falling through to the application root, where one bag was shared by every - // connection at once. - [typeof(IReplSessionState)] = new InMemoryReplSessionState(), }; + + // One DI scope per session, so a Scoped registration is per connection here just as it is per + // Run* on the other transports — and so discovery and execution read the SAME instance, which + // is what lets a module presence predicate gate on one. Resolving IReplSessionState from it + // rather than overriding it also keeps a consumer's own implementation reachable under MCP. + // A provider without scope support runs unscoped, as the Run* paths do. + var scope = _services.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory + ? scopeFactory.CreateAsyncScope() + : (AsyncServiceScope?)null; + var sessionServices = scope?.ServiceProvider ?? _services; + var context = new McpSessionContext( roots, - overlayServices, - new McpServiceProviderOverlay(_services, overlayServices)); + new McpServiceProviderOverlay(sessionServices, overlayServices), + scope); // The context rides in its own overlay so request handlers can recover their // originating session through the server's provider (the dictionary is captured by // reference, making this two-phase registration safe). diff --git a/src/Repl.Mcp/McpSessionContext.cs b/src/Repl.Mcp/McpSessionContext.cs index e009cbab..0dc8dace 100644 --- a/src/Repl.Mcp/McpSessionContext.cs +++ b/src/Repl.Mcp/McpSessionContext.cs @@ -1,4 +1,6 @@ -namespace Repl.Mcp; +using Microsoft.Extensions.DependencyInjection; + +namespace Repl.Mcp; /// /// State owned by one MCP transport session, or — on the reusable-options path — by the handler @@ -8,7 +10,7 @@ /// One can serve several concurrent sessions, so anything /// that varies per client lives here instead of on the handler: hard/soft roots, the /// generated snapshot cache (the tool graph can be gated on session capabilities), the -/// compatibility-shim intro state, and the session's service overlay. The context is +/// compatibility-shim intro state, the session's DI scope, and its service overlay. The context is /// registered in the provider passed to McpServer.Create, so request handlers /// recover their originating session through request.Server.Services — never /// through a destination-bound per-request server used as a surrogate session key. @@ -21,14 +23,16 @@ internal sealed class McpSessionContext : IDisposable private SnapshotCacheEntry? _snapshotCache; private int _compatibilityIntroServed; + private readonly AsyncServiceScope? _scope; + public McpSessionContext( McpClientRootsService roots, - IReadOnlyDictionary overrides, - IServiceProvider services) + IServiceProvider services, + AsyncServiceScope? scope) { Roots = roots; - Overrides = overrides; Services = services; + _scope = scope; } /// Session-owned hard/soft roots. @@ -37,41 +41,6 @@ public McpSessionContext( /// Per-session service overlay handed to McpServer.Create. public IServiceProvider Services { get; } - /// The session-owned services layered over whatever provider sits beneath them. - public IReadOnlyDictionary Overrides { get; } - - /// - /// Layers this session services over the provider the SDK opened for one request, so a Scoped - /// registration resolves per invocation while this session capability services keep answering. - /// - /// - /// The SDK creates an AsyncServiceScope per request handler invocation — - /// McpServerOptions.ScopeRequests defaults to — and exposes it as - /// MessageContext.Services. That scope descends from the APPLICATION root, so it carries - /// none of this session services: re-applying them on top is what keeps a handler injecting - /// resolvable. Falls back to the session provider when there is no - /// request scope to layer over, which is every path the SDK does not dispatch. - /// - public IServiceProvider ComposeOver(IServiceProvider? requestServices) => - requestServices is null || ReferenceEquals(requestServices, Services) - ? Services - : new McpServiceProviderOverlay(requestServices, Overrides); - - /// - /// Layers the session that belongs to over - /// , or returns it unchanged when it belongs to no session. - /// - /// - /// Every execution path resolves its provider through here, so the layering order — session - /// services above, the SDK per-request scope below — is stated once. A provider that carries no - /// is one of the prebuilt paths that never had a session to begin - /// with, and is handed back untouched. - /// - public static IServiceProvider Compose(IServiceProvider sessionServices, IServiceProvider? requestServices) => - sessionServices.GetService(typeof(McpSessionContext)) is McpSessionContext session - ? session.ComposeOver(requestServices) - : sessionServices; - /// Serializes snapshot builds for this session. public SemaphoreSlim SnapshotGate { get; } = new(initialCount: 1, maxCount: 1); @@ -108,7 +77,14 @@ public bool TryClaimCompatibilityIntro() => /// Re-arms the compatibility-shim intro after a routing invalidation. public void ResetCompatibilityIntro() => Interlocked.Exchange(ref _compatibilityIntroServed, 0); - public void Dispose() => SnapshotGate.Dispose(); + public void Dispose() + { + SnapshotGate.Dispose(); + // Releases this session's Scoped services. Disposed synchronously because the context is, + // which is the reason the scope is stored rather than awaited: nothing on an MCP connection + // teardown path is async, and a Scoped disposable here is an application object, not I/O. + _scope?.Dispose(); + } /// /// A generated snapshot and the routing version it was built at, published as ONE value. diff --git a/src/Repl.Mcp/McpToolAdapter.cs b/src/Repl.Mcp/McpToolAdapter.cs index 6ab1724a..cb4a9712 100644 --- a/src/Repl.Mcp/McpToolAdapter.cs +++ b/src/Repl.Mcp/McpToolAdapter.cs @@ -210,18 +210,17 @@ private async Task ExecuteThroughPipelineAsync( var invocableApp = _app as ISubInvocableReplApp ?? throw new InvalidOperationException("MCP tool adapter requires an app that supports sub-invocation."); - // This session's services, layered over the scope the SDK opened for THIS request, so a Scoped - // registration resolves per invocation instead of once for the whole application. - var invocationServices = ResolveInvocationServices(); - await McpClientRootsService.PrimeFromServicesAsync(invocationServices, ct).ConfigureAwait(false); + await McpClientRootsService.PrimeFromServicesAsync(_services, ct).ConfigureAwait(false); var outputWriter = new StringWriter(); var errorWriter = captureCommandOutput ? outputWriter : new StringWriter(); - var feedback = invocationServices.GetService(typeof(IMcpFeedback)) as IMcpFeedback; + var feedback = _services.GetService(typeof(IMcpFeedback)) as IMcpFeedback; var interactionChannel = new McpInteractionChannel( prefills, _options.InteractivityMode, server, progressToken, feedback); + // Over the SESSION's services, which carry its DI scope: discovery resolved the command graph + // from the same provider, so what was advertised resolves the same instances when it runs. var mcpServices = new McpServiceProviderOverlay( - invocationServices, + _services, new Dictionary { [typeof(IReplInteractionChannel)] = interactionChannel }); var feedbackService = feedback as McpFeedbackService; using var feedbackScope = feedbackService?.PushProgressToken(progressToken); @@ -244,13 +243,6 @@ private async Task ExecuteThroughPipelineAsync( output, error, completed.ExitCode, completed.Kind, completed.Failure, undelivered); } - /// - /// Resolves the provider one invocation runs against: this session's services, layered over the - /// per-invocation scope the SDK opened for the flowing request. - /// - private IServiceProvider ResolveInvocationServices() => - McpSessionContext.Compose(_services, _requestServers.Current?.Services); - /// Runs the command with the captured I/O and throwaway session registration of one call. private async Task RunSubInvocationAsync( ISubInvocableReplApp invocableApp, diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 0f6e5f8f..4eca405b 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -1,15 +1,22 @@ using Microsoft.Extensions.DependencyInjection; +using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; using Repl.Mcp; namespace Repl.McpTests; +/// +/// The DI scope an MCP connection owns. One scope per session — not per invocation — so discovery and +/// execution read the same instances and a command can carry state across the calls of one connection. +/// [TestClass] public sealed class Given_McpSessionScopes { private sealed class ScopedProbe { public Guid Id { get; } = Guid.NewGuid(); + + public bool SignedIn { get; set; } } private sealed class DisposableProbe(List disposed) : IDisposable @@ -19,59 +26,159 @@ private sealed class DisposableProbe(List disposed) : IDisposable public void Dispose() => disposed.Add(Id); } + private sealed class GatedModule : IReplModule + { + public void Map(IReplMap app) => app.Map("secret", () => "classified").ReadOnly(); + } + + private sealed class RecordingSessionState : IReplSessionState + { + public static int Constructed; + + public RecordingSessionState() => Interlocked.Increment(ref Constructed); + + private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); + + public bool TryGet(string key, out T? value) + { + if (_values.TryGetValue(key, out var existing) && existing is T typed) + { + value = typed; + return true; + } + + value = default; + return false; + } + + public T? Get(string key) => TryGet(key, out var value) ? value : default; + + public void Set(string key, T value) => _values[key] = value; + + public bool Remove(string key) => _values.Remove(key); + + public void Clear() => _values.Clear(); + } + [TestMethod] - [Description("Probe for the MCP half of per-session DI: the SDK creates an AsyncServiceScope per request handler invocation (McpServerOptions.ScopeRequests defaults to true) and exposes it as RequestContext.Services. Repl discarded it, so a Scoped service resolved from the application root and every tool call on every connection shared one instance.")] - public async Task When_TwoToolCallsShareAConnection_Then_EachResolvesItsOwnScopedInstance() + [Description("Issue #70 criterion 1 on MCP: two commands of one session must share their Scoped instance. Adopting the SDK's per-request scope (McpServerOptions.ScopeRequests defaults to true) would give each tool call a fresh instance, so a cart or an auth context could never survive from one call to the next on a connection.")] + public async Task When_TwoToolCallsShareAConnection_Then_TheyShareOneScopedInstance() { await using var fixture = await McpTestFixture.CreateAsync( app => app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()), services => services.AddScoped()).ConfigureAwait(false); - var first = await CallAsync(fixture, "scoped").ConfigureAwait(false); - var second = await CallAsync(fixture, "scoped").ConfigureAwait(false); + var first = await CallAsync(fixture.Client, "scoped").ConfigureAwait(false); + var second = await CallAsync(fixture.Client, "scoped").ConfigureAwait(false); first.Should().NotBeNullOrWhiteSpace(); - second.Should().NotBeNullOrWhiteSpace(); - first.Should().NotBe(second, "each MCP invocation is its own scope"); + first.Should().Be(second, "one connection is one session, and one session is one scope"); } [TestMethod] - [Description("Guards that the per-invocation scope is DISPOSED, not merely created: a Scoped IDisposable resolved by a tool call must be released when that call ends rather than accumulating for the life of the server process. The SDK disposes the scope it opened in a finally, so this holds only while Repl resolves from that scope instead of from the application root.")] - public async Task When_AToolCallResolvesAScopedDisposable_Then_ItIsDisposedWhenTheCallEnds() + [Description("Issue #70 criterion 6: concurrent MCP sessions must not observe each other's Scoped per-user state. Before the session scope existed, every connection resolved from the application root and shared one instance.")] + public async Task When_TwoConnectionsShareOneServer_Then_EachResolvesItsOwnScopedInstance() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.UseMcpServer(); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + + var first = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using var firstScope = first.ConfigureAwait(false); + var second = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using var secondScope = second.ConfigureAwait(false); + + var fromFirst = await CallAsync(first.Client, "scoped", token: cts.Token).ConfigureAwait(false); + var fromSecond = await CallAsync(second.Client, "scoped", token: cts.Token).ConfigureAwait(false); + + fromFirst.Should().NotBe(fromSecond, "another connection is another session"); + } + + [TestMethod] + [Description("Issue #70 criterion 3: a Scoped IDisposable resolved during a session is released when that session ends, not held for the life of the server process. The session scope is disposed with its McpSessionContext when the connection's RunAsync returns.")] + public async Task When_AConnectionEnds_Then_ItsScopedDisposablesAreDisposed() { var disposed = new List(); - await using var fixture = await McpTestFixture.CreateAsync( - app => app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()), - services => services.AddScoped(_ => new DisposableProbe(disposed))).ConfigureAwait(false); + var app = ReplApp.Create(services => services.AddScoped(_ => new DisposableProbe(disposed))); + app.UseMcpServer(); + app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()); - var reported = await CallAsync(fixture, "disposable").ConfigureAwait(false); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + + string reported; + var session = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using (session.ConfigureAwait(false)) + { + reported = await CallAsync(session.Client, "disposable", token: cts.Token).ConfigureAwait(false); + disposed.Should().BeEmpty("the session is still open"); + } - reported.Should().NotBeNullOrWhiteSpace(); - disposed.Should().ContainSingle( - "the scope the SDK opened for the call is disposed when the call returns"); + disposed.Should().ContainSingle(); reported.Should().Contain( disposed[0].ToString(), - "the instance disposed must be the one the command was handed"); + "the disposed instance must be the one the command was handed"); } [TestMethod] - [Description("Guards the composition ORDER, which is the invariant this change could break silently. The per-request scope descends from the application root and carries none of the session's services, so they are re-applied on top of it. Get that backwards and a command injecting IMcpClientRoots resolves nothing — and, because McpServiceProviderOverlay also answers IServiceProviderIsService, a prompt declaring one is reclassified as taking a client-supplied argument and stops being invocable at all.")] - public async Task When_ACommandInjectsACapabilityService_Then_ItStillResolvesThroughTheRequestScope() + [Description("Regression guard for the invariant PresenceServiceProvider exists to hold: what discovery advertised must be callable. Discovery and execution have to resolve a user Scoped service from the SAME instance — resolve execution from a per-invocation scope instead and a module gated on one is advertised after a tool sets it, then found absent when called. docs/module-presence.md recommends exactly this pattern.")] + public async Task When_APresencePredicateGatesOnAScopedService_Then_TheAdvertisedToolIsCallable() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.UseMcpServer(); + app.Map("signin", (ScopedProbe probe, ICoreReplApp core) => + { + probe.SignedIn = true; + core.InvalidateRouting(); + return "ok"; + }); + app.MapModule(new GatedModule(), (ScopedProbe probe) => probe.SignedIn); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + var session = await StartAsync( + handler, + cts.Token, + new McpClientOptions { ProtocolVersion = McpProtocolRevisions.LastWithSessions }).ConfigureAwait(false); + await using var scope = session.ConfigureAwait(false); + + (await session.Client.ListToolsAsync(cancellationToken: cts.Token).ConfigureAwait(false)) + .Should().NotContain(tool => string.Equals(tool.Name, "secret", StringComparison.Ordinal)); + + await CallAsync(session.Client, "signin", token: cts.Token).ConfigureAwait(false); + + (await session.Client.ListToolsAsync(cancellationToken: cts.Token).ConfigureAwait(false)) + .Should().Contain( + tool => string.Equals(tool.Name, "secret", StringComparison.Ordinal), + "the predicate reads the instance the tool call just wrote"); + + var body = await CallAsync(session.Client, "secret", token: cts.Token).ConfigureAwait(false); + body.Should().Contain("classified", "what discovery advertised has to run"); + } + + [TestMethod] + [Description("Guards the composition order. The session's capability services are layered OVER its DI scope, so a command resolves both; get it backwards and IMcpClientRoots resolves nothing — and because McpServiceProviderOverlay also answers IServiceProviderIsService, a declared capability is reclassified as a client-supplied argument and the primitive stops being invocable.")] + public async Task When_ACommandInjectsACapabilityService_Then_ItResolvesAlongsideTheSessionScope() { await using var fixture = await McpTestFixture.CreateAsync( app => app.Map( "capability", (IMcpClientRoots roots, IMcpFeedback feedback, ScopedProbe probe) => - $"{roots is not null}|{feedback is not null}|{probe.Id != Guid.Empty}"), + $"{roots is not null}|{feedback is not null}|{probe.Id}"), services => services.AddScoped()).ConfigureAwait(false); - var reported = await CallAsync(fixture, "capability").ConfigureAwait(false); + var first = await CallAsync(fixture.Client, "capability").ConfigureAwait(false); + var second = await CallAsync(fixture.Client, "capability").ConfigureAwait(false); - reported.Should().Contain("True|True|True"); + first.Should().Contain("True|True|"); + first.Should().Be(second, "the scoped probe beside the capabilities is the session's own"); } [TestMethod] - [Description("Guards the MCP half of issue #70: IReplSessionState is documented as per-session, but on MCP it fell through the session overlay to the application root, so two concurrent connections shared one bag. It is now session-owned, which is also what keeps the initialize-era dynamic-tools story working — a command writes it and the next tools/list on THAT connection sees it.")] + [Description("The MCP half of issue #70 for the framework's own session service: IReplSessionState fell through to the application root, so two concurrent connections shared one bag. It now resolves from the session scope, which is also what keeps the initialize-era dynamic-tools story working.")] public async Task When_TwoConnectionsShareOneServer_Then_NeitherReadsTheOtherSessionState() { var app = ReplApp.Create(); @@ -83,49 +190,69 @@ public async Task When_TwoConnectionsShareOneServer_Then_NeitherReadsTheOtherSes }); app.Map("recall", (IReplSessionState state) => state.Get("carried") ?? "(none)"); - var options = new ReplMcpServerOptions { TransportFactory = McpTestFixture.PipeTransportFactory }; - var handler = new McpServerHandler(app.Core, options, app.Services); using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); - var first = await McpPipeSession - .StartAsync(handler.RunAsync, clientOptions: null, cts.Token).ConfigureAwait(false); + var first = await StartAsync(handler, cts.Token).ConfigureAwait(false); await using var firstScope = first.ConfigureAwait(false); - var second = await McpPipeSession - .StartAsync(handler.RunAsync, clientOptions: null, cts.Token).ConfigureAwait(false); + var second = await StartAsync(handler, cts.Token).ConfigureAwait(false); await using var secondScope = second.ConfigureAwait(false); - await CallAsync(first, "remember", new Dictionary(StringComparer.Ordinal) - { - ["value"] = "alpha", - }, cts.Token).ConfigureAwait(false); + await CallAsync( + first.Client, + "remember", + new Dictionary(StringComparer.Ordinal) { ["value"] = "alpha" }, + cts.Token).ConfigureAwait(false); - var carried = await CallAsync(first, "recall", cancellationToken: cts.Token).ConfigureAwait(false); - var other = await CallAsync(second, "recall", cancellationToken: cts.Token).ConfigureAwait(false); + var carried = await CallAsync(first.Client, "recall", token: cts.Token).ConfigureAwait(false); + var other = await CallAsync(second.Client, "recall", token: cts.Token).ConfigureAwait(false); - carried.Should().Contain("alpha", "session state survives across calls on its own connection"); + carried.Should().Contain("alpha", "session state survives across the calls of its own connection"); other.Should().Contain("(none)", "another connection is another session"); } - private static Task CallAsync(McpTestFixture fixture, string tool) => - CallAsync(fixture.Client, tool, arguments: null, CancellationToken.None); + [TestMethod] + [Description("A consumer who registers their own IReplSessionState must get it on MCP too. Seeding the session's override map with a framework instance would win unconditionally over the container, so the one transport where per-connection state matters most would silently run a different implementation from the one the app composed.")] + public async Task When_AConsumerRegistersItsOwnSessionState_Then_McpResolvesIt() + { + RecordingSessionState.Constructed = 0; + var app = ReplApp.Create(services => services.AddScoped()); + app.UseMcpServer(); + app.Map("kind", (IReplSessionState state) => state.GetType().Name); - private static Task CallAsync( - McpPipeSession session, - string tool, - Dictionary? arguments = null, - CancellationToken cancellationToken = default) => - CallAsync(session.Client, tool, arguments, cancellationToken); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + var session = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using var scope = session.ConfigureAwait(false); + + var reported = await CallAsync(session.Client, "kind", token: cts.Token).ConfigureAwait(false); + + reported.Should().Contain(nameof(RecordingSessionState)); + RecordingSessionState.Constructed.Should().BeGreaterThan(0); + } + + private static McpServerHandler CreateHandler(ReplApp app) => + new( + app.Core, + new ReplMcpServerOptions { TransportFactory = McpTestFixture.PipeTransportFactory }, + app.Services); + + private static Task StartAsync( + McpServerHandler handler, + CancellationToken cancellationToken, + McpClientOptions? clientOptions = null) => + McpPipeSession.StartAsync(handler.RunAsync, clientOptions, cancellationToken); private static async Task CallAsync( - ModelContextProtocol.Client.McpClient client, + McpClient client, string tool, - Dictionary? arguments, - CancellationToken cancellationToken) + Dictionary? arguments = null, + CancellationToken token = default) { var result = await client.CallToolAsync( toolName: tool, arguments: arguments ?? new Dictionary(StringComparer.Ordinal), - cancellationToken: cancellationToken).ConfigureAwait(false); + cancellationToken: token).ConfigureAwait(false); return result.Content.OfType().First().Text; } } From 173146abec08994eb1ca42496f19a336d3cdabe4 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 07:02:41 -0400 Subject: [PATCH 08/24] fix(testing): stop disposal from tearing the scope out from under a running command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking the gate with a zero timeout stopped disposal from hanging, but the branch it left open was the wrong one: the single case where the gate is NOT acquired is the case where a command is mid-run, and disposal went on to release the session scope anyway. That command then resolves from a disposed provider and its scoped disposables die while it still holds them — a worse failure than the hang it replaced, and on a shipped type. Disposal now leaves the scope to the command that is still using it. The orphan outlives the handle and the process reclaims it; leaking a test session's scope is the lesser harm. The regression test observes the command's outcome instead of discarding it, so a fault there fails the test rather than surfacing as an unobserved task exception. The handle also stops masking IReplSessionState with a bag of its own. That mask predates the scope, and with a real scope it does harm: the registration is Scoped, so the session already has its own instance, while the mask silently ran a different implementation from the one a consumer composed — in the one harness whose job is to mirror production. The public doc-comment now says what disposal does: it releases the scope, and it does not wait for a running command. Refs #70 --- src/Repl.Testing/ReplSessionHandle.cs | 98 +++++++++------------------ 1 file changed, 33 insertions(+), 65 deletions(-) diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 8becb4c3..3b5fbc22 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -163,8 +163,14 @@ public SessionSnapshot GetSnapshot() } /// - /// Ends the session and removes it from its host. Disposing twice is a no-op. + /// Ends the session, removes it from its host, and releases the session's dependency-injection + /// scope — disposing every Scoped service it resolved. Disposing twice is a no-op. /// + /// + /// Does not wait for a command that is still running: an orphaned run would make this hang. Such a + /// run keeps the scope instead of having it disposed underneath it, so the scope outlives this + /// handle in that case. + /// public async ValueTask DisposeAsync() { if (_disposed) @@ -176,13 +182,22 @@ public async ValueTask DisposeAsync() _owner.RemoveSession(SessionId); ReplSessionIO.RemoveSession(SessionId); - // Best-effort: holding the gate keeps a command that starts between two runs from racing - // scoped disposal. Never WAIT for one that is already running — an orphaned timed-out run - // holds the gate until it finishes on its own, and waiting would turn an await using into a - // hang. Commands queued behind it stop at the re-check inside the gate instead. + // The gate is taken, never waited on. Waiting would turn an await using into a hang whenever a + // command is still running — an orphaned timed-out run holds the gate until it finishes on its + // own. Commands queued behind it stop at the re-check inside the gate instead. var held = await _commandGate.WaitAsync(TimeSpan.Zero).ConfigureAwait(false); try { + if (!held) + { + // A command is mid-run and resolving from this very provider. Disposing it here would + // pull the scope out from under that command, so its services start throwing + // ObjectDisposedException and its scoped disposables die while it still holds them. + // The orphan keeps the scope instead: it outlives the handle rather than faulting, and + // the process reclaims it. Leaking a test session's scope is the lesser harm. + return; + } + switch (_services) { case IAsyncDisposable asyncDisposable: @@ -351,69 +366,22 @@ public void OnInteractionEvent(ReplInteractionEvent evt) // open a fresh scope — and fresh Scoped instances — per command. private sealed class SessionScopedTestServices : IServiceProvider, IAsyncDisposable { - private readonly AsyncServiceScope? _scope; - private readonly IServiceProvider _inner; - private readonly InMemorySessionState _sessionState = new(); + private readonly AsyncServiceScope _scope; - private SessionScopedTestServices(AsyncServiceScope? scope, IServiceProvider inner) - { - _scope = scope; - _inner = inner; - } + private SessionScopedTestServices(AsyncServiceScope scope) => _scope = scope; - public static SessionScopedTestServices Create(ReplApp app) - { - var root = app.Services; - if (root.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory) - { - var scope = scopeFactory.CreateAsyncScope(); - return new SessionScopedTestServices(scope, scope.ServiceProvider); - } - - return new SessionScopedTestServices(scope: null, root); - } - - public object? GetService(Type serviceType) => - serviceType == typeof(IReplSessionState) ? _sessionState : _inner.GetService(serviceType); - - public ValueTask DisposeAsync() => _scope?.DisposeAsync() ?? ValueTask.CompletedTask; - } - - private sealed class InMemorySessionState : IReplSessionState - { - private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); + // ReplApp.Services is always a built ServiceProvider, which always supplies the factory, so + // there is no scope-less case to fall back to here — unlike the Run* paths, which take whatever + // provider a caller hands them. + public static SessionScopedTestServices Create(ReplApp app) => + new(app.Services.GetRequiredService().CreateAsyncScope()); - public bool TryGet(string key, out T? value) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - if (_values.TryGetValue(key, out var existing) && existing is T typed) - { - value = typed; - return true; - } - - value = default; - return false; - } - - public T? Get(string key) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - return TryGet(key, out var value) ? value : default; - } - - public void Set(string key, T value) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - _values[key] = value; - } - - public bool Remove(string key) - { - ArgumentException.ThrowIfNullOrWhiteSpace(key); - return _values.Remove(key); - } + // IReplSessionState is resolved from the scope like everything else, rather than masked with a + // harness-owned bag: the registration is Scoped, so the scope already gives this session its own + // — and masking it would run a consumer's own implementation in production while this harness + // silently exercised a different one. + public object? GetService(Type serviceType) => _scope.ServiceProvider.GetService(serviceType); - public void Clear() => _values.Clear(); + public ValueTask DisposeAsync() => _scope.DisposeAsync(); } } From 2771866e3261ea4fcc6782376e35cc21648b0a6f Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 07:02:58 -0400 Subject: [PATCH 09/24] fix(di): let a profile default the session scope, and refuse an undefined one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two contract corrections that cost nothing now and a deprecation cycle after the stable cut. ReplRunOptions.SessionScope is nullable, like its neighbour ProcessSignalHandling. With a non-nullable property, "PerRun" and "the caller said nothing" are the same value, so no composition profile could ever supply a session-scope default without changing the property type — a binary break. An undefined value is now rejected rather than treated as PerRun. The same file already argues the case for the neighbouring enum: configuration and deserialization can produce one, and the fall-through direction is the dangerous one here — a caller who meant to opt out gets a nested scope, silently duplicating the scoped graph they asked to share. DefaultsSessionState is deleted. It was byte-identical to Repl.Core's InMemoryReplSessionState, including its comment, and this series changed both in lockstep while a third copy in Repl.Testing quietly diverged — the maintenance cost demonstrated rather than hypothesised. The concurrency guard now covers the one implementation that ships, which is also the one MCP puts on the concurrent path. Its comment is re-justified too: the sharing it blamed is what the scoped registration removed. What still shares an instance is concurrent calls within one session, and a singleton that captured a scope. Refs #70 --- .../Session/InMemoryReplSessionState.cs | 6 +++-- src/Repl.Defaults/ReplApp.cs | 23 +++++++++++++++---- src/Repl.Defaults/ReplRunOptions.cs | 11 +++++++-- src/Repl.Defaults/SessionScopeBehavior.cs | 14 +++++++++-- src/Repl.Tests/Given_SessionState.cs | 4 ++-- 5 files changed, 45 insertions(+), 13 deletions(-) diff --git a/src/Repl.Core/Session/InMemoryReplSessionState.cs b/src/Repl.Core/Session/InMemoryReplSessionState.cs index 245b2e83..85dc00ae 100644 --- a/src/Repl.Core/Session/InMemoryReplSessionState.cs +++ b/src/Repl.Core/Session/InMemoryReplSessionState.cs @@ -4,8 +4,10 @@ namespace Repl; internal sealed class InMemoryReplSessionState : IReplSessionState { - // Every concurrent Telnet, WebSocket and MCP session can resolve this state, so writes from - // two sessions race. An unsynchronised Dictionary corrupts its bucket table under that. + // Registered Scoped, so sessions no longer share an instance — but concurrent calls WITHIN one + // session still do: several MCP tool calls run at once on one connection, and a singleton that + // captured a scope hands its instance to every later caller. An unsynchronised Dictionary does + // not merely interleave under that, it corrupts its bucket table. private readonly ConcurrentDictionary _values = new(StringComparer.OrdinalIgnoreCase); public bool TryGet(string key, out T? value) diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 0c12468d..d4511f35 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -424,7 +424,7 @@ private async ValueTask RunOutcomeAsync( private async ValueTask RunHostedLifecycleOutcomeAsync( string[] args, IServiceProvider services, - SessionScopeBehavior sessionScope, + SessionScopeBehavior? sessionScope, CancellationToken cancellationToken) { IReadOnlyList started = []; @@ -695,11 +695,24 @@ public async ValueTask RunAsync( /// private static async ValueTask RunInSessionScopeAsync( IServiceProvider services, - SessionScopeBehavior sessionScope, + SessionScopeBehavior? sessionScope, Func> run, CancellationToken cancellationToken) { - if (sessionScope == SessionScopeBehavior.CallerOwned + // Rejected rather than treated as PerRun, for the reason the process-signal mode above states: + // numeric configuration or deserialization can produce an undefined value, and falling through a + // negative test would nest a scope inside a caller who asked to own it — silently duplicating + // their scoped graph instead of sharing it. + var resolved = sessionScope ?? SessionScopeBehavior.PerRun; + if (resolved is not (SessionScopeBehavior.PerRun or SessionScopeBehavior.CallerOwned)) + { + throw new ArgumentOutOfRangeException( + nameof(sessionScope), + resolved, + $"ReplRunOptions.{nameof(ReplRunOptions.SessionScope)} is not a defined {nameof(SessionScopeBehavior)}."); + } + + if (resolved == SessionScopeBehavior.CallerOwned || services.GetService(typeof(IServiceScopeFactory)) is not IServiceScopeFactory scopeFactory) { return await run(services, cancellationToken).ConfigureAwait(false); @@ -982,7 +995,7 @@ private SessionOverlayServiceProvider CreateSessionOverlay(IServiceProvider exte { var defaults = new Dictionary { - [typeof(IReplSessionState)] = new DefaultsSessionState(), + [typeof(IReplSessionState)] = new InMemoryReplSessionState(), [typeof(IHistoryProvider)] = new InMemoryHistoryProvider(), [typeof(TimeProvider)] = TimeProvider.System, [typeof(IReplKeyReader)] = new ConsoleKeyReader(), @@ -1041,7 +1054,7 @@ private static void EnsureDefaultServices(IServiceCollection services, CoreReplA // Scoped, not singleton: Run* opens one scope per session, so this is the framework own // per-session service and a second session no longer reads what the first one stored. With a // provider that cannot scope, it resolves from the root exactly as it did before. - services.TryAddScoped(); + services.TryAddScoped(); services.TryAddSingleton(); services.TryAddSingleton(TimeProvider.System); services.TryAdd(ServiceDescriptor.Singleton(sp => diff --git a/src/Repl.Defaults/ReplRunOptions.cs b/src/Repl.Defaults/ReplRunOptions.cs index 382607d9..0f7b435f 100644 --- a/src/Repl.Defaults/ReplRunOptions.cs +++ b/src/Repl.Defaults/ReplRunOptions.cs @@ -17,9 +17,16 @@ public sealed record ReplRunOptions public HostedServiceLifecycleMode HostedServiceLifecycle { get; init; } = HostedServiceLifecycleMode.None; /// - /// Gets or sets how this run manages the session dependency-injection scope. + /// Gets how this run manages the session's dependency-injection scope. + /// uses the active application profile's default, which is + /// . /// - public SessionScopeBehavior SessionScope { get; init; } = SessionScopeBehavior.PerRun; + /// + /// Nullable for the same reason as : a composition profile has to + /// be able to supply a default, and that is only expressible if "the caller said nothing" is + /// distinguishable from an explicit value. + /// + public SessionScopeBehavior? SessionScope { get; init; } /// /// Gets or sets the ANSI support mode for the session. diff --git a/src/Repl.Defaults/SessionScopeBehavior.cs b/src/Repl.Defaults/SessionScopeBehavior.cs index 1dc6c310..4322af8a 100644 --- a/src/Repl.Defaults/SessionScopeBehavior.cs +++ b/src/Repl.Defaults/SessionScopeBehavior.cs @@ -1,14 +1,24 @@ namespace Repl; /// -/// How a Run* call manages the session's dependency-injection scope. +/// How a Run* call manages the session's dependency-injection scope. /// +/// +/// Governs the Run* family only. MCP execution never goes through it: an MCP connection is its +/// own session and opens its own scope, so there is nothing for a caller to own there. +/// public enum SessionScopeBehavior { /// - /// The run opens one DI scope for the session (the default): Scoped services resolve + /// The run opens one DI scope for the session (the default): Scoped services resolve /// per session and scoped disposables are released when the session ends. /// + /// + /// The scope comes from IServiceScopeFactory, which is a singleton, so a scope opened over a + /// provider that is itself a scope is a SIBLING rooted at the application root — not a child. Against + /// a caller's request scope that does not hide their scoped instances, it duplicates them: a second + /// unit of work alongside the live one. Use there. + /// PerRun = 0, /// diff --git a/src/Repl.Tests/Given_SessionState.cs b/src/Repl.Tests/Given_SessionState.cs index 7f0a7a29..598a5203 100644 --- a/src/Repl.Tests/Given_SessionState.cs +++ b/src/Repl.Tests/Given_SessionState.cs @@ -12,7 +12,7 @@ public sealed class Given_SessionState [Description("Regression guard: IReplSessionState is resolved by every concurrent Telnet, WebSocket and MCP session, so its default implementation must tolerate concurrent writers. An unsynchronised Dictionary does not: concurrent Set can lose entries or corrupt the bucket table during a resize.")] public async Task When_SessionsWriteStateConcurrently_Then_EveryWriteIsReadableAfterwards() { - var sut = new DefaultsSessionState(); + var sut = new InMemoryReplSessionState(); await Task.WhenAll(Enumerable.Range(0, Writers).Select(writer => Task.Run(() => { @@ -37,7 +37,7 @@ await Task.WhenAll(Enumerable.Range(0, Writers).Select(writer => Task.Run(() => [Description("Regression guard: reading session state while another session writes it must neither throw nor observe a torn entry. Enumeration-free readers still fault on a Dictionary being resized underneath them.")] public async Task When_OneSessionReadsWhileAnotherWrites_Then_NoReaderFaults() { - var sut = new DefaultsSessionState(); + var sut = new InMemoryReplSessionState(); var faults = new ConcurrentQueue(); using var done = new CancellationTokenSource(); From 7b7707eddb879781516ece75cdc0dba0d28ef67b Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 07:02:59 -0400 Subject: [PATCH 10/24] test: make the session-scope guards able to fail, and correct the DI boundary in the docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Falsifying the suite found three guards that could not detect the regression they name. ReplTestHost invokes its app factory once per session, so the two-session isolation test held two separate containers and two Scoped instances differed whatever lifetime was registered — it passed on a full revert. One app now backs both sessions. The hosted-service boundary test registered its probe as a singleton, which is always constructed in the root and always injected the root provider, so it reported the same instance wherever the scope boundary sat. Scoped, it reveals which provider started it. The pre-existing per-run isolation guard asserted Contain("0") on output that still carried the version banner, and its interactive sibling Contain("1") — both satisfied without either command running. That is how the defect this series fixes went unguarded by the two tests written for it. Both now suppress the banner and assert what the command rendered. The docs follow the connection scope: the DI row moves from the invocation to the connection row of the isolation table, the lifetime table says one client connection rather than one tool call — a cart that resets between two calls of one client is not a cart — and SessionScopeBehavior states that it governs Run* only, and that PerRun over a caller's scope creates a sibling rather than a child. Refs #70 --- docs/best-practices.md | 2 +- docs/glossary.md | 4 +- docs/mcp-transports.md | 31 +++++++------- .../Given_SessionScopedServices.cs | 40 ++++++++++++------- .../Given_SessionState.cs | 20 +++++++--- 5 files changed, 59 insertions(+), 38 deletions(-) diff --git a/docs/best-practices.md b/docs/best-practices.md index 17d60a45..ccea5958 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -112,7 +112,7 @@ Note: DI singleton factories are resolved lazily, so the values are available af | Lifetime | Resolves once per | Use it for | |---|---|---| | `Singleton` | application | caches, clients, anything shared by every session | -| `Scoped` | **session** — one `Run*` call: a CLI invocation, an interactive run, a Telnet or WebSocket connection; under MCP, one tool call | per-user state: auth context, a cart, a unit of work | +| `Scoped` | **session** — one `Run*` call: a CLI invocation, an interactive run, a Telnet or WebSocket connection; under MCP, one client connection | per-user state: auth context, a cart, a unit of work | | `Transient` | resolution | cheap stateless helpers | A `Scoped` disposable is disposed when its session ends, so session resources do not accumulate for the diff --git a/docs/glossary.md b/docs/glossary.md index 07fafb15..5fad48ec 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -56,8 +56,8 @@ Interface for packaging reusable command groups. ### IReplSessionState -Per-session state container for interactive and hosted sessions. Registered `Scoped`, so each session -gets its own; under MCP it belongs to the connection rather than to the individual tool call. +Per-session state container for interactive and hosted sessions. Registered `Scoped`, so each +session gets its own — under MCP, each client connection. ### Literal segment diff --git a/docs/mcp-transports.md b/docs/mcp-transports.md index 48df9b24..33008f0b 100644 --- a/docs/mcp-transports.md +++ b/docs/mcp-transports.md @@ -101,21 +101,22 @@ every request rather than once per connection. So the boundaries are not all the | Isolated per | What | |---|---| | Request | Client capabilities, the requested log level, the destination for sampling, elicitation and progress, and — on a reused `BuildMcpServerOptions()` result — native roots, resolved once per request and never cached across connections | -| Invocation | I/O capture — each tool call gets its own capture scope, not one per connection — **and the DI scope**: a `Scoped` registration resolves once per tool call, prompt fetch or resource read, and its disposables are released when that call returns | -| Connection (`mcp serve` only) | The MCP session object, the native roots cache, soft roots, session-aware routing state, and `IReplSessionState` | - -The DI scope is the SDK's own: `McpServerOptions.ScopeRequests` defaults to `true`, so it opens an -`AsyncServiceScope` per request handler invocation and disposes it when the handler returns. Repl runs -the command inside that scope with the session's services layered over it. That is why the unit is the -invocation and not the connection — `2026-07-28` has no connection to scope to — and it is also what -makes a future ASP.NET Core host behave correctly for free, since there the scope becomes the HTTP -request's own. - -`IReplSessionState` is the exception, and deliberately so: it is Repl's session abstraction rather than -a user service lifetime. On the initialize era a command writes it and calls `InvalidateRouting()`, and -the next `tools/list` on that connection has to see what it wrote. What that means for the advertised -tool set on each revision — and what to reach for instead on `2026-07-28` — is in -[Conformance](mcp-conformance.md#tool-list-invariance-on-2026-07-28). +| Invocation | I/O capture — each tool call gets its own capture scope, not one per connection | +| Connection (`mcp serve` only) | The MCP session object, the native roots cache, soft roots, session-aware routing state, `IReplSessionState`, and **the DI scope**: a `Scoped` registration resolves once per connection, and its disposables are released when the connection ends | + +The DI scope is the connection's, for the same reason it is the session's on every other transport: a +`Scoped` service is where per-user state lives, and state that resets between two calls of one client +is not per-user state. It is also what keeps discovery and execution honest — both resolve a module +presence predicate's services from the same scope, so a command that was advertised can be called. + +The MCP SDK opens a scope of its own per request (`McpServerOptions.ScopeRequests`, default `true`) and +exposes it as `RequestContext.Services`. Repl does not run commands in it: that scope descends from the +application root and is a sibling of the connection's, so adopting it would give the catalog and the +command two different instances of the same service. + +Under stateless HTTP hosting the two coincide — the SDK builds one short-lived server per request over +`HttpContext.RequestServices` — so the connection scope *is* the request scope there, with nothing extra +to configure. A server created from a reused `BuildMcpServerOptions()` result has everything above except the connection row; see the known limitation above. That matters especially when using dynamic tools, diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index d9acfe60..c12490f2 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -25,20 +25,24 @@ private sealed class DisposableProbe(List disposed) : IDisposable public void Dispose() => disposed.Add(Id); } - private static ReplTestHost CreateProbeHost(List? disposedTracker = null) => - ReplTestHost.Create(() => + // ONE app for every session this host opens. ReplTestHost invokes the factory per session, so + // returning a fresh ReplApp would give each session its own container — and then two sessions + // resolve distinct instances whatever lifetime is registered, which makes a cross-session + // isolation assertion pass on a full revert of the feature it claims to guard. + private static ReplTestHost CreateProbeHost(List? disposedTracker = null) + { + var app = ReplApp.Create(services => { - var app = ReplApp.Create(services => - { - services.AddScoped(); - services.AddTransient(); - services.AddScoped(_ => new DisposableProbe(disposedTracker ?? [])); - }).UseDefaultInteractive(); - app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); - app.Map("transient", (TransientProbe probe) => probe.Id.ToString()); - app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()); - return app; - }); + services.AddScoped(); + services.AddTransient(); + services.AddScoped(_ => new DisposableProbe(disposedTracker ?? [])); + }).UseDefaultInteractive(); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + app.Map("transient", (TransientProbe probe) => probe.Id.ToString()); + app.Map("disposable", (DisposableProbe probe) => probe.Id.ToString()); + + return ReplTestHost.Create(() => app); + } private static async Task RunAndCaptureAsync(ReplSessionHandle session, string command) { @@ -161,7 +165,10 @@ public void When_HostedServicesRun_Then_TheyStartOutsideTheSessionScope() { services.AddScoped(); services.AddSingleton(observed); - services.AddSingleton(); + // Scoped, not singleton: a singleton is always constructed in the root and always injected + // the root provider, so it reports the same instance wherever the scope boundary sits and + // the assertion below could never fail. + services.AddScoped(); }); sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); var options = new ReplRunOptions { HostedServiceLifecycle = HostedServiceLifecycleMode.Head }; @@ -230,11 +237,14 @@ public async Task When_ACommandIsStillRunning_Then_DisposingTheSessionDoesNotHan var disposedInTime = await Task.WhenAny(dispose, Task.Delay(TimeSpan.FromSeconds(5))) == dispose; release.TrySetResult(); - await Task.WhenAny(running, Task.Delay(TimeSpan.FromSeconds(30))); + var completed = await running.WaitAsync(TimeSpan.FromSeconds(30)); await dispose.WaitAsync(TimeSpan.FromSeconds(30)); disposedInTime.Should().BeTrue( "disposing a session must not wait unbounded on a command that is still running"); + completed.ExitCode.Should().Be( + 0, + "disposal must not tear the session scope out from under the command that is still using it"); } [TestMethod] diff --git a/src/Repl.IntegrationTests/Given_SessionState.cs b/src/Repl.IntegrationTests/Given_SessionState.cs index 7457aef5..575c587a 100644 --- a/src/Repl.IntegrationTests/Given_SessionState.cs +++ b/src/Repl.IntegrationTests/Given_SessionState.cs @@ -1,4 +1,4 @@ -namespace Repl.IntegrationTests; +namespace Repl.IntegrationTests; [TestClass] [DoNotParallelize] @@ -20,7 +20,15 @@ public void When_RunningInteractiveSession_Then_SessionStatePersistsAcrossComman var output = ConsoleCaptureHelper.CaptureWithInput("set\nget\nexit\n", () => sut.Run([])); output.ExitCode.Should().Be(0); - output.Text.Should().Contain("1"); + // The banner carries a version, so a bare Contain("1") passes without either command running. + // Counting rendered results discriminates instead: "set" prints 1, and "get" prints 1 only + // because the state persisted — without persistence it prints 0 and the count drops to one. + var results = output.Text + .Split('\n') + .Select(static line => line.Trim().TrimStart('>').Trim()) + .Count(static value => string.Equals(value, "1", StringComparison.Ordinal)); + + results.Should().Be(2, "both the write and the later read report the same counter"); } [TestMethod] @@ -35,11 +43,13 @@ public void When_RunningCliInvocations_Then_SessionStateIsIsolatedPerRun() }); sut.Map("get", (IReplSessionState state) => state.Get("counter")); - var setOutput = ConsoleCaptureHelper.Capture(() => sut.Run(["set"])); - var getOutput = ConsoleCaptureHelper.Capture(() => sut.Run(["get"])); + var setOutput = ConsoleCaptureHelper.Capture(() => sut.Run(["set", "--no-logo"])); + var getOutput = ConsoleCaptureHelper.Capture(() => sut.Run(["get", "--no-logo"])); setOutput.ExitCode.Should().Be(0); getOutput.ExitCode.Should().Be(0); - getOutput.Text.Should().Contain("0"); + // Exact, and with the banner suppressed: Contain("0") against un-suppressed output is satisfied + // by the version line alone, which is how this guard stayed green through the defect it names. + getOutput.Text.Trim().Should().Be("0"); } } From 27a2b8aa0083827ef503d225016b1117847a1154 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 07:04:59 -0400 Subject: [PATCH 11/24] test(mcp): pin the reusable-options carve-out, and name it where a consumer sets it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BuildMcpServerOptions() path shipped the new guarantee with no regression net: every test reached the server through mcp serve, which creates a context per connection. That path has one handler-lifetime context instead, so its DI scope is shared by every connection it serves — the same carve-out its native roots cache and soft roots already have, and now pinned so it stays a known limitation rather than becoming a silent cross-client leak. SessionDescriptor.ConfigureRunOptions says that SessionScope is ignored there. The handle owns the scope because one test session spans several one-shot runs; a consumer setting it had no diagnostic and no doc telling them so. Refs #70 --- src/Repl.McpTests/Given_McpSessionScopes.cs | 47 ++++++++++++++++++++- src/Repl.Testing/SessionDescriptor.cs | 5 +++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 4eca405b..0b765974 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -1,6 +1,7 @@ -using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection; using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; +using ModelContextProtocol.Server; using Repl.Mcp; namespace Repl.McpTests; @@ -231,6 +232,50 @@ public async Task When_AConsumerRegistersItsOwnSessionState_Then_McpResolvesIt() RecordingSessionState.Constructed.Should().BeGreaterThan(0); } + [TestMethod] + [Description("Pins the documented carve-out on the reusable BuildMcpServerOptions() path: that server has no connection row, so its one handler-lifetime context — and therefore one DI scope — is shared by every connection it serves, exactly as its native roots cache and soft roots are. Stated here so it stays a known limitation rather than becoming a silent cross-client leak.")] + public async Task When_ConnectionsShareAReusableOptionsResult_Then_TheyShareOneScope() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var options = app.BuildMcpServerOptions(); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var first = await StartSharedAsync(options, cts.Token).ConfigureAwait(false); + await using var firstScope = first.ConfigureAwait(false); + var second = await StartSharedAsync(options, cts.Token).ConfigureAwait(false); + await using var secondScope = second.ConfigureAwait(false); + + var fromFirst = await CallAsync(first.Client, "scoped", token: cts.Token).ConfigureAwait(false); + var fromSecond = await CallAsync(second.Client, "scoped", token: cts.Token).ConfigureAwait(false); + + fromFirst.Should().NotBeNullOrWhiteSpace(); + fromFirst.Should().Be( + fromSecond, + "a reused options result has one context for every connection, so it has one scope too"); + } + + private static Task StartSharedAsync( + McpServerOptions mcpOptions, + CancellationToken cancellationToken) => + McpPipeSession.StartAsync( + async (io, token) => + { + var transport = new StreamServerTransport(io.InputStream, io.OutputStream, "shared-options-server"); + var server = McpServer.Create(transport, mcpOptions); + try + { + await server.RunAsync(token).ConfigureAwait(false); + } + finally + { + await server.DisposeAsync().ConfigureAwait(false); + await transport.DisposeAsync().ConfigureAwait(false); + } + }, + clientOptions: null, + cancellationToken); + private static McpServerHandler CreateHandler(ReplApp app) => new( app.Core, diff --git a/src/Repl.Testing/SessionDescriptor.cs b/src/Repl.Testing/SessionDescriptor.cs index d71de1b9..e2b5ed20 100644 --- a/src/Repl.Testing/SessionDescriptor.cs +++ b/src/Repl.Testing/SessionDescriptor.cs @@ -45,6 +45,11 @@ public sealed record SessionDescriptor /// /// Optional session-specific run options customization. /// + /// + /// is ignored here: the session handle owns the scope, + /// because one test session spans several one-shot runs and each of them would otherwise open — and + /// discard — a scope of its own per command. + /// public Func? ConfigureRunOptions { get; init; } internal ReplRunOptions BuildRunOptions(ReplScenarioOptions scenario) From a297ed6aac995db396781cb06b959868be5562ed Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 08:55:56 -0400 Subject: [PATCH 12/24] fix(mcp): release the connection scope asynchronously, and stop opening one for the catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two published findings (Codex P1/P2, autocarl) plus the local panel converged on the same two defects, both in CreateSessionContext. McpSessionContext.Dispose() released the connection's AsyncServiceScope synchronously. Microsoft DI's ServiceProviderEngineScope.Dispose() throws InvalidOperationException rather than skip a service that implements only IAsyncDisposable — exactly the shape of the unit-of-work this branch's own lifetime guidance recommends registering Scoped. The throw escaped RunAsync and failed mcp serve at every affected connection's teardown, unlogged. The context is now IAsyncDisposable only — no type in this repository implements both — and RunAsync releases it with DisposeAsync. The catalog context (McpRootsScope.Request) opened a scope nothing could ever dispose: it is a handler-lifetime temporary with no owner, alive for the life of BuildMcpServerOptions() callers and mcp serve invocations alike. It now resolves straight from the app's services instead. This changes nothing observable: a Scoped registration resolved directly from a root ServiceProvider is cached by that root exactly as it would be by an owned scope, so the one-instance-per-context carve-out this path already documents holds unchanged. Refs #70, #74 --- src/Repl.Mcp/McpServerHandler.cs | 40 ++++++++++++------- src/Repl.Mcp/McpSessionContext.cs | 16 +++++--- src/Repl.McpTests/Given_McpSessionScopes.cs | 43 ++++++++++++++++++++- 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index 050621bf..a91c0e55 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -34,7 +34,9 @@ internal sealed class McpServerHandler private readonly McpFeedbackService _feedback; // Context for work that belongs to no MCP session: eager fail-fast validation, the pre-built // catalog behind BuildMcpServerOptions, and the snapshot test seams. One per handler, sharing its - // lifetime, so nothing disposes it. See McpRootsScope for what that costs anything stored here. + // lifetime, so nothing disposes it — and CreateSessionContext deliberately opens it no DI scope to + // dispose, since a handler-lifetime temporary with no owner would only ever leak one. See + // McpRootsScope for what that costs anything stored here. private readonly McpSessionContext _catalogContext; private readonly Lock _refreshLock = new(); private readonly Lock _attachLock = new(); @@ -91,12 +93,17 @@ private McpSessionContext CreateSessionContext(McpRootsScope rootsScope) [typeof(IMcpFeedback)] = _feedback, }; - // One DI scope per session, so a Scoped registration is per connection here just as it is per + // One DI scope per CONNECTION, so a Scoped registration is per connection here just as it is per // Run* on the other transports — and so discovery and execution read the SAME instance, which // is what lets a module presence predicate gate on one. Resolving IReplSessionState from it // rather than overriding it also keeps a consumer's own implementation reachable under MCP. - // A provider without scope support runs unscoped, as the Run* paths do. - var scope = _services.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory + // A provider without scope support runs unscoped, as the Run* paths do. The catalog context + // (McpRootsScope.Request) stands in for no session at all — it is a handler-lifetime temporary + // with no owner to dispose a scope for, so it resolves straight from _services instead. On the + // reusable-options path that also means Scoped resolves once from the root, matching the + // single-context, single-instance-for-every-connection carve-out that path already documents. + var scope = rootsScope == McpRootsScope.Connection + && _services.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory ? scopeFactory.CreateAsyncScope() : (AsyncServiceScope?)null; var sessionServices = scope?.ServiceProvider ?? _services; @@ -145,18 +152,21 @@ public async Task RunAsync(IReplIoContext io, CancellationToken ct) : new StdioServerTransport(serverName); try { - using var context = CreateSessionContext(McpRootsScope.Connection); - var server = McpServer.Create(transport, serverOptions, serviceProvider: context.Services); - AttachSession(context, server); - - try - { - await server.RunAsync(ct).ConfigureAwait(false); - } - finally + var context = CreateSessionContext(McpRootsScope.Connection); + await using (context.ConfigureAwait(false)) { - DetachSession(context); - await server.DisposeAsync().ConfigureAwait(false); + var server = McpServer.Create(transport, serverOptions, serviceProvider: context.Services); + AttachSession(context, server); + + try + { + await server.RunAsync(ct).ConfigureAwait(false); + } + finally + { + DetachSession(context); + await server.DisposeAsync().ConfigureAwait(false); + } } } finally diff --git a/src/Repl.Mcp/McpSessionContext.cs b/src/Repl.Mcp/McpSessionContext.cs index 0dc8dace..e90aacb5 100644 --- a/src/Repl.Mcp/McpSessionContext.cs +++ b/src/Repl.Mcp/McpSessionContext.cs @@ -18,7 +18,7 @@ namespace Repl.Mcp; /// through the per-request binding, which is /// finer-grained than the session. /// -internal sealed class McpSessionContext : IDisposable +internal sealed class McpSessionContext : IAsyncDisposable { private SnapshotCacheEntry? _snapshotCache; private int _compatibilityIntroServed; @@ -77,13 +77,17 @@ public bool TryClaimCompatibilityIntro() => /// Re-arms the compatibility-shim intro after a routing invalidation. public void ResetCompatibilityIntro() => Interlocked.Exchange(ref _compatibilityIntroServed, 0); - public void Dispose() + public async ValueTask DisposeAsync() { SnapshotGate.Dispose(); - // Releases this session's Scoped services. Disposed synchronously because the context is, - // which is the reason the scope is stored rather than awaited: nothing on an MCP connection - // teardown path is async, and a Scoped disposable here is an application object, not I/O. - _scope?.Dispose(); + // Releases this session's Scoped services. Must be awaited: a consumer may register the unit + // of work the lifetime guidance recommends Scoped as IAsyncDisposable only, and Microsoft DI's + // ServiceProviderEngineScope.Dispose() throws InvalidOperationException rather than skip it — + // which would otherwise fault RunAsync at every connection's teardown. + if (_scope is { } scope) + { + await scope.DisposeAsync().ConfigureAwait(false); + } } /// diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 0b765974..92138b49 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -27,6 +27,22 @@ private sealed class DisposableProbe(List disposed) : IDisposable public void Dispose() => disposed.Add(Id); } + /// + /// Disposable only asynchronously, which is the shape Microsoft DI refuses to release through a + /// synchronous scope disposal — the canonical example being the unit of work the lifetime guidance + /// recommends registering Scoped. + /// + private sealed class AsyncOnlyProbe(List disposed) : IAsyncDisposable + { + public Guid Id { get; } = Guid.NewGuid(); + + public ValueTask DisposeAsync() + { + disposed.Add(Id); + return ValueTask.CompletedTask; + } + } + private sealed class GatedModule : IReplModule { public void Map(IReplMap app) => app.Map("secret", () => "classified").ReadOnly(); @@ -124,6 +140,31 @@ public async Task When_AConnectionEnds_Then_ItsScopedDisposablesAreDisposed() "the disposed instance must be the one the command was handed"); } + [TestMethod] + [Description("The DI scope must be released asynchronously: Microsoft DI's ServiceProviderEngineScope throws InvalidOperationException from a synchronous Dispose() when it holds a service that implements only IAsyncDisposable, which is exactly the shape of a unit-of-work the lifetime guidance recommends registering Scoped. A synchronous release would fault RunAsync at connection teardown instead of running the command.")] + public async Task When_AConnectionEndsWithAnAsyncOnlyScopedService_Then_TeardownDoesNotThrow() + { + var disposed = new List(); + var app = ReplApp.Create(services => services.AddScoped(_ => new AsyncOnlyProbe(disposed))); + app.UseMcpServer(); + app.Map("disposable", (AsyncOnlyProbe probe) => probe.Id.ToString()); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + + var session = await StartAsync(handler, cts.Token).ConfigureAwait(false); + var teardown = async () => + { + await CallAsync(session.Client, "disposable", token: cts.Token).ConfigureAwait(false); + await session.DisposeAsync().ConfigureAwait(false); + }; + + await teardown.Should().NotThrowAsync( + "the connection's own AsyncServiceScope must be released with DisposeAsync, not Dispose") + .ConfigureAwait(false); + disposed.Should().ContainSingle(); + } + [TestMethod] [Description("Regression guard for the invariant PresenceServiceProvider exists to hold: what discovery advertised must be callable. Discovery and execution have to resolve a user Scoped service from the SAME instance — resolve execution from a per-invocation scope instead and a module gated on one is advertised after a tool sets it, then found absent when called. docs/module-presence.md recommends exactly this pattern.")] public async Task When_APresencePredicateGatesOnAScopedService_Then_TheAdvertisedToolIsCallable() @@ -233,7 +274,7 @@ public async Task When_AConsumerRegistersItsOwnSessionState_Then_McpResolvesIt() } [TestMethod] - [Description("Pins the documented carve-out on the reusable BuildMcpServerOptions() path: that server has no connection row, so its one handler-lifetime context — and therefore one DI scope — is shared by every connection it serves, exactly as its native roots cache and soft roots are. Stated here so it stays a known limitation rather than becoming a silent cross-client leak.")] + [Description("Pins the documented carve-out on the reusable BuildMcpServerOptions() path: that server has no connection row, so its one handler-lifetime context resolves Scoped registrations straight from the app root — no owned scope to dispose, so one root-cached instance is shared by every connection it serves, exactly as its native roots cache and soft roots are. Stated here so it stays a known limitation rather than becoming a silent cross-client leak.")] public async Task When_ConnectionsShareAReusableOptionsResult_Then_TheyShareOneScope() { var app = ReplApp.Create(services => services.AddScoped()); From 464b533fe4c3039cad43d37a12c662b8fd65692d Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:04:52 -0400 Subject: [PATCH 13/24] fix(testing): finish disposing a session's scope after the command that outlived it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deferring the running command's scope was correct — disposing it out from under a still-running command was the worse defect this replaced. But the deferral had no second half: DisposeAsync's early return, when the gate was held by a running command, was the ONLY attempt at disposal. _disposed was already true and the handle already removed from its host, so nothing ever came back to finish the job. autocarl reproduced this directly: resolve a Scoped IDisposable, dispose the handle while its command is blocked, release the command, observe the resource still undisposed after the command completes successfully. A single bool read-then-set could not close this without a race: whichever side (DisposeAsync or the command's own finally) reads "was disposal requested" a moment too early makes the wrong call, and nobody disposes. _serviceLifecycle replaces that with one field mutated only through Interlocked ops on both sides — a disposal-requested bit that latches once and a count of commands currently in flight. Interlocked.Or and Interlocked.Add on the same location are globally ordered relative to each other, so whichever side's read-modify-write observes the other's contribution already applied is the one that defers, and the one that observes it absent is the one that disposes: exactly one side ever does. The command registers as in-flight before its inner ThrowIfDisposed re-check, not after, which is what closes the remaining gap — a DisposeAsync racing into that specific window now sees the registration and correctly defers, rather than disposing under a command that believes it is still clear to run. DisposeServicesOnceAsync carries its own dispose-once guard, because a command that loses the inner ThrowIfDisposed race after registering as in-flight also observes the bit on its way out and would otherwise attempt a second disposal. _disposed becomes an int guarded by Interlocked.Exchange, read via Volatile.Read in ThrowIfDisposed: it was a plain bool read from one thread and written from another with no ordering edge, which is exactly the kind of gap this fix closes everywhere else in the file. ExecuteCommandCoreAsync crossed MA0051's 60-line cap with the added bookkeeping; the run itself is extracted into RunWithinGateAsync, which owns nothing but the pipeline and returns to the gate/lifecycle bookkeeping that now surrounds it. Refs #70, #74 --- .../Given_SessionScopedServices.cs | 42 ++++ src/Repl.Testing/ReplSessionHandle.cs | 198 ++++++++++-------- 2 files changed, 158 insertions(+), 82 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index c12490f2..c6730c81 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -247,6 +247,48 @@ public async Task When_ACommandIsStillRunning_Then_DisposingTheSessionDoesNotHan "disposal must not tear the session scope out from under the command that is still using it"); } + [TestMethod] + [Description("Guards the deferred half of disposal: DisposeAsync returning early while a command is still running must not be the ONLY disposal attempt. The command's own finally must finish the job once it completes, so a Scoped disposable it resolved is still released deterministically rather than left for the process to reclaim.")] + public async Task When_SessionIsDisposedWhileACommandRuns_Then_ScopedDisposablesAreStillReleased() + { + var disposed = new List(); + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var host = ReplTestHost.Create( + () => + { + var app = ReplApp.Create(services => + services.AddScoped(_ => new DisposableProbe(disposed))).UseDefaultInteractive(); + app.Map("block", async (DisposableProbe probe) => + { + started.TrySetResult(); + +#pragma warning disable VSTHRD003 + await release.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + return probe.Id.ToString(); + }); + return app; + }, + options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var running = session.RunCommandAsync("block --no-logo").AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + var dispose = session.DisposeAsync().AsTask(); + await Task.WhenAny(dispose, Task.Delay(TimeSpan.FromSeconds(5))); + disposed.Should().BeEmpty("disposal must not race the command that is still using the scope"); + + release.TrySetResult(); + var completed = await running.WaitAsync(TimeSpan.FromSeconds(30)); + await dispose.WaitAsync(TimeSpan.FromSeconds(30)); + + completed.ExitCode.Should().Be(0, "run output was: {0}", completed.OutputText); + disposed.Should().ContainSingle( + "the deferred command must finish the disposal DisposeAsync could not do while it was running"); + } + [TestMethod] [Description("Guards the framework own per-session service: IReplSessionState is documented as a per-session state container, so two sessions of one app must not read each other writes. Registered as a singleton it was one process-wide bag shared by every concurrent Telnet, WebSocket and MCP client.")] public void When_TwoRunsShareOneApp_Then_SessionStateIsNotShared() diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 3b5fbc22..2f4e1bbb 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -15,7 +15,20 @@ public sealed class ReplSessionHandle : IAsyncDisposable private readonly IReadOnlyDictionary? _sessionAnswers; private readonly SemaphoreSlim _commandGate = new(initialCount: 1, maxCount: 1); private readonly string _sessionId; - private bool _disposed; + private int _disposed; + + // Coordinates disposal with a command that is still running, without a check-then-act race: bit 0 + // latches "disposal was requested", the rest counts commands currently executing against _services + // (0 or 1 — _commandGate allows only one at a time, but a counter rather than a single bit keeps the + // arithmetic below correct however many false starts land here). Both DisposeAsync and a running + // command's own finally mutate this with one Interlocked op apiece; whichever one's read-modify-write + // observes the other side's contribution already applied is the one that defers, and the one that + // observes it absent is the one that disposes — so exactly one side ever does, never zero, never two + // without DisposeServicesOnceAsync's own guard catching the duplicate. + private int _serviceLifecycle; + private int _servicesDisposeClaimed; + private const int DisposalRequested = 1; + private const int CommandInFlight = 2; private ReplSessionHandle( ReplTestHost owner, @@ -86,55 +99,103 @@ private async ValueTask ExecuteCommandCoreAsync( await _commandGate.WaitAsync(cancellationToken).ConfigureAwait(false); try { + // Registered as in-flight BEFORE the re-check below, not after: this is what DisposeAsync's + // own Interlocked.Or observes to decide whether it or this command owns disposing _services, + // and that decision must already account for this command — otherwise a DisposeAsync racing + // in between the check and the registration could dispose _services while this command still + // believes it is clear to run. + Interlocked.Add(ref _serviceLifecycle, CommandInFlight); + // Re-checked inside the gate: a caller that passed the check above can be queued here while - // the session is disposed, and must not go on to run against a disposed scope. + // the session is disposed, and must not go on to run against a disposed scope. Guaranteed + // visible by now if a concurrent DisposeAsync's Or already observed the increment above, + // because Interlocked operations on _serviceLifecycle are full fences on both sides. ThrowIfDisposed(); - var startedAt = DateTimeOffset.UtcNow; - using var output = new StringWriter(); - var host = new TestSessionHost(_sessionId, output); - var observer = new SessionExecutionObserver(); - var args = BuildArgsWithAnswers(ReplTestText.Tokenize(commandText), _sessionAnswers, answers); - using var timeout = ReplTestTimeout.CreateSource(_options.CommandTimeout, cancellationToken); - var token = timeout?.Token ?? cancellationToken; - - _app.Core.ExecutionObserver = observer; - int exitCode; - try - { - exitCode = await _app.RunAsync(args, host, _services, _runOptions, token).ConfigureAwait(false); - } - catch (OperationCanceledException) when (ReplTestTimeout.Expired(timeout, cancellationToken)) - { - throw CreateTimeoutException(commandText); - } - finally + return await RunWithinGateAsync(commandText, answers, cancellationToken).ConfigureAwait(false); + } + finally + { + var after = Interlocked.Add(ref _serviceLifecycle, -CommandInFlight); + if ((after & DisposalRequested) != 0) { - _app.Core.ExecutionObserver = null; + // DisposeAsync ran while this command held the gate and deferred to it rather than + // disposing _services out from under a run in progress. Finish that disposal now, still + // inside the gate, so a caller queued behind this one hits the re-check above instead of + // a provider that is only half torn down. + await DisposeServicesOnceAsync().ConfigureAwait(false); } - ThrowIfCancelledByTimeout(observer, timeout, commandText, cancellationToken); - - var outputText = output.ToString(); - if (_options.NormalizeAnsi) - { - outputText = ReplTestText.NormalizeOutput(outputText); - } + _commandGate.Release(); + } + } - var timeline = BuildTimeline(outputText, observer.Events, observer.LastResult); - return new CommandExecution( - commandText, - exitCode, - outputText, - observer.LastResult, - observer.Events.ToArray(), - timeline, - startedAt, - DateTimeOffset.UtcNow); + /// Runs one command's pipeline once the gate and the in-flight registration are held. + private async ValueTask RunWithinGateAsync( + string commandText, + IReadOnlyDictionary? answers, + CancellationToken cancellationToken) + { + var startedAt = DateTimeOffset.UtcNow; + using var output = new StringWriter(); + var host = new TestSessionHost(_sessionId, output); + var observer = new SessionExecutionObserver(); + var args = BuildArgsWithAnswers(ReplTestText.Tokenize(commandText), _sessionAnswers, answers); + using var timeout = ReplTestTimeout.CreateSource(_options.CommandTimeout, cancellationToken); + var token = timeout?.Token ?? cancellationToken; + + _app.Core.ExecutionObserver = observer; + int exitCode; + try + { + exitCode = await _app.RunAsync(args, host, _services, _runOptions, token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (ReplTestTimeout.Expired(timeout, cancellationToken)) + { + throw CreateTimeoutException(commandText); } finally { - _commandGate.Release(); + _app.Core.ExecutionObserver = null; + } + + ThrowIfCancelledByTimeout(observer, timeout, commandText, cancellationToken); + + var outputText = output.ToString(); + if (_options.NormalizeAnsi) + { + outputText = ReplTestText.NormalizeOutput(outputText); + } + + var timeline = BuildTimeline(outputText, observer.Events, observer.LastResult); + return new CommandExecution( + commandText, + exitCode, + outputText, + observer.LastResult, + observer.Events.ToArray(), + timeline, + startedAt, + DateTimeOffset.UtcNow); + } + + /// Disposes exactly once, however many callers request it. + private async ValueTask DisposeServicesOnceAsync() + { + if (Interlocked.Exchange(ref _servicesDisposeClaimed, 1) != 0) + { + return; + } + + switch (_services) + { + case IAsyncDisposable asyncDisposable: + // Releases the session DI scope, disposing its Scoped services. + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; } } @@ -167,59 +228,32 @@ public SessionSnapshot GetSnapshot() /// scope — disposing every Scoped service it resolved. Disposing twice is a no-op. /// /// - /// Does not wait for a command that is still running: an orphaned run would make this hang. Such a - /// run keeps the scope instead of having it disposed underneath it, so the scope outlives this - /// handle in that case. + /// Never waits on a command that is still running — that would turn an await using into a + /// hang for the lifetime of an orphaned run. It also never simply abandons that command's scope: + /// disposal is handed to whichever side, this method or the running command's own finally, + /// is the one still active when the other checks. See . /// public async ValueTask DisposeAsync() { - if (_disposed) + if (Interlocked.Exchange(ref _disposed, 1) != 0) { return; } - _disposed = true; _owner.RemoveSession(SessionId); ReplSessionIO.RemoveSession(SessionId); - // The gate is taken, never waited on. Waiting would turn an await using into a hang whenever a - // command is still running — an orphaned timed-out run holds the gate until it finishes on its - // own. Commands queued behind it stop at the re-check inside the gate instead. - var held = await _commandGate.WaitAsync(TimeSpan.Zero).ConfigureAwait(false); - try - { - if (!held) - { - // A command is mid-run and resolving from this very provider. Disposing it here would - // pull the scope out from under that command, so its services start throwing - // ObjectDisposedException and its scoped disposables die while it still holds them. - // The orphan keeps the scope instead: it outlives the handle rather than faulting, and - // the process reclaims it. Leaking a test session's scope is the lesser harm. - return; - } - - switch (_services) - { - case IAsyncDisposable asyncDisposable: - // Releases the session DI scope, disposing its Scoped services. - await asyncDisposable.DisposeAsync().ConfigureAwait(false); - break; - case IDisposable disposable: - disposable.Dispose(); - break; - } - } - finally + var before = Interlocked.Or(ref _serviceLifecycle, DisposalRequested); + if ((before & CommandInFlight) == 0) { - if (held) - { - _commandGate.Release(); - } + // No command was registered as in-flight at the moment this bit was set: none can appear + // afterward, since ThrowIfDisposed now observes _disposed and refuses every later one before + // it reaches the increment that would make it visible here. This call owns the disposal. + await DisposeServicesOnceAsync().ConfigureAwait(false); } - - // The semaphore is deliberately not disposed: disposing it while a caller is queued on - // WaitAsync strands that caller forever, and it owns no unmanaged resource here because - // AvailableWaitHandle is never used. + // Otherwise a command holds the gate right now. Its own finally, decrementing _serviceLifecycle + // on the way out, is guaranteed to observe DisposalRequested — set here, and never cleared — and + // disposes on this call's behalf before releasing the gate. } internal static ValueTask StartAsync( @@ -319,7 +353,7 @@ private TimeoutException CreateTimeoutException(string commandText) => private void ThrowIfDisposed() { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); } private sealed class TestSessionHost(string sessionId, TextWriter output) : IReplSessionHost From 0113febbea7545227768dce0eb29ec3fd8126fae Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:10:09 -0400 Subject: [PATCH 14/24] fix(testing): dispose the app's root provider when the host that built it is disposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing, and the one item in this series Codex raised in July that was never answered: ReplSessionHandle.StartAsync forces ReplApp.Services (building its root ServiceProvider) to compose the session's scope, and nothing ever disposed that root — the handle only ever owned its own scope. autocarl revalidated it against this head with a disposable singleton: dispose the session, dispose the host, the singleton stays undisposed. ReplTestHost now invokes the app factory itself, instead of handing it to the handle, so it can see every distinct ReplApp a session was opened against (tracked by reference identity — a factory that deliberately returns the SAME app for several sessions, as this file's own CreateProbeHost helper does, must count as one entry, not one per session) and dispose each one's root provider in its own DisposeAsync, after every session. ReplApp itself stays without a disposal surface. It has 661 call sites across this repository, none in a using, and it registers itself as a singleton in its own container — giving it IAsyncDisposable would create a self-referential disposal cycle everywhere else it is used. The host is the one place that both builds an app's provider and knows every one it built, so it can own releasing them without touching anything outside Repl.Testing. Refs #70, #74 --- .../Given_SessionScopedServices.cs | 53 +++++++++++++++++++ src/Repl.Testing/ReplSessionHandle.cs | 8 +-- src/Repl.Testing/ReplTestHost.cs | 39 +++++++++++++- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index c6730c81..f117fad5 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -339,4 +339,57 @@ public void When_ASingletonInjectsSessionState_Then_ItCapturesTheFirstSessionIns "alpha", "a singleton holds the scope it was first resolved in, so it still sees that session"); } + + private sealed class SingletonProbe : IDisposable + { + public bool Disposed { get; private set; } + + public void Dispose() => Disposed = true; + } + + [TestMethod] + [Description("The host, not only the session, must release what a session's DI scope pulled its app's root provider into building: a singleton registered on the app is constructed once the first session's scope resolves it, and nothing but the host disposing that root provider ever releases it. Reproduces the leak by resolving a disposable singleton, disposing the session, then the host, and asserting it is disposed only at that second point — never left for the process to reclaim.")] + public async Task When_TheHostIsDisposed_Then_TheAppRootProviderIsDisposedToo() + { + var probe = new SingletonProbe(); + await using var host = ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => services.AddSingleton(_ => probe)).UseDefaultInteractive(); + app.Map("touch", (SingletonProbe p) => p.Disposed.ToString()); + return app; + }); + + var session = await host.OpenSessionAsync(); + var result = await session.RunCommandAsync("touch --no-logo"); + result.ExitCode.Should().Be(0, "run output was: {0}", result.OutputText); + + await session.DisposeAsync(); + probe.Disposed.Should().BeFalse( + "disposing the session releases its DI scope, not the app's root provider it descends from"); + + await host.DisposeAsync(); + probe.Disposed.Should().BeTrue( + "the host built the app's root provider to open this session and must release it at its own disposal"); + } + + [TestMethod] + [Description("Guards against a double-dispose crash on the shared-app shape this file's own CreateProbeHost helper relies on: several sessions opened against ONE ReplApp must have that app's root provider disposed exactly once when the host is disposed, not once per session.")] + public async Task When_SeveralSessionsShareOneApp_Then_TheHostDisposesItsRootProviderOnce() + { + var probe = new SingletonProbe(); + var app = ReplApp.Create(services => services.AddSingleton(_ => probe)).UseDefaultInteractive(); + app.Map("touch", (SingletonProbe p) => p.Disposed.ToString()); + await using var host = ReplTestHost.Create(() => app); + + await using var sessionA = await host.OpenSessionAsync(); + await using var sessionB = await host.OpenSessionAsync(); + (await sessionA.RunCommandAsync("touch --no-logo")).ExitCode.Should().Be(0); + (await sessionB.RunCommandAsync("touch --no-logo")).ExitCode.Should().Be(0); + + var dispose = async () => await host.DisposeAsync().ConfigureAwait(false); + + await dispose.Should().NotThrowAsync("one shared app must be disposed once, not once per session") + .ConfigureAwait(false); + probe.Disposed.Should().BeTrue(); + } } diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 2f4e1bbb..86c40f7e 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -256,22 +256,24 @@ public async ValueTask DisposeAsync() // disposes on this call's behalf before releasing the gate. } + // Takes the ReplApp instance rather than the factory that produces it: the host invokes the factory + // itself now, so it can track every distinct app a session was opened against and dispose each one's + // root provider at its own teardown — see ReplTestHost.DisposeAsync. internal static ValueTask StartAsync( ReplTestHost owner, - Func appFactory, + ReplApp app, SessionDescriptor descriptor, ReplScenarioOptions options, CancellationToken cancellationToken) { ArgumentNullException.ThrowIfNull(owner); - ArgumentNullException.ThrowIfNull(appFactory); + ArgumentNullException.ThrowIfNull(app); ArgumentNullException.ThrowIfNull(descriptor); ArgumentNullException.ThrowIfNull(options); cancellationToken.ThrowIfCancellationRequested(); var sessionId = $"session-{Guid.NewGuid():N}"; ReplSessionIO.EnsureSession(sessionId); - var app = appFactory(); // The handle owns the session DI scope (each command is a separate one-shot run), // so every run opts out of per-run scoping. var runOptions = descriptor.BuildRunOptions(options) with { SessionScope = SessionScopeBehavior.CallerOwned }; diff --git a/src/Repl.Testing/ReplTestHost.cs b/src/Repl.Testing/ReplTestHost.cs index 803d1453..55168346 100644 --- a/src/Repl.Testing/ReplTestHost.cs +++ b/src/Repl.Testing/ReplTestHost.cs @@ -1,4 +1,5 @@ using System.Collections.Concurrent; +using System.Runtime.CompilerServices; namespace Repl.Testing; @@ -11,6 +12,12 @@ public sealed class ReplTestHost : IAsyncDisposable private readonly ReplScenarioOptions _options; private readonly ConcurrentDictionary _sessions = new(StringComparer.Ordinal); + // The host, not the handle, invokes the factory — because that is what lets it see every DISTINCT + // ReplApp a session was opened against and dispose each one's root ServiceProvider exactly once at + // teardown. Keyed by reference identity, deliberately: a factory that closes over and returns the + // SAME app for every session (so several sessions can share one container) must count as one entry, + // not one per session, or the second session's own scope would be torn down with the first's. + private readonly ConcurrentDictionary _apps = new(ReferenceEqualityComparer.Instance); private bool _disposed; private ReplTestHost(Func appFactory, ReplScenarioOptions options) @@ -50,9 +57,11 @@ public async ValueTask OpenSessionAsync( { ThrowIfDisposed(); descriptor ??= new SessionDescriptor(); + var app = _appFactory(); + _apps.TryAdd(app, 0); var handle = await ReplSessionHandle.StartAsync( this, - _appFactory, + app, descriptor, _options, cancellationToken).ConfigureAwait(false); @@ -94,8 +103,19 @@ internal void RemoveSession(string sessionId) } /// - /// Disposes every session this host still owns. Disposing twice is a no-op. + /// Disposes every session this host still owns, then the root of + /// every distinct a session was opened against. Disposing twice is a no-op. /// + /// + /// The app's provider is disposed here, and only here: exposes no disposal + /// surface of its own — 661 call sites of + /// across this repository, none in a using, plus an app registers itself as a singleton in + /// its own container, so giving it one would create a self-referential disposal cycle everywhere + /// else it is used. This host is the one place that both builds an app's provider (through + /// ReplSessionHandle.StartAsync) and knows every one it built, so it is the one place that + /// can own releasing them, without changing anything for a caller outside Repl.Testing. + /// Apps come after sessions: a session's DI scope is a CHILD of its app's root provider. + /// public async ValueTask DisposeAsync() { if (_disposed) @@ -110,6 +130,21 @@ public async ValueTask DisposeAsync() } _sessions.Clear(); + + foreach (var app in _apps.Keys) + { + switch (app.Services) + { + case IAsyncDisposable asyncDisposable: + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; + } + } + + _apps.Clear(); } private void ThrowIfDisposed() From fefb668d9f0d6bd64092e500807510e8e3e95fd9 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:15:52 -0400 Subject: [PATCH 15/24] docs(mcp): correct two claims with a security consequence, and stop wasting a scope per request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two doc claims, both wrong in the direction that hides the risk. best-practices.md's lifetime table promised one Scoped instance per MCP client connection, unqualified, and named auth context as the use case. That holds for mcp serve; it does not hold for a server built from a reused BuildMcpServerOptions() result — the only public path to ASP.NET Core and other custom-transport hosts — where one handler-lifetime context serves every connection, so a Scoped auth-context or cart registered there leaks between clients. The row is now qualified, mirrored into the conformance page's deliberate-gaps table, and paired with the concurrency consequence that follows from the unit being a whole connection rather than one call: several MCP tool calls can run at once against that one instance. mcp-transports.md claimed that under stateless HTTP hosting "the connection scope IS the request scope, with nothing extra to configure." False: Repl's connection context is built by BuildDynamicServerOptions, which is internal and reached only from RunAsync — stdio. The public HTTP path is the same reusable-options path as above, which has no connection row at all, so there was nothing for the request scope to coincide with. BuildDynamicServerOptions now sets ScopeRequests = false. Repl never ran commands in the SDK's per-request scope — it always used its own connection-scoped context — so the SDK was opening and discarding one for nothing on every request. BuildStaticServerOptions, which has no connection to scope to, is left alone: a host reusing that result owns request-level scoping itself. Separately: a caller-supplied provider with no IServiceScopeFactory degraded to unscoped resolution silently — reproducing #70's own symptom, on the release that fixes it, with nothing to distinguish it from SessionScopeBehavior.CallerOwned's deliberate opt-out. A diagnostic now names the defect, following the same WriteDiagnostic precedent as the existing ignored-signal-handling warning, and fires once per provider rather than once per run. Refs #70, #74 --- docs/best-practices.md | 21 ++++++++- docs/mcp-conformance.md | 1 + docs/mcp-transports.md | 23 ++++++---- src/Repl.Defaults/ReplApp.cs | 29 +++++++++++- .../Given_SessionScopedServices.cs | 46 +++++++++++++++++++ src/Repl.Mcp/McpServerHandler.cs | 7 +++ 6 files changed, 114 insertions(+), 13 deletions(-) diff --git a/docs/best-practices.md b/docs/best-practices.md index ccea5958..e7647d23 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -112,12 +112,25 @@ Note: DI singleton factories are resolved lazily, so the values are available af | Lifetime | Resolves once per | Use it for | |---|---|---| | `Singleton` | application | caches, clients, anything shared by every session | -| `Scoped` | **session** — one `Run*` call: a CLI invocation, an interactive run, a Telnet or WebSocket connection; under MCP, one client connection | per-user state: auth context, a cart, a unit of work | +| `Scoped` | **session** — one `Run*` call: a CLI invocation, an interactive run, a Telnet or WebSocket connection; under MCP, one client connection\* | per-user state: auth context, a cart, a unit of work | | `Transient` | resolution | cheap stateless helpers | A `Scoped` disposable is disposed when its session ends, so session resources do not accumulate for the life of a long-running host. +\* **Except a server built from a reused `BuildMcpServerOptions()` result** — the shape ASP.NET Core and +other custom-transport hosts use. That path has no per-connection context, so it has no per-connection DI +scope either: one `Scoped` instance is shared by every client it serves, disposed only when the process +ends. A `Scoped` auth-context or cart registered there leaks between clients. See the [known +limitation](mcp-conformance.md#deliberate-gaps) and [Isolation boundaries](mcp-transports.md#what-is-isolated-and-at-which-boundary). + +Two concurrency notes that follow from the unit being the whole connection rather than one call: several +MCP tool calls can run at once on one connection, so a `Scoped` instance can be entered concurrently — +make it thread-safe, or serialize access to it yourself, the way a `DbContext` (a poor fit for concurrent +entry) would need. And on `mcp serve`, that `Scoped` instance is exactly what carries state from one tool +call to the next within a connection — the earlier calls in this section describing `Scoped` as +resolving "once per session" are what makes that possible. + The trap worth naming: a **singleton** that injects a `Scoped` service captures whichever scope first resolved it, and keeps it for the life of the application — silently, because the provider is built without `ValidateScopes`. Inject the scoped service into the handler instead, or register the holder @@ -126,7 +139,11 @@ without `ValidateScopes`. Inject the scoped service into the handler instead, or When your caller's provider already represents the session — a Blazor circuit, an ASP.NET request scope, or a session owner running several one-shot calls — set `ReplRunOptions.SessionScope = SessionScopeBehavior.CallerOwned` so the run resolves from it directly -instead of nesting a second scope inside it, which would hide the caller's own scoped instances. +instead of opening a second scope beside it. `IServiceScopeFactory` is itself a singleton, so that second +scope is not nested inside the caller's — it is a SIBLING rooted at the application root, and it does not +hide the caller's scoped instances so much as duplicate them: a second unit of work running alongside the +live one. `SessionScopeBehavior` governs the `Run*` family only; MCP's connection-scoped DI is unconditional +and has no equivalent opt-out. ## Group related options with `[ReplOptionsGroup]` diff --git a/docs/mcp-conformance.md b/docs/mcp-conformance.md index 08b9834c..6096ae3e 100644 --- a/docs/mcp-conformance.md +++ b/docs/mcp-conformance.md @@ -129,6 +129,7 @@ returned by a creation tool and passed back as an argument, rather than implicit | --- | --- | --- | | No per-caller command graph | The one variance `2026-07-28` permits is by the authorization presented on the request. Repl has no request-authorization concept yet, so it advertises one graph to everyone. | [#97](https://github.com/yllibed/repl/issues/97) | | `*/list_changed` is advertised on the reusable-options path but never fires there | The SDK forces the flag true for any non-null collection, and the pre-built catalog always supplies one. | [#94](https://github.com/yllibed/repl/issues/94) | +| A `Scoped` DI registration is shared by every client on the reusable-options path | That path has one handler-lifetime context serving all connections, so it has no per-connection scope to give `Scoped` — one instance lives for the process, same as the native roots cache and soft roots on the same path. A `Scoped` auth-context or cart registered there leaks between clients. See [best-practices.md](best-practices.md#pick-the-lifetime-that-matches-the-boundary). | — | | A multi-connection custom transport sees considerations this page does not solve | `mcp serve` is one connection per process; a host that multiplexes connections over one options instance owns the isolation questions that follow. See [Transports](mcp-transports.md). | — | ## Extensions and SEPs diff --git a/docs/mcp-transports.md b/docs/mcp-transports.md index 33008f0b..c88b60b1 100644 --- a/docs/mcp-transports.md +++ b/docs/mcp-transports.md @@ -112,14 +112,19 @@ presence predicate's services from the same scope, so a command that was adverti The MCP SDK opens a scope of its own per request (`McpServerOptions.ScopeRequests`, default `true`) and exposes it as `RequestContext.Services`. Repl does not run commands in it: that scope descends from the application root and is a sibling of the connection's, so adopting it would give the catalog and the -command two different instances of the same service. - -Under stateless HTTP hosting the two coincide — the SDK builds one short-lived server per request over -`HttpContext.RequestServices` — so the connection scope *is* the request scope there, with nothing extra -to configure. - -A server created from a reused `BuildMcpServerOptions()` result has everything above except the -connection row; see the known limitation above. That matters especially when using dynamic tools, -roots, or session-specific modules. +command two different instances of the same service. `BuildDynamicServerOptions` — the `mcp serve` path +that owns a real connection context — sets `ScopeRequests = false`, so the SDK does not open that +sibling scope there to begin with. + +A server created from a reused `BuildMcpServerOptions()` result — which is how an ASP.NET Core or other +custom-transport host reaches Repl — has everything above **except the connection row**: see the known +limitation above. There is no per-connection context to give a DI scope to either, so `Scoped` resolves +once from the application root and is shared by every client the process serves, for as long as it runs. +That is true under stateless HTTP hosting too, even though the SDK itself builds one short-lived server +per request there: Repl's own scope is a property of the *context* it built at construction, not of the +SDK's per-request server, so it does not follow the SDK's request boundary. A host that needs real +per-connection or per-request isolation on this path has to build it itself — for example by calling +`BuildMcpServerOptions()` fresh per connection rather than reusing one result — which is exactly the +"multiplexes connections over one options instance" case flagged as a caller responsibility below. For those higher-level patterns, see [mcp-advanced.md](mcp-advanced.md). diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index d4511f35..38df1b57 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -1,6 +1,7 @@ using System.Diagnostics.CodeAnalysis; using System.Linq.Expressions; using System.Reflection; +using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; @@ -24,6 +25,11 @@ public sealed class ReplApp : IReplApp // resolution at registration time happens outside any session. private ServiceProvider? _sharedProvider; private ProcessSignalHandlingMode _defaultProcessSignalHandling = ProcessSignalHandlingMode.None; + // Tracks which providers RunInSessionScopeAsync has already warned about, so a long-lived host + // issuing many one-shot runs against the same scope-less provider gets the diagnostic once rather + // than once per run. Keyed by identity and weakly held, the same shape CoreReplApp uses for its + // routing cache, so this is never what keeps a caller-supplied provider alive. + private static readonly ConditionalWeakTable DiagnosedUnscopedProviders = new(); // Extension packages (e.g. Repl.Spectre) park per-app configuration here so it stays // reachable even when the shared provider was materialized before the Use* call — @@ -712,12 +718,31 @@ private static async ValueTask RunInSessionScopeAsync( $"ReplRunOptions.{nameof(ReplRunOptions.SessionScope)} is not a defined {nameof(SessionScopeBehavior)}."); } - if (resolved == SessionScopeBehavior.CallerOwned - || services.GetService(typeof(IServiceScopeFactory)) is not IServiceScopeFactory scopeFactory) + if (resolved == SessionScopeBehavior.CallerOwned) { return await run(services, cancellationToken).ConfigureAwait(false); } + if (services.GetService(typeof(IServiceScopeFactory)) is not IServiceScopeFactory scopeFactory) + { + // Degrading silently here reproduces #70's own symptom on the release that fixes it: every + // session sharing one Scoped instance. CallerOwned above is a deliberate, silent opt-out by + // design — this is the OTHER case, a provider that was never given a scope to begin with, + // which one release note is not enough to make every caller check for. + if (DiagnosedUnscopedProviders.TryAdd(services, services)) + { + ProcessSignalCoordinator.WriteDiagnostic( + "Warning: the service provider passed to Run* has no IServiceScopeFactory, so " + + "Scoped services resolve unscoped — one instance shared by every session, " + + "the exact defect ReplRunOptions.SessionScope exists to prevent. Register " + + "Microsoft.Extensions.DependencyInjection's ServiceCollection.BuildServiceProvider() " + + "result (which always supplies one), or pass SessionScopeBehavior.CallerOwned " + + "if this provider deliberately IS the session scope."); + } + + return await run(services, cancellationToken).ConfigureAwait(false); + } + var scope = scopeFactory.CreateAsyncScope(); await using (scope.ConfigureAwait(false)) { diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index f117fad5..594086ca 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -392,4 +392,50 @@ await dispose.Should().NotThrowAsync("one shared app must be disposed once, not .ConfigureAwait(false); probe.Disposed.Should().BeTrue(); } + + // Deliberately not built via ServiceCollection.BuildServiceProvider(): the point of this provider is + // that it supplies no IServiceScopeFactory, which is exactly the case RunInSessionScopeAsync degrades + // for — a caller-supplied provider that was never given a scope, as opposed to CallerOwned, where the + // caller opted out on purpose. + private sealed class ScopeLessServiceProvider(ReplApp app) : IServiceProvider + { + public object? GetService(Type serviceType) => + serviceType == typeof(IServiceScopeFactory) ? null : app.Services.GetService(serviceType); + } + + [TestMethod] + [Description("Issue #70's own symptom must not reappear silently: a caller-supplied provider with no IServiceScopeFactory makes every session share one Scoped instance, exactly what SessionScope exists to prevent. Unlike SessionScopeBehavior.CallerOwned, which is a deliberate and silent opt-out, this is a provider nobody chose to leave unscoped, so it must say so.")] + public async Task When_ProviderHasNoScopeFactory_Then_ARunTimeDiagnosticNamesTheDefect() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var services = new ScopeLessServiceProvider(sut); + using var diagnostics = new StringWriter(); + using var session = ReplSessionIO.SetSession(TextWriter.Null, TextReader.Null, error: diagnostics); + + var exitCode = await sut.RunAsync(["scoped", "--no-logo"], services).ConfigureAwait(false); + + exitCode.Should().Be(0); + diagnostics.ToString().Should().Contain( + "IServiceScopeFactory", + "a provider left unscoped by accident must say so, not silently reproduce #70"); + } + + [TestMethod] + [Description("The diagnostic for a scope-less provider must not repeat on every run: a long-lived host issuing many one-shot runs against the same provider would otherwise get one warning per run forever.")] + public async Task When_TheSameScopeLessProviderRunsTwice_Then_TheDiagnosticFiresOnlyOnce() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + var services = new ScopeLessServiceProvider(sut); + using var diagnostics = new StringWriter(); + using var session = ReplSessionIO.SetSession(TextWriter.Null, TextReader.Null, error: diagnostics); + + await sut.RunAsync(["scoped", "--no-logo"], services).ConfigureAwait(false); + await sut.RunAsync(["scoped", "--no-logo"], services).ConfigureAwait(false); + + var text = diagnostics.ToString(); + var occurrences = text.Split("IServiceScopeFactory", StringSplitOptions.None).Length - 1; + occurrences.Should().Be(1, "the same provider must not be warned about twice"); + } } diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index a91c0e55..c879c565 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -209,6 +209,13 @@ internal McpServerOptions BuildDynamicServerOptions() ToolCollection = _toolListChanged, ResourceCollection = _resourceListChanged, PromptCollection = _promptListChanged, + // This path owns a real per-connection context (CreateSessionContext(McpRootsScope.Connection) + // in RunAsync) with its own DI scope; commands run inside it, never inside the SDK's own + // per-request scope. Leaving ScopeRequests at its true default would still have the SDK open + // and discard one of those on every request for nothing. BuildStaticServerOptions, which has + // no connection to scope to, does not set this — a host reusing that result owns request + // scoping itself, and this handler must not silently decide that for it. + ScopeRequests = false, }; } From 2719a836cce68a9776daac7117cc9994e49e1283 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:18:21 -0400 Subject: [PATCH 16/24] test: pin SessionScope's untested default and rejection, and tighten a loose assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps the panel found and confirmed unguarded: the nullable default and the undefined-value rejection landed in the earlier commits without a test, while Given_RunOptions.cs pins every other ReplRunOptions default and Given_ProcessSignalExitCodePolicy.cs pins the identical rejection for the sibling enum. Mirrors both here, plus the same style guard for the enum's own zero value. Also corrects the SessionScope doc comment: it claimed a composition profile could already supply its own default, the way ProcessSignalHandling's profiles do — no such hook exists, RunInSessionScopeAsync coalesces null straight to PerRun. Restated as what the nullability is FOR (so that could change later without a binary break) rather than what it does today. And tightens one assertion: the host-side scoped-disposable guard used Contain where issue #70's criterion 3 says disposed exactly once; its MCP twin already used ContainSingle. A double-dispose regression would have passed the looser form. Refs #70, #74 --- src/Repl.Defaults/ReplRunOptions.cs | 12 +++++++----- .../Given_SessionScopedServices.cs | 17 ++++++++++++++++- src/Repl.Tests/Given_RunOptions.cs | 16 ++++++++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/Repl.Defaults/ReplRunOptions.cs b/src/Repl.Defaults/ReplRunOptions.cs index 0f7b435f..dd09aa82 100644 --- a/src/Repl.Defaults/ReplRunOptions.cs +++ b/src/Repl.Defaults/ReplRunOptions.cs @@ -18,13 +18,15 @@ public sealed record ReplRunOptions /// /// Gets how this run manages the session's dependency-injection scope. - /// uses the active application profile's default, which is - /// . + /// resolves to . /// /// - /// Nullable for the same reason as : a composition profile has to - /// be able to supply a default, and that is only expressible if "the caller said nothing" is - /// distinguishable from an explicit value. + /// Nullable for the same reason as , but no profile currently sets + /// a different default the way 's profiles do — every run resolves + /// the same way today. The nullability exists so that could change without a + /// binary break: a composition profile can only supply its own default if "the caller said nothing" + /// is distinguishable from an explicit value, which a non-nullable property could never express. + /// Governs the Run* family only; MCP's DI scope is unconditional and has no equivalent setting. /// public SessionScopeBehavior? SessionScope { get; init; } diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 594086ca..2cedbe3a 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -66,6 +66,21 @@ public void When_TwoRunsShareOneApp_Then_ScopedInstancesDiffer() first.Text.Trim().Should().NotBe(second.Text.Trim()); } + [TestMethod] + [Description("Regression guard: verifies an undefined SessionScopeBehavior is rejected instead of falling through a negative test into PerRun, where it would silently nest a scope inside a caller who may have asked to own it — hiding the caller's own scoped instances rather than sharing them, the same class of risk ProcessSignalHandling's own undefined-value rejection guards.")] + public async Task When_TheSessionScopeIsUndefined_Then_TheRunIsRejected() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + + Func act = () => sut.RunAsync( + ["scoped", "--no-logo"], + new ReplRunOptions { SessionScope = (SessionScopeBehavior)42 }) + .AsTask(); + + await act.Should().ThrowAsync().ConfigureAwait(false); + } + [TestMethod] [Description("Guards the CallerOwned opt-out: a caller whose provider already represents the session scope (Blazor circuit, per-request scope, multi-run session owners) must keep its own Scoped instances across runs — the entry points must not open a nested sibling scope that would hide them.")] public async Task When_CallerOwnsTheSessionScope_Then_RunsShareTheCallerScopedInstances() @@ -142,7 +157,7 @@ public async Task When_SessionEnds_Then_ScopedDisposablesAreDisposed() disposed.Should().NotContain(Guid.Parse(id)); } - disposed.Should().Contain(Guid.Parse(id)); + disposed.Should().ContainSingle(guid => guid == Guid.Parse(id)); } private sealed class ScopeObservingHostedService(IServiceProvider services, List observed) : IHostedService diff --git a/src/Repl.Tests/Given_RunOptions.cs b/src/Repl.Tests/Given_RunOptions.cs index 5a603489..ff0583f4 100644 --- a/src/Repl.Tests/Given_RunOptions.cs +++ b/src/Repl.Tests/Given_RunOptions.cs @@ -45,4 +45,20 @@ public void When_CreatingRunOptions_Then_TerminalOverridesDefaultToNull() options.TerminalOverrides.Should().BeNull(); } + + [TestMethod] + [Description("Regression guard: verifies a new run-options record leaves the session DI scope behavior unspecified. Nullable for the same reason as ProcessSignalHandling — a composition profile could not supply its own default otherwise, since 'the caller said nothing' would be indistinguishable from an explicit PerRun.")] + public void When_CreatingRunOptions_Then_SessionScopeIsNull() + { + var options = new ReplRunOptions(); + + options.SessionScope.Should().BeNull(); + } + + [TestMethod] + [Description("Regression guard: verifies the zero value of the scope-behavior enum is PerRun. Enum zero is what an unset configuration field, a zero-initialized struct, or an explicit default() yields, so it must agree with the value a null SessionScope resolves to rather than silently opting a run out of scoping.")] + public void When_UsingDefaultSessionScopeBehavior_Then_ValueIsPerRun() + { + default(SessionScopeBehavior).Should().Be(SessionScopeBehavior.PerRun); + } } From 30074a773d35e62dedc518d39cb2d7c84092e625 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:24:13 -0400 Subject: [PATCH 17/24] style: fix dropped possessives, a forgiveness operator, and bundle a risky parameter list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six comments and test descriptions across two files dropped a possessive apostrophe ("the framework own", "read each other writes", "an earlier session state") — an artefact of writing them through shell heredocs, where an apostrophe needs escaping and sometimes didn't get it. Restored throughout. Given_SessionScopedServices.cs:90 cast a GetService(typeof(...)) result and forgave its nullability where the same PR already writes GetRequiredService() one project over; now consistent. McpToolAdapter.ExecuteThroughPipelineAsync's split (to stay under MA0051's 60-line cap) left RunSubInvocationAsync with seven positional parameters, three of which — two adjacent StringWriters and a bare bool — are exactly the shape a transposition compiles silently through. Bundled into CommandOutputCapture, a small record struct beside the existing McpPipelineInvocation: the call site is now one value instead of three, constructed once by CommandOutputCapture.Create so the "share the writer when capturing combined" branch lives in one place instead of being read out at two call sites. A missing blank line ahead of RunInSessionScopeAsync's doc comment is restored. RecordingSessionState.Constructed, asserted only as BeGreaterThan(0), was proposed to tighten to Be(1) — measuring it first showed why not: the catalog context's warm-up and the connection's own scope are two independent scopes by design, and each legitimately constructs its own instance, so the count is 2 on a correctly-working system. Deleted instead, along with the reset and the static counter: the existing Contain(nameof(RecordingSessionState)) assertion already proves the consumer's own implementation is what answered, which is the entire contract this test guards. ArgumentOutOfRangeException naming RunInSessionScopeAsync's own parameter rather than the public ReplRunOptions.SessionScope it validates is left as is: naming the property directly trips MA0015 (a nameof of something that is not a parameter of the enclosing method), and fixing that properly means hoisting the check into a helper taking ReplRunOptions, which ripples into three call sites for a ParamName mismatch nothing currently depends on. Refs #70, #74 --- src/Repl.Defaults/ReplApp.cs | 11 ++++-- .../Given_SessionScopedServices.cs | 8 ++--- src/Repl.Mcp/McpToolAdapter.cs | 34 ++++++++++++------- src/Repl.McpTests/Given_McpSessionScopes.cs | 10 ++---- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 38df1b57..4f415a95 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -681,7 +681,7 @@ public async ValueTask RunAsync( } // The session overlay composes on top of the session scope, so overlay lookups that fall - // through to the caller provider observe this session Scoped instances. + // through to the caller provider observe this session's Scoped instances. return await RunInSessionScopeAsync( services, runOptions.SessionScope, @@ -690,6 +690,7 @@ public async ValueTask RunAsync( cancellationToken).ConfigureAwait(false); } } + /// /// Runs one session inside its own DI scope, so Scoped services resolve per session and scoped /// disposables are released when the session ends. @@ -712,6 +713,12 @@ private static async ValueTask RunInSessionScopeAsync( var resolved = sessionScope ?? SessionScopeBehavior.PerRun; if (resolved is not (SessionScopeBehavior.PerRun or SessionScopeBehavior.CallerOwned)) { + // ParamName names this private helper's own parameter, not ReplRunOptions.SessionScope: MA0015 + // rejects nameof-ing a symbol that is not actually a parameter of the enclosing method, and + // resolving that properly means hoisting this check into a helper that takes ReplRunOptions + // directly — which ripples into RunHostedLifecycleOutcomeAsync's signature and all three call + // sites for a ParamName mismatch nothing currently depends on. The message text already + // names the public property correctly. throw new ArgumentOutOfRangeException( nameof(sessionScope), resolved, @@ -1076,7 +1083,7 @@ private static void EnsureDefaultServices(IServiceCollection services, CoreReplA services.AddReplLogging(); services.TryAddSingleton(core); services.TryAddSingleton(core); - // Scoped, not singleton: Run* opens one scope per session, so this is the framework own + // Scoped, not singleton: Run* opens one scope per session, so this is the framework's own // per-session service and a second session no longer reads what the first one stored. With a // provider that cannot scope, it resolves from the root exactly as it did before. services.TryAddScoped(); diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 2cedbe3a..517ad30b 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -87,7 +87,7 @@ public async Task When_CallerOwnsTheSessionScope_Then_RunsShareTheCallerScopedIn { var sut = ReplApp.Create(services => services.AddScoped()); sut.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); - var scopeFactory = (IServiceScopeFactory)sut.Services.GetService(typeof(IServiceScopeFactory))!; + var scopeFactory = sut.Services.GetRequiredService(); var callerScope = scopeFactory.CreateAsyncScope(); await using (callerScope.ConfigureAwait(false)) { @@ -172,7 +172,7 @@ public Task StartAsync(CancellationToken cancellationToken) } [TestMethod] - [Description("Guards the scope boundary on the hosted-service path: the run pipeline is wrapped in the session scope but HostedServiceLifecycleCoordinator.StartAsync is not, so a hosted service resolves from the unscoped provider and must NOT see the command Scoped instance. Placing the scope around the whole hosted branch instead would tie app-level services to a session lifetime.")] + [Description("Guards the scope boundary on the hosted-service path: the run pipeline is wrapped in the session scope but HostedServiceLifecycleCoordinator.StartAsync is not, so a hosted service resolves from the unscoped provider and must NOT see the command's Scoped instance. Placing the scope around the whole hosted branch instead would tie app-level services to a session lifetime.")] public void When_HostedServicesRun_Then_TheyStartOutsideTheSessionScope() { var observed = new List(); @@ -305,7 +305,7 @@ public async Task When_SessionIsDisposedWhileACommandRuns_Then_ScopedDisposables } [TestMethod] - [Description("Guards the framework own per-session service: IReplSessionState is documented as a per-session state container, so two sessions of one app must not read each other writes. Registered as a singleton it was one process-wide bag shared by every concurrent Telnet, WebSocket and MCP client.")] + [Description("Guards the framework's own per-session service: IReplSessionState is documented as a per-session state container, so two sessions of one app must not read each other's writes. Registered as a singleton it was one process-wide bag shared by every concurrent Telnet, WebSocket and MCP client.")] public void When_TwoRunsShareOneApp_Then_SessionStateIsNotShared() { var sut = ReplApp.Create(); @@ -321,7 +321,7 @@ public void When_TwoRunsShareOneApp_Then_SessionStateIsNotShared() write.ExitCode.Should().Be(0, "run output was: {0}", write.Text); read.ExitCode.Should().Be(0, "run output was: {0}", read.Text); - read.Text.Trim().Should().Be("(none)", "a later session must not read an earlier session state"); + read.Text.Trim().Should().Be("(none)", "a later session must not read an earlier session's state"); } private sealed class SessionStateCapturingSingleton(IReplSessionState state) diff --git a/src/Repl.Mcp/McpToolAdapter.cs b/src/Repl.Mcp/McpToolAdapter.cs index cb4a9712..f19cb538 100644 --- a/src/Repl.Mcp/McpToolAdapter.cs +++ b/src/Repl.Mcp/McpToolAdapter.cs @@ -212,8 +212,7 @@ private async Task ExecuteThroughPipelineAsync( await McpClientRootsService.PrimeFromServicesAsync(_services, ct).ConfigureAwait(false); - var outputWriter = new StringWriter(); - var errorWriter = captureCommandOutput ? outputWriter : new StringWriter(); + var capture = CommandOutputCapture.Create(captureCommandOutput); var feedback = _services.GetService(typeof(IMcpFeedback)) as IMcpFeedback; var interactionChannel = new McpInteractionChannel( prefills, _options.InteractivityMode, server, progressToken, feedback); @@ -232,12 +231,11 @@ private async Task ExecuteThroughPipelineAsync( var effectiveTokens = new List(tokens.Count + 1) { $"--output:{ForcedOutputFormat}" }; effectiveTokens.AddRange(tokens); - var completed = await RunSubInvocationAsync( - invocableApp, effectiveTokens, mcpServices, outputWriter, errorWriter, captureCommandOutput, ct) + var completed = await RunSubInvocationAsync(invocableApp, effectiveTokens, mcpServices, capture, ct) .ConfigureAwait(false); - var output = outputWriter.ToString().Trim(); - var error = captureCommandOutput ? string.Empty : errorWriter.ToString().Trim(); + var output = capture.Output.ToString().Trim(); + var error = capture.CaptureCommandOutput ? string.Empty : capture.Error.ToString().Trim(); var undelivered = undeliveredScope?.Messages.Drain() ?? []; return new McpPipelineInvocation( output, error, completed.ExitCode, completed.Kind, completed.Failure, undelivered); @@ -248,22 +246,20 @@ private async Task RunSubInvocationAsync( ISubInvocableReplApp invocableApp, List effectiveTokens, IServiceProvider mcpServices, - StringWriter outputWriter, - StringWriter errorWriter, - bool captureCommandOutput, + CommandOutputCapture capture, CancellationToken ct) { var inputReader = new StringReader(string.Empty); // Command-backed resources expose the rendered return value as the resource body. // Low-level handler writes to IReplIoContext.Output/Error are side-channel output, not resource content. - var commandOutput = captureCommandOutput ? outputWriter : TextWriter.Null; + var commandOutput = capture.CaptureCommandOutput ? capture.Output : TextWriter.Null; using (ReplSessionIO.SetSession( - output: outputWriter, + output: capture.Output, input: inputReader, ansiMode: Rendering.AnsiMode.Never, sessionId: $"mcp-{Guid.NewGuid():N}", commandOutput: commandOutput, - error: errorWriter, + error: capture.Error, isHostedSession: true, // Minted for this call alone; inference would read it as a lifetime we own and never release. removeSessionOnDispose: true)) @@ -294,6 +290,20 @@ private readonly record struct McpPipelineInvocation( Exception? Failure, IReadOnlyList UndeliveredMessages); + /// + /// The two writers one pipeline run captures, and whether they are the same one. Bundled instead of + /// three positional parameters — two of them adjacent s — because a + /// transposed pair compiles silently and a bare bool flag is unreadable at a call site. + /// + private readonly record struct CommandOutputCapture(StringWriter Output, StringWriter Error, bool CaptureCommandOutput) + { + public static CommandOutputCapture Create(bool captureCommandOutput) + { + var output = new StringWriter(); + return new CommandOutputCapture(output, captureCommandOutput ? output : new StringWriter(), captureCommandOutput); + } + } + /// /// What a failed run is allowed to tell the client. /// diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 92138b49..7a942c9c 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -50,10 +50,6 @@ private sealed class GatedModule : IReplModule private sealed class RecordingSessionState : IReplSessionState { - public static int Constructed; - - public RecordingSessionState() => Interlocked.Increment(ref Constructed); - private readonly Dictionary _values = new(StringComparer.OrdinalIgnoreCase); public bool TryGet(string key, out T? value) @@ -257,7 +253,6 @@ await CallAsync( [Description("A consumer who registers their own IReplSessionState must get it on MCP too. Seeding the session's override map with a framework instance would win unconditionally over the container, so the one transport where per-connection state matters most would silently run a different implementation from the one the app composed.")] public async Task When_AConsumerRegistersItsOwnSessionState_Then_McpResolvesIt() { - RecordingSessionState.Constructed = 0; var app = ReplApp.Create(services => services.AddScoped()); app.UseMcpServer(); app.Map("kind", (IReplSessionState state) => state.GetType().Name); @@ -269,8 +264,9 @@ public async Task When_AConsumerRegistersItsOwnSessionState_Then_McpResolvesIt() var reported = await CallAsync(session.Client, "kind", token: cts.Token).ConfigureAwait(false); - reported.Should().Contain(nameof(RecordingSessionState)); - RecordingSessionState.Constructed.Should().BeGreaterThan(0); + reported.Should().Contain( + nameof(RecordingSessionState), + "a framework instance seeded in the override map would win unconditionally over the container"); } [TestMethod] From df2395dd74e72c2da4522b996ce565b089ec4297 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:40:07 -0400 Subject: [PATCH 18/24] fix(testing): make the shared-provider race, cancellation ordering, and host-vs-command hazard right MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings Codex raised on the previous push, all against code this same branch just landed — reviewed and confirmed against current code before fixing, not reacted to. ReplApp.EnsureSharedProvider's `??=` is a check-then-assign, not an atomic publish. Two concurrent first callers — a caller-owned host opening several sessions against one shared app, the exact shape this branch's own CreateProbeHost/shared-app tests use — could each build a provider and each assign; the loser is silently orphaned along with any disposable singleton it had already started constructing, and ReplTestHost's new disposal loop would only ever find the winner. Interlocked.CompareExchange now publishes exactly one; the loser, having resolved nothing, is disposed unused. RED verified: 8/8 runs failed before the fix, 8/8 pass after. ReplTestHost.OpenSessionAsync moved the app-factory call ahead of StartAsync's own cancellation check when the host, not the handle, took over invoking the factory. An already-canceled token now ran the factory's side effects anyway and tracked the resulting app for disposal nothing would ever trigger, since no session existed to release it. The check now runs before the factory call, where it always did. Disposing a host while one of its sessions has deferred DI-scope release to a command still in flight (see the previous commit) must not dispose that session's app root either: the command's scope is a child of that root, and doing so is the same hazard already refused at the session level, one layer up. ReplSessionHandle now exposes when its services have actually finished disposing — completed by whichever side's DisposeServicesOnceAsync call wins, immediately if nothing was deferred — and ReplTestHost reads it, per app, to skip a root whose session hasn't released yet. Reads, never awaits: awaiting would reintroduce, at the host level, the exact unbounded wait on a possibly-orphaned command DisposeAsync itself was written to avoid. Refs #70, #74 --- src/Repl.Defaults/ReplApp.cs | 29 ++++++++++++- .../Given_SessionScopedServices.cs | 41 ++++++++++++++++++ .../Given_TestingToolkit.cs | 18 ++++++++ src/Repl.Testing/ReplSessionHandle.cs | 42 +++++++++++++++---- src/Repl.Testing/ReplTestHost.cs | 20 ++++++++- src/Repl.Tests/Given_ReplApp.cs | 12 ++++++ 6 files changed, 151 insertions(+), 11 deletions(-) diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 4f415a95..9d0e7ed7 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -869,8 +869,33 @@ IReplMap IReplMap.MapModule(IReplModule module, Func - private ServiceProvider EnsureSharedProvider() => - _sharedProvider ??= _services.BuildServiceProvider(); + private ServiceProvider EnsureSharedProvider() + { + if (_sharedProvider is { } existing) + { + return existing; + } + + // `??=` is a check-then-assign, not an atomic publish — two concurrent first callers (a + // caller-owned host opening several sessions against one shared app, a supported shape) could + // each build a provider and each assign, silently orphaning whichever one lost the race along + // with any disposable singleton it had already started constructing. CompareExchange publishes + // exactly one; the loser, having resolved nothing yet, is disposed unused rather than kept. + var candidate = _services.BuildServiceProvider(); + var winner = Interlocked.CompareExchange(ref _sharedProvider, candidate, comparand: null); + if (winner is not null) + { + // EnsureSharedProvider is itself synchronous, reached from a plain property getter with no + // async context to hand this off to; nothing was ever resolved from the losing candidate, so + // disposing it releases only the empty engine it built, never anything with real cleanup work. +#pragma warning disable MA0045 + candidate.Dispose(); +#pragma warning restore MA0045 + return winner; + } + + return candidate; + } private TModule ResolveModuleFromServices<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TModule>() where TModule : class, IReplModule diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 517ad30b..ade3d61c 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -453,4 +453,45 @@ public async Task When_TheSameScopeLessProviderRunsTwice_Then_TheDiagnosticFires var occurrences = text.Split("IServiceScopeFactory", StringSplitOptions.None).Length - 1; occurrences.Should().Be(1, "the same provider must not be warned about twice"); } + + [TestMethod] + [Description("Disposing the host while one of its sessions has deferred scope disposal to a still-running command must not dispose that session's app root out from under it: the command's own scope is a child of that root, so a singleton the command resolves after the host disposal call returns must still resolve without throwing ObjectDisposedException. Reproduces the shape autocarl's earlier finding pointed at one level up — the same hazard, at the app-root boundary instead of the session-scope boundary.")] + public async Task When_TheHostIsDisposedWhileACommandRuns_Then_ItsAppRootSurvivesUntilTheCommandFinishes() + { + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var host = ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => services.AddSingleton()) + .UseDefaultInteractive(); + app.Map("block", async (SingletonProbe before) => + { + started.TrySetResult(); + +#pragma warning disable VSTHRD003 + await release.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + + // Resolved AFTER release, once the host's own DisposeAsync call below has already + // returned: this is what proves the app root was not disposed out from under this + // command. A disposed root would make this throw ObjectDisposedException instead. + return before.Disposed.ToString(); + }); + return app; + }, options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var running = session.RunCommandAsync("block --no-logo").AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + await host.DisposeAsync(); + + release.TrySetResult(); + var completed = await running.WaitAsync(TimeSpan.FromSeconds(30)); + + completed.ExitCode.Should().Be( + 0, + "the singleton must still resolve after host disposal returns while this command was in flight: {0}", + completed.OutputText); + } } diff --git a/src/Repl.IntegrationTests/Given_TestingToolkit.cs b/src/Repl.IntegrationTests/Given_TestingToolkit.cs index 6f10c570..e67407f7 100644 --- a/src/Repl.IntegrationTests/Given_TestingToolkit.cs +++ b/src/Repl.IntegrationTests/Given_TestingToolkit.cs @@ -322,4 +322,22 @@ private static ReplApp CreateEchoApp() return app; } + [TestMethod] + [Description("Regression guard: an already-canceled token must stop OpenSessionAsync before it invokes the app factory — not after. Running the factory anyway executes arbitrary caller side effects for a session that will never exist, and tracks the returned app for disposal that nothing will ever trigger, since no session was created to release it.")] + public async Task When_TheTokenIsAlreadyCanceled_Then_TheFactoryIsNeverInvoked() + { + var invocations = 0; + await using var host = ReplTestHost.Create(() => + { + invocations++; + return CreateEchoApp(); + }); + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + var open = async () => await host.OpenSessionAsync(cancellationToken: cts.Token).ConfigureAwait(false); + + await open.Should().ThrowAsync().ConfigureAwait(false); + invocations.Should().Be(0, "a canceled open must not run the factory's side effects at all"); + } } diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 86c40f7e..d34dfaf4 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -29,6 +29,12 @@ public sealed class ReplSessionHandle : IAsyncDisposable private int _servicesDisposeClaimed; private const int DisposalRequested = 1; private const int CommandInFlight = 2; + // Completed once _services has actually been disposed — whether DisposeAsync did it directly, or a + // command still running at the time did it later in its own finally. The owning ReplTestHost reads + // this (never awaits it) to decide whether this session's app root is safe to dispose yet: awaiting + // it would reintroduce, at the host level, the exact unbounded wait DisposeAsync itself refuses to + // take on an orphaned command. + private readonly TaskCompletionSource _servicesDisposed = new(TaskCreationOptions.RunContinuationsAsynchronously); private ReplSessionHandle( ReplTestHost owner, @@ -54,6 +60,16 @@ private ReplSessionHandle( /// public string SessionId => _sessionId; + /// The app this session was opened against — uses this to + /// correlate a session with the app root it may still be using at disposal time. + internal ReplApp App => _app; + + /// + /// Completes once this session's DI scope has actually been released — including when + /// deferred that to a command that was still running when it was called. + /// + internal Task ServicesDisposed => _servicesDisposed.Task; + /// /// Runs a command in this session. /// @@ -187,15 +203,25 @@ private async ValueTask DisposeServicesOnceAsync() return; } - switch (_services) + try + { + switch (_services) + { + case IAsyncDisposable asyncDisposable: + // Releases the session DI scope, disposing its Scoped services. + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; + } + } + finally { - case IAsyncDisposable asyncDisposable: - // Releases the session DI scope, disposing its Scoped services. - await asyncDisposable.DisposeAsync().ConfigureAwait(false); - break; - case IDisposable disposable: - disposable.Dispose(); - break; + // Signaled from the one caller whose CAS above actually won, whichever of the two that + // turns out to be — DisposeAsync itself, or the command that was still running when it was + // called. ReplTestHost reads this to know its root provider is now safe to dispose. + _servicesDisposed.TrySetResult(); } } diff --git a/src/Repl.Testing/ReplTestHost.cs b/src/Repl.Testing/ReplTestHost.cs index 55168346..3115081a 100644 --- a/src/Repl.Testing/ReplTestHost.cs +++ b/src/Repl.Testing/ReplTestHost.cs @@ -56,6 +56,10 @@ public async ValueTask OpenSessionAsync( CancellationToken cancellationToken = default) { ThrowIfDisposed(); + // Before the factory call, not after: the factory can carry arbitrary caller side effects, and + // a canceled open must not run them for a session that will never exist — nor track an app for + // disposal that nothing would ever trigger, since no session would be created to release it. + cancellationToken.ThrowIfCancellationRequested(); descriptor ??= new SessionDescriptor(); var app = _appFactory(); _apps.TryAdd(app, 0); @@ -124,7 +128,11 @@ public async ValueTask DisposeAsync() } _disposed = true; - foreach (var session in _sessions.Values) + // Captured before disposing: ReplSessionHandle.DisposeAsync can defer releasing its DI scope to + // a command that was still running when it was called (see ReplSessionHandle), and this list is + // what lets the app-root loop below tell which apps that affects, after _sessions is cleared. + var sessions = _sessions.Values.ToArray(); + foreach (var session in sessions) { await session.DisposeAsync().ConfigureAwait(false); } @@ -133,6 +141,16 @@ public async ValueTask DisposeAsync() foreach (var app in _apps.Keys) { + // An app whose session deferred its own scope release to a command still in flight must not + // have its root disposed here either: that command's scope is a child of this root, and + // disposing the root out from under it is the same hazard ReplSessionHandle.DisposeAsync + // itself refuses to create. Never WAIT for that command the way it never does — leaving the + // root undisposed alongside the deferred scope is the lesser harm; the process reclaims both. + if (Array.Exists(sessions, s => ReferenceEquals(s.App, app) && !s.ServicesDisposed.IsCompleted)) + { + continue; + } + switch (app.Services) { case IAsyncDisposable asyncDisposable: diff --git a/src/Repl.Tests/Given_ReplApp.cs b/src/Repl.Tests/Given_ReplApp.cs index 48a6f81e..e64514bf 100644 --- a/src/Repl.Tests/Given_ReplApp.cs +++ b/src/Repl.Tests/Given_ReplApp.cs @@ -1,5 +1,6 @@ using System.Reflection; using AwesomeAssertions; +using Microsoft.Extensions.DependencyInjection; namespace Repl.Tests; @@ -467,6 +468,17 @@ public void When_InspectingRoutingInvalidatedEvent_Then_LegacyAccessorSignatureI addAccessor.Should().NotBeNull(); } + [TestMethod] + [Description("Regression guard: ReplApp.Services must publish exactly one root provider even when several callers race to build it — a caller-owned host that opens more than one session against one shared app (a supported shape: Repl.Testing's own CreateProbeHost pattern) resolves Services from more than one thread. The lazy '??=' this used to read from is a check-then-assign, not an atomic publish, so a race could build two providers and silently orphan whichever one lost — along with any disposable singleton the loser had already started constructing.")] + public async Task When_ServicesIsRacedFromManyThreads_Then_EveryCallerObservesTheSameProvider() + { + var sut = ReplApp.Create(services => services.AddSingleton()); + + var providers = await Task.WhenAll(Enumerable.Range(0, 32).Select(_ => Task.Run(() => sut.Services))); + + providers.Should().OnlyContain(provider => ReferenceEquals(provider, providers[0])); + } + private sealed class StubTransformer : IOutputTransformer { public string Name => "stub"; From 432bf4ffe7c0bc846907bda78c45d75ab531b8e1 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 09:54:52 -0400 Subject: [PATCH 19/24] fix(di): validate the scope mode before hosted services start; correct two doc claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third Codex pass, all three checked against current code before acting. RunHostedLifecycleOutcomeAsync started hosted services — real startup and shutdown side effects — before reaching RunInSessionScopeAsync, the only place that validated SessionScope. An undefined value on the hosted path therefore ran those side effects before the option error surfaced, unlike the non-hosted path, and a shutdown failure could even replace it. The validate-and-resolve logic is now ValidateSessionScope, a standalone helper RunHostedLifecycleOutcomeAsync calls first, before anything starts; RunInSessionScopeAsync calls the same helper where it always validated. RED verified: hosted services ran (tracker.StartCount == 1) before the fix, never run after. Two doc corrections, both to prose this same branch wrote and both checked against measured behavior before publishing, not just re-argued from source: best-practices.md's lifetime table had no carve-out for a module's own constructor. MapModule() resolves T once, at mapping time, before any session exists — the same timing as a singleton — so a Scoped constructor dependency is captured exactly as a singleton's would be. Documented alongside the existing singleton-capture trap, with the same shape of pinning test, and noted on MapModule()'s own doc comment. mcp-transports.md recommended calling BuildMcpServerOptions() fresh per connection for isolation on the reused-options path. Measured, not just read: two fresh calls against the same ReplApp still share Scoped, because the ReplApp overload always passes that app's one cached Services root regardless of how many times it runs. Replaced with what actually works, also measured: the ICoreReplApp overload takes an explicit IServiceProvider, so a host supplying a genuinely different scope per connection gets real isolation — at the cost of owning that scope's lifetime itself, which Repl does not manage on this path. Refs #70, #74 --- docs/best-practices.md | 8 +++ docs/mcp-transports.md | 17 ++++-- src/Repl.Defaults/ReplApp.cs | 44 ++++++++++++---- .../Given_HostedServicesLifecycle.cs | 28 ++++++++++ .../Given_SessionScopedServices.cs | 25 +++++++++ src/Repl.McpTests/Given_McpSessionScopes.cs | 52 +++++++++++++++++++ 6 files changed, 159 insertions(+), 15 deletions(-) diff --git a/docs/best-practices.md b/docs/best-practices.md index e7647d23..bec3efb0 100644 --- a/docs/best-practices.md +++ b/docs/best-practices.md @@ -136,6 +136,14 @@ resolved it, and keeps it for the life of the application — silently, because without `ValidateScopes`. Inject the scoped service into the handler instead, or register the holder `Scoped` too. +The same trap catches a **module's own constructor**. `MapModule()` resolves `TModule` once, +at mapping time — before any session exists — the same way a singleton would. A module constructor +that takes a `Scoped` auth-context or cart captures it exactly as the singleton case above does, and +every session's handlers then share that one instance. The [module example above](#structure-commands-with-modules) +avoids this because its handlers take `IContactStore` as a **handler parameter**, resolved fresh per +invocation — not as a constructor dependency. Keep any `Scoped` service out of a module's constructor; +inject it into the handler that needs it instead. + When your caller's provider already represents the session — a Blazor circuit, an ASP.NET request scope, or a session owner running several one-shot calls — set `ReplRunOptions.SessionScope = SessionScopeBehavior.CallerOwned` so the run resolves from it directly diff --git a/docs/mcp-transports.md b/docs/mcp-transports.md index c88b60b1..0be72565 100644 --- a/docs/mcp-transports.md +++ b/docs/mcp-transports.md @@ -122,9 +122,18 @@ limitation above. There is no per-connection context to give a DI scope to eithe once from the application root and is shared by every client the process serves, for as long as it runs. That is true under stateless HTTP hosting too, even though the SDK itself builds one short-lived server per request there: Repl's own scope is a property of the *context* it built at construction, not of the -SDK's per-request server, so it does not follow the SDK's request boundary. A host that needs real -per-connection or per-request isolation on this path has to build it itself — for example by calling -`BuildMcpServerOptions()` fresh per connection rather than reusing one result — which is exactly the -"multiplexes connections over one options instance" case flagged as a caller responsibility below. +SDK's per-request server, so it does not follow the SDK's request boundary. + +**Calling `BuildMcpServerOptions()` again for each connection does not isolate `Scoped` either.** The +`ReplApp` overload always passes that app's one `Services` root (`app.Services`, cached for the app's +whole lifetime), so two calls against the same `ReplApp` still share the same root — and therefore the +same `Scoped` instances — no matter how often the method runs. Genuine per-connection isolation on this +path requires the host to supply a **different** `IServiceProvider` per connection, through the +`ICoreReplApp.BuildMcpServerOptions(configure, services)` overload: build a scope yourself (for example +`app.Services.GetRequiredService().CreateAsyncScope()`), pass its +`ServiceProvider` in for that connection, and dispose the scope yourself when the connection ends — Repl +does not manage that scope's lifetime on this path, the same way it does not manage the connection +itself. This is the "multiplexes connections over one options instance" case flagged as a caller +responsibility below, made concrete. For those higher-level patterns, see [mcp-advanced.md](mcp-advanced.md). diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 9d0e7ed7..142d5dd2 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -183,6 +183,12 @@ public IContextBuilder Context(string segment, Action configure, Deleg /// /// Maps a module resolved through runtime DI activation. /// + /// + /// is constructed once, here, before any session exists — the same + /// timing as a singleton. A constructor dependency registered Scoped is captured at that one + /// resolution and shared by every session afterward. Keep Scoped services out of a module's + /// constructor; inject them into the handler that needs them instead, where they resolve per session. + /// public ReplApp MapModule<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TModule>() where TModule : class, IReplModule { @@ -433,6 +439,13 @@ private async ValueTask RunHostedLifecycleOutcomeAsync( SessionScopeBehavior? sessionScope, CancellationToken cancellationToken) { + // Validated before hosted services start: on this path RunInSessionScopeAsync's own check is + // reached only after HostedServiceLifecycleCoordinator.StartAsync below, so an undefined value + // would otherwise run real startup (and rollback/shutdown) side effects before the option error + // surfaces, unlike the non-hosted path. The resolved value is unused here — opening the scope + // still happens once the pipeline actually runs below. + ValidateSessionScope(sessionScope); + IReadOnlyList started = []; ExecutionOutcome? outcome = null; Exception? propagating = null; @@ -700,11 +713,15 @@ public async ValueTask RunAsync( /// the session scope — a Blazor circuit, an ASP.NET request scope, or a session owner spanning /// several one-shot runs. A provider without scope support runs unscoped, as before. /// - private static async ValueTask RunInSessionScopeAsync( - IServiceProvider services, - SessionScopeBehavior? sessionScope, - Func> run, - CancellationToken cancellationToken) + // Split from RunInSessionScopeAsync so the hosted-lifecycle path can validate before starting + // anything: HostedServiceLifecycleCoordinator.StartAsync runs before this helper's own scope-creation + // body would ever be reached, so a caller reaching RunHostedLifecycleOutcomeAsync must resolve and + // validate SessionScope first, standalone, to fail exactly as fast as the non-hosted path does. + // + // ParamName below names this parameter, not ReplRunOptions.SessionScope: MA0015 rejects nameof-ing a + // symbol that is not actually a parameter of the enclosing method, and both call sites would need + // their own equally-wrong local name. The message text already names the public property correctly. + private static SessionScopeBehavior ValidateSessionScope(SessionScopeBehavior? sessionScope) { // Rejected rather than treated as PerRun, for the reason the process-signal mode above states: // numeric configuration or deserialization can produce an undefined value, and falling through a @@ -713,18 +730,23 @@ private static async ValueTask RunInSessionScopeAsync( var resolved = sessionScope ?? SessionScopeBehavior.PerRun; if (resolved is not (SessionScopeBehavior.PerRun or SessionScopeBehavior.CallerOwned)) { - // ParamName names this private helper's own parameter, not ReplRunOptions.SessionScope: MA0015 - // rejects nameof-ing a symbol that is not actually a parameter of the enclosing method, and - // resolving that properly means hoisting this check into a helper that takes ReplRunOptions - // directly — which ripples into RunHostedLifecycleOutcomeAsync's signature and all three call - // sites for a ParamName mismatch nothing currently depends on. The message text already - // names the public property correctly. throw new ArgumentOutOfRangeException( nameof(sessionScope), resolved, $"ReplRunOptions.{nameof(ReplRunOptions.SessionScope)} is not a defined {nameof(SessionScopeBehavior)}."); } + return resolved; + } + + private static async ValueTask RunInSessionScopeAsync( + IServiceProvider services, + SessionScopeBehavior? sessionScope, + Func> run, + CancellationToken cancellationToken) + { + var resolved = ValidateSessionScope(sessionScope); + if (resolved == SessionScopeBehavior.CallerOwned) { return await run(services, cancellationToken).ConfigureAwait(false); diff --git a/src/Repl.IntegrationTests/Given_HostedServicesLifecycle.cs b/src/Repl.IntegrationTests/Given_HostedServicesLifecycle.cs index 45af8daf..96f5d38c 100644 --- a/src/Repl.IntegrationTests/Given_HostedServicesLifecycle.cs +++ b/src/Repl.IntegrationTests/Given_HostedServicesLifecycle.cs @@ -486,6 +486,34 @@ public async Task When_PipelineCancelsAndShutdownAlsoFails_Then_TheShutdownFailu exitCode.ExitCode.Should().Be(70); } + [TestMethod] + [Description("Regression guard: an undefined SessionScope must be rejected before hosted services start, on the hosted-lifecycle path exactly as it already is on the non-hosted one. Validating it only inside RunInSessionScopeAsync — reached after HostedServiceLifecycleCoordinator.StartAsync has already run — lets a malformed configuration value execute real startup (and then shutdown) side effects before the option error is even raised.")] + public async Task When_TheSessionScopeIsUndefinedOnTheHostedPath_Then_HostedServicesNeverStart() + { + var tracker = new LifecycleTracker(); + var services = new ServiceCollection() + .AddSingleton(tracker) + .AddSingleton(); + using var provider = services.BuildServiceProvider(); + + var sut = ReplApp.Create(); + sut.Map("work", () => "unreachable"); + + var act = () => sut.RunAsync( + ["work", "--no-logo"], + provider, + new ReplRunOptions + { + HostedServiceLifecycle = HostedServiceLifecycleMode.Head, + SessionScope = (SessionScopeBehavior)42, + }).AsTask(); + + await act.Should().ThrowAsync().ConfigureAwait(false); + tracker.StartCount.Should().Be( + 0, + "the option error must surface before hosted services run any startup side effect"); + } + // Cancels the caller's token from inside StartAsync, then observes it: the shape that reaches // ReplApp as a HostedServiceLifecycleException wrapping an OperationCanceledException. private sealed class CallerCancellingHostedService(CancellationTokenSource callerTokenSource) : IHostedService diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index ade3d61c..f3b7687f 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -355,6 +355,31 @@ public void When_ASingletonInjectsSessionState_Then_ItCapturesTheFirstSessionIns "a singleton holds the scope it was first resolved in, so it still sees that session"); } + private sealed class ScopedProbeCapturingModule(ScopedProbe probe) : IReplModule + { + public void Map(IReplMap map) + { + map.Map("id", () => probe.Id.ToString()); + } + } + + [TestMethod] + [Description("Documents the module twin of the singleton-capture trap: MapModule() resolves T once, at mapping time, before any session exists — the same timing as a singleton. A constructor dependency registered Scoped is captured at that one resolution and shared by every session afterward, contradicting the per-session promise a reader would expect from the lifetime table. Keep Scoped dependencies out of a module's constructor; inject them into the handler instead.")] + public void When_AModuleConstructorInjectsAScopedService_Then_EverySessionSeesTheSameCapturedInstance() + { + var sut = ReplApp.Create(services => services.AddScoped()); + sut.MapModule(); + + var first = ConsoleCaptureHelper.Capture(() => sut.Run(["id", "--no-logo"])); + var second = ConsoleCaptureHelper.Capture(() => sut.Run(["id", "--no-logo"])); + + first.ExitCode.Should().Be(0, "run output was: {0}", first.Text); + second.ExitCode.Should().Be(0, "run output was: {0}", second.Text); + first.Text.Trim().Should().Be( + second.Text.Trim(), + "MapModule() resolves T once at mapping time, so a Scoped constructor dependency is captured just like a singleton's would be"); + } + private sealed class SingletonProbe : IDisposable { public bool Disposed { get; private set; } diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 7a942c9c..7c44bb0e 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -292,6 +292,58 @@ public async Task When_ConnectionsShareAReusableOptionsResult_Then_TheyShareOneS "a reused options result has one context for every connection, so it has one scope too"); } + [TestMethod] + [Description("Guards against advice this branch's own docs once gave and had to retract: calling app.BuildMcpServerOptions() again — a genuinely fresh McpServerOptions and McpServerHandler each time, not the one-instance-shared-across-connections shape the carve-out test above pins — still shares Scoped, because the ReplApp overload always passes that app's one cached Services root. Each fresh options build gets its own McpServerHandler and its own catalog context, but that context resolves Scoped directly from the SAME root either way.")] + public async Task When_BuildMcpServerOptionsIsCalledFreshPerConnection_Then_ScopedIsStillShared() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + var first = await StartSharedAsync(app.BuildMcpServerOptions(), cts.Token).ConfigureAwait(false); + await using var firstScope = first.ConfigureAwait(false); + var second = await StartSharedAsync(app.BuildMcpServerOptions(), cts.Token).ConfigureAwait(false); + await using var secondScope = second.ConfigureAwait(false); + + var fromFirst = await CallAsync(first.Client, "scoped", token: cts.Token).ConfigureAwait(false); + var fromSecond = await CallAsync(second.Client, "scoped", token: cts.Token).ConfigureAwait(false); + + fromFirst.Should().NotBeNullOrWhiteSpace(); + fromFirst.Should().Be( + fromSecond, + "two fresh BuildMcpServerOptions() calls against one app still share its one Services root"); + } + + [TestMethod] + [Description("Pins the documented workaround for the carve-out above: calling BuildMcpServerOptions(configure) again on the SAME ReplApp does NOT isolate Scoped (it always passes that app's one Services root), but the ICoreReplApp overload accepts an explicit IServiceProvider — so a host that builds and passes a genuinely different scope per connection gets real isolation on the reused-options path, at the cost of owning that scope's lifetime itself.")] + public async Task When_DifferentScopesAreSuppliedPerOptionsBuild_Then_EachGetsItsOwnScopedInstance() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + var scopeA = app.Services.GetRequiredService().CreateAsyncScope(); + await using var scopeAOwner = scopeA.ConfigureAwait(false); + var scopeB = app.Services.GetRequiredService().CreateAsyncScope(); + await using var scopeBOwner = scopeB.ConfigureAwait(false); + + var optionsA = app.Core.BuildMcpServerOptions(configure: null, scopeA.ServiceProvider); + var optionsB = app.Core.BuildMcpServerOptions(configure: null, scopeB.ServiceProvider); + + var first = await StartSharedAsync(optionsA, cts.Token).ConfigureAwait(false); + await using var firstScope = first.ConfigureAwait(false); + var second = await StartSharedAsync(optionsB, cts.Token).ConfigureAwait(false); + await using var secondScope = second.ConfigureAwait(false); + + var fromFirst = await CallAsync(first.Client, "scoped", token: cts.Token).ConfigureAwait(false); + var fromSecond = await CallAsync(second.Client, "scoped", token: cts.Token).ConfigureAwait(false); + + fromFirst.Should().NotBeNullOrWhiteSpace(); + fromFirst.Should().NotBe( + fromSecond, + "a distinct IServiceProvider supplied per options build must give each its own Scoped instance"); + } + private static Task StartSharedAsync( McpServerOptions mcpOptions, CancellationToken cancellationToken) => From 10d247c1fdee6f39c7f064912627f16450e7cc6a Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 10:23:33 -0400 Subject: [PATCH 20/24] fix(testing): track pending app disposal independent of session removal, and schedule it instead of abandoning it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit autocarl reproduced two distinct gaps in the previous fix, both real: 1. Skipping an app whose session had a command still in flight, then unconditionally clearing _apps right after, meant nothing ever came back once that command finished and its own deferred scope release ran. The root was abandoned forever, not merely until it was safe. 2. ReplSessionHandle.DisposeAsync removes a session from its host immediately, even when it defers releasing the session's own scope to a command still running. A caller that disposes the SESSION directly — not through the host — makes that session invisible to the host's own bookkeeping before its scope is actually released. A host disposed afterward had no way to know that app's root was still unsafe to touch, and disposed it under the running command. Both trace to the same root cause: "is this app's root safe to dispose" was answered by inspecting _sessions' CURRENT membership, which a session leaves as soon as its own DisposeAsync is called — regardless of whether its deferred scope release has actually run yet. _pendingServiceDisposals now records every session's ServicesDisposed task against its app at OpenSessionAsync time, independent of _sessions and of whatever later happens to the handle. At host disposal, an app with any still-incomplete task is not skipped and forgotten — its disposal is scheduled to run once every one of those tasks completes, via Task.WhenAll, without this call waiting for them. That keeps the property that made the previous fix safe (never block on a possibly-orphaned command) while closing the gap it left (never permanently abandon what only needed to wait). ReplSessionHandle.App is removed — the new tracking no longer needs to correlate a handle back to its app after the fact. RED verified: both of autocarl's exact repro shapes reproduced as regression tests first — a polling wait that never completed for gap 1, an ObjectDisposedException for gap 2 — before the fix, both pass after. Refs #70, #74 --- .../Given_SessionScopedServices.cs | 58 +++++++++++++++++ src/Repl.Testing/ReplSessionHandle.cs | 4 -- src/Repl.Testing/ReplTestHost.cs | 64 +++++++++++++------ 3 files changed, 102 insertions(+), 24 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index f3b7687f..684d6d57 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -483,6 +483,7 @@ public async Task When_TheSameScopeLessProviderRunsTwice_Then_TheDiagnosticFires [Description("Disposing the host while one of its sessions has deferred scope disposal to a still-running command must not dispose that session's app root out from under it: the command's own scope is a child of that root, so a singleton the command resolves after the host disposal call returns must still resolve without throwing ObjectDisposedException. Reproduces the shape autocarl's earlier finding pointed at one level up — the same hazard, at the app-root boundary instead of the session-scope boundary.")] public async Task When_TheHostIsDisposedWhileACommandRuns_Then_ItsAppRootSurvivesUntilTheCommandFinishes() { + SingletonProbe? probe = null; var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var host = ReplTestHost.Create(() => @@ -491,6 +492,7 @@ public async Task When_TheHostIsDisposedWhileACommandRuns_Then_ItsAppRootSurvive .UseDefaultInteractive(); app.Map("block", async (SingletonProbe before) => { + probe = before; started.TrySetResult(); #pragma warning disable VSTHRD003 @@ -518,5 +520,61 @@ public async Task When_TheHostIsDisposedWhileACommandRuns_Then_ItsAppRootSurvive 0, "the singleton must still resolve after host disposal returns while this command was in flight: {0}", completed.OutputText); + + // The command has now finished and its own deferred scope release has run — the app root must + // not be abandoned forever just because it was unsafe to dispose at the moment host.DisposeAsync + // was called. Polled rather than asserted immediately: disposal is scheduled to run once the + // session's own deferred release completes, not synchronously with it. + var probeDisposed = () => probe!.Disposed; + await Task.Run(async () => + { + while (!probeDisposed()) + { + await Task.Delay(10).ConfigureAwait(false); + } + }).WaitAsync(TimeSpan.FromSeconds(10)); + } + + [TestMethod] + [Description("The other ownership gap: a caller can dispose the SESSION handle directly (not through the host) while its command is still running. ReplSessionHandle.DisposeAsync removes the session from its host immediately, before the deferred scope release happens — so a host disposed afterward must still know that session's app root is not yet safe to dispose, even though the session no longer appears in the host's own tracking.")] + public async Task When_ASessionIsDisposedDirectlyWhileItsCommandRuns_Then_TheHostStillWaitsForItsAppRoot() + { + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var host = ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => services.AddSingleton()) + .UseDefaultInteractive(); + app.Map("block", async (SingletonProbe before) => + { + started.TrySetResult(); + +#pragma warning disable VSTHRD003 + await release.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + + return before.Disposed.ToString(); + }); + return app; + }, options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var running = session.RunCommandAsync("block --no-logo").AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + // Disposes the SESSION directly, not the host — this is what removes it from the host's + // _sessions before the command has released its scope. + await session.DisposeAsync(); + + // Now the host, which no longer has this session in _sessions at all. + await host.DisposeAsync(); + + release.TrySetResult(); + var completed = await running.WaitAsync(TimeSpan.FromSeconds(30)); + + completed.ExitCode.Should().Be( + 0, + "the app root must still be alive when the command resolves the singleton after release: {0}", + completed.OutputText); } } diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index d34dfaf4..8e418fc1 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -60,10 +60,6 @@ private ReplSessionHandle( /// public string SessionId => _sessionId; - /// The app this session was opened against — uses this to - /// correlate a session with the app root it may still be using at disposal time. - internal ReplApp App => _app; - /// /// Completes once this session's DI scope has actually been released — including when /// deferred that to a command that was still running when it was called. diff --git a/src/Repl.Testing/ReplTestHost.cs b/src/Repl.Testing/ReplTestHost.cs index 3115081a..917ab7ef 100644 --- a/src/Repl.Testing/ReplTestHost.cs +++ b/src/Repl.Testing/ReplTestHost.cs @@ -18,6 +18,13 @@ public sealed class ReplTestHost : IAsyncDisposable // SAME app for every session (so several sessions can share one container) must count as one entry, // not one per session, or the second session's own scope would be torn down with the first's. private readonly ConcurrentDictionary _apps = new(ReferenceEqualityComparer.Instance); + // Every session ever opened against an app contributes its ServicesDisposed task here, at open time + // — independent of _sessions, which a session leaves as soon as ITS OWN DisposeAsync is called, even + // while its command is still running and its scope release is deferred. Without this, a caller that + // disposes a session handle directly (not through the host) before its command finishes makes that + // session invisible to the app-root check below despite its scope still being live. + private readonly ConcurrentDictionary> _pendingServiceDisposals = + new(ReferenceEqualityComparer.Instance); private bool _disposed; private ReplTestHost(Func appFactory, ReplScenarioOptions options) @@ -69,6 +76,9 @@ public async ValueTask OpenSessionAsync( descriptor, _options, cancellationToken).ConfigureAwait(false); + // Recorded regardless of what happens to the handle afterward — including a caller disposing it + // directly, which would otherwise remove it from _sessions before its deferred scope release runs. + _pendingServiceDisposals.GetOrAdd(app, static _ => new ConcurrentQueue()).Enqueue(handle.ServicesDisposed); if (!_sessions.TryAdd(handle.SessionId, handle)) { await handle.DisposeAsync().ConfigureAwait(false); @@ -128,11 +138,7 @@ public async ValueTask DisposeAsync() } _disposed = true; - // Captured before disposing: ReplSessionHandle.DisposeAsync can defer releasing its DI scope to - // a command that was still running when it was called (see ReplSessionHandle), and this list is - // what lets the app-root loop below tell which apps that affects, after _sessions is cleared. - var sessions = _sessions.Values.ToArray(); - foreach (var session in sessions) + foreach (var session in _sessions.Values) { await session.DisposeAsync().ConfigureAwait(false); } @@ -141,28 +147,46 @@ public async ValueTask DisposeAsync() foreach (var app in _apps.Keys) { - // An app whose session deferred its own scope release to a command still in flight must not - // have its root disposed here either: that command's scope is a child of this root, and - // disposing the root out from under it is the same hazard ReplSessionHandle.DisposeAsync - // itself refuses to create. Never WAIT for that command the way it never does — leaving the - // root undisposed alongside the deferred scope is the lesser harm; the process reclaims both. - if (Array.Exists(sessions, s => ReferenceEquals(s.App, app) && !s.ServicesDisposed.IsCompleted)) + var pending = _pendingServiceDisposals.TryGetValue(app, out var queue) + ? Array.FindAll([.. queue], static t => !t.IsCompleted) + : []; + if (pending.Length > 0) { + // Scheduled, not abandoned: disposing this root now would pull it out from under + // whichever session's command is still running and holding a scope that descends from + // it — the same hazard ReplSessionHandle.DisposeAsync itself refuses to create. Waiting + // HERE would reintroduce, at the host level, the unbounded wait on a possibly-orphaned + // command that design already rejected — so the disposal runs once every one of this + // app's still-in-flight sessions actually finishes, whenever that turns out to be, + // without this call blocking on it. + _ = DisposeAppOnceSessionsFinishAsync(app, pending); continue; } - switch (app.Services) - { - case IAsyncDisposable asyncDisposable: - await asyncDisposable.DisposeAsync().ConfigureAwait(false); - break; - case IDisposable disposable: - disposable.Dispose(); - break; - } + await DisposeAppAsync(app).ConfigureAwait(false); } _apps.Clear(); + _pendingServiceDisposals.Clear(); + } + + private static async Task DisposeAppOnceSessionsFinishAsync(ReplApp app, Task[] pending) + { + await Task.WhenAll(pending).ConfigureAwait(false); + await DisposeAppAsync(app).ConfigureAwait(false); + } + + private static async ValueTask DisposeAppAsync(ReplApp app) + { + switch (app.Services) + { + case IAsyncDisposable asyncDisposable: + await asyncDisposable.DisposeAsync().ConfigureAwait(false); + break; + case IDisposable disposable: + disposable.Dispose(); + break; + } } private void ThrowIfDisposed() From d5f100016ce85e62a19b26af89680b6986a65776 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 10:39:12 -0400 Subject: [PATCH 21/24] fix(testing): narrow the open-vs-dispose race, avoid wasted construction, and surface deferred cleanup failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth Codex pass on the file this whole redesign lives in, all three checked against current code and, where a claim was testable at all, measured before acting. Track the app only after StartAsync succeeds, not right after the factory call. Every throw point in StartAsync — the null checks, its own cancellation re-check, a bad descriptor — runs before it forces app.Services open, so a failure there means nothing was ever built. Tracking (and, on the disposal-race branch, disposing) it anyway would force the provider open for nothing, solely to immediately tear it back down — the exact waste Codex named. The claim that this is genuinely unreachable is stated in the comment on the fix and re-verified by reading StartAsync again before writing it; it is not independently provable by a black-box test, since building and disposing a container nothing was ever resolved from has no observable side effect — measured directly by falsifying a first attempt at such a test, which passed unchanged against the reverted code. The kept test instead pins what IS observable: cancellation propagates correctly and the host stays fully usable afterward. Re-check ThrowIfDisposed after StartAsync returns, not just once at entry. The factory and StartAsync can both take arbitrarily long, so a host disposed while either was running could previously let the app and session they produced escape untracked past that disposal — orphaned forever, the same way the app-root cleanup two commits back was. This narrows the race rather than closing it outright: a concurrent DisposeAsync could still finish its own _apps/_sessions iteration in the small gap between this recheck and the registrations right after it, now a few in-memory operations instead of an arbitrarily long factory call. Closing that fully would need a coarser lock around the whole open sequence, disproportionate for a test harness at this severity. Deferred app-root disposals are now retained in _deferredAppDisposals instead of discarded as a bare `_ = ...` — DisposeAsync has already returned success by the time one of them runs, so a singleton's Dispose() throwing during it previously had no observer at all. WaitForDeferredCleanupAsync awaits them explicitly for a caller that needs to know cleanup actually finished, or to surface a failure that would otherwise never surface. RED verified: the failure was swallowed before retaining the task, observed after. Refs #70, #74 --- .../Given_SessionScopedServices.cs | 85 +++++++++++++++++++ src/Repl.Testing/ReplTestHost.cs | 44 +++++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index 684d6d57..b5be1034 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -577,4 +577,89 @@ public async Task When_ASessionIsDisposedDirectlyWhileItsCommandRuns_Then_TheHos "the app root must still be alive when the command resolves the singleton after release: {0}", completed.OutputText); } + + private sealed class ThrowingSingleton : IDisposable + { + public void Dispose() => throw new InvalidOperationException("cleanup deliberately failed"); + } + + [TestMethod] + [Description("A failure during a deferred app-root disposal must be observable, not merely retained: DisposeAsync has already returned success by the time a deferred cleanup runs, so nothing but WaitForDeferredCleanupAsync gives a caller any way to learn a singleton's Dispose() threw during it.")] + public async Task When_ADeferredAppDisposalFails_Then_WaitForDeferredCleanupAsyncSurfacesIt() + { + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var host = ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => services.AddSingleton()) + .UseDefaultInteractive(); + app.Map("block", async (ThrowingSingleton _) => + { + started.TrySetResult(); +#pragma warning disable VSTHRD003 + await release.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + return "done"; + }); + return app; + }, options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var running = session.RunCommandAsync("block --no-logo").AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + var disposeHost = async () => await host.DisposeAsync().ConfigureAwait(false); + await disposeHost.Should().NotThrowAsync( + "the failure has not happened yet — cleanup is deferred until the command finishes") + .ConfigureAwait(false); + + release.TrySetResult(); + await running.WaitAsync(TimeSpan.FromSeconds(30)).ConfigureAwait(false); + + var waitForCleanup = async () => await host.WaitForDeferredCleanupAsync().ConfigureAwait(false); + await waitForCleanup.Should().ThrowAsync() + .WithMessage("cleanup deliberately failed") + .ConfigureAwait(false); + } + + [TestMethod] + [Description("A canceled open, after a slow app factory already ran, must still surface OperationCanceledException and leave the host in a state where every later operation — DisposeAsync, WaitForDeferredCleanupAsync — completes cleanly, whether or not the orphaned app it produced was tracked. Tracking it (the pre-fix behavior) is not independently observable through black-box assertions here — building and disposing a container with nothing ever resolved from it has no side effect this test can see — so the actual 'not tracked' claim rests on reading StartAsync: every one of its throw points runs before it forces app.Services open, so a failure here means nothing was ever built for this ordering to avoid touching. That reasoning is what the code comment on the fix states; this test pins the behavior a caller can actually observe.")] + public async Task When_TheOpenIsCanceledWhileTheFactoryRuns_Then_TheHostStaysUsableAfterward() + { + var factoryEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseFactory = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var host = ReplTestHost.Create(() => + { + factoryEntered.TrySetResult(); +#pragma warning disable VSTHRD002 + releaseFactory.Task.GetAwaiter().GetResult(); +#pragma warning restore VSTHRD002 + return ReplApp.Create().UseDefaultInteractive(); + }); + + using var cts = new CancellationTokenSource(); + // VSTHRD003: the task is started right here, on the line above the analyzer flags — the factory + // blocks the pool thread Task.Run gives it, so this test drives release/cancellation from its + // own thread while that call is in flight. +#pragma warning disable VSTHRD003 + var opening = Task.Run(async () => await host.OpenSessionAsync(cancellationToken: cts.Token).ConfigureAwait(false)); +#pragma warning restore VSTHRD003 + await factoryEntered.Task.WaitAsync(TimeSpan.FromSeconds(30)); + await cts.CancelAsync(); + releaseFactory.TrySetResult(); + + // VSTHRD003: same task, already started above; this lambda only exists to hand it to the + // assertion helper. +#pragma warning disable VSTHRD003 + var act = async () => await opening.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + await act.Should().ThrowAsync().ConfigureAwait(false); + + // Whatever the outcome for the orphaned app, the host itself must remain fully usable: disposal + // and the deferred-cleanup wait both have to complete cleanly afterward. + var dispose = async () => await host.DisposeAsync().ConfigureAwait(false); + await dispose.Should().NotThrowAsync().ConfigureAwait(false); + var waitForCleanup = async () => await host.WaitForDeferredCleanupAsync().ConfigureAwait(false); + await waitForCleanup.Should().NotThrowAsync().ConfigureAwait(false); + } } diff --git a/src/Repl.Testing/ReplTestHost.cs b/src/Repl.Testing/ReplTestHost.cs index 917ab7ef..772aefd2 100644 --- a/src/Repl.Testing/ReplTestHost.cs +++ b/src/Repl.Testing/ReplTestHost.cs @@ -25,6 +25,11 @@ public sealed class ReplTestHost : IAsyncDisposable // session invisible to the app-root check below despite its scope still being live. private readonly ConcurrentDictionary> _pendingServiceDisposals = new(ReferenceEqualityComparer.Instance); + // App-root disposals DisposeAsync scheduled rather than awaited, because a session's command was + // still running at the time. Retained so a failure during one of them — a singleton's Dispose() + // throwing — is observable through WaitForDeferredCleanupAsync instead of becoming an unobserved + // task exception, since DisposeAsync itself has already returned success by the time these run. + private readonly ConcurrentBag _deferredAppDisposals = new(); private bool _disposed; private ReplTestHost(Func appFactory, ReplScenarioOptions options) @@ -69,13 +74,32 @@ public async ValueTask OpenSessionAsync( cancellationToken.ThrowIfCancellationRequested(); descriptor ??= new SessionDescriptor(); var app = _appFactory(); - _apps.TryAdd(app, 0); + // Tracked only once StartAsync actually succeeds, not right after the factory call: StartAsync + // cannot fail once it has forced app.Services open (every throw in it — null checks, the + // cancellation re-check, a bad descriptor — happens before that), so a failure here means + // nothing Repl controls has touched the provider yet. Tracking (and, on the disposal race below, + // disposing) it anyway would force it open for nothing, solely to immediately tear it back down. var handle = await ReplSessionHandle.StartAsync( this, app, descriptor, _options, cancellationToken).ConfigureAwait(false); + + // Re-checked after StartAsync ran — it and the factory above can both take arbitrarily long — so + // a host disposed while this was in flight cannot let the app and session just produced escape + // untracked past that disposal. Narrows the race rather than closing it outright: a concurrent + // DisposeAsync could still finish its own iteration in the gap between this check and the + // registrations below, though that gap is now a few in-memory operations instead of an + // arbitrarily long factory call. + if (_disposed) + { + await handle.DisposeAsync().ConfigureAwait(false); + await DisposeAppAsync(app).ConfigureAwait(false); + ThrowIfDisposed(); + } + + _apps.TryAdd(app, 0); // Recorded regardless of what happens to the handle afterward — including a caller disposing it // directly, which would otherwise remove it from _sessions before its deferred scope release runs. _pendingServiceDisposals.GetOrAdd(app, static _ => new ConcurrentQueue()).Enqueue(handle.ServicesDisposed); @@ -158,8 +182,9 @@ public async ValueTask DisposeAsync() // HERE would reintroduce, at the host level, the unbounded wait on a possibly-orphaned // command that design already rejected — so the disposal runs once every one of this // app's still-in-flight sessions actually finishes, whenever that turns out to be, - // without this call blocking on it. - _ = DisposeAppOnceSessionsFinishAsync(app, pending); + // without this call blocking on it. Retained rather than discarded (see + // _deferredAppDisposals) so a failure in it is observable instead of unobserved. + _deferredAppDisposals.Add(DisposeAppOnceSessionsFinishAsync(app, pending)); continue; } @@ -170,6 +195,19 @@ public async ValueTask DisposeAsync() _pendingServiceDisposals.Clear(); } + /// + /// Awaits every app-root disposal scheduled rather than performed + /// directly, because a session's command was still running at the time it was called. + /// + /// + /// itself never waits for these — doing so would hang for as long as an + /// orphaned command does, exactly what that design avoids. Call this afterward only when a test + /// needs to know deferred cleanup has actually finished, or to surface a cleanup failure that would + /// otherwise never reach anything: has already returned success by the + /// time a deferred disposal runs, so nothing else observes it. + /// + public Task WaitForDeferredCleanupAsync() => Task.WhenAll(_deferredAppDisposals); + private static async Task DisposeAppOnceSessionsFinishAsync(ReplApp app, Task[] pending) { await Task.WhenAll(pending).ConfigureAwait(false); From 003a7c713279ae64525e04cbe156f426d19dfd4f Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 11:14:48 -0400 Subject: [PATCH 22/24] fix(testing): release the command gate on a throwing deferred disposal, and stop a rejected open from disposing an app another session still owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more autocarl passes on Repl.Testing's session-lifetime machinery, both reproduced against the exact head they were raised on before being fixed. ExecuteCommandCoreAsync's outer finally decremented _serviceLifecycle, ran the deferred DisposeServicesOnceAsync() when DisposeAsync had marked the session disposed while this command held the gate, and only then released _commandGate — as three statements in one finally block. If a Scoped IAsyncDisposable thrown from that deferred disposal, the release was never reached: a second command already queued behind this one on the gate would wait forever, since CommandTimeout's token is not created until RunWithinGateAsync, which it never reaches. Moved the release into its own inner finally so it always runs whatever the deferred disposal does. RED verified: reverted the inner finally, ran When_DeferredScopeDisposalFails_Then_TheCommandGateIsStillReleased (command A holds a scoped async disposable that throws, command B is queued behind it, the session is disposed mid-command) and watched B hang past its 10-second wait instead of observing ObjectDisposedException, before restoring the fix. d5f1000's own re-check narrowed the open-vs-dispose race but did not close it: autocarl's follow-up reproduced a residual defect in the SAME rejected-open cleanup that re-check guards. Session A holds a live command on a shared ReplApp; session B is still opening, blocked inside the app factory, when the host is disposed — DisposeAsync correctly defers the shared app's disposal since A's command is still in flight, but B's own StartAsync then resumes, reaches the disposed re-check with its own new scope on that SAME app, and the old code called DisposeAppAsync(app) unconditionally — tearing the app root out from under A's still-running command. Replaced the plain bool _disposed re-check with a lock (_gate) shared by OpenSessionAsync's registration step and DisposeAsync's own drain, so the two can never interleave: either an open's registrations (into _apps, _pendingServiceDisposals and _sessions together) land before the flip and are then part of what DisposeAsync sees, or the flip lands first and the open always takes the rejection branch. That branch now claims disposal ownership with _apps.TryAdd rather than disposing unconditionally: TryAdd succeeds only for an app nothing else ever registered, so a shared app already owned by another session (or already scheduled for deferred disposal) is left alone. _apps and _pendingServiceDisposals are consequently never cleared any more — clearing either would make a later TryAdd falsely report ownership, racing whichever side legitimately still holds it. RED verified: reverted RejectOpenAsync to the old unconditional DisposeAppAsync(app) call, ran the new When_TheHostIsDisposedWhileAnotherOpenIsRejected_Then_ASharedAppRootSurvivesForTheOtherSession and watched A's in-flight command fail with the app root disposed under it instead of completing, before restoring the fix. Refs #70, #74 --- .../Given_SessionScopedServices.cs | 136 ++++++++++++++++++ src/Repl.Testing/ReplSessionHandle.cs | 25 ++-- src/Repl.Testing/ReplTestHost.cs | 89 +++++++++--- 3 files changed, 220 insertions(+), 30 deletions(-) diff --git a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs index b5be1034..5fe11f13 100644 --- a/src/Repl.IntegrationTests/Given_SessionScopedServices.cs +++ b/src/Repl.IntegrationTests/Given_SessionScopedServices.cs @@ -662,4 +662,140 @@ public async Task When_TheOpenIsCanceledWhileTheFactoryRuns_Then_TheHostStaysUsa var waitForCleanup = async () => await host.WaitForDeferredCleanupAsync().ConfigureAwait(false); await waitForCleanup.Should().NotThrowAsync().ConfigureAwait(false); } + + private sealed class ThrowingAsyncScopedProbe : IAsyncDisposable + { + public ValueTask DisposeAsync() => throw new InvalidOperationException("dispose-boom"); + } + + [TestMethod] + [Description("The command gate must be released even when a command's own deferred scope disposal throws: DisposeAsync marks the session disposed while command A holds the gate, so releasing A's scope becomes A's own responsibility in its finally. If a Scoped IAsyncDisposable throws there, the gate must still be released — otherwise command B, already queued behind A, waits forever: the CommandTimeout-linked token does not exist yet at that point, it is created only once RunWithinGateAsync is reached.")] + public async Task When_DeferredScopeDisposalFails_Then_TheCommandGateIsStillReleased() + { + var aStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseA = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var host = ReplTestHost.Create(() => + { + var app = ReplApp.Create(services => services.AddScoped()) + .UseDefaultInteractive(); + app.Map("a", async (ThrowingAsyncScopedProbe _) => + { + aStarted.TrySetResult(); +#pragma warning disable VSTHRD003 + await releaseA.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + return "a-done"; + }); + app.Map("b", () => "b-done"); + return app; + }, options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var session = await host.OpenSessionAsync(); + var runningA = session.RunCommandAsync("a --no-logo").AsTask(); + await aStarted.Task.WaitAsync(TimeSpan.FromSeconds(30)); + + // Queues behind A on the command gate. + var runningB = session.RunCommandAsync("b --no-logo").AsTask(); + + // Disposes the session while A holds the gate — defers releasing A's scope to A's own finally. + var disposeSession = session.DisposeAsync().AsTask(); + + releaseA.TrySetResult(); + + // VSTHRD003: both tasks were started above, on this same thread, before this point. +#pragma warning disable VSTHRD003 + var actA = async () => await runningA.ConfigureAwait(false); + await actA.Should().ThrowAsync() + .WithMessage("dispose-boom") + .ConfigureAwait(false); + + // B must not hang on the gate A held — it must be released and B must observe disposal instead. + var actB = async () => await runningB.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); +#pragma warning restore VSTHRD003 + await actB.Should().ThrowAsync().ConfigureAwait(false); + + await disposeSession.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + } + + [TestMethod] + [Description("Narrowing the open-vs-dispose race to a single locked check-and-register step is not enough on its own: the rejected-open cleanup it guards must also refuse to dispose an app another, still-registered session already owns. Reproduces autocarl's exact shape — session A holds a live command on a shared ReplApp while session B is still opening (blocked inside the app factory) when the host is disposed; B's rejected open must not tear the shared app root out from under A's in-flight command.")] + public async Task When_TheHostIsDisposedWhileAnotherOpenIsRejected_Then_ASharedAppRootSurvivesForTheOtherSession() + { + var probe = new SingletonProbe(); + var app = ReplApp.Create(services => services.AddSingleton(_ => probe)).UseDefaultInteractive(); + var aStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseA = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + app.Map("block", async (SingletonProbe before) => + { + aStarted.TrySetResult(); +#pragma warning disable VSTHRD003 + await releaseA.Task.ConfigureAwait(false); +#pragma warning restore VSTHRD003 + // Resolved after the host disposal call below has already returned: proves the shared app + // root was not disposed out from under this still-running command by B's rejected open. + return before.Disposed.ToString(); + }); + + var bFactoryEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseBFactory = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factoryCalls = 0; + var host = ReplTestHost.Create( + () => + { + if (Interlocked.Increment(ref factoryCalls) == 2) + { + bFactoryEntered.TrySetResult(); +#pragma warning disable VSTHRD002 + releaseBFactory.Task.GetAwaiter().GetResult(); +#pragma warning restore VSTHRD002 + } + + return app; + }, + options => options.CommandTimeout = TimeSpan.FromMinutes(5)); + + var sessionA = await host.OpenSessionAsync(); + var runningA = sessionA.RunCommandAsync("block --no-logo").AsTask(); + await aStarted.Task.WaitAsync(TimeSpan.FromSeconds(30)).ConfigureAwait(false); + + // VSTHRD003: the task is started right here — the factory blocks the pool thread Task.Run gives + // it, so this test drives release/disposal from its own thread while the open is in flight. +#pragma warning disable VSTHRD003 + var openingB = Task.Run(async () => await host.OpenSessionAsync().ConfigureAwait(false)); +#pragma warning restore VSTHRD003 + await bFactoryEntered.Task.WaitAsync(TimeSpan.FromSeconds(30)).ConfigureAwait(false); + + // A's command is still running, so the host schedules the shared app's disposal as deferred + // instead of performing it now — this call must return without waiting for A. + await host.DisposeAsync().ConfigureAwait(false); + + // B's StartAsync now resumes and reaches the disposed re-check with its own new scope on the + // SAME shared app: the fix must see A's app already registered and leave it alone. + releaseBFactory.TrySetResult(); +#pragma warning disable VSTHRD003 + var actB = async () => await openingB.WaitAsync(TimeSpan.FromSeconds(30)).ConfigureAwait(false); +#pragma warning restore VSTHRD003 + await actB.Should().ThrowAsync().ConfigureAwait(false); + + await AssertSharedAppSurvivesForAAndIsEventuallyReleasedAsync(host, probe, releaseA, runningA).ConfigureAwait(false); + } + + private static async Task AssertSharedAppSurvivesForAAndIsEventuallyReleasedAsync( + ReplTestHost host, + SingletonProbe probe, + TaskCompletionSource releaseA, + Task runningA) + { + releaseA.TrySetResult(); + var completedA = await runningA.WaitAsync(TimeSpan.FromSeconds(30)).ConfigureAwait(false); + completedA.ExitCode.Should().Be( + 0, + "the shared app root must still be alive for A's in-flight command after B's rejected open: {0}", + completedA.OutputText); + + // The app is still expected to be released eventually, once A's own deferred scope release lets + // the host's originally scheduled cleanup proceed — not leaked just because B backed off. + await host.WaitForDeferredCleanupAsync().WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + probe.Disposed.Should().BeTrue("the shared app root must still be released once every session using it is done"); + } } diff --git a/src/Repl.Testing/ReplSessionHandle.cs b/src/Repl.Testing/ReplSessionHandle.cs index 8e418fc1..f565894e 100644 --- a/src/Repl.Testing/ReplSessionHandle.cs +++ b/src/Repl.Testing/ReplSessionHandle.cs @@ -129,16 +129,25 @@ private async ValueTask ExecuteCommandCoreAsync( finally { var after = Interlocked.Add(ref _serviceLifecycle, -CommandInFlight); - if ((after & DisposalRequested) != 0) + try { - // DisposeAsync ran while this command held the gate and deferred to it rather than - // disposing _services out from under a run in progress. Finish that disposal now, still - // inside the gate, so a caller queued behind this one hits the re-check above instead of - // a provider that is only half torn down. - await DisposeServicesOnceAsync().ConfigureAwait(false); + if ((after & DisposalRequested) != 0) + { + // DisposeAsync ran while this command held the gate and deferred to it rather than + // disposing _services out from under a run in progress. Finish that disposal now, + // still inside the gate, so a caller queued behind this one hits the re-check above + // instead of a provider that is only half torn down. + await DisposeServicesOnceAsync().ConfigureAwait(false); + } + } + finally + { + // In its own inner finally: a scoped disposable throwing from DisposeServicesOnceAsync + // must not skip this. The gate has no timeout of its own — CommandTimeout only links a + // token once RunWithinGateAsync is reached — so a queued caller left waiting here would + // hang until the process ends rather than reach the in-gate ThrowIfDisposed re-check. + _commandGate.Release(); } - - _commandGate.Release(); } } diff --git a/src/Repl.Testing/ReplTestHost.cs b/src/Repl.Testing/ReplTestHost.cs index 772aefd2..598bba60 100644 --- a/src/Repl.Testing/ReplTestHost.cs +++ b/src/Repl.Testing/ReplTestHost.cs @@ -30,6 +30,13 @@ public sealed class ReplTestHost : IAsyncDisposable // throwing — is observable through WaitForDeferredCleanupAsync instead of becoming an unobserved // task exception, since DisposeAsync itself has already returned success by the time these run. private readonly ConcurrentBag _deferredAppDisposals = new(); + // Guards _disposed together with every registration into _sessions/_apps/_pendingServiceDisposals, so + // OpenSessionAsync and DisposeAsync can never interleave: either an open's registrations all land + // before DisposeAsync's own drain (below) and are then part of what it sees, or DisposeAsync's flip + // runs first and the open always takes the rejection branch instead — closing the race a post-hoc + // re-check of _disposed alone could only ever narrow. Held only across synchronous dictionary/field + // operations, never across an await. + private readonly Lock _gate = new(); private bool _disposed; private ReplTestHost(Func appFactory, ReplScenarioOptions options) @@ -86,24 +93,32 @@ public async ValueTask OpenSessionAsync( _options, cancellationToken).ConfigureAwait(false); - // Re-checked after StartAsync ran — it and the factory above can both take arbitrarily long — so - // a host disposed while this was in flight cannot let the app and session just produced escape - // untracked past that disposal. Narrows the race rather than closing it outright: a concurrent - // DisposeAsync could still finish its own iteration in the gap between this check and the - // registrations below, though that gap is now a few in-memory operations instead of an - // arbitrarily long factory call. - if (_disposed) + // The disposed check and every registration below run as one atomic step under _gate — the same + // lock DisposeAsync's own drain takes — so StartAsync running arbitrarily long above can no longer + // let a disposed-in-the-meantime host register (or half-register) this app and session. + bool disposed; + bool duplicateSessionId = false; + lock (_gate) { - await handle.DisposeAsync().ConfigureAwait(false); - await DisposeAppAsync(app).ConfigureAwait(false); - ThrowIfDisposed(); + disposed = _disposed; + if (!disposed) + { + _apps.TryAdd(app, 0); + // Recorded regardless of what happens to the handle afterward — including a caller + // disposing it directly, which would otherwise remove it from _sessions before its + // deferred scope release runs. + _pendingServiceDisposals.GetOrAdd(app, static _ => new ConcurrentQueue()) + .Enqueue(handle.ServicesDisposed); + duplicateSessionId = !_sessions.TryAdd(handle.SessionId, handle); + } } - _apps.TryAdd(app, 0); - // Recorded regardless of what happens to the handle afterward — including a caller disposing it - // directly, which would otherwise remove it from _sessions before its deferred scope release runs. - _pendingServiceDisposals.GetOrAdd(app, static _ => new ConcurrentQueue()).Enqueue(handle.ServicesDisposed); - if (!_sessions.TryAdd(handle.SessionId, handle)) + if (disposed) + { + await RejectOpenAsync(app, handle).ConfigureAwait(false); + } + + if (duplicateSessionId) { await handle.DisposeAsync().ConfigureAwait(false); throw new InvalidOperationException($"A session with id '{handle.SessionId}' already exists."); @@ -112,6 +127,27 @@ public async ValueTask OpenSessionAsync( return handle; } + /// + /// Cleans up a session and its app after observed the host already + /// disposed, then rethrows as . + /// + private async ValueTask RejectOpenAsync(ReplApp app, ReplSessionHandle handle) + { + await handle.DisposeAsync().ConfigureAwait(false); + // _apps.TryAdd claims disposal ownership rather than merely checking membership: an app no earlier + // session ever registered is this rejected open's own responsibility, but one already registered — + // shared with a session DisposeAsync's own drain already knows about — must be left alone. _apps is + // never cleared (see DisposeAsync), so this claim stays meaningful for the host's whole remaining + // (disposed) lifetime, and disposing it here too would tear it out from under that other session's + // still-running command. + if (_apps.TryAdd(app, 0)) + { + await DisposeAppAsync(app).ConfigureAwait(false); + } + + ThrowIfDisposed(); + } + /// /// Snapshots every session currently open on this host, ordered by session id so an assertion does /// not depend on the order they were opened in. @@ -156,19 +192,31 @@ internal void RemoveSession(string sessionId) /// public async ValueTask DisposeAsync() { - if (_disposed) + ReplSessionHandle[] sessionsSnapshot; + lock (_gate) { - return; + if (_disposed) + { + return; + } + + _disposed = true; + sessionsSnapshot = [.. _sessions.Values]; } - _disposed = true; - foreach (var session in _sessions.Values) + foreach (var session in sessionsSnapshot) { await session.DisposeAsync().ConfigureAwait(false); } _sessions.Clear(); + // _apps is read here but deliberately never cleared (nor is _pendingServiceDisposals): a + // concurrent OpenSessionAsync whose own _gate section ran before the flip above is guaranteed by + // that same lock to have finished registering into both before this line runs, so the live read + // below already sees it. One that instead observes _disposed == true relies on _apps.TryAdd still + // reporting "already present" for an app registered here — clearing it would make that claim-check + // always succeed, racing this method's own disposal of the same app. See OpenSessionAsync. foreach (var app in _apps.Keys) { var pending = _pendingServiceDisposals.TryGetValue(app, out var queue) @@ -190,9 +238,6 @@ public async ValueTask DisposeAsync() await DisposeAppAsync(app).ConfigureAwait(false); } - - _apps.Clear(); - _pendingServiceDisposals.Clear(); } /// From e592ac7625562ccfe90ad6261317dc2ceb151812 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 11:15:11 -0400 Subject: [PATCH 23/24] fix(mcp): resolve warm-up and mcp serve's own scope from the connection, not a different one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more findings against this branch's MCP half, both reproduced before being fixed. RunAsync called BuildDynamicServerOptions() — whose eager warm-up evaluates every presence predicate, including one gated on a Scoped dependency, to preserve fail-fast validation when CommandFilter is unset — before creating this connection's own session context. Warm-up therefore resolved a Scoped predicate dependency from _catalogContext, the handler-lifetime stand-in for no session at all, while discovery and execution afterward resolved it from the real per-connection scope: two distinct instances for one connection, and a wasted construction nothing ever reads, since the warm-up model itself is discarded. Reordered RunAsync to create the connection context first and pass its Services into BuildDynamicServerOptions, which now takes the provider to warm up against as a parameter instead of reading _catalogContext directly; BuildStaticServerOptions, which has no per-connection context by design, is deliberately unchanged. RED verified: reverted the warm-up call back to _catalogContext.Services, ran the new When_APresencePredicateGatesOnAScopedService_Then_WarmUpSharesTheConnectionsInstance (a Scoped registration recording every instance DI ever constructs for it) and watched it construct two, before restoring the fix. Separately: the mcp serve CLI command (McpModule) hands McpServerHandler whatever provider the Repl command pipeline bound for that invocation — which, since mcp serve IS the whole Run* invocation for a stdio connection, is already the one DI scope Run* opened for it. CreateSessionContext's connection branch resolved IServiceScopeFactory from that provider and called CreateAsyncScope() regardless, unaware it might already be holding a scope: IServiceScopeFactory is itself a singleton bound to the true application root, so the scope it builds is a SIBLING of the one already open, not nested inside it — any Scoped instance the run's own middleware or command pipeline resolved before this handler ran would be invisible to MCP discovery and every tool call afterward. Added servicesAreSessionScoped to the constructor, defaulting false (every other caller — BuildMcpServerOptions's application-root provider, and this repository's own tests constructing the handler directly to exercise session-scope behavior in isolation — is unaffected); McpModule now passes true. RED verified: reverted the new !_servicesAreSessionScoped guard, ran the new When_ServicesAreAlreadySessionScoped_Then_TheConnectionReusesThatSameScope (resolves a Scoped probe from a pre-opened scope, hands that scope to the handler, asserts a tool call on the resulting connection observes the SAME instance) and watched it observe a second, different one, before restoring the fix. Refs #70, #74 --- src/Repl.Mcp/McpModule.cs | 5 +- src/Repl.Mcp/McpServerHandler.cs | 68 +++++++++++++++---- src/Repl.McpTests/Given_McpSessionScopes.cs | 72 +++++++++++++++++++++ 3 files changed, 132 insertions(+), 13 deletions(-) diff --git a/src/Repl.Mcp/McpModule.cs b/src/Repl.Mcp/McpModule.cs index 1af8eb04..3a4beaf9 100644 --- a/src/Repl.Mcp/McpModule.cs +++ b/src/Repl.Mcp/McpModule.cs @@ -17,7 +17,10 @@ public void Map(IReplMap map) // instance captured at registration time, so both configuration // paths converge on a single options instance. var resolved = services.GetService(typeof(ReplMcpServerOptions)) as ReplMcpServerOptions ?? options; - var handler = new McpServerHandler(app, resolved, services); + // services is this command's own Run*-opened scope, spanning the whole "mcp serve" + // invocation — i.e. this entire connection — so the handler must reuse it rather than + // opening a second, sibling scope. See McpServerHandler's constructor doc. + var handler = new McpServerHandler(app, resolved, services, servicesAreSessionScoped: true); await handler.RunAsync(io, ct).ConfigureAwait(false); return Results.Exit(0); }) diff --git a/src/Repl.Mcp/McpServerHandler.cs b/src/Repl.Mcp/McpServerHandler.cs index c879c565..4f7bf72d 100644 --- a/src/Repl.Mcp/McpServerHandler.cs +++ b/src/Repl.Mcp/McpServerHandler.cs @@ -26,6 +26,7 @@ internal sealed class McpServerHandler private readonly ICoreReplApp _app; private readonly ReplMcpServerOptions _options; private readonly IServiceProvider _services; + private readonly bool _servicesAreSessionScoped; private readonly TimeProvider _timeProvider; private readonly char _separator; private readonly McpRequestServerAccessor _requestServers = new(); @@ -63,14 +64,35 @@ internal sealed class McpServerHandler private readonly McpServerResourceCollection _resourceListChanged = new(); private readonly McpServerPrimitiveCollection _promptListChanged = new(); + /// The core Repl app. + /// MCP server configuration. + /// + /// Services for this handler. Used directly, or as the root a per-connection scope is created from — + /// see . + /// + /// + /// True when already IS one connection's lifetime-scoped DI container — + /// e.g. Run*'s own per-run scope, handed down through the mcp serve CLI command — so + /// must reuse it as-is instead of resolving its + /// and opening a second, sibling scope: that factory is itself a singleton bound to the true + /// application root, so a scope built from it never nests inside the one + /// already is, and any Scoped instance established by the run's own middleware or command pipeline + /// before this handler ran would be invisible to MCP discovery and tool execution. Default false + /// (create a new per-connection scope from ) matches every other caller — + /// 's + /// application-root provider, and this repository's own tests exercising session-scope behavior in + /// isolation. + /// public McpServerHandler( ICoreReplApp app, ReplMcpServerOptions options, - IServiceProvider services) + IServiceProvider services, + bool servicesAreSessionScoped = false) { _app = app; _options = options; _services = services; + _servicesAreSessionScoped = servicesAreSessionScoped; _timeProvider = services.GetService(typeof(TimeProvider)) as TimeProvider ?? TimeProvider.System; _separator = McpToolNameFlattener.ResolveSeparator(options.ToolNamingSeparator); // Sampling/elicitation/feedback are stateless (they resolve the request-bound server @@ -102,7 +124,12 @@ private McpSessionContext CreateSessionContext(McpRootsScope rootsScope) // with no owner to dispose a scope for, so it resolves straight from _services instead. On the // reusable-options path that also means Scoped resolves once from the root, matching the // single-context, single-instance-for-every-connection carve-out that path already documents. + // _servicesAreSessionScoped skips scope creation even for a connection context: _services is + // already the one scope this whole run uses (see the constructor doc), and IServiceScopeFactory + // is itself a singleton bound to the application root — a scope built from it would be a SIBLING + // of _services, not nested inside it, splitting one connection across two unrelated Scoped graphs. var scope = rootsScope == McpRootsScope.Connection + && !_servicesAreSessionScoped && _services.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory ? scopeFactory.CreateAsyncScope() : (AsyncServiceScope?)null; @@ -145,15 +172,22 @@ private static McpSessionContext ResolveContext(McpServer? requestServer) => Justification = "MCP server handler runs in a context where all types are preserved.")] public async Task RunAsync(IReplIoContext io, CancellationToken ct) { - var serverOptions = BuildDynamicServerOptions(); - var serverName = serverOptions.ServerInfo?.Name ?? "repl-mcp-server"; - var transport = _options.TransportFactory is { } factory - ? factory(serverName, io) - : new StdioServerTransport(serverName); + // Created before BuildDynamicServerOptions, not after: that call's eager warm-up resolves + // presence-predicate dependencies, and they must see THIS connection's scope — the one + // discovery and every execution use afterward — rather than the handler-lifetime catalog + // context, which would hand a Scoped predicate dependency a different instance than the + // connection it is gating ever sees again. Disposed last (the outermost finally below), + // preserving the same unwind order server/transport already used before this changed: server, + // then transport, then context. + var context = CreateSessionContext(McpRootsScope.Connection); try { - var context = CreateSessionContext(McpRootsScope.Connection); - await using (context.ConfigureAwait(false)) + var serverOptions = BuildDynamicServerOptions(context.Services); + var serverName = serverOptions.ServerInfo?.Name ?? "repl-mcp-server"; + var transport = _options.TransportFactory is { } factory + ? factory(serverName, io) + : new StdioServerTransport(serverName); + try { var server = McpServer.Create(transport, serverOptions, serviceProvider: context.Services); AttachSession(context, server); @@ -168,14 +202,21 @@ public async Task RunAsync(IReplIoContext io, CancellationToken ct) await server.DisposeAsync().ConfigureAwait(false); } } + finally + { + await transport.DisposeAsync().ConfigureAwait(false); + } } finally { - await transport.DisposeAsync().ConfigureAwait(false); + await context.DisposeAsync().ConfigureAwait(false); } } - internal McpServerOptions BuildDynamicServerOptions() + /// Builds the dynamic mcp serve options, warming up presence predicates against + /// — the connection's own scope, so a Scoped predicate dependency sees + /// the same instance during warm-up as discovery and execution see afterward. + internal McpServerOptions BuildDynamicServerOptions(IServiceProvider services) { var serverName = _options.ServerName ?? ResolveAppName() ?? "repl-mcp-server"; var serverVersion = _options.ServerVersion ?? "1.0.0"; @@ -186,8 +227,11 @@ internal McpServerOptions BuildDynamicServerOptions() if (_options.CommandFilter is null) { // No request is flowing at construction, so this warm-up builds the legacy view; the first - // modern request rebuilds for its own era. - _ = CreateDocumentationModel(_catalogContext.Services, sessionless: false); + // modern request rebuilds for its own era. Against the connection's own services, passed in + // by the caller — not _catalogContext, which is the reusable-options path's handler-lifetime + // stand-in for a session and would hand a Scoped predicate dependency a different instance + // than the one this connection's discovery and execution resolve afterward. + _ = CreateDocumentationModel(services, sessionless: false); } return new McpServerOptions diff --git a/src/Repl.McpTests/Given_McpSessionScopes.cs b/src/Repl.McpTests/Given_McpSessionScopes.cs index 7c44bb0e..fc6e53a9 100644 --- a/src/Repl.McpTests/Given_McpSessionScopes.cs +++ b/src/Repl.McpTests/Given_McpSessionScopes.cs @@ -197,6 +197,48 @@ public async Task When_APresencePredicateGatesOnAScopedService_Then_TheAdvertise body.Should().Contain("classified", "what discovery advertised has to run"); } + private sealed class ConstructionTrackingProbe + { + public Guid Id { get; } = Guid.NewGuid(); + } + + [TestMethod] + [Description("BuildDynamicServerOptions's eager warm-up (CreateDocumentationModel, run once per connection so fail-fast validation still happens when no CommandFilter defers it) evaluates every presence predicate, including one gated on a Scoped dependency. That resolution must happen against THIS connection's own scope — the one discovery and execution use afterward — not a different one: otherwise warm-up silently constructs a second, wasted instance nothing ever reads, and this connection's Scoped graph is already split in two before its first real request.")] + public async Task When_APresencePredicateGatesOnAScopedService_Then_WarmUpSharesTheConnectionsInstance() + { + // Recorded inside the DI factory itself, not by a method the test calls afterward: what this + // test cares about is how many DISTINCT instances DI ever built for the connection, and a second, + // wasted one built by warm-up in the wrong scope would never otherwise be observed — nothing reads + // the discarded model it contributes to. A Scoped registration invokes this factory at most once + // per scope, so the entry count IS the distinct-scope count. + var constructed = new List(); + var app = ReplApp.Create(services => services.AddScoped(_ => + { + var probe = new ConstructionTrackingProbe(); + constructed.Add(probe.Id); + return probe; + })); + app.UseMcpServer(); + app.Map("id", (ConstructionTrackingProbe probe) => probe.Id.ToString()); + // Always true: only the ACT of evaluating the predicate — which forces probe to resolve — matters + // here, not the value it returns. + app.MapModule(new GatedModule(), (ConstructionTrackingProbe probe) => probe.Id != Guid.Empty); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = CreateHandler(app); + var session = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using var scope = session.ConfigureAwait(false); + + var reported = await CallAsync(session.Client, "id", token: cts.Token).ConfigureAwait(false); + + constructed.Should().ContainSingle( + "warm-up's presence-predicate resolution and this tool call must share the SAME connection " + + "scope; a second entry means warm-up built its own, separate instance in a different scope"); + reported.Should().Contain( + constructed[0].ToString(), + "the one instance DI ever built for this connection must be the one the tool call observes"); + } + [TestMethod] [Description("Guards the composition order. The session's capability services are layered OVER its DI scope, so a command resolves both; get it backwards and IMcpClientRoots resolves nothing — and because McpServiceProviderOverlay also answers IServiceProviderIsService, a declared capability is reclassified as a client-supplied argument and the primitive stops being invocable.")] public async Task When_ACommandInjectsACapabilityService_Then_ItResolvesAlongsideTheSessionScope() @@ -344,6 +386,36 @@ public async Task When_DifferentScopesAreSuppliedPerOptionsBuild_Then_EachGetsIt "a distinct IServiceProvider supplied per options build must give each its own Scoped instance"); } + [TestMethod] + [Description("The mcp serve CLI command hands McpServerHandler the scope Run* already opened for that whole invocation — one connection, one Run* scope — so RunAsync must reuse it instead of resolving a second one from IServiceScopeFactory, which is itself a singleton bound to the application root and would build a SIBLING, not a nested child. Reproduces the shape Codex found: a Scoped instance a caller's own middleware or command pipeline resolves BEFORE this handler runs must still be the one MCP discovery and tool execution see afterward.")] + public async Task When_ServicesAreAlreadySessionScoped_Then_TheConnectionReusesThatSameScope() + { + var app = ReplApp.Create(services => services.AddScoped()); + app.Map("scoped", (ScopedProbe probe) => probe.Id.ToString()); + + // Stands in for the outer Run*-opened scope McpModule hands down for "mcp serve": resolving here, + // before the handler ever runs, is what a caller's own middleware or command pipeline would do. + var outerScope = app.Services.GetRequiredService().CreateAsyncScope(); + await using var outerScopeOwner = outerScope.ConfigureAwait(false); + var establishedBeforehand = outerScope.ServiceProvider.GetRequiredService().Id; + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + var handler = new McpServerHandler( + app.Core, + new ReplMcpServerOptions { TransportFactory = McpTestFixture.PipeTransportFactory }, + outerScope.ServiceProvider, + servicesAreSessionScoped: true); + var session = await StartAsync(handler, cts.Token).ConfigureAwait(false); + await using var scope = session.ConfigureAwait(false); + + var reported = await CallAsync(session.Client, "scoped", token: cts.Token).ConfigureAwait(false); + + reported.Should().Contain( + establishedBeforehand.ToString(), + "the connection must reuse the caller's own already-open scope, not a new sibling one, so " + + "state established before RunAsync ran is still visible to discovery and execution"); + } + private static Task StartSharedAsync( McpServerOptions mcpOptions, CancellationToken cancellationToken) => From 5b51a25be816290c1eea4882ae334305d0724a94 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Mon, 21 Sep 2026 11:15:23 -0400 Subject: [PATCH 24/24] fix(di): observe an already-cancelled token before opening a run's session scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex found the non-hosted branch of RunOutcomeAsync — reached by every RunAsync overload that is not HostedServiceLifecycle-managed, including the one taking a caller-supplied IServiceProvider — went straight to RunInSessionScopeAsync without checking cancellation first. That helper resolves IServiceScopeFactory and calls CreateAsyncScope() unconditionally before the pipeline's own cancellation policy ever runs, so an already-cancelled token still paid for scope creation instead of short-circuiting to the configured ExitCodes.Cancelled outcome — and for a caller-supplied provider that is itself disposed, a realistic shape when a caller cancels and tears its own provider down together, that lookup throws ObjectDisposedException instead. Both the IReplHost overload and RunOutcomeAsync's own hosted branch already call _core.TryObserveCallerCancellation before doing anything with the provider; the non-hosted branch was the one path missing it. Added the same check, mirroring the exact pattern already used at both other sites. RED verified: reverted the check, ran the new When_PreCancelledTokenAndTheCallerProviderIsDisposed_Then_CancellationIsObservedBeforeTheProviderIsTouched (a disposed IServiceProvider plus an already-cancelled token) and watched RunInSessionScopeAsync throw ObjectDisposedException instead of the run reporting the configured Cancelled outcome, before restoring the fix. Refs #70, #74 --- src/Repl.Defaults/ReplApp.cs | 9 +++++++++ src/Repl.Tests/Given_ExitCodes.cs | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/Repl.Defaults/ReplApp.cs b/src/Repl.Defaults/ReplApp.cs index 142d5dd2..59dd9f40 100644 --- a/src/Repl.Defaults/ReplApp.cs +++ b/src/Repl.Defaults/ReplApp.cs @@ -410,6 +410,15 @@ private async ValueTask RunOutcomeAsync( { if (runOptions.HostedServiceLifecycle is HostedServiceLifecycleMode.None or HostedServiceLifecycleMode.Guest) { + // Checked before RunInSessionScopeAsync, which resolves IServiceScopeFactory and creates a + // scope unconditionally: for an already-cancelled token that would run scope-creation side + // effects (or throw, for a caller-supplied provider that is itself disposed) instead of + // honouring ExitCodes.Cancelled the same way the IReplHost and hosted overloads already do. + if (_core.TryObserveCallerCancellation(cancellationToken) is { } cancelledEarly) + { + return cancelledEarly; + } + return await RunInSessionScopeAsync( services, runOptions.SessionScope, diff --git a/src/Repl.Tests/Given_ExitCodes.cs b/src/Repl.Tests/Given_ExitCodes.cs index 99d91225..ef0f7ab7 100644 --- a/src/Repl.Tests/Given_ExitCodes.cs +++ b/src/Repl.Tests/Given_ExitCodes.cs @@ -331,6 +331,25 @@ public async Task When_PreCancelledTokenAndCancelledIsMapped_Then_ExitCodeIs130( recorder.Last!.Kind.Should().Be(ReplExecutionOutcomeKind.Cancelled); } + [TestMethod] + [Description("Regression guard: the RunAsync(args, IServiceProvider, ...) overload must observe an already-cancelled token before RunInSessionScopeAsync's own IServiceScopeFactory lookup and CreateAsyncScope() call, exactly as the IReplHost overload already does. A caller-supplied provider that is itself disposed — a realistic shape when a caller cancels and tears its own provider down together — throws from that lookup instead of letting the run report the configured Cancelled outcome.")] + public async Task When_PreCancelledTokenAndTheCallerProviderIsDisposed_Then_CancellationIsObservedBeforeTheProviderIsTouched() + { + var services = new ServiceCollection().BuildServiceProvider(); + await services.DisposeAsync().ConfigureAwait(false); + using var cts = new CancellationTokenSource(); + await cts.CancelAsync().ConfigureAwait(false); + var recorder = new OutcomeRecorder(); + var sut = CreateApp(recorder, options => options.ExitCodes.Cancelled = 130); + sut.Map("work", () => "never"); + using var session = OpenSession(out _); + + var exitCode = await sut.RunAsync(["work"], services, cancellationToken: cts.Token).ConfigureAwait(false); + + exitCode.Should().Be(130); + recorder.Last!.Kind.Should().Be(ReplExecutionOutcomeKind.Cancelled); + } + [TestMethod] [Description("Regression guard: verifies the UsageError code is configurable so that applications can publish their own exit-code contract.")] public void When_UsageErrorIsRemapped_Then_ConfiguredCodeIsReturned()