feat(mcp): tear down stdio MCP children at ApplicationStopping (#1667) - #1681
Open
Aaronontheweb wants to merge 5 commits into
Open
feat(mcp): tear down stdio MCP children at ApplicationStopping (#1667)#1681Aaronontheweb wants to merge 5 commits into
Aaronontheweb wants to merge 5 commits into
Conversation
…aw-dev#1667) Stdio MCP child processes were only torn down by McpClientManager's IHostedService.StopAsync, which .NET's LIFO hosted-service ordering places after AkkaHostedService.StopAsync — which blocks on the full session-drain CoordinatedShutdown phase (up to 190s). MCP teardown therefore ran strictly after drain instead of concurrently with it, and the CLI force-kill path never runs it at all, orphaning MCP child processes. Implements openspec/changes/mcp-shutdown-teardown: - Trigger teardown from IHostApplicationLifetime.ApplicationStopping, which fires before any IHostedService.StopAsync, independent of registration order. - Make teardown idempotent via one memoized TeardownAsync() Task shared by the ApplicationStopping hook, StopAsync, and Dispose — whichever runs first does the real work, the others converge on the same completion. - Gate ConnectAsync/TryReconnectAsync on a _stopping flag set synchronously before any await, closing the tool-call-retry and McpReconnectionService reconnect paths without touching either caller. - Dispose all configured clients in parallel (Task.WhenAll + per-client try/catch) instead of sequentially, bounding total teardown by the slowest single server. Tests (McpShutdownTeardownTests.cs, real spawned child processes via McpSmokeHarness): idempotent double/triple teardown with no spurious warnings; TryReconnectAsync returns false and installs no new client once teardown starts; parallel teardown of 2 servers takes ~10s, not the ~20s a sequential regression produces (verified by temporarily reverting to sequential and confirming the test fails); an in-flight tool call fails with the standard "Error: MCP tool '...' failed" result within milliseconds of teardown starting, not a hang. Fixes netclaw-dev#1667. The OpenSpec change ships in this PR; sync/archive follows post-merge.
There was a problem hiding this comment.
Pull request overview
This PR changes Netclaw’s daemon shutdown behavior for stdio-based MCP servers so that MCP client disposal (and owned child-process teardown) starts at IHostApplicationLifetime.ApplicationStopping, allowing it to run concurrently with Akka’s session drain / CoordinatedShutdown rather than being sequenced strictly after it. It also makes teardown idempotent (shared memoized task), blocks reconnect attempts once shutdown begins, and disposes multiple configured MCP clients in parallel to bound total teardown time by the slowest server.
Changes:
- Trigger MCP teardown from
ApplicationStoppingand refactor it into a single memoizedTeardownAsync()shared byApplicationStopping,StopAsync, andDispose. - Add a shutdown guard (
_stopping) that preventsConnectAsync/TryReconnectAsyncfrom creating new clients/processes once teardown begins, and dispose clients concurrently viaTask.WhenAll. - Add real-process shutdown/teardown tests (plus a smoke-server
sleeptool) and update OpenSpec + ops skill guidance to document the behavior change.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Netclaw.SmokeMcpServer/Program.cs | Adds a deterministic sleep(ms) MCP tool to hold calls in-flight for lifecycle/teardown testing. |
| src/Netclaw.Daemon/Mcp/McpClientManager.cs | Starts teardown on ApplicationStopping, memoizes teardown across stop paths, blocks reconnect during shutdown, and disposes clients in parallel. |
| src/Netclaw.Daemon.Tests/Mcp/McpSmokeHarness.cs | Extends the MCP smoke harness to supply an application-lifetime implementation to McpClientManager for shutdown signaling in tests. |
| src/Netclaw.Daemon.Tests/Mcp/McpShutdownTeardownTests.cs | New real-process tests validating idempotent teardown, no reconnect after teardown begins, parallel teardown latency, and clean in-flight failure behavior. |
| src/Netclaw.Daemon.Tests/Mcp/McpOAuthHeaderConflictTests.cs | Updates test manager construction for the new IHostApplicationLifetime dependency. |
| src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs | Updates test manager construction for the new IHostApplicationLifetime dependency. |
| src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs | Updates test manager construction for the new IHostApplicationLifetime dependency. |
| openspec/changes/mcp-shutdown-teardown/tasks.md | Captures the implementation + proof tasks for this OpenSpec change. |
| openspec/changes/mcp-shutdown-teardown/specs/netclaw-mcp/spec.md | Documents the modified shutdown requirements: early teardown, parallel disposal, idempotency, and no reconnect during shutdown. |
| openspec/changes/mcp-shutdown-teardown/proposal.md | Summarizes the motivation, scope, and intended behavioral changes for MCP shutdown teardown. |
| openspec/changes/mcp-shutdown-teardown/design.md | Details the design decisions (ApplicationStopping hook, memoized teardown, stopping guard, parallel disposal) and trade-offs. |
| openspec/changes/mcp-shutdown-teardown/.openspec.yaml | Declares the OpenSpec change metadata. |
| feeds/skills/.system/files/netclaw-operations/SKILL.md | Bumps netclaw-operations skill version to ship updated operational guidance. |
| feeds/skills/.system/files/netclaw-operations/references/tools.md | Documents the new expected behavior for MCP teardown timing and in-flight tool-call failures during shutdown. |
| .slopwatch/baseline.json | Updates the slopwatch baseline timestamps and entries (including the intentional Task.Delay(ms) in the smoke MCP server). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+102
to
+107
| public static async Task<string> Sleep( | ||
| [Description("Milliseconds to sleep before returning.")] int ms) | ||
| { | ||
| await Task.Delay(ms); | ||
| return "awake"; | ||
| } |
Comment on lines
+6
to
+8
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Hosting.Internal; | ||
| using Microsoft.Extensions.Logging; |
Comment on lines
10
to
12
| using System.Text.Json; | ||
| using Microsoft.Extensions.Hosting.Internal; | ||
| using Microsoft.Extensions.Logging.Abstractions; |
Comment on lines
89
to
+93
| oauthService, | ||
| NullNotificationSink.Instance, | ||
| TimeProvider.System, | ||
| NullLogger<McpClientManager>.Instance); | ||
| NullLogger<McpClientManager>.Instance, | ||
| new ApplicationLifetime(NullLogger<ApplicationLifetime>.Instance)); |
Comment on lines
299
to
+303
| oauthService, | ||
| NullNotificationSink.Instance, | ||
| TimeProvider.System, | ||
| NullLogger<McpClientManager>.Instance); | ||
| NullLogger<McpClientManager>.Instance, | ||
| new ApplicationLifetime(NullLogger<ApplicationLifetime>.Instance)); |
The sleep tool added to Netclaw.SmokeMcpServer for the MCP shutdown
teardown tests bumped the smoke server's tool count from 4 to 5, which
two end-to-end fixtures still hard-asserted:
- tests/smoke/scenarios/mcp-setup.sh failed on the daemon connection
log line ("connected (5 tools)" vs expected "(4 tools)")
- tests/smoke/tapes/screenshots/mcp-permissions.tape hung forever in
its readiness loop grepping for 'connected (4 tools)', so the TUI
never launched and both screenshot frames timed out
Update both to expect 5 tools and remove the stale approved baselines
so the next Screenshot Regression run regenerates them via the
documented workflow (tests/smoke/tapes/README.md § Baseline workflow:
a run with no baseline fails and uploads the captured actual for
review). The server-list baseline must be removed rather than left to
fail on its own: its only pixel delta is the single "4"->"5" glyph
(AE=146), which sits inside the harness's cursor tolerance
(SHOT_AE_TOLERANCE=1000), so CI would keep passing it against the
stale image and never emit a fresh actual.
Validated locally: run-smoke.sh mcp-setup passes (5/5); the
mcp-permissions tape completes with both frames captured, tool grid
showing all five rows with the header intact (issue netclaw-dev#1424 guard).
The previous commit (c0c8e3b) removed the stale mcp-permissions approved baselines so CI would regenerate them per the documented baseline workflow (tests/smoke/tapes/README.md § Baseline workflow). That run failed as expected ("no approved baseline") and uploaded the actual frames. Reviewed both actual frames from run 29535030964: - mcp-permissions-server-list: shows smoke-math (Connected, 5 tools) - mcp-permissions-tool-grid: shows all 5 tool rows (add, echo, process-info, record-tasks, sleep) with the header intact Committing the CI-rendered PNGs as the new approved baselines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stdio MCP child processes were only torn down by
McpClientManager.StopAsync, which .NET's LIFO hosted-service ordering places afterAkkaHostedService.StopAsync— and that blocks on the full session-drainCoordinatedShutdownphase (up to 190s). MCP teardown therefore ran strictly after drain instead of concurrently with it, and today's per-server teardown is sequential; the CLI force-kill path never runs teardown at all, orphaning MCP child processes in non-systemd deployments.Four design decisions (see
openspec/changes/mcp-shutdown-teardown/design.md):IHostApplicationLifetime.ApplicationStopping, which fires before anyIHostedService.StopAsync, independent of registration order.TeardownAsync()Task shared by theApplicationStoppinghook,StopAsync, andDispose— whichever runs first does the real work, the others converge on the same completion.ConnectAsync/TryReconnectAsyncon a_stoppingflag set synchronously before any await, closing the tool-call-retry andMcpReconnectionServicereconnect paths without touching either caller.Task.WhenAll+ per-client try/catch) instead of sequentially, bounding total teardown by the slowest single server.Tests
McpShutdownTeardownTests.cs(new), using real spawned child processes viaMcpSmokeHarness:ApplicationStopping→StopAsync→Dispose): child process actually exits, success log fires exactly once, no spurious warnings.TryReconnectAsyncafter teardown starts returnsfalseand installs no new client (proving no new child process)."Error: MCP tool '...' failed"result within milliseconds of teardown starting (pending MCP requests are cancelled as the first step of client disposal), not a hang.Full solution:
dotnet build0 warnings/0 errors;dotnet testall green (Daemon.Tests 855, Actors.Tests 2636, Cli.Tests 1280, others unaffected);openspec validate mcp-shutdown-teardown --strictpasses;dotnet slopwatch analyze0 new issues; file headers verified.Fixes #1667.
The OpenSpec change (
openspec/changes/mcp-shutdown-teardown/) ships in this PR; sync-to-main-specs and archive follow post-merge.