Skip to content

fix(mcp): stop leaking an unawaited watcher coroutine per stdio tool call (salvage #96044) - #96251

Closed
kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/96044-watch-coro-leak
Closed

kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/96044-watch-coro-leak

Conversation

@kshitijk4poor

@kshitijk4poor kshitijk4poor commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

The stdio fast-fail gate no longer leaks an unawaited coroutine on every MCP stdio tool call: the _watch_ok probe inspected awaitability by calling the child watcher (inspect.isawaitable(_watch_children())), creating a fresh never-awaited coroutine per call ("coroutine was never awaited" RuntimeWarning spam + GC churn). It now inspects the function without invoking it.

Salvage of the unique hunk from #96044 by @loulanyue (authorship preserved). The other half of that PR — the _stdio_children_dead polarity fix — was already on main via #94339.

Changes

  • tools/mcp_tool.py (@loulanyue): probe _watch_children without calling it. Our follow-up tightens the replacement to inspect.iscoroutinefunction(_watch_children) alone — the originally proposed or callable(...) arm would have flipped MagicMock-stubbed sessions into the fast-fail race (callable(MagicMock) is True), breaking the plain-await path the surrounding comment routes stubs to.
  • 2 regression tests: source pin that the probe no longer calls the watcher (behavioral test infeasible — the leak only manifests as a GC-time RuntimeWarning), plus a semantic contract test (real async def / AsyncMock → race path, MagicMock → plain await).

Validation

tests/tools/test_mcp_stdio_children_dead.py + test_mcp_tool.py 109 passed
Mutation check (mcp_tool.py reverted to main) pin test fails → restored passes
Probe semantics (live) iscoroutinefunction: real async def True, AsyncMock True, MagicMock False
Behavior-delta audit only disagreement case (sync def returning awaitable) unreachable in-repo, degrades to plain await (safe)
Sibling sweep 25 other isawaitable() sites all probe already-produced values — this was the only call-to-probe
ruff clean

Credit

Coroutine-leak find and fix by @loulanyue in #96044. Closes #96044 (its polarity half was superseded by #94339).

First report of the leak: @glupta in #95938 (also reported independently by @ilgo in #96030). @liuhao1024 shipped an equivalent earlier fix in #95939 (create-once-and-reuse shape) — see that PR for the credit note on why this shape was preferred.

Closes #95938. Closes #96030. Refs #96016.

loulanyue and others added 2 commits August 27, 2026 15:12
…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.
…function

Follow-up to the salvaged NousResearch#96044 hunk: drop the 'or callable(...)' arm —
callable(MagicMock) is True, which would have flipped stubbed sessions
into the fast-fail race the surrounding comment explicitly routes to the
plain-await path. inspect.iscoroutinefunction alone reproduces the old
isawaitable(call) split exactly (real async def / AsyncMock -> race,
MagicMock -> plain await) without creating the leaked coroutine.
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 27, 2026 09:48
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth labels Aug 27, 2026
auto-merge was automatically disabled August 27, 2026 20:04

Pull request was closed

@kshitijk4poor kshitijk4poor reopened this Aug 27, 2026
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 27, 2026 20:04
novnski added a commit to novnski/hermes-agent that referenced this pull request Aug 28, 2026
Adopt reviewed ideas from upstream PRs NousResearch#96251, NousResearch#96498, NousResearch#96748, NousResearch#96884, and NousResearch#96885 with local integration tests.

Co-authored-by: loulanyue <260355617@qq.com>

Co-authored-by: 686f6c61 <github@00b.tech>

Co-authored-by: ahrazzle <ahraz.arifuddin@gmail.com>

Co-authored-by: Agi-Asi <Agi-Asi@users.noreply.github.com>
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

fix(mcp): stop leaking an unawaited watcher coroutine per stdio tool call — correct leak fix

  • Fix: tools/mcp_tool.py:6174 changes inspect.isawaitable(_watch_children())inspect.iscoroutinefunction(_watch_children). The old probe called the async watcher to test awaitability, creating a coroutine that was never awaited — RuntimeWarning: coroutine was never awaited under -W error and GC churn per fix(mcp): correct _stdio_children_dead logic and prevent coroutine leak (#96016, #96030) #96044.
  • Semantics preserved: test_mcp_stdio_children_dead.py:11-40 proves iscoroutinefunction splits the world identically for the fast-fail race: real async def and AsyncMockTrue, plain MagicMock (used in unit stubs) → False, so mocked watchers still take the plain-await branch without the leak.
  • Guard: source-level test asserts isawaitable(_watch_children()) no longer present and iscoroutinefunction(_watch_children) present — durable.

Non-blocking — minimal, precise.

bvisible added a commit to bvisible/hermes-agent that referenced this pull request Sep 1, 2026
…ssifier

'Bonjour Nora' cost two serial LLM round-trips (classify, then the light-path
completion): 2.8 s on a loaded Olares, measured 2026-09-01 07:49:29.39 →
07:49:32.15, before the desk even polled. A greeting, a thank-you, a 'how are
you' or a farewell has one right answer per language: say it from a template
(fr/de/it/en, vouvoiement), delivered exactly like the light path. Gate is
stricter than the light path's — the message must be small talk and nothing
else (residue ≤ 1 word), so 'Bonjour, qui es-tu ?' still gets the LLM.

Also: point the mcp_tool.py marker at upstream PR NousResearch#96251 (our NousResearch#100110 was
closed as its duplicate).
@teknium1

teknium1 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Landed on main via #100920 (rebase-merge, your commits preserved as-authored: fix by @loulanyue, tests by @kshitijk4poor — main fe3e5dc). The branch here had gone stale against main so it was re-based in a fresh PR rather than pushed to. Thanks for carrying the salvage.

@teknium1 teknium1 closed this Sep 2, 2026
auto-merge was automatically disabled September 2, 2026 05:11

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

5 participants