Skip to content

Conversation

dbschmigelski
Copy link
Member

@dbschmigelski dbschmigelski commented Sep 9, 2025

Description

Fixed a critical race condition in MCPClient that caused hanging when initialization failed due to timeout. The root issue was misunderstanding Python's context manager protocol: exit is only called if enter succeeds. Since we performed heavy initialization work in enter, failures left background threads running indefinitely with no cleanup mechanism.

The fix adds defensive cleanup directly in the start() method to call stop() on any initialization failure, ensuring proper resource cleanup regardless of where the failure occurs. Made stop() method robust to handle partial initialization states where event loop or session may not exist yet.

I added TODO comment acknowledging that the current approach of doing heavy work in enter is non-idiomatic Python. The proper solution would be lazy initialization on first method call, but this fix ensures no resource leaks in the current architecture.

Related Issues

#664

Documentation PR

Note needed

Type of Change

Bug fix

Testing

  • Added unit tests for timeout cleanup, defensive stop behavior, and client reuse after failure.

  • Added integration test that reproduces the exact hanging scenario with real stdio_client. Verified no thread leaks occur during timeout scenarios and tested client recovery by adjusting timeout and successful reuse.

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dbschmigelski dbschmigelski marked this pull request as ready for review September 9, 2025 22:38
Unshure
Unshure previously approved these changes Sep 11, 2025
JackYPCOnline
JackYPCOnline previously approved these changes Sep 11, 2025
@dbschmigelski dbschmigelski merged commit 03c62f7 into strands-agents:main Sep 16, 2025
11 of 12 checks passed
@Unshure Unshure mentioned this pull request Sep 17, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants