-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix: mcp-proxy process leak #10988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: mcp-proxy process leak #10988
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/backend/base/langflow/api/v2/mcp.py (1)
209-213: Wrap each disconnect in try/except to ensure both are attempted.If
mcp_stdio_client.disconnect()raises an exception,mcp_streamable_http_client.disconnect()will be skipped, potentially still causing leaks. As per coding guidelines, use try/except for proper cleanup.finally: # Always disconnect clients to prevent mcp-proxy process leaks # These clients spawn subprocesses that need to be explicitly terminated - await mcp_stdio_client.disconnect() - await mcp_streamable_http_client.disconnect() + try: + await mcp_stdio_client.disconnect() + except Exception: + await logger.aexception(f"Error disconnecting stdio client for {server_name}") + try: + await mcp_streamable_http_client.disconnect() + except Exception: + await logger.aexception(f"Error disconnecting http client for {server_name}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/base/langflow/api/v2/mcp.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
src/backend/**/*.py: Use FastAPI async patterns withawaitfor async operations in component execution methods
Useasyncio.create_task()for background tasks and implement proper cleanup with try/except forasyncio.CancelledError
Usequeue.put_nowait()for non-blocking queue operations andasyncio.wait_for()with timeouts for controlled get operations
Files:
src/backend/base/langflow/api/v2/mcp.py
src/backend/base/langflow/api/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
Backend API endpoints should be organized by version (v1/, v2/) under
src/backend/base/langflow/api/with specific modules for features (chat.py, flows.py, users.py, etc.)
Files:
src/backend/base/langflow/api/v2/mcp.py
🧬 Code graph analysis (1)
src/backend/base/langflow/api/v2/mcp.py (1)
src/lfx/src/lfx/base/mcp/util.py (3)
MCPStdioClient(1021-1231)MCPStreamableHttpClient(1234-1506)update_tools(1514-1659)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Lint Backend / Run Mypy (3.11)
- GitHub Check: Lint Backend / Run Mypy (3.10)
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Test Docker Images / Test docker images
- GitHub Check: Test Starter Templates
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Update Component Index
- GitHub Check: Update Starter Projects
🔇 Additional comments (2)
src/backend/base/langflow/api/v2/mcp.py (2)
152-157: Good approach: Creating locally-owned client instances.Creating the MCP client instances within the function scope and passing them to
update_toolsgives this code explicit control over their lifecycle, enabling proper cleanup in the finally block.
159-164: Correct: Passing owned client instances toupdate_tools.This ensures
update_toolsuses the caller-provided instances rather than creating its own, which previously caused the leak.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10988 +/- ##
==========================================
- Coverage 33.02% 33.02% -0.01%
==========================================
Files 1388 1388
Lines 65544 65549 +5
Branches 9680 9680
==========================================
+ Hits 21648 21649 +1
- Misses 42798 42802 +4
Partials 1098 1098
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| server_name=server_name, | ||
| server_config=server_list["mcpServers"][server_name], | ||
| mcp_stdio_client=mcp_stdio_client, | ||
| mcp_streamable_http_client=mcp_streamable_http_client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I say we get this in for v1.7.
Let's add a follow up to the update_tools function that reminds us to refactor how it creates clients, so nobody can call it in the future without cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point Jordan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you think of a clean refactor where it cleans up after itself?
* fix leak * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Move MCP client imports out of loop iteration in get_servers (#10993) * Initial plan * Move MCPStdioClient and MCPStreamableHttpClient imports to get_servers function Co-authored-by: phact <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: phact <[email protected]> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Edwin Jose <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: phact <[email protected]> Co-authored-by: Adam Aghili <[email protected]>
* fix: mcp-proxy process leak (#10988) * fix leak * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Move MCP client imports out of loop iteration in get_servers (#10993) * Initial plan * Move MCPStdioClient and MCPStreamableHttpClient imports to get_servers function Co-authored-by: phact <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: phact <[email protected]> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Edwin Jose <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: phact <[email protected]> Co-authored-by: Adam Aghili <[email protected]> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: Sebastián Estévez <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Edwin Jose <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: phact <[email protected]>
The /api/v2/mcp/servers?action_count=true endpoint in src/backend/base/langflow/api/v2/mcp.py calls update_tools() for each server to count tools which spanws mcp-proxy subprocesses but never cleans them up.
Noticed this when langflow was using 13gb of memory:
about 500 procs each w/ 10's of mb of RAM allocated after a couple of days of langflow being up.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.