Bind stdio MCP server lifetime to the spawning client (fixes multi-GB orphan accumulation) - #266
Open
jungdaesuh wants to merge 4 commits into
Open
Bind stdio MCP server lifetime to the spawning client (fixes multi-GB orphan accumulation)#266jungdaesuh wants to merge 4 commits into
jungdaesuh wants to merge 4 commits into
Conversation
Stdio MCP servers had no exit path besides stdin EOF, but real clients abandon server instances without ever closing their pipes: startup-timeout retries and reconnects spawn a replacement fleet while the previous one is kept alive by the client's open pipe fds (observed live: one Codex session holding 28 gpd servers in 4 batches; 281 leaked processes / ~13 GB RSS on one host). Clients that die during the ~1 s server import leave instances that were never connected at all. Add a POSIX stdio lifecycle guard, auto-installed by run_mcp_server and wired into the arxiv bridge's custom entry point: - Reparent watchdog: a daemon thread exits the process when the spawning client dies, covering clients that never deliver EOF. An initial parent of pid 1 means the client died during startup, so exit immediately instead of guarding init/launchd forever. - Superseded-instance takeover: each instance records its pid keyed by (server, client pid); the replacement instance a client spawns verifies the recorded process is still that client's gpd server (ppid + command line via ps) and SIGTERMs it, erasing retry accumulation within seconds. All verification steps fail safe (skip, never kill) and the guard is a no-op on Windows and non-stdio transports. Regression tests cover takeover, non-gpd bystander safety, reparent exit, and the orphaned-at-startup race; the orphan test was proven to fail against the previous behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMCP stdio servers now install a POSIX lifecycle guard that records ownership, terminates superseded instances, and exits when the spawning client disappears. The generic runner and Arxiv bridge enable the guard, with regression tests covering lifecycle behavior and related test updates. ChangesMCP stdio lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant run_mcp_server
participant _install_stdio_lifecycle_guard
participant PID_directory
Client->>run_mcp_server: start with stdio transport
run_mcp_server->>_install_stdio_lifecycle_guard: install lifecycle guard
_install_stdio_lifecycle_guard->>PID_directory: record current PID
_install_stdio_lifecycle_guard->>Client: poll parent ownership
_install_stdio_lifecycle_guard->>run_mcp_server: force exit after client death or reparenting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gpd/mcp/servers/__init__.py`:
- Around line 211-237: The _terminate_superseded_instance transaction must be
serialized per server/client key to prevent concurrent retries from racing and
sharing the same staging file. Add a per-key interprocess lock around the
pid-file read, process verification/termination, and marker replacement, using a
unique lock path derived from the same key; ensure the lock covers the entire
takeover sequence and add coverage for concurrent contenders.
- Around line 192-208: The process check in _is_gpd_server_spawned_by must use a
stable, server-specific identity instead of the broad "gpd.mcp.servers"
substring; validate the exact invocation and process start token while retaining
the parent-PID check. In tests/mcp/test_server_regressions.py lines 838-847,
replace the fixture with a genuine matching-server process and add a bystander
process for a different GPD server, asserting only the matching server is
identified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1b7a4a1-8c2c-41f0-8c72-6d9899eb3a09
📒 Files selected for processing (3)
src/gpd/mcp/servers/__init__.pysrc/gpd/mcp/servers/arxiv_bridge.pytests/mcp/test_server_regressions.py
Review follow-ups from psi-oss#266: - Require the predecessor's command line to contain this server's own entry-point name (argv[0] stem) in addition to the package family marker and parent pid, so a recycled pid landing on the same client's *other* GPD server can never be misidentified and terminated. - Hold a per-(server, client) flock for the whole read-verify-kill-record sequence and stage the pid marker under a per-pid name, so concurrent replacements cannot interleave and leave a live instance unrecorded. Adds regression coverage: a different-GPD-server bystander must be spared, and four concurrent takeover contenders must serialize cleanly (contenders run from a script file — a -c source would put the family marker into their own command lines and make them take each other over legitimately). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/mcp/test_server_regressions.py (1)
879-910: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise actual takeover between matching contenders.
absent_tokenprevents every contender from matching its predecessor, and all processes exit immediately after printing. This only tests marker serialization—not that concurrent replacements terminate superseded instances and leave one live recorded server.Use a token matching
contender_script, keep contenders alive, then assert the marker PID survives while every other contender is SIGTERMed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp/test_server_regressions.py` around lines 879 - 910, Update test_lifecycle_guard_takeover_serializes_concurrent_contenders so each contender uses a token matching contender_script, remains alive after takeover, and can be observed for termination. After all contenders start, assert the PID recorded in the marker survives while every other contender is terminated by SIGTERM, while retaining the checks for successful execution and cleanup of staging files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/mcp/test_server_regressions.py`:
- Around line 879-910: Update
test_lifecycle_guard_takeover_serializes_concurrent_contenders so each contender
uses a token matching contender_script, remains alive after takeover, and can be
observed for termination. After all contenders start, assert the PID recorded in
the marker survives while every other contender is terminated by SIGTERM, while
retaining the checks for successful execution and cleanup of staging files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 854b8336-34c0-4a79-a0b8-0450df349bd2
📒 Files selected for processing (2)
src/gpd/mcp/servers/__init__.pytests/mcp/test_server_regressions.py
Two regressions introduced by the takeover-serialization change: - fcntl.flock had no failure handling, so filesystems where locking is unavailable (e.g. NFS-backed tmp) crashed the server at startup instead of degrading. Locking failure now skips the takeover quietly; the reparent watchdog still guards the instance. - The takeover blocks on the per-key lock, but ran before the watchdog thread existed: a starter waiting behind a hung lock holder had no client-death protection and could leak forever. The watchdog now starts first, so a blocked starter still exits when its client dies. Both regression tests were proven to fail against the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups from psi-oss#266, round two: - The invocation-token check was a substring match, so a server name that merely extends ours (state_server vs state_server_extra) could satisfy identity and be terminated. Each command-line word is now compared exactly against the token via its path basename and dot-split halves, covering console-script, dotted-module, and script-file invocations. Regression: a prefix-colliding bystander must be spared (proven to fail under substring matching). - The concurrent-contender test only proved serialization: its token matched nothing, so no takeover ever fired. Contenders now carry the family marker in their script path, use a token matching their own invocation, and stay alive after recording — the test asserts the chain terminates every superseded contender (SIGTERM) and exactly the recorded one survives. - Decoy fixtures use single-line -c sources: ps renders embedded newlines as non-printables, corrupting the marker word the exact matcher must see (the previous fixtures passed vacuously under exact matching). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
|
d7187fd addresses the remaining review items: exact entry-point identity matching (no substring/prefix admission, with a prefix-collision bystander regression proven to fail under substring matching), and the concurrent-contender test now exercises the real takeover chain — contenders match their own invocation, stay alive after recording, and the test asserts every superseded contender is SIGTERMed while exactly the recorded one survives. Full tests/mcp suite: 839 passed. |
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
2 participants
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.
Problem
Stdio MCP servers have exactly one exit signal: stdin EOF. Real MCP clients abandon server instances without ever closing their pipes:
lsof.Each abandoned instance idles at ~5 MB (macOS) to ~46 MB (Linux, after use). On one host this accumulated to 281 processes ≈ 13 GB RSS, which contributed to a machine-wide memory livelock — and the resulting slowdown pushed more server startups past client timeouts, spawning more retry batches. The leak feeds itself.
The servers themselves are well-behaved: with the current
mcpSDK they exit instantly on EOF. They are simply never told to die.Fix
A POSIX stdio lifecycle guard, auto-installed by
run_mcp_server(all 8 servers) and wired into the arxiv bridge's custom entry point:os._exit(0)s when the spawning client dies (5 s poll), covering clients that never deliver EOF. An initial parent of pid 1 means the client died during our startup → exit immediately instead of guarding init/launchd forever.(server name, client pid)in a per-user tmp dir; the replacement instance a client spawns verifies the recorded process is still that client's gpd server (ppid + command line viaps) and SIGTERMs it. Retry accumulation is erased within seconds instead of session-lifetime.Every verification step fails safe (skip, never kill). No config knobs. No-op on Windows and non-stdio transports; behavior for healthy client lifecycles is unchanged.
Verification
tests/mcp/suite: 834 passed.ruff check/ruff formatclean.Known limits (documented, judged not worth the complexity)
🤖 Generated with Claude Code
Summary by CodeRabbit