diff --git a/.slopwatch/baseline.json b/.slopwatch/baseline.json index 73ed51960..14b22a920 100644 --- a/.slopwatch/baseline.json +++ b/.slopwatch/baseline.json @@ -1,7 +1,7 @@ { "version": 1, "createdAt": "2026-05-12T17:20:55.7365203+00:00", - "updatedAt": "2026-06-10T19:44:08.3092383+00:00", + "updatedAt": "2026-07-16T16:54:31.2122436+00:00", "description": "Initial baseline created by 'slopwatch init' on 2026-05-12 17:20:55 UTC", "entries": [ { @@ -31,24 +31,6 @@ "message": "Adding warnings to NoWarn: OPENAI001", "baselinedAt": "2026-05-12T17:20:55.7420301+00:00" }, - { - "hash": "1a29ed65e4ed3efb", - "ruleId": "SW004", - "filePath": "src/Netclaw.Daemon.Tests/Services/ConfigWatcherServiceTests.cs", - "lineNumber": 139, - "codeSnippet": "Task.Delay(50, ct)", - "message": "Test uses Task.Delay(50) which may indicate a timing-dependent test", - "baselinedAt": "2026-05-12T17:20:55.7420338+00:00" - }, - { - "hash": "fcb5e461d7f70a7c", - "ruleId": "SW004", - "filePath": "src/Netclaw.Daemon.Tests/Services/ConfigWatcherServiceTests.cs", - "lineNumber": 154, - "codeSnippet": "Task.Delay(100, ct)", - "message": "Test uses Task.Delay(100) which may indicate a timing-dependent test", - "baselinedAt": "2026-05-12T17:20:55.7420454+00:00" - }, { "hash": "6ea5c8bbead4b59c", "ruleId": "SW004", @@ -112,15 +94,6 @@ "message": "Test uses Task.Delay(25 * (i + 1)) which may indicate a timing-dependent test", "baselinedAt": "2026-05-12T17:20:55.7421146+00:00" }, - { - "hash": "c00fb5b6beafab8b", - "ruleId": "SW004", - "filePath": "src/Netclaw.Actors.Tests/SubAgents/SubAgentActorTests.cs", - "lineNumber": 459, - "codeSnippet": "Task.Delay(Delay, cancellationToken)", - "message": "Test uses Task.Delay(Delay) which may indicate a timing-dependent test", - "baselinedAt": "2026-05-12T17:20:55.7421212+00:00" - }, { "hash": "b691cefe260611c6", "ruleId": "SW004", @@ -174,6 +147,15 @@ "codeSnippet": "Fact(SkipUnless = nameof(IsPosix), Skip = \"POSIX-only — matcher routes through BashParser on POSIX\")", "message": "Test method 'IsApproved_git_tag_grant_matches_both_version_forms' is disabled: POSIX-only — matcher routes through BashParser on POSIX", "baselinedAt": "2026-06-10T19:44:08.3092375+00:00" + }, + { + "hash": "92870afae0455103", + "ruleId": "SW004", + "filePath": "tests/Netclaw.SmokeMcpServer/Program.cs", + "lineNumber": 105, + "codeSnippet": "Task.Delay(ms)", + "message": "Test uses Task.Delay(ms) which may indicate a timing-dependent test", + "baselinedAt": "2026-07-16T16:54:31.2120133+00:00" } ] } \ No newline at end of file diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 119bab907..9c748a10b 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.32.0" + version: "2.33.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index 6a3f13284..ffa52eb01 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -121,3 +121,16 @@ JSON blob — so you can act on it directly: Both name the server explicitly (`server/tool`), so when two servers expose a same-named tool you can tell which one failed. + +### MCP teardown during daemon shutdown + +Once the daemon begins a graceful shutdown (SIGTERM or `netclaw daemon stop`), +MCP client teardown starts immediately — concurrently with session drain, +not strictly after it — and every configured server tears down at the same +time (bounded by the slowest single server, not the sum). If your tool call +is in flight when its server's client gets disposed, expect the same +`Error: MCP tool 'server/tool' failed: ` result described above, +returned promptly rather than hanging until the process exits — this is +expected end-of-life behavior, not a bug to retry around. Once shutdown has +started, no MCP server reconnects for any reason (not the automatic +reconnect above, not a background health check) — the daemon is exiting. diff --git a/openspec/changes/mcp-shutdown-teardown/.openspec.yaml b/openspec/changes/mcp-shutdown-teardown/.openspec.yaml new file mode 100644 index 000000000..cd2ce7e9f --- /dev/null +++ b/openspec/changes/mcp-shutdown-teardown/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-16 diff --git a/openspec/changes/mcp-shutdown-teardown/design.md b/openspec/changes/mcp-shutdown-teardown/design.md new file mode 100644 index 000000000..d2f752af7 --- /dev/null +++ b/openspec/changes/mcp-shutdown-teardown/design.md @@ -0,0 +1,249 @@ +## Context + +`McpClientManager` is registered as an `IHostedService` (and singleton +`IMcpReconnectable`) before `services.AddAkka(...)` in +`ConfigureDaemonServices` (`src/Netclaw.Daemon/Program.cs`). .NET's generic +host stops hosted services in reverse registration order (LIFO). Akka's own +hosted service (`Akka.Hosting.AkkaHostedService`, registered internally by +`AddAkka`) is therefore stopped *before* `McpClientManager`. Its `StopAsync` +override does not consult the `CancellationToken` it's given — it always +awaits `CoordinatedShutdown.Run(...)` to natural completion: + +```csharp +public virtual async Task StopAsync(CancellationToken cancellationToken) +{ + if (CoordinatedShutdown == null) return; + await CoordinatedShutdown.Run(CoordinatedShutdown.ClrExitReason.Instance) + .ConfigureAwait(false); +} +``` + +`CoordinatedShutdown`'s `before-service-unbind` phase is where +`drain-llm-sessions` runs (`Program.cs` ~1071), bounded to +`DaemonConfig.BoundedDrainTimeout` (190s, per #1673) but not shorter than +that by design — sessions mid-LLM-call need room to finish. Only once that +phase (and the rest of `CoordinatedShutdown`) completes does +`AkkaHostedService.StopAsync` return, unblocking the host's stop sequence to +reach `McpClientManager.StopAsync`. Today that method disposes clients with a +plain `foreach` + `await` — sequential, not parallel — each client's SDK +dispose being a graceful wait then a process-tree kill (10s + 10s per +server). `McpReconnectionService` (a `BackgroundService` registered +immediately after `McpClientManager`) polls every 30s and calls +`TryReconnectAsync` for any server in `Unreachable` state; because it too +sits before `AddAkka` in registration order, its own `StopAsync` (which +cancels its `stoppingToken`) doesn't fire until after `AkkaHostedService` +stops — so it keeps ticking, and could reconnect (launching a new child +process) for the entire ~190s drain window, independent of this change. + +MCP server state was established as daemon-scoped (not session-isolated) by +#1636 (`2026-07-14-make-stdio-mcp-process-bound`); that change's design.md +explicitly deferred shutdown-sequencing changes as out of scope. This design +picks that up. + +## Goals / Non-Goals + +**Goals:** + +- Start MCP child-process teardown as early in the shutdown sequence as the + host lifecycle allows, so it runs concurrently with session drain instead + of strictly after the full Akka `CoordinatedShutdown` completes. +- Make teardown idempotent so it is safe to trigger from an early hook and + still let the existing `IHostedService.StopAsync`/`IDisposable.Dispose` + path run unconditionally afterward. +- Close the reconnect-during-shutdown gap: once teardown starts, no code + path (invocation retry, or `McpReconnectionService`'s poll) may create a + new client or child process. +- Bound total MCP teardown time by the slowest single server, not the sum + across configured servers. +- Preserve the existing clean-tool-error behavior for a tool call that loses + its transport mid-flight — this already exists at the `McpToolAdapter` + layer and needs no new plumbing, only confirmation it still applies. + +**Non-Goals:** + +- Lazy/on-demand MCP process startup or idle-process reclamation. +- Per-session MCP process isolation. +- Any change to `HostOptions.ShutdownTimeout`, `DaemonConfig.*ShutdownBudget*`, + or the Akka `before-service-unbind` phase timeout — those stay exactly as + #1673 left them. This change only moves *when MCP teardown starts* + relative to those existing budgets, not the budgets themselves. +- Solving the CLI force-kill orphan gap from inside the daemon (see Risks). +- Changing the per-client SDK dispose timing (10s graceful + 10s kill) or the + `StdioClientTransportOptions.ShutdownTimeout` value. + +## Decisions + +### Trigger teardown from `IHostApplicationLifetime.ApplicationStopping` + +The generic host fires `ApplicationStopping` at the very start of +`Host.StopAsync`, before any hosted service's `StopAsync` is invoked +(including `AkkaHostedService`'s). Registering a callback there +(`appLifetime.ApplicationStopping.Register(...)`, or an +`ApplicationStopping.Register` wired in during DI setup) gives +`McpClientManager` a shutdown signal that fires regardless of which hosted +service stops first, and regardless of hosted-service registration order — +so this fix does not depend on reordering `McpClientManager`'s or `AddAkka`'s +registration (a more fragile change that would need re-verifying on every +future service addition). + +`ApplicationStopping` also fires for every shutdown path that calls +`IHostApplicationLifetime.StopApplication()` or lets the host observe +SIGTERM/Ctrl-C — including the `daemon stop` graceful path and the config-reload +restart path (`DaemonRestartCoordinator`) — so this is a single hook, not one +more of several call sites to keep in sync. + +Alternative considered: reorder hosted-service registration so +`McpClientManager` stops before `AkkaHostedService`. Rejected — LIFO ordering +is implicit and easy to silently break with a future registration reorder; +`ApplicationStopping` is an explicit, order-independent signal for exactly +this purpose. + +### Idempotent teardown via a shared, memoized operation + +`McpClientManager` gains one internal `TeardownAsync()` that both the new +`ApplicationStopping` callback and the existing `StopAsync`/`Dispose` route +through. The first caller performs the real work (disposes each client, +clears `_clients`/`_sharedToolFunctions`); it also sets a `Volatile` +`_stopping` flag (see next decision) synchronously, before any awaits, so a +racing second call can observe it immediately. The teardown work itself is +memoized behind a single cached `Task` so a second caller awaits the same +completion instead of redoing (or racing) disposal — this is the same +"first caller does the work, others await it" shape already used for +one-time async initialization elsewhere in the codebase, applied here to +one-time async teardown. + +This means `StopAsync` (still called by the host afterward, since removing +the `IHostedService` registration is not proposed — the host's own lifecycle +guarantees are worth keeping as a backstop) becomes a no-op await on already- +completed work in the common case, and the log line for "MCP client shut +down" is only emitted once, by whichever call actually performed the +dispose. + +Alternative considered: guard with `Interlocked.CompareExchange` on a bool and +skip entirely on the second call without awaiting anything. Rejected — +if `ApplicationStopping`'s teardown is still in flight when `StopAsync` runs +(a real possibility, since `StopAsync` can be reached quickly if other +hosted services stop fast), a bare skip would let `StopAsync` return before +disposal is actually complete, which could race the process exit against +still-running child-process kills. Awaiting the same memoized task avoids +that. + +### Stopping flag gates `ConnectAsync`/`TryReconnectAsync` + +`ConnectAsync` and `TryReconnectAsync` both check the same `_stopping` flag +set by teardown and return `false`/no-op immediately if set, before touching +the transport or spawning a process. This is the single choke point both +existing reconnect callers go through: + +- `InvokeSharedAsync`'s failure-recovery path (a tool call whose function + invocation throws attempts `TryReconnectAsync` before surfacing the + error) — during shutdown, the exception it's recovering from may be the + very disposal this change introduces; without the guard, that recovery + path would spin up a brand-new child process seconds before the daemon + exits. +- `McpReconnectionService`'s periodic poll — already able to run for the + full drain window today (see Context); the guard closes this off as a + side effect without requiring any change to that service itself, keeping + this change scoped to `McpClientManager`. + +Alternative considered: only guard `McpReconnectionService` (e.g., stop its +background loop from the same `ApplicationStopping` hook). Rejected — that +leaves the tool-call retry path unguarded, and duplicates the "are we +shutting down" signal into a second place instead of one shared flag in the +component that already owns client lifecycle. + +### Parallel per-server teardown + +`TeardownAsync()` disposes all clients via a `Task.WhenAll` projection +instead of the current sequential `foreach` + `await`. Each server's dispose +keeps its own try/catch so one server's disposal failure doesn't prevent or +delay others' (matching today's per-iteration try/catch, just no longer +serialized). With `N` configured servers this bounds total MCP teardown +latency at roughly the slowest single dispose (~20s worst case) instead of +`N × 20s` — meaningful once `ApplicationStopping` makes MCP teardown a +concurrent, budget-competing activity alongside session drain rather than an +afterthought that ran once the drain had already finished. + +Alternative considered: leave teardown sequential since it now starts +earlier and usually has more elapsed time to work with. Rejected — "usually" +is exactly the kind of soft guarantee the #1664/#1665 postmortems warned +against; a deployment with several configured MCP servers and a slow drain +could still exhaust the remaining budget under sequential disposal, and +parallelizing is a small, low-risk, independently testable change. + +### In-flight tool call failure path: no new plumbing required + +`McpToolAdapter.ExecuteAsync` already wraps `_invoker.InvokeAsync(...)` in a +try/catch that converts any exception into +`"Error: MCP tool '{Name}' failed: {ex.Message}"` — a clean, attributed tool +result, not a hang or an unhandled fault. When teardown disposes a client +that has an in-flight call, the SDK's own graceful-then-kill dispose timeline +determines how soon that call's underlying transport read/write faults; that +fault propagates through `InvokeSharedAsync` → `McpToolAdapter.ExecuteAsync` +into the existing clean-error path unchanged. This satisfies the "no silent +fallback" bar (the session sees a loud, attributed error) without adding any +new exception handling. The bound on "how soon" is inherited from the +existing per-client SDK dispose timeout, not newly introduced by this +change — see Risks for why that upper bound (not the guaranteed lower bound) +is what we can actually promise here. + +### Force-kill orphan gap: not solved from inside the daemon + +The `netclaw daemon stop` force-kill path (#1665's `CliForceKillBudget` +escalation) sends `SIGKILL` to the main daemon PID from the CLI process. A +`SIGKILL` is not observable by the target process — nothing inside the +daemon, including this change's `ApplicationStopping` hook, ever runs, because +`ApplicationStopping` requires a graceful stop signal (SIGTERM or an explicit +`StopApplication()` call) to reach the host in the first place. The only +things that can reap the orphaned MCP child tree in that case are external to +the daemon process: systemd's cgroup-wide kill on unit stop (already covers +systemd-managed installs), or an external supervisor doing the same. In a +non-systemd deployment (bare `netclawd`, container entrypoint without a +process-group reaper, manual dev shell), a force-killed daemon still orphans +its MCP children after this change, exactly as it does today. This change +narrows the *window* in which force-kill is reached at all (graceful teardown +now finishes sooner, so fewer stops escalate to force-kill in practice) but +does not close the gap once force-kill happens. CLI-side child-PID tracking +(the CLI recording and killing MCP child PIDs itself) could close this +residual gap; it is out of scope here per the proposal and noted for future +work. + +## Risks / Trade-offs + +- **In-flight call failure timing is inherited, not new** → We bound "when + does an in-flight call fail" by the same SDK dispose timeout that already + exists (10s graceful + 10s kill per server), not something this change + invents. If that upper bound proves too slow in practice (e.g., a session + actor's own turn timeout is shorter and fires first, which is fine and + independent), tightening it is a follow-up, not part of this change. +- **Idempotency race is real, not hypothetical** → `ApplicationStopping` and + the host's normal hosted-service stop sequence can both reach + `McpClientManager` in a short window on a fast shutdown. The memoized-task + design (see Decisions) is chosen specifically so both paths converge on one + completion instead of a bare double-dispose or a skip that returns before + work is done. This is exactly the kind of concurrency detail that should be + scrutinized in review and covered by a real (not simulated) double-teardown + test using `McpSmokeHarness`'s spawned child processes. +- **Guarding reconnect could mask a legitimate late reconnect need** → + Once teardown starts there is no legitimate reason to reconnect (the daemon + is exiting), so returning `false`/failing fast is correct; the risk is + narrow (making sure the guard only engages once teardown has actually + begun, not preemptively during normal operation). +- **Residual non-systemd force-kill orphan gap remains** → Documented above + rather than glossed over; systemd-managed installs are covered by the + existing cgroup sweep, and this change reduces how often force-kill is + reached, but does not eliminate the gap for non-systemd deployments. + CLI-side child-PID tracking is the follow-up that would close it, called + out as future work, not silently implied as solved. + +## Migration Plan + +No configuration or persisted-state migration. Deploying this change moves +*when* MCP teardown starts and *how* per-server disposal is scheduled; +external behavior (schema, tool names, grants) is unchanged. Rollback +restores the prior strictly-sequenced, sequential-per-server teardown with no +data implications either direction. + +## Open Questions + +None for this change. diff --git a/openspec/changes/mcp-shutdown-teardown/proposal.md b/openspec/changes/mcp-shutdown-teardown/proposal.md new file mode 100644 index 000000000..14750478d --- /dev/null +++ b/openspec/changes/mcp-shutdown-teardown/proposal.md @@ -0,0 +1,94 @@ +## Why + +Stdio MCP child processes are only torn down by `McpClientManager.StopAsync` +(`src/Netclaw.Daemon/Mcp/McpClientManager.cs:73-90`), which is registered as an +`IHostedService` before `AddAkka`. .NET's LIFO hosted-service stop ordering +means `AkkaHostedService.StopAsync` runs first, and it awaits +`CoordinatedShutdown.Run()` to full completion while ignoring the host's +shutdown `CancellationToken` (`Akka.Hosting.AkkaHostedService.StopAsync`). +Session drain (`SessionDrainHelper.DrainAsync`, bounded to +`DaemonConfig.BoundedDrainTimeout` = 190s by #1673) therefore consumes nearly +all of the shutdown budget before `McpClientManager.StopAsync` gets to run at +all — and today's per-server teardown there is sequential (10s graceful + 10s +kill wait, per configured server), so with multiple servers it can still +overrun whatever budget remains. The CLI force-kill path +(`netclaw daemon stop`, #1673) SIGKILLs only the main daemon PID, so +`McpClientManager.StopAsync`/`Dispose` never runs at all on that path — +orphaning the MCP child tree in any non-systemd deployment (container +entrypoint, manual `netclawd`, dev shell). Production evidence: idle +Playwright MCP children observed still alive 90+ seconds into a drain, +reaped only by systemd's cgroup sweep after the main PID was force-killed. + +#1673 fixed the two race conditions in the session-drain/CLI-kill budget +layering (#1664, #1665) but did not touch MCP teardown sequencing or ordering +— it is a distinct defect, tracked as #1667. The prior MCP ownership change +(#1636, `2026-07-14-make-stdio-mcp-process-bound`) explicitly preserved +shutdown behavior as out of scope; this change is the first to address it. + +Source issue: netclaw-dev/netclaw#1667. Related: #1664, #1665, #1636. + +## What Changes + +- Trigger MCP client/child-process teardown from + `IHostApplicationLifetime.ApplicationStopping` (fires before any + `IHostedService.StopAsync`, including `AkkaHostedService`'s), so MCP + teardown runs concurrently with session drain instead of strictly after it. +- Make `McpClientManager` teardown idempotent: the `ApplicationStopping` + callback and the existing `StopAsync`/`Dispose` path converge on one + teardown operation; whichever runs second observes already-disposed state + and does not log spurious warnings or attempt to reconnect. +- Guard reconnect attempts (`ConnectAsync`/`TryReconnectAsync`) once teardown + has started, so neither an in-flight tool call's retry-after-failure path + nor `McpReconnectionService`'s periodic background poll can launch a new + MCP child process during shutdown. +- Tear down configured servers in parallel instead of today's sequential + `foreach`, so total MCP teardown time is bounded by the slowest single + server's dispose instead of the sum across all configured servers. +- Preserve the existing dispose behavior per client (graceful wait, then + process-tree kill) and the existing clean-tool-error path + (`McpToolAdapter.ExecuteAsync` already converts any invoker exception into + `"Error: MCP tool '{Name}' failed: {ex.Message}"`) for calls in flight when + teardown begins. + +Out of scope: lazy MCP startup, idle-process teardown, per-session MCP +lifecycle, CLI-side child-PID tracking or cleanup (the CLI force-kill path +still cannot reach children of an already-SIGKILLed main PID from outside the +process; see design.md for why this residual gap is not solved here), and any +change to the per-client SDK dispose timeout or process-tree kill mechanism. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `netclaw-mcp`: the "daemon shutdown owns local child cleanup" behavior + changes from "runs after the full actor-system shutdown completes" to + "starts at `ApplicationStopping`, runs concurrently with session drain, is + idempotent, and does not reconnect once started"; per-server teardown + changes from sequential to parallel. + +## Impact + +- Code: `McpClientManager` gains an `ApplicationStopping` hook, an + idempotency guard shared with `StopAsync`/`Dispose`, a stopping flag + consulted by `ConnectAsync`/`TryReconnectAsync`, and parallel (rather than + sequential) per-server disposal. `Program.cs`'s MCP service registration + gains the lifecycle wiring; no hosted-service registration order changes + are required (ordering was never the fix — the new trigger is). +- Tests: MCP teardown tests exercising real spawned child processes + (`McpSmokeHarness`, extending `McpProcessBoundStdioTests.cs`) prove + idempotent double-teardown, no reconnect after teardown starts, and + parallel (not summed) multi-server teardown latency. +- Security: no ACL/policy change. An in-flight MCP tool call that loses its + transport during shutdown fails loudly and immediately as a clean tool + error, rather than hanging — this is a desired, documented behavior change, + not a silent degradation. +- Operations: MCP children are reclaimed sooner during a graceful `daemon + stop`/SIGTERM, reducing (but not eliminating, on non-systemd deployments) + the window in which a force-kill of the main PID orphans MCP child + processes. +- Configuration/schema: unchanged. +- Dependencies and public APIs: unchanged. diff --git a/openspec/changes/mcp-shutdown-teardown/specs/netclaw-mcp/spec.md b/openspec/changes/mcp-shutdown-teardown/specs/netclaw-mcp/spec.md new file mode 100644 index 000000000..55d4502a9 --- /dev/null +++ b/openspec/changes/mcp-shutdown-teardown/specs/netclaw-mcp/spec.md @@ -0,0 +1,55 @@ +## MODIFIED Requirements + +### Requirement: Configured MCP server has daemon-bound client ownership + +The system SHALL maintain at most one live MCP client connection for each enabled configured MCP server within a daemon process. For a local STDIO server, that connection SHALL own the server child process and SHALL be shared by every Netclaw session authorized to invoke the server. + +#### Scenario: Different sessions invoke one local STDIO server + +- **GIVEN** a local STDIO MCP server is enabled and available to two authorized sessions +- **WHEN** both sessions invoke tools from that server +- **THEN** both invocations use the same configured MCP client connection +- **AND** Netclaw does not launch a child process for either session identity + +#### Scenario: Session identity does not partition MCP state + +- **GIVEN** an authorized session changes state held by an MCP server +- **WHEN** another authorized session invokes that server +- **THEN** the second invocation uses the same daemon-scoped server state + +#### Scenario: Daemon shutdown starts child cleanup before session drain completes + +- **GIVEN** one or more enabled local STDIO MCP servers are connected +- **WHEN** the Netclaw daemon begins graceful shutdown (SIGTERM or `daemon stop`) +- **THEN** Netclaw begins disposing configured MCP clients at the point the host signals application stop, without waiting for actor-system shutdown or session drain to complete +- **AND** each disposed client's transport terminates its owned child process +- **AND** MCP teardown and session drain proceed concurrently rather than one strictly after the other + +#### Scenario: MCP teardown across multiple servers runs in parallel + +- **GIVEN** more than one enabled local STDIO MCP server is connected +- **WHEN** the Netclaw daemon begins graceful shutdown +- **THEN** Netclaw disposes all configured MCP clients concurrently +- **AND** total MCP teardown time is bounded by the slowest single server's dispose, not the sum across all configured servers + +#### Scenario: Teardown is idempotent across shutdown paths + +- **GIVEN** MCP teardown has already run once during daemon shutdown +- **WHEN** the host's normal hosted-service stop sequence subsequently invokes MCP shutdown again +- **THEN** the repeated teardown observes already-disposed clients +- **AND** it does not log a warning or error for the already-disposed state +- **AND** it does not attempt to reconnect or relaunch a child process + +#### Scenario: No reconnect once teardown has started + +- **GIVEN** MCP daemon shutdown has begun and client teardown is in progress or complete +- **WHEN** an in-flight tool call's failure-recovery path, or the periodic MCP reconnection check, attempts to reconnect to a configured server +- **THEN** Netclaw does not create a new client connection or launch a new child process +- **AND** the reconnect attempt reports failure without side effects + +#### Scenario: In-flight tool call fails cleanly when teardown begins + +- **GIVEN** a session has an MCP tool call in flight against a local STDIO server +- **WHEN** daemon shutdown begins and that server's client is disposed +- **THEN** the in-flight call fails within the same bounded time as the client's own dispose +- **AND** the caller receives an attributed tool error rather than an indefinite hang diff --git a/openspec/changes/mcp-shutdown-teardown/tasks.md b/openspec/changes/mcp-shutdown-teardown/tasks.md new file mode 100644 index 000000000..326854536 --- /dev/null +++ b/openspec/changes/mcp-shutdown-teardown/tasks.md @@ -0,0 +1,20 @@ +## 1. Decouple MCP Teardown From Actor-System Shutdown + +- [x] 1.1 Add an `IHostApplicationLifetime.ApplicationStopping` hook that triggers MCP client/child-process teardown independent of `IHostedService` stop ordering. +- [x] 1.2 Refactor `McpClientManager` teardown into a single memoized `TeardownAsync()` operation shared by the `ApplicationStopping` callback, `StopAsync`, and `Dispose`, so a second caller awaits the same completed work instead of re-disposing or racing it. +- [x] 1.3 Add a `_stopping` flag set synchronously when teardown begins, consulted by `ConnectAsync` and `TryReconnectAsync` so neither the tool-call retry-after-failure path nor `McpReconnectionService`'s periodic poll can create a new client or child process once teardown has started. +- [x] 1.4 Change per-server disposal in `TeardownAsync()` from sequential `foreach` + `await` to a parallel `Task.WhenAll` projection, preserving today's per-server try/catch so one server's disposal failure doesn't block or delay others. + +## 2. Automated Proof + +- [x] 2.1 Extend `McpProcessBoundStdioTests.cs` (using the real-process `McpSmokeHarness`) with a test proving double-teardown (`ApplicationStopping` hook followed by `StopAsync`) disposes the underlying child process exactly once, without a spurious warning on the second call. (Landed as a new sibling file `McpShutdownTeardownTests.cs` reusing `McpSmokeHarness`, rather than growing the unrelated process-ownership test file — also exercises a third entry point, `Dispose()`, in the same test.) +- [x] 2.2 Add a test proving that once teardown has started, a subsequent `TryReconnectAsync` call does not launch a new child process. +- [x] 2.3 Add a test proving multiple configured servers tear down concurrently: total teardown wall-clock time is bounded by the slowest single server's dispose, not the sum across servers. +- [x] 2.4 Add a test proving an in-flight tool call against a server whose client is disposed mid-call surfaces a clean, attributed tool error (via the existing `McpToolAdapter.ExecuteAsync` catch path) rather than hanging past the client's own dispose timeout. +- [x] 2.5 Run targeted MCP tests and the full `Netclaw.Daemon.Tests` project. + +## 3. Guidance and Quality Gates + +- [x] 3.1 Update `netclaw-operations` guidance to note that MCP child-process teardown now starts at application-stopping (concurrent with session drain) rather than after actor-system shutdown, and that in-flight MCP tool calls fail fast during shutdown; bump the skill version. +- [x] 3.2 Confirm the eval suite is not applicable because this change does not alter production tool definitions, skill matching, prompts, or model behavior. +- [x] 3.3 Run OpenSpec validation, Slopwatch, file-header verification, and `git diff --check`. diff --git a/src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs b/src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs index 028adce84..4e36ac95b 100644 --- a/src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs +++ b/src/Netclaw.Daemon.Tests/Gateway/DaemonRuntimeStatusServiceTests.cs @@ -6,6 +6,7 @@ using System.Net; using Microsoft.Extensions.AI; using Microsoft.Data.Sqlite; +using Microsoft.Extensions.Hosting.Internal; using Microsoft.Extensions.Options; using Microsoft.Extensions.Logging.Abstractions; using Netclaw.Actors.Channels; @@ -298,7 +299,8 @@ public async Task IncludesMcpConnectorHealthFromRuntimeStatuses() oauthService, NullNotificationSink.Instance, TimeProvider.System, - NullLogger.Instance); + NullLogger.Instance, + new ApplicationLifetime(NullLogger.Instance)); await manager.StartAsync(CancellationToken.None); try diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs index 27ce28cb0..8d7925a89 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpEndpointRouteBuilderExtensionsTests.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -88,7 +89,8 @@ private async Task CreateAppAsync( oauthService, NullNotificationSink.Instance, TimeProvider.System, - NullLogger.Instance); + NullLogger.Instance, + new ApplicationLifetime(NullLogger.Instance)); builder.Services.AddSingleton(oauthService); builder.Services.AddSingleton(mcpManager); diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthHeaderConflictTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthHeaderConflictTests.cs index c4b2abafb..b3e392195 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpOAuthHeaderConflictTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpOAuthHeaderConflictTests.cs @@ -8,6 +8,7 @@ using System.Reflection; using System.Text; using System.Text.Json; +using Microsoft.Extensions.Hosting.Internal; using Microsoft.Extensions.Logging.Abstractions; using Netclaw.Actors.Tools; using Netclaw.Configuration; @@ -223,7 +224,8 @@ private McpClientManager CreateManager( oauthService, NullNotificationSink.Instance, TimeProvider.System, - NullLogger.Instance); + NullLogger.Instance, + new ApplicationLifetime(NullLogger.Instance)); } private static HttpClient CreateDiscoveryClientThatReturnsOAuthHints() diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpShutdownTeardownTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpShutdownTeardownTests.cs new file mode 100644 index 000000000..fdf145798 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Mcp/McpShutdownTeardownTests.cs @@ -0,0 +1,252 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Diagnostics; +using System.Text.Json; +using Microsoft.Extensions.Logging; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Daemon.Mcp; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Daemon.Tests.Mcp; + +/// +/// Real-process proofs for the mcp-shutdown-teardown change +/// (openspec/changes/mcp-shutdown-teardown, GitHub #1667): idempotent +/// double-teardown, no reconnect once teardown has started, parallel +/// multi-server teardown, and clean failure for a tool call in flight when +/// its client is disposed. Uses 's real spawned +/// child MCP server processes throughout — no faked transports or clients. +/// +public sealed class McpShutdownTeardownTests +{ + [Fact] + public async Task DoubleTeardown_DisposesChildProcessExactlyOnce_WithoutSpuriousWarnings() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var recordingLogger = new RecordingLogger(); + await using var harness = McpSmokeHarness.Create( + new Dictionary { ["teardown_probe"] = CreateEntry() }, + new ToolRegistry(), + recordingLogger); + + await harness.Manager.StartAsync(cts.Token); + + var info = await GetProcessInfoAsync(harness, "teardown_probe", "slack/channel/thread", cts.Token); + using var process = Process.GetProcessById(info.ProcessId); + + // Simulate the real shutdown sequence this change introduces: + // ApplicationStopping (the new hook, registered in StartAsync) + // fires first, then the host's normal IHostedService.StopAsync + // runs afterward regardless of which one actually did the work. + harness.AppLifetime.StopApplication(); + await harness.Manager.StopAsync(cts.Token); + + // A third entry point — IDisposable.Dispose, e.g. DI container + // teardown — must also converge on the same completed work rather + // than redoing (or racing) disposal. + harness.Manager.Dispose(); + + await process.WaitForExitAsync(cts.Token); + Assert.True(process.HasExited); + + // The success log fires exactly once — by whichever of the three + // callers actually did the work — never once per caller. + Assert.Single(recordingLogger.Entries, e => e.Level == LogLevel.Information + && e.Message.Contains("MCP clients shut down", StringComparison.Ordinal)); + + // The second and third entries observe already-disposed state and + // must not log a warning or error for it. + Assert.DoesNotContain(recordingLogger.Entries, e => e.Level is LogLevel.Warning or LogLevel.Error); + } + + [Fact] + public async Task TryReconnectAsync_AfterTeardownStarted_ReturnsFalseAndCreatesNoNewClient() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var serverName = new McpServerName("teardown_probe"); + await using var harness = McpSmokeHarness.Create( + new Dictionary { [serverName.Value] = CreateEntry() }, + new ToolRegistry()); + + await harness.Manager.StartAsync(cts.Token); + + var originalClient = harness.Manager.GetClient(serverName); + Assert.NotNull(originalClient); + + // ApplicationLifetime.StopApplication() cancels the ApplicationStopping + // token synchronously on this thread, which synchronously invokes our + // registered callback and sets McpClientManager's _stopping flag — + // all before this call returns. There is no race window to wait out + // before the next line: TryReconnectAsync is guaranteed to observe + // _stopping == true. + harness.AppLifetime.StopApplication(); + + var reconnected = await harness.Manager.TryReconnectAsync(serverName, cts.Token); + Assert.False(reconnected); + + // Had the guard not engaged, TryReconnectAsync would have removed + // the original client, spawned a brand-new child process, and + // installed a new McpClient in its place. Instead the slot is + // either untouched (the background teardown triggered by + // StopApplication() hasn't reached it yet) or cleared to null by + // that same teardown — never replaced with something new. Since a + // new child process can only come into existence via a new McpClient + // (ConnectAsync creates the transport — and its process — before + // installing the client), this is a direct proof no process spawned. + var clientAfterReconnectAttempt = harness.Manager.GetClient(serverName); + Assert.True( + clientAfterReconnectAttempt is null || ReferenceEquals(clientAfterReconnectAttempt, originalClient), + "TryReconnectAsync must not install a new client once teardown has started."); + + await harness.Manager.StopAsync(cts.Token); + } + + [Fact] + public async Task ParallelTeardown_DisposesConfiguredServersConcurrently() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + await using var harness = McpSmokeHarness.Create( + new Dictionary + { + ["teardown_probe_a"] = CreateEntry(), + ["teardown_probe_b"] = CreateEntry(), + }, + new ToolRegistry()); + + await harness.Manager.StartAsync(cts.Token); + + // Each configured server's dispose is dominated by a real, fixed + // cost inherent to the MCP SDK, not something this test injects: + // StdioClientSessionTransport.CleanupAsync (see + // ModelContextProtocol.Client.StdioClientSessionTransport, v1.4.1) + // idly awaits the child process's own exit for up to + // StdioClientTransportOptions.ShutdownTimeout (hardcoded to 10s in + // McpClientManager.CreateTransport) before force-killing the + // process tree — the SDK never proactively closes the child's + // stdin first, so a healthy smoke-server process (which never + // exits on its own) reliably burns close to that full 10s per + // dispose. Two servers therefore give a real, deterministic + // ~10s-per-client cost: ~10s total if disposed in parallel, + // ~20s if disposed sequentially — a large enough gap that a + // generous bound comfortably distinguishes the two without + // flaking on a slow CI runner. + var stopwatch = Stopwatch.StartNew(); + await harness.Manager.StopAsync(cts.Token); + stopwatch.Stop(); + + Assert.True( + stopwatch.Elapsed < TimeSpan.FromSeconds(16), + $"Expected concurrent teardown of 2 servers to take roughly one ~10s dispose, not the ~20s sum of two sequential disposes. Actual: {stopwatch.Elapsed}."); + } + + [Fact] + public async Task InFlightToolCall_FailsCleanly_WhenTeardownDisposesItsClient() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var registry = new ToolRegistry(); + await using var harness = McpSmokeHarness.Create( + new Dictionary { ["teardown_probe"] = CreateEntry() }, + registry); + + await harness.Manager.StartAsync(cts.Token); + + // Go through the registered McpToolAdapter (not McpClientManager + // directly) so this test exercises the exact path a real session + // uses, including McpToolAdapter.ExecuteAsync's catch that formats + // any invoker exception into the clean "Error: MCP tool '...' + // failed" result. + var tool = registry.GetByName("teardown_probe/sleep"); + Assert.NotNull(tool); + + var invocation = TestToolExecutionContext + .CreateBound("slack/channel/thread", null, TrustAudience.Personal) + .Invocation; + + // 30s is far longer than this test needs to wait: McpSessionHandler + // cancels every outstanding request's TaskCompletionSource as the + // very first step of McpClient.DisposeAsync (before any process + // teardown), so the in-flight call fails within milliseconds of + // StopAsync starting — well before the tool's own 30s would elapse + // and well before the SDK's ~10s process-kill timeline. + var callTask = tool!.ExecuteAsync( + new Dictionary { ["ms"] = 30_000 }, invocation, cts.Token); + + // No delay before triggering teardown: ExecuteAsync's async call + // chain (McpToolAdapter -> McpClientManager -> the MCP SDK's + // SendRequestAsync) runs synchronously on this thread up to its + // first genuine suspension point — which is after the pending + // request is already registered — before control returns here. + await harness.Manager.StopAsync(cts.Token); + + var result = await callTask.WaitAsync(TimeSpan.FromSeconds(5), cts.Token); + Assert.StartsWith("Error: MCP tool 'teardown_probe/sleep' failed:", result, StringComparison.Ordinal); + } + + private static McpServerEntry CreateEntry() + => new() + { + Transport = "stdio", + Command = "dotnet", + Arguments = [SmokeMcpServerLocator.LocateDll()], + Enabled = true, + }; + + private static async Task GetProcessInfoAsync( + McpSmokeHarness harness, + string serverName, + string sessionId, + CancellationToken ct) + { + var result = await harness.Manager.InvokeAsync( + serverName, + "process-info", + null, + TestToolExecutionContext.CreateBound(sessionId, null, TrustAudience.Personal).Invocation, + ct); + + return JsonSerializer.Deserialize(result, JsonOptions)!; + } + + private static readonly JsonSerializerOptions JsonOptions = new() + { + PropertyNameCaseInsensitive = true, + }; + + private sealed record ProcessInfo(int ProcessId, string[] Arguments); + + /// + /// Captures every log call made through it so tests can assert on + /// message content and level without a mocking framework. Thread-safe: + /// parallel teardown logs from multiple concurrent dispose tasks. + /// + private sealed class RecordingLogger : ILogger + { + private readonly List<(LogLevel Level, string Message)> _entries = []; + + public IReadOnlyList<(LogLevel Level, string Message)> Entries + { + get { lock (_entries) return _entries.ToList(); } + } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + var message = formatter(state, exception); + lock (_entries) + _entries.Add((logLevel, message)); + } + } +} diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpSmokeHarness.cs b/src/Netclaw.Daemon.Tests/Mcp/McpSmokeHarness.cs index 75760d66d..476ae3789 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpSmokeHarness.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpSmokeHarness.cs @@ -3,6 +3,9 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Hosting.Internal; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Netclaw.Actors.Tools; using Netclaw.Configuration; @@ -24,18 +27,33 @@ internal sealed class McpSmokeHarness : IAsyncDisposable private readonly HttpClient _pkceHttp; private readonly HttpClient _oauthHttp; - private McpSmokeHarness(McpClientManager manager, HttpClient pkceHttp, HttpClient oauthHttp) + private McpSmokeHarness( + McpClientManager manager, + ApplicationLifetime appLifetime, + HttpClient pkceHttp, + HttpClient oauthHttp) { Manager = manager; + AppLifetime = appLifetime; _pkceHttp = pkceHttp; _oauthHttp = oauthHttp; } public McpClientManager Manager { get; } + /// + /// The real implementation backing + /// . Tests call + /// to fire ApplicationStopping synchronously, exactly as the + /// generic host does on SIGTERM/graceful stop, without needing a full + /// . + /// + public ApplicationLifetime AppLifetime { get; } + public static McpSmokeHarness Create( Dictionary serverEntries, - ToolRegistry registry) + ToolRegistry registry, + ILogger? logger = null) { var pkceHttp = new HttpClient(); var oauthHttp = new HttpClient(); @@ -46,6 +64,7 @@ public static McpSmokeHarness Create( NullLogger.Instance, new OAuthPkceService(pkceHttp), NullNotificationSink.Instance); + var appLifetime = new ApplicationLifetime(NullLogger.Instance); var manager = new McpClientManager( serverEntries, registry, @@ -53,8 +72,9 @@ public static McpSmokeHarness Create( oauthService, NullNotificationSink.Instance, TimeProvider.System, - NullLogger.Instance); - return new McpSmokeHarness(manager, pkceHttp, oauthHttp); + logger ?? NullLogger.Instance, + appLifetime); + return new McpSmokeHarness(manager, appLifetime, pkceHttp, oauthHttp); } public async ValueTask DisposeAsync() diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 7f0e3c08e..106a14b29 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -24,6 +24,7 @@ internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolIn private readonly IOperationalNotificationSink _notificationSink; private readonly TimeProvider _timeProvider; private readonly ILogger _logger; + private readonly IHostApplicationLifetime _appLifetime; private readonly int _maxToolDescriptionChars; private readonly int _maxToolSchemaWarnChars; @@ -33,6 +34,22 @@ internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolIn private readonly ConcurrentDictionary _statuses = new(); + // Guards the memoized teardown Task below. Teardown can be reached from + // three independent callers (the ApplicationStopping hook, the host's + // IHostedService.StopAsync, and IDisposable.Dispose) and must run its real + // work exactly once; every caller after the first converges on the same + // Task instead of re-disposing already-disposed clients or racing them. + private readonly object _teardownGate = new(); + private Task? _teardownTask; + + // Set synchronously (before any await) the moment teardown begins, so a + // racing ConnectAsync/TryReconnectAsync call started on another thread can + // observe it immediately and bail out before touching the transport or + // spawning a process. Volatile, not Interlocked-guarded state, is + // sufficient here: the flag is only ever written once (inside + // _teardownGate) and is read-only everywhere else. + private volatile bool _stopping; + public McpClientManager( Dictionary serverEntries, ToolRegistry toolRegistry, @@ -41,6 +58,7 @@ public McpClientManager( IOperationalNotificationSink notificationSink, TimeProvider timeProvider, ILogger logger, + IHostApplicationLifetime appLifetime, SessionConfig? sessionConfig = null) { _serverEntries = serverEntries; @@ -50,12 +68,23 @@ public McpClientManager( _notificationSink = notificationSink; _timeProvider = timeProvider; _logger = logger; + _appLifetime = appLifetime; _maxToolDescriptionChars = sessionConfig?.Tuning.MaxToolDescriptionChars ?? 0; _maxToolSchemaWarnChars = sessionConfig?.Tuning.MaxToolSchemaWarnChars ?? 0; } public async Task StartAsync(CancellationToken cancellationToken) { + // ApplicationStopping fires before any IHostedService.StopAsync runs — + // including AkkaHostedService's, which blocks on CoordinatedShutdown's + // full session-drain phase (up to DaemonConfig.BoundedDrainTimeout). + // Hooking here lets MCP child-process teardown start concurrently with + // session drain instead of waiting for it to finish first. StopAsync + // and Dispose route through the same memoized TeardownAsync() below as + // a backstop, so whichever of the three callers runs first does the + // real work and the others just await its completion. + _appLifetime.ApplicationStopping.Register(() => _ = TeardownAsync()); + foreach (var (name, entry) in _serverEntries) { var serverName = new McpServerName(name); @@ -70,23 +99,47 @@ public async Task StartAsync(CancellationToken cancellationToken) } } - public async Task StopAsync(CancellationToken cancellationToken) + public Task StopAsync(CancellationToken cancellationToken) => TeardownAsync(); + + /// + /// Returns the single, memoized teardown operation for this manager's + /// lifetime, creating it on first call. Sets + /// synchronously (inside the lock, before + /// reaches its first await) so a racing reconnect call started on another + /// thread cannot slip a new client past this point. + /// + private Task TeardownAsync() + { + lock (_teardownGate) + { + if (_teardownTask is not null) + return _teardownTask; + + _stopping = true; + _teardownTask = TeardownCoreAsync(); + return _teardownTask; + } + } + + private async Task TeardownCoreAsync() { - foreach (var (name, client) in _clients) + var disposals = _clients.ToArray().Select(async entry => { try { - await client.DisposeAsync(); - _logger.LogInformation("MCP client '{Name}' shut down", name.Value); + await entry.Value.DisposeAsync(); } catch (Exception ex) { - _logger.LogWarning(ex, "Error shutting down MCP client '{Name}'", name.Value); + _logger.LogWarning(ex, "Error shutting down MCP client '{Name}'", entry.Key.Value); } - } + }); + + await Task.WhenAll(disposals); _clients.Clear(); _sharedToolFunctions.Clear(); + _logger.LogInformation("MCP clients shut down"); } public McpClient? GetClient(McpServerName serverName) @@ -109,6 +162,14 @@ public IReadOnlyList GetToolNames(McpServerName serverName) public async Task TryReconnectAsync(McpServerName serverName, CancellationToken ct = default) { + // Once teardown has begun there is no legitimate reason to reconnect — + // the daemon is exiting. This is the single choke point both the + // tool-call retry-after-failure path (InvokeSharedAsync) and + // McpReconnectionService's periodic poll go through, so gating here + // closes off both without touching either caller. + if (_stopping) + return false; + if (!_serverEntries.TryGetValue(serverName.Value, out var entry) || !entry.Enabled) return false; @@ -206,6 +267,13 @@ private bool TryGetSharedFunction(McpServerName serverName, string toolName, out private async Task ConnectAsync(McpServerName name, McpServerEntry entry, CancellationToken ct) { + // Same shutdown guard as TryReconnectAsync: refuse to spawn a new + // child process once teardown has started. StartAsync's own initial + // connect loop always runs before _stopping can be set, so this only + // ever short-circuits reconnect-triggered calls. + if (_stopping) + return false; + // Holds the client until ownership passes to _clients. If the connect // fails after the client — and its child process — is created but // before that handoff (e.g. ListToolsAsync throws), the finally @@ -589,16 +657,19 @@ private void LogToolDrift(McpServerName serverName, IList discove } } + // Routes through the same memoized teardown as StopAsync/ApplicationStopping + // rather than its own separate synchronous dispose pass, so all three + // entry points converge on one real disposal (graceful wait, then + // process-tree kill) instead of Dispose racing a weaker, synchronous-only + // teardown against the other two. Blocking here is safe: by the time the + // generic host disposes this singleton, StopAsync has already run and the + // memoized task is normally already complete, making this a synchronous + // no-op in the common case (the backstop case is a caller that only ever + // calls Dispose(), e.g. a test harness or a crash path that never reached + // StopAsync). public void Dispose() { - foreach (var client in _clients.Values) - { - try { (client as IDisposable)?.Dispose(); } - catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client during shutdown"); } - } - - _clients.Clear(); - _sharedToolFunctions.Clear(); + TeardownAsync().GetAwaiter().GetResult(); } } diff --git a/tests/Netclaw.SmokeMcpServer/Program.cs b/tests/Netclaw.SmokeMcpServer/Program.cs index 840e73e11..21b212496 100644 --- a/tests/Netclaw.SmokeMcpServer/Program.cs +++ b/tests/Netclaw.SmokeMcpServer/Program.cs @@ -20,13 +20,16 @@ // echo(text) -> text // record-tasks(tasks, ref) -> a summary of the structured arguments // process-info() -> process ID and command-line arguments +// sleep(ms) -> waits ms milliseconds, then "awake" // // Determinism is the whole point: add(2, 2) is always 4, so a smoke // scenario can hard-assert on the tool RESULT even though the // surrounding LLM prose is random. `record-tasks` additionally gives // netclaw's schema-driven argument coercion a tool with an // array-of-objects parameter to exercise end to end (see -// SmokeMcpServerArgumentCoercionTests). +// SmokeMcpServerArgumentCoercionTests). `sleep` gives MCP teardown tests +// a way to hold a tool call in flight for a known duration (see +// McpShutdownTeardownTests). // // CRITICAL: stdout is the MCP JSON-RPC protocol channel. Nothing // other than protocol frames may be written to stdout. All @@ -87,6 +90,22 @@ public static string RecordTasks( return $"reference={reference} count={tasks.Length} kinds=[{kinds}]"; } + /// + /// Deterministic-duration tool used to hold a call in flight for teardown + /// tests: the caller knows exactly how long the server will keep the + /// request open, so a test can start this call, trigger client disposal + /// concurrently, and assert on how the in-flight call resolves without + /// racing on real elapsed time to decide whether it was "in flight." + /// + [McpServerTool(Name = "sleep")] + [Description("Sleeps for the given number of milliseconds, then returns 'awake'. Used to hold a call in flight for lifecycle tests.")] + public static async Task Sleep( + [Description("Milliseconds to sleep before returning.")] int ms) + { + await Task.Delay(ms); + return "awake"; + } + [McpServerTool(Name = "process-info")] [Description("Returns this server process ID and command-line arguments for lifecycle tests.")] public static string ProcessInfo() @@ -150,6 +169,7 @@ private static async Task RunStdioAsync() McpServerTool.Create(Echo, new McpServerToolCreateOptions { Name = "echo" }), McpServerTool.Create(RecordTasks, new McpServerToolCreateOptions { Name = "record-tasks" }), McpServerTool.Create(ProcessInfo, new McpServerToolCreateOptions { Name = "process-info" }), + McpServerTool.Create(Sleep, new McpServerToolCreateOptions { Name = "sleep" }), }; var options = new McpServerOptions diff --git a/tests/smoke/scenarios/mcp-setup.sh b/tests/smoke/scenarios/mcp-setup.sh index 5a5b16bd9..61c0b70ce 100755 --- a/tests/smoke/scenarios/mcp-setup.sh +++ b/tests/smoke/scenarios/mcp-setup.sh @@ -87,12 +87,12 @@ else die "daemon log: no 'MCP server ${MCP_SERVER_NAME} connected' line — stdio handshake failed" fi -# The test server exposes exactly four tools (add, echo, record-tasks, process-info) — -# confirm the daemon registered all of them. -if [[ "$connect_line" == *"(4 tools)"* ]]; then - pass "daemon log: MCP server registered 4 tools (add, echo, record-tasks, process-info)" +# The test server exposes exactly five tools (add, echo, record-tasks, +# process-info, sleep) — confirm the daemon registered all of them. +if [[ "$connect_line" == *"(5 tools)"* ]]; then + pass "daemon log: MCP server registered 5 tools (add, echo, record-tasks, process-info, sleep)" else - die "daemon log: expected '(4 tools)' in the connection line, got: $connect_line" + die "daemon log: expected '(5 tools)' in the connection line, got: $connect_line" fi summarize diff --git a/tests/smoke/screenshots/mcp-permissions-server-list.approved.png b/tests/smoke/screenshots/mcp-permissions-server-list.approved.png index 94100e399..46be08c5a 100644 Binary files a/tests/smoke/screenshots/mcp-permissions-server-list.approved.png and b/tests/smoke/screenshots/mcp-permissions-server-list.approved.png differ diff --git a/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png index c3db20a85..59077e5c5 100644 Binary files a/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png and b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png differ diff --git a/tests/smoke/tapes/screenshots/mcp-permissions.tape b/tests/smoke/tapes/screenshots/mcp-permissions.tape index 90e437fa0..b12c21023 100644 --- a/tests/smoke/tapes/screenshots/mcp-permissions.tape +++ b/tests/smoke/tapes/screenshots/mcp-permissions.tape @@ -7,10 +7,11 @@ # # Frames captured: # mcp-permissions-server-list — ServerList state: smoke-math appears as -# "Connected, 4 tools" below the header. +# "Connected, 5 tools" below the header. # mcp-permissions-tool-grid — ToolGrid state: header rows (Audience, # Server enabled, Server default) and all -# four tool rows (add, echo, record-tasks, process-info) +# five tool rows (add, echo, record-tasks, +# process-info, sleep) # simultaneously visible. This is the direct # regression check for issue #1424 — the # scroll container must not overwrite the header. @@ -37,7 +38,8 @@ Type "netclaw model set main smoke-llm qwen2:0.5b" Enter Wait+Screen@10s /TAPE\$/ -# Register the deterministic smoke MCP server (add, echo, record-tasks, process-info). +# Register the deterministic smoke MCP server (add, echo, record-tasks, +# process-info, sleep). # --grant-all means every tool is auto-approved for all audiences. Type "netclaw mcp add --transport stdio --grant-all smoke-math -- __NETCLAW_SMOKE_MCP_SERVER__" Enter @@ -48,12 +50,12 @@ Type "netclaw daemon start" Enter Wait+Screen@20s /TAPE\$/ -# Poll until the daemon reports smoke-math as fully connected with 4 tools. +# Poll until the daemon reports smoke-math as fully connected with 5 tools. # Replaces the former fixed Sleep 5s: we wait for the actual CLI signal # rather than guessing. `netclaw mcp list` queries the daemon's cached -# server state; "connected (4 tools)" means the stdio handshake and tool +# server state; "connected (5 tools)" means the stdio handshake and tool # indexing are complete. The Wait+Screen@60s is a safety net for a hard hang. -Type "until netclaw mcp list 2>/dev/null | grep -q 'connected (4 tools)'; do sleep 2; done" +Type "until netclaw mcp list 2>/dev/null | grep -q 'connected (5 tools)'; do sleep 2; done" Enter Wait+Screen@60s /TAPE\$/ Show @@ -63,26 +65,26 @@ Type "netclaw mcp permissions" Enter # ─── Frame 1: ServerList ───────────────────────────────────────────── -# smoke-math should appear as "Connected, 4 tools". Do NOT anchor on -# "smoke-math" or "4 tools" alone — both already sit in the shell +# smoke-math should appear as "Connected, 5 tools". Do NOT anchor on +# "smoke-math" or "5 tools" alone — both already sit in the shell # scrollback before the TUI ever paints: # - "smoke-math" appears in the setup output above ("Added MCP server # 'smoke-math' (stdio)" / "...adjust approvals for 'smoke-math'."). -# - "4 tools" appears in this tape's own typed readiness-loop command, -# still visible on screen: `... grep -q 'connected (4 tools)' ...`. +# - "5 tools" appears in this tape's own typed readiness-loop command, +# still visible on screen: `... grep -q 'connected (5 tools)' ...`. # Immediately after Enter, before the TUI switches to the alternate # screen buffer, Wait+Screen can match that leftover transcript text and # return instantly, capturing the raw shell instead of the rendered TUI # (README.md rule 5: anchor on the *next view*, not text that predates # it). Anchor on TUI-only chrome instead: "MCP Permissions" is the page # title from McpToolPermissionsPage.BuildHeader (proves the alt screen -# painted), and "Connected, 4 tools" is the exact rendered server-row +# painted), and "Connected, 5 tools" is the exact rendered server-row # text from McpToolPermissionsPage.BuildServerList — "{Name} ({Status}, # {ToolCount} tools)" with capital-C Status and a comma, which the -# transcript's lowercase, comma-less "connected (4 tools)" never +# transcript's lowercase, comma-less "connected (5 tools)" never # produces. Neither anchor occurs anywhere in the shell transcript. Wait+Screen@15s /MCP Permissions/ -Wait+Screen@5s /Connected, 4 tools/ +Wait+Screen@5s /Connected, 5 tools/ # Sleep 3s: let the server-list frame fully settle before capturing. The # first match of the anchors above can be a transient render; the # daemon may push a state update (re-index, status refresh) immediately @@ -98,7 +100,7 @@ Screenshot "/tmp/shot-mcp-permissions-server-list.png" # land on an empty server list (daemon cleared it mid-transition), causing # the TUI to navigate to a blank or non-existent tool grid. Wait+Screen@20s /smoke-math/ -Wait+Screen@10s /4 tools/ +Wait+Screen@10s /5 tools/ # Sleep 5s: extended settle guard (was 2s). Daemon MCP state updates can # arrive at any point; 5s provides substantially more headroom under CI # load where the re-index cycle can be slow. @@ -107,7 +109,8 @@ Enter # ─── Frame 2: ToolGrid ─────────────────────────────────────────────── # All header rows (Server, Audience, Server enabled, Server default) plus -# all tool rows (add, echo, record-tasks, process-info) must be visible simultaneously. +# all tool rows (add, echo, record-tasks, process-info, sleep) must be +# visible simultaneously. # If the #1424 regression reappears, tool rows will overwrite the header. # Timeout is 30s (was 15s) to give the tool grid more headroom to load # under CI load. /Server default:/ (with colon) matches the rendered