Repository navigation
fix: update probe no longer orphans its process tree on timeout - #157
Merged
Merged
Conversation
) Sixth attempt. Research killed the naive form first: every manifest server launches via npx/uvx, which are EPHEMERAL RUNNERS -- 'npm ls @upstash/context7-mcp' returns (empty), so shelling out to npm ls / uv pip show would have silenced every notice, exactly the failure mode of attempt 4. The runner CACHES are queryable, and that is the source all five prior attempts lacked: each ~/.npm/_npx/<hash>/package.json names its package and node_modules holds the resolved version (verified: @brightdata/mcp 2.11.1, @eslint/mcp 0.3.8), and uv's environments-v2 envs carry .dist-info metadata. Bound to package identity by construction, unlike descriptions_cache.version (an upstream snapshot) or serverInfo.version (an implementation version). Cost measured: 0.36s for 114 npx entries, filesystem only, no subprocess. Rests on #155, which made the comparator fail closed.
Sampled six npx cache entries: each declares exactly ONE dependency (the invoked package) while node_modules holds the transitive closure -- 212 packages in one entry. So matching must be against 'dependencies'; matching node_modules would resolve any transitive package. Checked before writing code because it is the load-bearing assumption of the npm probe.
…AGREE) B1 (fatal): the plan kept descriptions_cache.version as a fallback while its own acceptance criteria required silence in that case. That value is an upstream snapshot (refresher.py:243), so identity agreement cannot make it an observed installed version -- the sixth attempt would still emit inferred notices. My proposed test specified 'no cache entry AND no orderable fallback', so it would have passed while the real regression went uncovered: the same test-shape failure as rounds 1-4 of PR #154. B2: neither probe binds a CONFIGURED SERVER to the cache entry it runs. Verified on this machine: the npx cache holds @consiliency/agent-board-mcp at 1.2.0, 1.2.1 AND 1.2.2 concurrently, so newest-by-mtime can pick an unrelated invocation -- including update_server's own pkg@latest probe. uv is worse: an env holds its full transitive closure (30 dist-infos observed) with no single-declared-dependency marker, so a name scan matches transitives. The caches do hold real installed versions -- that research stands. What they lack is server->entry provenance. Recorded three candidate directions.
…guard (#150) Seventh attempt. Maintainer decision after the runner-cache plan was rejected 3/3: record at spawn, plus a periodic guard so a long-running server does not keep a stale baseline forever. Spawn-time capture differs from all six prior attempts because provenance is by CONSTRUCTION rather than inference: at manager.py:1365 pmcp calls create_subprocess_exec with an argv it chose, so there is no scan, no mtime heuristic, and no ambiguity about which cache entry is in play. Attempt 6 failed precisely there -- verified the npx cache holds one package at 1.2.0, 1.2.1 and 1.2.2 concurrently. The maintainer's guard requirement is confirmed as a real gap: the existing hourly sweep re-checks UPSTREAM latest, but a spawn-time value is captured only at spawn, so a server left running for weeks keeps its baseline indefinitely. The guard extends the existing _stale_indexer_loop rather than adding new scheduling, and re-reads from disk without ever respawning a running server. Carries the board's B1 finding forward: NO fallback to descriptions_cache.version (an upstream snapshot), and the test for that case now includes an orderable fallback, which the last plan's test omitted.
B1 (fatal): 'argv is authoritative provenance' is false. create_subprocess_exec returns once NPX starts, before it has selected/fetched/launched the package, and npm may resolve to a local, global or cache-backed copy -- only one branch produces an _npx dir. Verified independently before the verdicts: a live 'npx -y @eslint/mcp' tree exposes NO _npx path in any descendant's cmdline, cwd or fds. Knowing the argv tells pmcp what it ASKED FOR, not what npm CHOSE. B2: the plan changed only the 'current' source. Both notice writers still take 'latest' from _get_server_config_for_update, which prefers the manifest and ignores the .mcp.json override -- so B's installed version can still be compared against A's upstream latest, which is the original #150 defect intact. Seven attempts have now failed at the same joint: pmcp cannot observe which package artifact is actually executing for a running server.
Maintainer decision after the spawn-provenance plan was rejected. Running the
package's own --version is the only method that observes the artifact that
actually runs, because the artifact answers for itself.
Measured against 12 real manifest servers before planning, not assumed:
6/12 (50%) parsed a version
@circleci/mcp-server-circleci IGNORED the flag and hit the 90s cap
@eslint/mcp printed 'server is running' and EXITED 0 -- exit status is not a
usability signal
firecrawl-mcp emitted only npm noise containing koa-router@14.0.0, exactly
what a permissive regex would grab as a version
Strict whole-line matching verified to extract 4.0.2 and 0.0.79 while rejecting
both bad cases. Median latency 5s, max 90s -> a 20s timeout and bounded
concurrency are load-bearing, and ~390s serial across 98 servers makes caching
required rather than an optimisation.
Carries both prior board findings forward: no fallback to
descriptions_cache.version (B1), and 'latest' must come from the same effective
config as the probe so an override cannot be compared against the manifest
package's latest (B2).
Open question recorded for the board: the probe STARTS the package, so a server
that ignores --version briefly runs with whatever side effects it has at
startup. If that is unacceptable the honest conclusion may be to remove the
feature rather than make it 50% correct.
…ockets Probing @circleci/mcp-server-circleci --version (a server that ignores the flag) spawns 7 processes and opens 5 network sockets, sustained until killed. That is a real server starting and connecting outward, caused by an advisory update check, for a server that returns no version anyway. @eslint/mcp by contrast exits before 10s, so its cost is bounded. Recorded because it is the strongest argument against this approach and the maintainer asked for the board's view on exactly this point.
…feature Both readable legs answered the disqualifying question directly. grok: 'Automatic --version probes are disqualifying. Remove the automatic notices rather than ship a 50%-correct scheduled spawn.' codex: 'It executes third-party server code merely to produce an advisory.' Both rejected the 'probe only running servers' mitigation as relocating rather than removing the hazard -- those servers are the ones most likely to hold live credentials. Matches the measurement taken before the verdicts: one flag-ignoring server spawned 7 processes and 5 sockets. Separately, grok found and I verified a LIVE bug in shipped code: _run_update_probe_command spawns without start_new_session and its 60s wait_for abandons the child on timeout -- no kill, no process-group cleanup -- so today's gateway.update_server can orphan a whole process tree. Independent of #150; to be filed separately.
asyncio.TimeoutError only became an alias of the builtin TimeoutError in 3.11. This project supports 3.10, where 'except (TimeoutError, ...)' does not catch what wait_for raises -- so the timeout escaped and _terminate_process_tree never ran. The orphan fix silently did nothing on the oldest supported version. CI caught it: test (3.10) failed while 3.11 and 3.12 passed, and the runner log showed 'Terminate orphan process: pid (3919) (python)' -- my own regression test leaking the processes it spawns. Reproduced locally on 3.10, then verified all three versions pass with the fix.
The test must spawn real process trees -- a mock cannot demonstrate an orphan. But doing that inside the pytest process perturbs state neighbouring tests measure: tests/runtime/test_downstream_remote.py asserts on THIS process's open socket count and on the process-global sse_starlette.AppStatus latch. My test knocked over two different tests on CI's 3.10 lane while passing locally. Moves the scenario into a subprocess, so the real-process proof survives without touching the suite's shared state. Stdlib subprocess -- no test-runner plugin and no new dependency for one test. Verified: reverting the fix makes it report 'ORPHANED' (RED); with the fix 'REAPED'; and test_tools.py + test_downstream_remote.py together now pass 188/188 on 3.10, the combination that was failing.
This was referenced Aug 19, 2026
Flaky: test_ec_p2_7_reconnect_does_not_leak_transports fails intermittently across Python lanes
#158
Closed
Merged
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.
Found by the advisor board while reviewing an unrelated plan for #150, then verified independently. This is a live bug in shipped code, not a hypothetical.
The bug
_run_update_probe_commandruns the downstream package's own code —npx <pkg> --help,uvx --refresh <pkg> --help— on behalf ofgateway.update_server. It spawned like this:Two problems compounding:
start_new_session, so the child is not a process-group leader and cannot be reaped as a tree.wait_forcancels the await, not the process. On timeout it raises and the child is simply abandoned — never signalled, never waited on.So a package that ignores the probe flag and runs as a server leaves its entire process tree alive. That is not a rare shape: measured against this repo's own manifest,
@circleci/mcp-server-circleciignores the flag and was still running at 90 seconds with 7 processes and 5 open sockets.The worst case is one this codebase already documents elsewhere:
_terminate_process_tree's own docstring notes that an orphaned@playwright/mcpgrandchild keeps the browser profile'sSingletonLockand breaks the next launch.The fix
Spawn with
start_new_session=Trueand reap the group via the existing_terminate_process_treehelper on timeout or cancellation. No new cleanup logic — this reuses the patternClientManageralready uses for exactly this reason.Verification
RED proof: reverting only the src change makes the new test fail with a real orphan, not an assertion about intent:
The test spawns a real process tree — a parent that starts a grandchild, records its pid and hangs, which is the shape of a server ignoring the probe flag. A mocked subprocess cannot demonstrate an orphan, and this bug is entirely about what survives the call.
pytest -q→ 2555 passed, 3 skipped, 0 failed (2553 baseline + 2)ruff check .,ruff format --check src/ tests/,mypy src— cleanScope
Deliberately narrow: the probe's process handling only. It does not touch what the probe is for, which is the open question in #150.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.