Fix AsyncIO transport cleanup and eliminate curl dependency #40
Conversation
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
- Remove curl dependency from check_api_server_available() method - Replace curl fallback with more aggressive aiohttp retry - Update _get_system_info() to use aiohttp instead of curl for port testing - Maintain same timeout behavior (3.0 seconds) - Preserve diagnostic information collection for system info - Eliminate external curl dependency across codebase
- Create subprocess_utils.py with proper async context managers - Add pipe_transport_reader() to handle _UnixReadPipeTransport cleanup - Add subprocess_exec_with_cleanup() for guaranteed subprocess cleanup - Add subprocess_shell_with_cleanup() and managed_subprocess() utilities - Update bundle.py to use pipe_transport_reader for sbctl output reading - Update bundle.py subprocess calls to use cleanup utilities - Update kubectl.py to use subprocess_exec_with_cleanup - Ensure proper timeout handling and process termination - Prevent ResourceWarning about unclosed transports
- Add proper type annotations to subprocess utilities - Fix linting issues in test files - Ensure all type checking passes - Remove unused imports from test files
- Transform test_demonstrate_curl_dependency_failure to test_demonstrate_curl_dependency_fix_success - Transform test_demonstrate_expected_transport_cleanup_failure to test_demonstrate_transport_cleanup_fix_success - Tests now verify that curl dependency has been eliminated - Tests now verify that transport cleanup utilities work correctly - Tests now PASS instead of failing, confirming fixes are working - Add proper mocking for async subprocess operations - Verify aiohttp usage and eliminate curl subprocess calls
- Replace previous curl dependency check with functional health check test - Test verifies API health check works without curl being available - Test confirms aiohttp is used exclusively for HTTP operations - Test verifies no curl subprocess calls are made - Better functional test than just checking if curl is called
- Update test_kubectl_executor_run_kubectl_command_timeout to mock subprocess_exec_with_cleanup - Update test_kubectl_timeout_behavior to mock subprocess_exec_with_cleanup - Skip obsolete curl dependency reproduction tests that are no longer relevant - Tests now properly verify timeout handling with new subprocess utilities - Timeout cleanup is handled internally by subprocess utilities
- Create comprehensive integration tests in tests/integration/test_subprocess_utilities_integration.py - Remove 5 obsolete tests that tested old curl dependency behavior - Replace flaky async mocking with actual subprocess operations - Add functional tests for subprocess_exec_with_cleanup and subprocess_shell_with_cleanup - Add transport warning detection tests that work without complex mocks - Tests now verify actual functionality rather than reproducing old issues - All tests pass and are more reliable than previous mocked versions
## Summary - Fix _UnixReadPipeTransport cleanup warnings during garbage collection - Eliminate external curl command dependency in bundle.py - Replace curl subprocess calls with aiohttp HTTP client - Add proper AsyncIO transport cleanup utilities - Fix async decorator issues in e2e container tests ## Technical Changes - Created subprocess_utils.py with proper transport cleanup - Updated bundle.py to use aiohttp instead of curl subprocess - Updated kubectl.py to use new subprocess utilities - Fixed missing @pytest.mark.asyncio decorators in container tests - Updated transport cleanup tests to verify fixes work ## Test Results - All 160 unit tests pass - All 7 subprocess utilities integration tests pass - All code quality checks pass (black, ruff, mypy) - Curl dependency completely eliminated - Transport cleanup warnings resolved
- Allow environment-specific shell warnings in subprocess shell test - Fixes CI failure from 'getcwd() failed' warnings in GitHub Actions - Test still validates command success and output correctness
chris-sanders
added a commit
that referenced
this pull request
Jul 29, 2025
- fix-container-name-conflicts.md (PR #46 merged) - fix-asyncio-transport-python313.md (PR #42 merged) - fix-container-shutdown-race-condition.md (PR #41 merged) - fix-transport-cleanup-and-curl-dependency.md (PR #40 merged) - implement-comprehensive-testing-improvements.md (PR #39 merged) - switch-to-melange-apko-build.md (completed previously)
chris-sanders
added a commit
that referenced
this pull request
Aug 11, 2025
* Replace curl subprocess with aiohttp in bundle.py * Implement proper transport cleanup utilities * Update demonstration tests to verify fixes work * Fix kubectl timeout tests for subprocess_exec_with_cleanup * Fix AsyncIO transport cleanup and eliminate curl dependency ## Summary - Fix _UnixReadPipeTransport cleanup warnings during garbage collection - Eliminate external curl command dependency in bundle.py - Replace curl subprocess calls with aiohttp HTTP client - Add proper AsyncIO transport cleanup utilities - Fix async decorator issues in e2e container tests ## Technical Changes - Created subprocess_utils.py with proper transport cleanup - Updated bundle.py to use aiohttp instead of curl subprocess - Updated kubectl.py to use new subprocess utilities - Fixed missing @pytest.mark.asyncio decorators in container tests - Updated transport cleanup tests to verify fixes work ## Test Results - All 160 unit tests pass - All 7 subprocess utilities integration tests pass - All code quality checks pass (black, ruff, mypy) - Curl dependency completely eliminated - Transport cleanup warnings resolved
chris-sanders
added a commit
that referenced
this pull request
Aug 11, 2025
- fix-container-name-conflicts.md (PR #46 merged) - fix-asyncio-transport-python313.md (PR #42 merged) - fix-container-shutdown-race-condition.md (PR #41 merged) - fix-transport-cleanup-and-curl-dependency.md (PR #40 merged) - implement-comprehensive-testing-improvements.md (PR #39 merged) - switch-to-melange-apko-build.md (completed previously)
chris-sanders
added a commit
that referenced
this pull request
Aug 11, 2025
* Replace curl subprocess with aiohttp in bundle.py * Implement proper transport cleanup utilities * Update demonstration tests to verify fixes work * Fix kubectl timeout tests for subprocess_exec_with_cleanup * Fix AsyncIO transport cleanup and eliminate curl dependency ## Summary - Fix _UnixReadPipeTransport cleanup warnings during garbage collection - Eliminate external curl command dependency in bundle.py - Replace curl subprocess calls with aiohttp HTTP client - Add proper AsyncIO transport cleanup utilities - Fix async decorator issues in e2e container tests ## Technical Changes - Created subprocess_utils.py with proper transport cleanup - Updated bundle.py to use aiohttp instead of curl subprocess - Updated kubectl.py to use new subprocess utilities - Fixed missing @pytest.mark.asyncio decorators in container tests - Updated transport cleanup tests to verify fixes work ## Test Results - All 160 unit tests pass - All 7 subprocess utilities integration tests pass - All code quality checks pass (black, ruff, mypy) - Curl dependency completely eliminated - Transport cleanup warnings resolved
chris-sanders
added a commit
that referenced
this pull request
Aug 11, 2025
- fix-container-name-conflicts.md (PR #46 merged) - fix-asyncio-transport-python313.md (PR #42 merged) - fix-container-shutdown-race-condition.md (PR #41 merged) - fix-transport-cleanup-and-curl-dependency.md (PR #40 merged) - implement-comprehensive-testing-improvements.md (PR #39 merged) - switch-to-melange-apko-build.md (completed previously)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
_UnixReadPipeTransportcleanup warnings during garbage collectionTest plan
Technical Details
Transport Cleanup Issues Fixed
src/mcp_server_troubleshoot/subprocess_utils.pywith proper cleanup utilitiespipe_transport_readercontext manager for safe transport handlingkubectl.pyto use new cleanup utilities instead of direct subprocess calls_UnixReadPipeTransportcleanup warnings during garbage collectionCurl Dependency Eliminated
bundle.pywith aiohttp HTTP clientTest Suite Improvements
@pytest.mark.asynciodecorators in e2e container testsThis resolves the transport cleanup and curl dependency issues described in the task.