fix(mcp): un-invert the stdio children liveness check - #94339
Conversation
|
Verified end-to-end on Windows 11 (hermes-agent 0.20.5, commit bc47fcd, editable install). Reproduced the bug in a desktop-app CLI session with a stdio MCP server ( Verified this fix approach locally: applied the corrected liveness loop, and a fresh-session call to a real MCP tool returned correct results immediately, no other changes needed. The server itself was healthy throughout — a standalone NDJSON MCP probe completed initialize → tools/list → tools/call against the same subprocess while Hermes-side calls kept failing, confirming the inverted check was the only broken path. The regression tests cover the matrix that matters here: live → not dead, all-dead → dead, mixed → not dead, plus the no-pids/HTTP fail-open cases. This unblocks every stdio MCP server on affected installs — worth merging promptly. |
|
Thanks @tanao521 — this is a textbook end-to-end verification, and every data point maps directly onto what the fix changes:
For the maintainers: this independently confirms the fix on the platform where the issue was reported (Windows 11, editable install, npx-spawned stdio server) — beyond the mocked-subprocess tests, it exercises a real stdio MCP server through the desktop CLI session and shows the failure disappearing with only this change applied. As noted, the test matrix (live → not dead, all-dead → dead, mixed → not dead, plus the no-pids/HTTP fail-open cases) covers the decision table the old check got wrong. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head c1b39a86bad20786f135b7620f6b87784ea839e9 against current main regression #94637 and the newer duplicate #94661.
The implementation restores the documented decision table at the actual pre-call fast-fail boundary: no captured PIDs / HTTP transport => fail open; every tracked child dead => true; any live tracked child => false. The four focused regressions pin exactly those cases, and the exact head is hosted-green on CI 32798064054, Docker 32798063539, and Nix 32798063527. Independent Windows 11 end-to-end evidence on this PR additionally reproduced a live stdio child being misclassified dead and showed real tools/call recovery with only this polarity correction.
The later #94661 carries the same primary fix plus an ImportError/probe-error fail-open guard. psutil==7.2.2 is an exact core dependency on current main, so that extra guard is not required to close the released regression; it is a defensiveness extension, not a reason to keep two permanent owners. Preserve any unique test/provenance evidence from #94661, but keep this older fix as the canonical liveness-polarity owner and do not merge both implementations.
No blocker found on this head.
andrexibiza
left a comment
There was a problem hiding this comment.
Canonicalization review — keep #94339 as the sole runtime owner
I re-verified exact head c1b39a86bad20786f135b7620f6b87784ea839e9 against current main (1bbb6e5bce56e721ab685af4cd87df21bbff4d35) and the overlapping carriers #94521, #94586, and #94661.
The polarity repair here is the correct canonical implementation: one live tracked child means the aggregate is not dead. Do not merge four implementations of the same boolean/state-machine defect.
Before merge, absorb only the genuinely unique closure evidence from the siblings:
1. Watcher-consumer proof from #94521
The current suite pins _stdio_children_dead() directly, but the destructive consumer is _watch_stdio_children(), which races the RPC and cancels it when the aggregate verdict becomes true. Add deterministic tests proving both edges:
def test_watcher_does_not_resolve_while_a_child_is_alive():
async def _run():
with patch("psutil.pid_exists", return_value=True):
with pytest.raises(asyncio.TimeoutError):
await asyncio.wait_for(
_task_with_pids([60634])._watch_stdio_children(),
timeout=0.05,
)
asyncio.run(_run())
def test_watcher_resolves_when_all_children_are_dead():
async def _run():
with patch("psutil.pid_exists", return_value=False):
await asyncio.wait_for(
_task_with_pids([111, 222])._watch_stdio_children(),
timeout=0.1,
)
asyncio.run(_run())This preserves #94521's unique state-machine coverage without /bin/sh, real-process timing, or platform-specific fixtures.
2. Explicit unknown/fail-open proof from #94661
The helper documents unknown → don't fail fast, but this head still performs a bare dependency import and an unguarded probe. Dependency/probe failure is not evidence that every child exited and therefore cannot authorize the destructive fast-fail:
try:
import psutil
except ImportError:
return False # unknown → don't fail fast
for pid in pids:
try:
alive = psutil.pid_exists(pid)
except Exception:
return False # unknown → don't fail fast
if alive:
return False # at least one child alive → not all dead
return True # every tracked child has exitedAdd direct tests for missing psutil and a pid_exists() probe exception. The existing regression file will need asyncio, builtins, and pytest imports.
Carrier disposition
- #94339: canonical runtime owner; correct polarity and strongest focused base suite.
- #94521: duplicate implementation; unique watcher evidence distilled above.
- #94586: duplicate implementation; no unique regression evidence.
- #94661: duplicate implementation; unique dependency/probe fail-open evidence distilled above.
Please rebase this carrier onto current main, add only those four tests plus the local fail-open guard, and run the focused file plus adjacent MCP fast-fail/cancellation tests. Once that exact head is green, the other three carriers should remain closed as duplicates.
Contributor credit is preserved: watcher cases come from #94521; dependency/probe fail-open cases come from #94661. This remains one canonical fix for one aggregate-liveness defect.
Apply-ready absorption patch for exact head
|
|
Agreed — closing in favor of #94339 per the canonicalization. For the record, here are the two unique closures this carrier contributed, offered for absorption into #94339 before merge: 1. Unknown → fail-open contract ( try:
import psutil
except ImportError:
return False # unknown liveness → don't fail fast (best-effort contract)
for pid in self._stdio_child_pids:
try:
if psutil.pid_exists(pid):
return False # at least one child alive
except Exception:
return False # probe error → unknown → don't fail fast
return True2. Regression suite (7 cases, RED on pre-fix / GREEN post-fix, sabotage-verified): live pid → not all dead; all dead → fast-fail eligible; mixed live+dead; real spawned live child; psutil missing (ImportError → False); probe raises (→ False); stale pid list after child exit. Available at Happy to rebase/contribute these directly onto #94339 if that's easier. |
|
Verified the same fix on macOS (hermes-agent v2026.8.19-826, installed at Reproduced the exact failure mode after today's update: every Applied the same polarity change locally (live child → LGTM from a macOS production-install perspective. |
|
Independent real-world reproduction + fix verification (Windows 11, packaged desktop install,
One note for whoever merges: the same inverted block still exists verbatim on |
|
Confirming the issue and fix as others here |
Independent macOS fixture confirmationVerified on macOS 26.6.2 against the affected v0.20.5 source and upstream base
Also ran the canonical targeted suites at this PR head: 4/4 new liveness tests and 119/119 related MCP tests pass; Ruff passes for the changed files. This confirms the defect is the liveness-predicate polarity, not Streamable HTTP, |
|
Independent macOS/Moraine reproduction and control-path confirmation:
This independently confirms the PR fixes the exact discovery-green / first-call-fast-fail behavior on macOS arm64 with Moraine 0.6.4. |
|
This is an exemplary close-out — thank you for doing the work before the PR rather than after it. The state matrix (live / dead-Windows / dead-Linux-zombie / dead-Linux-reaped) covering every way the walk could differ, measured on both platforms, is exactly the evidence a "provably no-op" claim needs, and declining to spend your tested-by-author provenance on dead code in a core file is the right call. The coupled-lifetime observation on Windows (child holds the shim's stdio pipes) is a particularly useful detail — it closes the escape hatch in the failure direction too. One addition from my side that further supports your conclusion: since your earlier comment, upstream landed a spawn-ledger change ( Your re-open condition (a launcher topology where the real server survives the shim's exit AND remains reachable via the shim's PID) is well-framed — a pidfile/snapshot variant captured at spawn time would also slot naturally into the ledger if such an environment ever reports in. Either way, the try-import / fail-open baseline from this PR's head is the contract everything else builds on. |
|
Thanks — glad the evidence landed. The spawn-ledger piece ( With both halves in place (polarity fix here + spawn ledger upstream), the re-open condition for the walk becomes very narrow: a launcher topology where the real server outlives the shim and the shim's PID is still the only handle to it at check time. If that ever shows up in the wild, I'll bring a pidfile/snapshot variant that slots into the ledger. For now, the try-import / fail-open baseline is the contract, and the polarity fix is the full solution for the observed environment. No further action needed from my side. |
|
Confirmed and validated the fix end-to-end on macOS 26.6.1 with Hermes v0.20.5 (local HEAD Before the patch:
I then applied the current #94339 patch locally, including the fail-open handling for unavailable/failed PID probes. Verification:
One operational detail: a Desktop/serve process that had imported the old module continued to exhibit the old failure until process reload, while a fresh CLI cron process and restarted Gateway loaded the patch and succeeded. This is expected Python module lifetime behavior, but it explains why merely editing the file does not repair already-running sessions. This independently confirms that #94339 fixes the real long-lived cron/gateway use case on macOS, not only a unit-level or one-shot reproduction. |
|
@Stone441 — thank you, this is the second independent end-to-end confirmation (after @liuhao1024's Windows run), and the real-cron macOS data point is exactly what this fix needed: the cursor-advancing workflow through the agent-visible handlers, not One note on commit state: you tested Your module-lifetime observation is expected and consistent with what we saw in the Windows validation — an already-running process that imported the old module keeps the old behavior until reload; fresh processes pick up the patch. Good callout for anyone upgrading with a long-lived gateway. |
|
Independent confirmation, desktop-app scenario (complements the existing gateway confirmations). Environment: Windows 11, Hermes desktop app (git install, Repro: every real
Both server subprocesses stayed alive throughout ( Fix confirmed: applying the polarity change (alive child → Also worth noting: one of the two servers runs through a Windows uv-venv shim that re-execs into a uv-managed CPython, matching the Windows-specific descendant-walk concern already raised here — so that extension PR is genuinely needed, not just theoretical. Happy to share the local diff if useful. |
|
Thanks @C-Huai-Jie — the desktop-app data point is a useful complement to the gateway/oneshot confirmations: two unrelated stdio servers, identical fast-fail while both children were verifiably alive, and both recovered by the polarity change alone. On the descendant-walk point: your shim → uv CPython re-exec observation matches what was measured in this thread — the premise is real and was confirmed on the very gateway that hit the bug (comment 5434311617). The conclusion there was that the walk is a provably no-op on every reachable state:
Measured across the full live/dead/mixed matrix on Windows plus the zombie/reap matrix on Linux — every case returns the same verdict under both implementations. One more Windows-specific detail: the child holds the stdio pipes the shim owns, so the shim cannot exit while the real server keeps running — the "shim exits, server survives" scenario does not occur even in the failure direction. And since that measurement, upstream landed So the polarity fix in this PR is the complete fix for the topology you describe — no extension PR needed. If that narrow re-open condition ever shows up in the wild, a pidfile/snapshot variant would slot into the ledger. |
|
Merged with your commits preserved via rebase (merge commit ef46ec0). This was the canonical carrier for the stdio liveness inversion — the fail-open hardening (psutil/probe failures = unknown, never fast-fail) and the 8-test suite made it the clear winner over the later duplicates (#95947's MCP half closed pointing here). Thanks for the disciplined rebase and for shepherding the verification wave. |
|
Independent verification confirmed by @lilaflo on issue #95150 (v0.20.5, commit 786f370): after this fix, MCP server (stdio) tool registration appears at startup and all stdio MCP tool calls work end-to-end; before it, every call failed fast with TimeoutError despite healthy server processes. Thanks for the clean 2-line fix. |
…k probe The fast-fail gate probed the stdio child watcher by CALLING it — inspect.isawaitable(_watch_children()) — creating a fresh coroutine on every stdio MCP tool call that was never awaited (RuntimeWarning spam + gc churn). Inspect the function instead of invoking it. Salvaged (unique hunk only) from PR #96044; the bundled _stdio_children_dead polarity fix was already on main via #94339.
…k probe The fast-fail gate probed the stdio child watcher by CALLING it — inspect.isawaitable(_watch_children()) — creating a fresh coroutine on every stdio MCP tool call that was never awaited (RuntimeWarning spam + gc churn). Inspect the function instead of invoking it. Salvaged (unique hunk only) from PR NousResearch#96044; the bundled _stdio_children_dead polarity fix was already on main via NousResearch#94339.
What does this PR do?
_stdio_children_dead()returnedTrue("all children dead") on the first live pid — the intendedFalsesat dead-coded directly beneath it. In any spawn path that captures child PIDs (observed inhermes -zoneshots), the #81995 pre-call fast-fail then raisedTimeoutError: MCP stdio subprocess ... has exited; failing the call faston everytools/callwhile the subprocess was demonstrably alive (the reporter's child-watcher poll caught it running at the moment of failure). Long-lived gateway/dashboard sessions were unaffected only because_stdio_child_pidsstays empty there (theif not pidsshort-circuit) — which is also why production gateways never noticed.This PR restores the documented decision table at the fast-fail boundary — no captured PIDs / HTTP transport ⇒ fail open; every tracked child dead ⇒
True; any live tracked child ⇒False— and additionally fails open whenpsutilis unavailable or a PID probe raises, so unknown liveness can never authorize the destructive fast-fail. The destructive consumer_watch_stdio_children()stays pending while a child is alive and resolves once every tracked child has exited.Related Issue
Fixes #94335
Type of Change
Changes Made
tools/mcp_tool.py(MCPServerTask._stdio_children_dead) — restore the aggregate liveness polarity and fail open whenpsutilis unavailable or a PID probe raises; unknown liveness cannot authorize the pre-call fast-fail.tests/tools/test_mcp_stdio_children_dead.py— eight regressions covering live, all-dead, mixed, absent-PID/HTTP, missing-psutil, probe-error, watcher-live, and watcher-all-dead behavior.Provenance and Credit
How to Test
.venv/bin/python -m pytest tests/tools/test_mcp_stdio_children_dead.py -q8 passed. On unfixedmainthe same file reports5 failed, 3 passed— failing exactly the live-child, mixed-liveness, missing-psutil, probe-error, and watcher-live pins.test_mcp_bridge_single_failure.py,test_mcp_circuit_breaker.py,test_mcp_cancelled_error_propagation.py,test_mcp_failure_classification.py):30 passed.Checklist