fix(mcp): bounded per-peer mount so one hung peer can't darken the aggregator - #347
Merged
Merged
Conversation
…gregator
The umbrella `scitex serve` aggregator fronts ~33 packages' MCP tools by
importing each peer's `_mcp_server` and mounting its FastMCP at module-load
time. Previously each peer was resolved SEQUENTIALLY and SYNCHRONOUSLY. If a
single peer's import hangs at init (real case: scitex-todo store-wedge stalls
20s+), the whole `scitex._mcp` import never completes and EVERY peer's tools
go dark — the failure concentrates 33x vs the per-server split.
Fix: resolve each peer's FastMCP concurrently in a bounded DAEMON thread
(`_resolve_peers_bounded`). Threads start at once and are joined against one
shared deadline, so total load time is ~max(peer) bounded by the per-peer
budget — never the sum, and a hung thread can't block interpreter exit. On
timeout / exception the peer is SKIPPED with a warning ("... unavailable this
session") and the healthy peers still mount. Budget is configurable via
`SCITEX_MCP_PEER_TIMEOUT` (default 8s).
Mounting stays on the MAIN thread: only the peer IMPORT is slow/hang-prone;
`safe_mount` is fast and mutates shared parent-server state, so it is kept
single-threaded to avoid concurrent-mutation races.
Preserves all prior behavior: `_is_enabled` env gate, `_NAMESPACE_ALIASES`,
`_SKIP_CATEGORIES`, `register_peer_extras`, `register_umbrella_tools`, the
mounted-count log, and `SCITEX_MCP_USE_<NS>=0` gating. `register_all_tools`
gains injectable `iter_registry` / `peer_timeout` params (defaults unchanged)
so tests drive the real path without patching module globals.
Tests (tests/scitex/test_mcp_bounded_mount.py): real fixture peers written to
disk — a 30s-sleep hung peer, an ImportError peer, a sys.exit peer, fast
peers. Assert the bounded resolve returns within the budget, the hung peer is
skipped + warned, the fast peers still mount, and the env gate holds.
…ked CI collection PR #347's pytest-matrix hung for 1h30m (job timeout, all 3 py versions) with "test session starts" printed and then nothing — i.e. a hang during COLLECTION, before any test ran, where pytest-timeout's per-test SIGALRM cannot fire. Root cause: the first cut resolved every peer's `_mcp_server` import CONCURRENTLY (one daemon thread per peer, ~33 fired at once). Importing a large, interdependent package set from many threads simultaneously races the CPython import machinery and can deadlock via cross-thread circular imports that never occur when modules are imported one at a time. That deadlock strikes at `from scitex import _mcp` import (register_all_tools runs at module load) — which every test module triggers at collection. `develop` passes because its resolve is sequential; the concurrency was the regression. Fix: resolve peers SERIALLY. Each peer still imports in its own bounded daemon thread (`_resolve_one_peer_bounded`, joined for at most `SCITEX_MCP_PEER_TIMEOUT`, default 8s) so a hung/wedged import is abandoned + skipped and can never block interpreter exit — but only ONE import is ever in flight, so imports stay as safe as develop's sequential loop while gaining the per-peer hang bound. Wall cost is the sum of the healthy peers' (fast) import times plus the timeout per wedged peer — a one-time startup cost paid for reliability (the original "~max(peer)" concurrency goal is knowingly traded away; correctness first). Verified locally: real `from scitex import _mcp` completes in ~8s and leaves NO lingering non-daemon threads (so the main-thread timeout signal can always fire); `test_mcp_entrypoint.py` matches the develop baseline (no new failures, no hang). Tests: added `_INFINITE_PEER` (import blocks on `threading.Event().wait()`, never set — the real store-wedge shape, not a bounded sleep) with a fixture asserting the bounded resolve returns within the budget, skips the wedged peer, and still mounts the co-resident fast peer. All hang fixtures are written to tmp dirs and imported ONLY inside the bounded code path at test time — never at module/collection time, so pytest can never auto-collect or import them into a collection hang. 25 tests pass.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
The umbrella
scitex serveaggregator (src/scitex/_mcp/__init__.py) is thesingle MCP surface that will front ALL ~33 packages' tools. At module-load time
it calls
register_all_tools(mcp), which iterated the ecosystem registry andresolved each peer's FastMCP sequentially + synchronously via
importlib.import_module(f"{import_name}._mcp_server").If ONE peer's
_mcp_serverimport HANGS at init (verified real case:scitex-todo's store-wedge stalls 20s+ at mcp-start), the whole loop blocks →
scitex._mcpimport never completes → every peer's tools go dark. Theaggregator concentrates this cascade 33x versus the current per-server split,
so it must be fixed before fleet-wide deployment.
Fix
Each peer's resolve now runs concurrently in a bounded daemon thread
(
_resolve_peers_bounded):total load time is ~
max(peer)bounded by the per-peer budget — never thesum. A single hung future can't stall collection of the others.
(
MCP peer '<ns>' resolve timed out ... — its tools will be unavailable this session) and the loop CONTINUES. Healthy peers still mount.SCITEX_MCP_PEER_TIMEOUT(default 8s).safe_mountstays on the main thread — only the peer IMPORT isslow/hang-prone; the mount is fast and mutates shared parent-server state, so
it is kept single-threaded to avoid concurrent-mutation races.
Existing
BaseExceptionhandling in_resolve_peer_mcpis preserved, so a peerthat
sys.exit()s or raisesImportErrorat import is skipped, not fatal.All prior behavior preserved:
_is_enabledenv gate,_NAMESPACE_ALIASES,_SKIP_CATEGORIES,register_peer_extras,register_umbrella_tools, themounted-count log, and
SCITEX_MCP_USE_<NS>=0gating.register_all_toolsgains injectable
iter_registry/peer_timeoutkwargs (defaults unchanged) sotests can drive the real code path with hand-rolled fixture peers instead of
patching module globals (also satisfies the no-
monkeypatchhouse rule).Tests —
tests/scitex/test_mcp_bounded_mount.py(22 tests, all green)Real fixture peers written to disk and imported for real (no mocks):
time.sleep(30)at import):_resolve_peers_boundedreturnswithin the budget, reports the peer skipped with a "timed out" reason, and the
co-resident fast peer still resolves to a real FastMCP.
register_all_toolspath with an injected registry of one fast +one hung peer: does not hang (< 20s despite the 30s sleep), hung namespace NOT
mounted, fast namespace mounted, a warning naming the hung peer is logged
("unavailable").
sys.exit()peer: skipped, don't block the fastpeer.
SCITEX_MCP_USE_<NS>=0) and_peer_timeoutparsing (default /override / invalid-fallback).
The whole suite finishes in ~15s even though it imports two 30s-sleep peers —
direct proof the daemon-thread bounding works.
Reviewer notes
register_all_tools(mcp)still runs atsrc/scitex/_mcp/__init__.pyimport (line ~255). The fix bounds that call so ahung peer degrades to "its tools missing" instead of hanging the import; it
does not move the call off import time.
test_mcp_entrypoint.py(io/stats/scholar/figrecipe not installed in the sandbox venv) fail identically
on pristine
develop— environmental, not caused by this change.