fix(mcp): isolate orphan-reap sleep so test patches don't catch other threads - #27656
Closed
bbernstein616 wants to merge 2 commits into
Closed
fix(mcp): isolate orphan-reap sleep so test patches don't catch other threads#27656bbernstein616 wants to merge 2 commits into
bbernstein616 wants to merge 2 commits into
Conversation
…prevent bearer token exfiltration
… threads
Patching ``tools.mcp_tool.time.sleep`` (the previous "fix") is identical to
patching the global ``time.sleep`` — ``tools.mcp_tool.time`` *is* the time
module, and ``sleep`` is a module attribute. On CI under pytest-xdist the
test_kill_orphaned_uses_sigkill_when_available mock captured 207k calls
because worker/heartbeat threads kept calling ``time.sleep(1)`` while the
mock was active, blowing up ``assert_called_once_with(2)``.
Introduce a private ``_orphan_reap_sleep`` indirection in tools.mcp_tool and
have the test patch that symbol instead. The wrapper is unique to mcp_tool
so unrelated background sleepers can't pollute the mock's call list.
Verified with a stress harness (background thread spamming time.sleep(1))
that previously caused thousands of phantom call(1)s — mock now sees only
the intended call(2).
Tests: tests/tools/test_mcp_stability.py — 16/16 pass.
tests/tools/ -k mcp — 399/399 pass.
alt-glitch
added
type/test
Collaborator
|
Follow-up to merged #27589 (same MCP orphan-reap sleep mock CI flake). #27589 narrowed the mock target but didn't fully isolate — this PR adds the Note: This PR also bundles an unrelated security hardening change in |
Author
Author
|
Closing in favor of clean replacement #27676 (same MCP fix, no unrelated nous_portal commit). |
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
3 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.
Summary
tools.mcp_tool.time.sleepis identical to patching the globaltime.sleep—tools.mcp_tool.timeis the time module, so the previous "fix" (04b4f76) did not isolate anything.time.sleep(1)while the mock was active, sotest_kill_orphaned_uses_sigkill_when_availablesaw 207,243 calls andassert_called_once_with(2)failed (see fix(security): validate inference_base_url against host allowlist to prevent bearer token exfiltration #27612 CI job 76430224275)._orphan_reap_sleep(seconds)indirection intools.mcp_tooland routes_kill_orphaned_mcp_children's 2-second SIGTERM→SIGKILL gap through it. Tests patch the new symbol instead; it's exclusive tomcp_tool, so unrelated background sleepers can't pollute the mock.assert mock_sleep.calledtomock_sleep.assert_called_once_with(2)so both branches lock the same property.Verification
time.sleep(1)while_kill_orphaned_mcp_childrenruns: mock now captures onlycall(2)(vs. thousands before).tests/tools/test_mcp_stability.py— 16/16 pass locally.tests/tools/ -k mcp— 399/399 pass locally.Test plan
testjob green on Linux pytest-xdist (the surface where this previously failed)tests/tools/test_mcp_stability.py::TestStdioPidTrackingall pass