Skip to content

Commit 37a0324

Browse files
committed
fix: increase thread join timeout and clarify redundant None assignments
Changes based on Copilot AI review (2 issues): 1. Thread join timeout was shorter than cleanup timeout (Issue #1): - Changed _THREAD_JOIN_TIMEOUT from 5 to 12 seconds - Must be >= cleanup timeout (10s) plus buffer for loop.stop() - Prevents thread abandonment during active cleanup 2. Added detailed comment for redundant None assignments (Issue #2): - Explained why we set _session/_stdio_context to None even if _cleanup_stdio_async() already did it - Documents the safety cases: timeout, failure, skip, cancellation - Makes code intent clear for future maintainers
1 parent 837ef2d commit 37a0324

File tree

1 file changed

+7
-3
lines changed

1 file changed

+7
-3
lines changed

core/framework/runner/mcp_client.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ def _call_tool_http(self, tool_name: str, arguments: dict[str, Any]) -> Any:
391391

392392
# Cleanup timeout should match or exceed connection timeout (10 seconds in _connect_stdio)
393393
_CLEANUP_TIMEOUT = 10 # seconds
394-
_THREAD_JOIN_TIMEOUT = 5 # seconds - should be proportional to cleanup timeout
394+
_THREAD_JOIN_TIMEOUT = 12 # seconds - must be >= cleanup timeout to allow full cleanup plus buffer
395395

396396
async def _cleanup_stdio_async(self) -> None:
397397
"""Async cleanup for STDIO session and context managers.
@@ -479,8 +479,12 @@ def disconnect(self) -> None:
479479
self._loop_thread.join(timeout=self._THREAD_JOIN_TIMEOUT)
480480

481481
# Clear remaining references
482-
# Note: _session and _stdio_context are cleared in _cleanup_stdio_async()
483-
# if cleanup was successful, but we clear all references here for safety
482+
# Note: _session and _stdio_context may already be None if _cleanup_stdio_async()
483+
# succeeded. This redundant assignment is intentional for safety in cases where:
484+
# 1. Cleanup timed out or failed
485+
# 2. Cleanup was skipped (loop not running)
486+
# 3. CancelledError interrupted cleanup
487+
# Setting None to None is safe and ensures clean state.
484488
self._session = None
485489
self._stdio_context = None
486490
self._read_stream = None

0 commit comments

Comments
 (0)