fix: improve process cleanup in MCP tests#9354
Conversation
WalkthroughRefactors an MCP integration test to add timeout marking, introduce async helper utilities for waiting on tools and child-process termination, and harden subprocess cleanup via a revamped process_tracker fixture. Updates tests to use the new helpers and adjusts a test signature for explicit post-cleanup validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Session as MCP Session
participant Proc as MCP Subprocess
participant PT as process_tracker
participant PS as psutil
Test->>Session: wait_tools(session)
Session-->>Test: list_tools()
Test->>PT: use fixture (start MCP)
PT->>Proc: track child processes
Test->>PT: teardown/cleanup
PT->>PS: wait_procs(children, timeout)
alt exited
PS-->>PT: exited list
else still running
PT->>Proc: terminate()
PT->>PS: wait_procs(timeout)
alt still running
PT->>Proc: kill()
end
end
Test->>Proc: wait_no_children(proc, target)
Proc-->>Test: confirmed no children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py (3)
23-25: Add docstring to the helper function.The
wait_toolshelper is a good abstraction, but should include a docstring for clarity.async def wait_tools(session, t=20): + """Wait for session tools to be available with timeout. + + Args: + session: The MCP session to query tools from + t: Timeout in seconds (default: 20) + + Returns: + The tools response from session.list_tools() + + Raises: + asyncio.TimeoutError: If tools are not available within timeout + """ return await asyncio.wait_for(session.list_tools(), timeout=t)
27-37: Add docstring and consider edge cases in process polling.The
wait_no_childrenhelper is well-implemented but needs documentation. Also consider that rapid process spawning/exiting between polls could be missed.async def wait_no_children(proc, max_wait=10, target=None): + """Wait for child processes to terminate or reach target count. + + Args: + proc: psutil.Process instance to check for children + max_wait: Maximum seconds to wait (default: 10) + target: Optional target count of children to wait for + + Returns: + True if target reached or no children remain, False on timeout + """ deadline = time.monotonic() + max_wait while time.monotonic() < deadline: children = proc.children(recursive=True) if target is not None and len(children) <= target: return True if not children: return True await asyncio.sleep(0.2) return False
251-252: Consider using wait_tools for consistency.While the
wait_no_childrenaddition at line 266 is good, lines 251-252 still use directlist_tools()calls without the timeout wrapper, unlike other tests.- tools1 = await session1.list_tools() - tools2 = await session2.list_tools() + tools1 = await wait_tools(session1) + tools2 = await wait_tools(session2)Also applies to: 266-266
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py(11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src/backend/**/*.py,tests/**/*.py,Makefile}
📄 CodeRabbit Inference Engine (.cursor/rules/backend_development.mdc)
{src/backend/**/*.py,tests/**/*.py,Makefile}: Run make format_backend to format Python code before linting or committing changes
Run make lint to perform linting checks on backend Python code
Files:
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py
src/backend/tests/**/*.py
📄 CodeRabbit Inference Engine (.cursor/rules/testing.mdc)
src/backend/tests/**/*.py: Unit tests for backend code must be located in the 'src/backend/tests/' directory, with component tests organized by component subdirectory under 'src/backend/tests/unit/components/'.
Test files should use the same filename as the component under test, with an appropriate test prefix or suffix (e.g., 'my_component.py' → 'test_my_component.py').
Use the 'client' fixture (an async httpx.AsyncClient) for API tests in backend Python tests, as defined in 'src/backend/tests/conftest.py'.
When writing component tests, inherit from the appropriate base class in 'src/backend/tests/base.py' (ComponentTestBase, ComponentTestBaseWithClient, or ComponentTestBaseWithoutClient) and provide the required fixtures: 'component_class', 'default_kwargs', and 'file_names_mapping'.
Each test in backend Python test files should have a clear docstring explaining its purpose, and complex setups or mocks should be well-commented.
Test both sync and async code paths in backend Python tests, using '@pytest.mark.asyncio' for async tests.
Mock external dependencies appropriately in backend Python tests to isolate unit tests from external services.
Test error handling and edge cases in backend Python tests, including using 'pytest.raises' and asserting error messages.
Validate input/output behavior and test component initialization and configuration in backend Python tests.
Use the 'no_blockbuster' pytest marker to skip the blockbuster plugin in tests when necessary.
Be aware of ContextVar propagation in async tests; test both direct event loop execution and 'asyncio.to_thread' scenarios to ensure proper context isolation.
Test error handling by mocking internal functions using monkeypatch in backend Python tests.
Test resource cleanup in backend Python tests by using fixtures that ensure proper initialization and cleanup of resources.
Test timeout and performance constraints in backend Python tests using 'asyncio.wait_for' and timing assertions.
Test Langflow's Messag...
Files:
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py
src/backend/**/components/**/*.py
📄 CodeRabbit Inference Engine (.cursor/rules/icons.mdc)
In your Python component class, set the
iconattribute to a string matching the frontend icon mapping exactly (case-sensitive).
Files:
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py
🔇 Additional comments (6)
src/backend/tests/integration/components/mcp/test_mcp_memory_leak.py (6)
12-12: Good addition of timeout protection for integration tests.The
timeimport and pytest timeout marker are appropriate additions that will prevent test hangs and improve CI reliability.Also applies to: 20-22
69-94: Excellent multi-stage cleanup implementation.The refactored cleanup logic follows best practices with progressive escalation (wait → terminate → kill) and comprehensive logging. The assertion ensures test failures on cleanup issues, which is crucial for detecting subprocess leaks.
117-117: Consistent application of helper functions improves test reliability.Good use of
wait_toolsfor resilient tool retrieval andwait_no_childrenfor cleanup verification.Also applies to: 130-130, 135-135
150-150: Properly applied helper functions for consistency.The changes maintain consistency with other tests and improve reliability.
Also applies to: 160-160
182-182: Consistent helper usage across health check tests.The modifications properly apply the helper functions for improved test reliability.
Also applies to: 215-215, 220-220
363-365: Good integration of process_tracker fixture for cleanup validation.The signature change and use of
wait_no_childrenenables proper verification that the cleanup_all method actually terminates subprocesses.Also applies to: 399-399
3 participants
Improves subprocess cleanup in MCP integration tests to prevent timeouts and leftover processes
Summary by CodeRabbit
Tests
Chores