Skip to content

fix(mcp): narrow time.sleep mock target to tools.mcp_tool to stop background-thread mock pollution - #27589

Merged
teknium1 merged 1 commit into
mainfrom
hermes/salvage-tier2
May 17, 2026
Merged

fix(mcp): narrow time.sleep mock target to tools.mcp_tool to stop background-thread mock pollution#27589
teknium1 merged 1 commit into
mainfrom
hermes/salvage-tier2

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

Summary

Final remaining failure on main Tests workflow. tests/tools/test_mcp_stability.py::TestStdioPidTracking::test_kill_orphaned_uses_sigkill_when_available got 'sleep' called 225571 times in CI but passed locally. Classic global-patch interaction with concurrent threads.

Root cause

_kill_orphaned_mcp_children() did import time as _time inside the function body and called _time.sleep(2). The test patched the global time.sleep with unittest.mock.patch("time.sleep") — which replaces time.sleep for the entire process. Background daemon threads from tools/process_registry.py:1055 and tools/terminal_tool.py:1331 (both running periodic 1s-cadence reaper loops) suddenly hit a MagicMock instead of real sleep, looped as fast as possible, and accumulated ~225k spurious call(1) entries during the 2s the patch context was open.

Local runs only see this when those background loops are warm; CI runners have them up earlier in the process lifecycle so the race fires reliably.

Changes

  • tools/mcp_tool.py — drop the import time as _time inside _kill_orphaned_mcp_children; use the module-level time import (top of file). Production semantics unchanged.
  • tests/tools/test_mcp_stability.py — both test_kill_orphaned_* tests now patch tools.mcp_tool.time.sleep instead of time.sleep, so the mock is scoped to the production module's time reference only. Background threads in other modules sleep normally.

Validation

  • scripts/run_tests.sh tests/tools/test_mcp_stability.py -q → 16/16 pass.
  • Ruff clean.

After this, main's Tests workflow should be green for the first time in many hours.

@teknium1
teknium1 merged commit 04b4f76 into main May 17, 2026
17 checks passed
@teknium1
teknium1 deleted the hermes/salvage-tier2 branch May 17, 2026 20:33
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Lint report: hermes/salvage-tier2 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8707 on HEAD, 8707 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 4589 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have tool/mcp MCP client and OAuth labels May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have tool/mcp MCP client and OAuth type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants