fix(mcp): drain the MCP loop and retain parked startup tasks for clean shutdown - #74139
Merged
kshitijk4poor merged 7 commits intoJul 29, 2026
Merged
Conversation
_stop_mcp_loop() stopped and closed the background loop without reaping
the tasks still on it. A task left suspended is resumed later by the GC,
whose finalizer drives its cleanup against the now-closed loop:
Exception ignored in: <coroutine object MCPServerTask.run ...>
File "tools/mcp_tool.py", line 2947, in run
parked = await self._wait_for_reconnect_or_shutdown(
File "tools/mcp_tool.py", line 2161, in _wait_for_reconnect_or_shutdown
t.cancel()
RuntimeError: Event loop is closed
shutdown_mcp_servers() only reaps servers held in _servers, so a server
that parked after exhausting its initial-connect budget — never inserted
there, because start() raises _error before the caller registers it — has
no owner to signal it and stays suspended until the loop is gone.
Drain the loop the way asyncio.run() does: cancel the remaining tasks and
gather them while the loop is still open, so each runs its own finally.
Cancel alone is not enough — Task.cancel() only schedules the throw.
This resolves the reported traceback, but not the ownership bug that
strands the task in the first place; that needs a follow-up. Deliberately
not using "Fixes" so NousResearch#60197 stays open for it.
Addresses NousResearch#60197
Addresses NousResearch#66113
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…reap Follow-up to the salvaged NousResearch#62026 ownership fix, folding in NousResearch#72054's CancelledError rule by @adurham: start() already cancels/reaps its own run task when the caller's connect timeout cancels start() itself, so _connect_server() must propagate cancellation without awaiting a redundant shutdown() inside a cancelled context. Non-cancellation failures on the unclaimed (standalone probe) path still reap the parked task, now with the reap failure logged instead of raising over the real error. Also maps mrz@mrzlab630.pw for the attribution check. Co-authored-by: Adam Durham <amdnative@gmail.com>
kshitijk4poor
enabled auto-merge (rebase)
July 29, 2026 14:56
kshitijk4poor
disabled auto-merge
July 29, 2026 14:57
This was referenced Jul 29, 2026
This was referenced Jul 30, 2026
This was referenced Jul 30, 2026
adurham
added a commit
to adurham/hermes-agent
that referenced
this pull request
Jul 31, 2026
…s on all 7 PRs Documents the real external engagement on the 7 upstream PRs filed 2026-07-26, and the 3 substantive follow-up fixes pushed in response: - NousResearch#72054 closed as superseded, but merged anyway via NousResearch#74139 (contributor CrowLoki's reconciliation with NousResearch#62026, credited via Co-authored-by). - NousResearch#72087, NousResearch#72151, NousResearch#72152, NousResearch#72153, NousResearch#72155, NousResearch#72164 all reviewed by the repo's automated sweeper -- keep_open/high on all 6. - Fixed NousResearch#72087 (payload-proportional test assertions, catching a future allowlist-regression risk flagged by both the sweeper and an independent contributor who measured it precisely on their own fork). - Fixed NousResearch#72152 (extracted ProfileRail's focus/visibilitychange wiring into a tested hook, matching the directory's own established use-profile-prewarm.ts pattern). - Rebased NousResearch#72155 past a real merge conflict (an unrelated upstream test-pruning pass removed 3 tests my diff's context touched). All fixes verified by simulating the exact regression each review was warning about and confirming the new tests catch it, then restoring the real fix. Also noted a real environment issue found this session: the `upstream` remote's SSH URL intermittently fails to connect from this network; a one-off HTTPS fetch into a separate ref works around it without touching the configured remote. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Ends the
RuntimeError: Event loop is closedshutdown noise from parked MCP tasks (#60197) by combining the two complementary community fixes: the MCP loop owner now drains pending tasks before closing the loop, and a server parked after a recoverable initial connect failure is retained in_serversso shutdown owns it and/mcp-refresh revival actually works.Root cause (per @shady2k's analysis on the issue): a server that exhausts
_MAX_INITIAL_CONNECT_RETRIESparks itsrun()task to self-probe, butstart()raises before_servers[name]is written — soshutdown_mcp_servers()never reaps the task,_stop_mcp_loop()closes the loop with the coroutine still suspended, and GC finalization drivest.cancel()into the closed loop. Under systemd this turns a clean SIGTERM stop into exit code 1 /failedunit state (see issue thread).What's salvaged
_stop_mcp_loop()schedules a single loop-owned_drain_and_stop_mcp_loop()that cancels and boundedly reaps pending tasks, then stops the loop from its own thread, so an outer timeout can't letloop.stop()overtake the drain. Direct, blocked-loop, and end-to-end regressions included._connect_serverclaim contextvar + adoption of recoverable parked startup tasks into_servers; standalone probe failures still reap locally. The_register_discovered_tools_if_needed()hunks were reconciled with main's106d1822e(kept main's_servers.get(self.name) is selfownership check, added the stale connect-error clear on successful publication).CancelledErrorrule from fix(mcp): reap orphaned parked server task in _connect_server #72054 by @adurham (start()reaps its own task on external cancellation; don't await a redundantshutdown()inside a cancelled context), logs orphan-reap failures, and replaces thenonlocalclaim rebind with a list-based claim to keep thetydiff clean.Validation
upstream/mainRuntimeError: Event loop is closed×2,Task was destroyed×3, server absent from_serversbroken-testretained in_servers, exit 0tests/tools/-k mcp, 812 tests)git diff --checkty check tools/mcp_tool.pyupstream/mainbaselineCredit
106d1822edone here.Co-authored-by.Closes #60197