fix: Add graceful subprocess cleanup during shutdown#10909
Conversation
…ppress to improve code readability test(mcp_cleanup.py): update tests to use context manager for patching to enhance clarity and maintainability
|
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 WalkthroughThis PR introduces cleanup utilities for MCP (Model Context Protocol) sessions in Langflow. The main application shutdown flow is enhanced to eagerly terminate MCP sessions before other cleanup steps, with graceful fallback to process termination on Unix systems. Comprehensive unit tests validate all cleanup scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (7 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 |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (39.26%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10909 +/- ##
==========================================
+ Coverage 32.96% 33.03% +0.06%
==========================================
Files 1387 1388 +1
Lines 65472 65539 +67
Branches 9680 9680
==========================================
+ Hits 21586 21648 +62
- Misses 42788 42793 +5
Partials 1098 1098
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/backend/tests/unit/utils/test_mcp_cleanup.py (2)
63-71: Import error simulation may not work as intended.Setting a module to
Noneinsys.modulesdoesn't raiseImportErroron import—it returnsNone. This test might pass accidentally because accessing attributes onNoneraisesAttributeError, which is caught bycontextlib.suppress(Exception), but it doesn't actually test theImportErrorpath.Consider using a side effect that actually raises
ImportError:async def test_cleanup_handles_import_error(self): """Test cleanup handles import errors gracefully.""" + import builtins + original_import = builtins.__import__ + + def mock_import(name, *args, **kwargs): + if name == "lfx.base.mcp.util": + raise ImportError("Test import error") + return original_import(name, *args, **kwargs) + with ( - patch.dict("sys.modules", {"lfx.base.mcp.util": None}), + patch.object(builtins, "__import__", side_effect=mock_import), patch("langflow.utils.mcp_cleanup._kill_mcp_processes", new_callable=AsyncMock) as mock_kill, ): # Should not raise, should silently continue await cleanup_mcp_sessions() mock_kill.assert_called_once()
309-326: Consider simplifying the exception-raising mock.The generator-based pattern
property(lambda _: (_ for _ in ()).throw(...))is creative but obscure. A simpler approach would usePropertyMock:+ from unittest.mock import PropertyMock + mock_proc = MagicMock() - mock_proc.info = property(lambda _: (_ for _ in ()).throw(mock_psutil.AccessDenied)) - - # Make info raise AccessDenied - type(mock_proc).info = property(lambda _: (_ for _ in ()).throw(mock_psutil.AccessDenied)) + type(mock_proc).info = PropertyMock(side_effect=mock_psutil.AccessDenied)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/backend/base/langflow/main.py(2 hunks)src/backend/base/langflow/utils/mcp_cleanup.py(1 hunks)src/backend/tests/unit/utils/test_mcp_cleanup.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/tests/unit/utils/test_mcp_cleanup.pysrc/backend/base/langflow/main.pysrc/backend/base/langflow/utils/mcp_cleanup.py
src/backend/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
src/backend/tests/**/*.py: Place backend unit tests insrc/backend/tests/directory, component tests insrc/backend/tests/unit/components/organized by component subdirectory, and integration tests accessible viamake integration_tests
Use same filename as component with appropriate test prefix/suffix (e.g.,my_component.py→test_my_component.py)
Use theclientfixture (FastAPI Test Client) defined insrc/backend/tests/conftest.pyfor API tests; it provides an asynchttpx.AsyncClientwith automatic in-memory SQLite database and mocked environment variables. Skip client creation by marking test with@pytest.mark.noclient
Inherit from the correctComponentTestBasefamily class located insrc/backend/tests/base.pybased on API access needs:ComponentTestBase(no API),ComponentTestBaseWithClient(needs API), orComponentTestBaseWithoutClient(pure logic). Provide three required fixtures:component_class,default_kwargs, andfile_names_mapping
Create comprehensive unit tests for all new backend components. If unit tests are incomplete, create a corresponding Markdown file documenting manual testing steps and expected outcomes
Test both sync and async code paths, mock external dependencies appropriately, test error handling and edge cases, validate input/output behavior, and test component initialization and configuration
Use@pytest.mark.asynciodecorator for async component tests and ensure async methods are properly awaited
Test background tasks usingasyncio.create_task()and verify completion withasyncio.wait_for()with appropriate timeout constraints
Test queue operations using non-blockingqueue.put_nowait()andasyncio.wait_for(queue.get(), timeout=...)to verify queue processing without blocking
Use@pytest.mark.no_blockbustermarker to skip the blockbuster plugin in specific tests
For database tests that may fail in batch runs, run them sequentially usinguv run pytest src/backend/tests/unit/test_database.pyr...
Files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
**/test_*.py
📄 CodeRabbit inference engine (Custom checks)
**/test_*.py: Review test files for excessive use of mocks that may indicate poor test design - check if tests have too many mock objects that obscure what's actually being tested
Warn when mocks are used instead of testing real behavior and interactions, and suggest using real objects or test doubles when mocks become excessive
Ensure mocks are used appropriately for external dependencies only, not for core logic
Backend test files should follow the naming convention test_*.py with proper pytest structure
Test files should have descriptive test function names that explain what is being tested
Tests should be organized logically with proper setup and teardown
Consider including edge cases and error conditions for comprehensive test coverage
Verify tests cover both positive and negative scenarios where appropriate
For async functions in backend tests, ensure proper async testing patterns are used with pytest
For API endpoints, verify both success and error response testing
Files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
🧠 Learnings (6)
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Create comprehensive unit tests for all new backend components. If unit tests are incomplete, create a corresponding Markdown file documenting manual testing steps and expected outcomes
Applied to files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Test both sync and async code paths, mock external dependencies appropriately, test error handling and edge cases, validate input/output behavior, and test component initialization and configuration
Applied to files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Use async fixtures with proper cleanup using try/finally blocks to ensure resources are properly released after tests complete
Applied to files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Use `monkeypatch` fixture to mock internal functions for testing error handling scenarios; validate error status codes and error message content in responses
Applied to files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Each test should have a clear docstring explaining its purpose; complex test setups should be commented; mock usage should be documented; expected behaviors should be explicitly stated
Applied to files:
src/backend/tests/unit/utils/test_mcp_cleanup.py
📚 Learning: 2025-11-24T19:46:09.104Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/backend_development.mdc:0-0
Timestamp: 2025-11-24T19:46:09.104Z
Learning: Applies to src/backend/base/langflow/components/**/__init__.py : Update `__init__.py` with alphabetically sorted imports when adding new components
Applied to files:
src/backend/base/langflow/main.py
🧬 Code graph analysis (3)
src/backend/tests/unit/utils/test_mcp_cleanup.py (1)
src/backend/base/langflow/utils/mcp_cleanup.py (5)
_kill_mcp_processes(44-65)_terminate_child_mcp_processes(68-82)_terminate_orphaned_mcp_processes(85-101)_try_terminate_mcp_process(104-125)cleanup_mcp_sessions(21-41)
src/backend/base/langflow/main.py (1)
src/backend/base/langflow/utils/mcp_cleanup.py (1)
cleanup_mcp_sessions(21-41)
src/backend/base/langflow/utils/mcp_cleanup.py (2)
src/lfx/src/lfx/base/mcp/util.py (2)
MCPSessionManager(459-1018)cleanup_all(956-993)src/backend/tests/unit/base/mcp/test_mcp_util.py (1)
session_manager(28-33)
⏰ 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). (17)
- GitHub Check: Lint Backend / Run Mypy (3.10)
- GitHub Check: Lint Backend / Run Mypy (3.11)
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Test Docker Images / Test docker images
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- 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 / Integration Tests - Python 3.10
- GitHub Check: Test Starter Templates
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Update Component Index
- GitHub Check: Run Ruff Check and Format
- GitHub Check: Update Starter Projects
🔇 Additional comments (13)
src/backend/base/langflow/main.py (2)
49-49: LGTM!The import is correctly placed with other langflow utility imports.
318-320: Well-placed cleanup for maximum reliability.Positioning the MCP cleanup at the very beginning of the finally block ensures subprocesses are terminated even if subsequent shutdown steps are interrupted. The comment clearly explains the rationale, and the function's internal
contextlib.suppress(Exception)wrappers ensure this won't block other shutdown logic.src/backend/base/langflow/utils/mcp_cleanup.py (6)
1-18: LGTM!Clean module structure with proper docstring documenting platform constraints and appropriate use of
TYPE_CHECKINGfor the optionalpsutildependency.
21-41: Well-structured two-phase cleanup.The separation of graceful session manager cleanup (first phase) and fallback process termination (second phase) in independent
contextlib.suppressblocks ensures both are attempted regardless of individual failures. Lazy imports inside the function body correctly handle the case where dependencies may not be available.
44-65: LGTM!Proper platform guard for Unix-only functionality, graceful handling of missing
psutil, and appropriate logging strategy (only when processes are actually killed).
68-82: LGTM!Recursive child enumeration correctly captures nested MCP subprocesses, and
NoSuchProcesshandling covers race conditions during shutdown.
85-101: LGTM!The
ppid == 1check correctly identifies orphaned processes on Unix systems (those reparented to init/systemd). Exception handling covers the common race conditions during process enumeration.
104-125: LGTM!The graceful termination pattern (SIGTERM → wait with timeout → SIGKILL) follows best practices. The cmdline matching is intentionally broad to catch MCP processes regardless of exact invocation pattern, and edge cases (empty/None cmdline) are handled correctly.
src/backend/tests/unit/utils/test_mcp_cleanup.py (5)
95-186: Good coverage of the _kill_mcp_processes function.The tests properly verify platform guards, both termination paths, and conditional logging. Note that the
sys.modulespatching at line 108 has the same limitation mentioned earlier regarding import error simulation.
189-234: LGTM!Tests correctly verify child process termination and the
NoSuchProcessedge case. The patching of_try_terminate_mcp_processisolates the function under test appropriately.
329-472: Excellent test coverage.Comprehensive testing of all code paths including the graceful termination sequence (terminate → wait → kill on timeout), exception handling for various psutil errors, and edge cases with empty/None cmdlines. Each test has a clear docstring explaining its purpose. Based on learnings, this follows best practices for test documentation.
475-536: Good integration-style tests.These tests properly validate the complete cleanup flow including error resilience. The silent failure test at lines 516-536 is particularly valuable for ensuring shutdown reliability—it confirms that errors during cleanup won't produce unexpected log noise.
1-15: LGTM!Good test structure with module-level
pytestmarkfor async tests and appropriate imports. Testing the private helper functions is justified here as they contain critical cleanup logic that warrants direct verification.
This pull request introduces a robust mechanism for cleaning up MCP server subprocesses during the shutdown phase of Langflow. The main goal is to ensure all MCP-related child and orphaned processes are properly terminated, preventing resource leaks and zombie processes. The cleanup logic is executed at the very beginning of the shutdown sequence for reliability and works on macOS and Linux platforms.
Graceful MCP subprocess cleanup:
langflow/utils/mcp_cleanup.pythat provides thecleanup_mcp_sessionsasync function to terminate MCP server subprocesses during shutdown, including fallback logic to forcibly kill lingering processes usingpsutilon Unix systems.cleanup_mcp_sessionsinto the shutdown sequence inlangflow/main.py, ensuring it runs before any other shutdown logic for maximum reliability. [1] [2]mcp-graceful-shutdown-testing.md
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.