-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix: Properly await cancelled tasks in voice STT cleanup #1977
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR fixes a resource leak in OpenAISTTTranscriptionSession by ensuring cancelled tasks are properly awaited during cleanup. Previously, tasks were cancelled but not awaited, which could lead to unhandled task exception warnings and potential resource leaks.
Key Changes:
- Made
_cleanup_tasks()async and added proper awaiting of cancelled tasks usingasyncio.gather() - Added validation in
Agent.__post_init__()to check that each tool in the tools list is a valid Tool object - Added comprehensive test coverage for the new tool validation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/agents/voice/models/openai_stt.py |
Made _cleanup_tasks() async and added proper awaiting of cancelled tasks to prevent resource leaks |
src/agents/agent.py |
Added tool type validation in __post_init__() to raise UserError for invalid tool objects |
tests/test_agent_config.py |
Added test cases for tool validation covering raw functions, strings, and mixed valid/invalid tools |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
10c275c to
de5bd08
Compare
|
@codex review |
…asks Problem: The _cleanup_tasks() method in VoiceStreamResult was only calling task.cancel() on pending tasks but not awaiting them. Additionally, _check_errors() could raise CancelledError when checking cancelled tasks. This could lead to: 1. Unhandled task exception warnings 2. Potential resource leaks from abandoned tasks 3. CancelledError masking real exceptions Evidence: - Similar to fixed guardrail tasks cleanup (PR openai#1976) - Similar to fixed voice STT cleanup (PR openai#1977) - Similar to fixed websocket cleanup (PR openai#1955) - Bug documented in .claude/bug-analysis/03-resource-leaks.md Solution: 1. Made _cleanup_tasks() async 2. Collect all real asyncio.Task objects that need to be awaited 3. Added await asyncio.gather() with return_exceptions=True to properly collect exceptions from cancelled tasks 4. Updated _check_errors() to skip cancelled tasks using task.cancelled() check to avoid CancelledError when calling task.exception() 5. Updated stream() async generator to await _cleanup_tasks() Testing: - Linting passes - No breaking changes to public API - Follows same pattern as PR openai#1976, openai#1977, openai#1955
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
||
| if self._listener_task and not self._listener_task.done(): | ||
| self._listener_task.cancel() | ||
| if isinstance(self._listener_task, asyncio.Task): |
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.
Perhaps, this is due to the same reason with #1976 but we don't want to have this check, which is unnecessary for main code
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.
any chance to add unit tests for this code path?
Problem: The _cleanup_tasks() method in OpenAISTTTranscriptionSession was only calling task.cancel() on pending tasks (listener, process_events, stream_audio, connection) but not awaiting them. This could lead to: 1. Unhandled task exception warnings 2. Potential resource leaks (websocket connections, file descriptors) 3. Improper cleanup of background tasks Evidence: - Similar to recently fixed guardrail tasks cleanup (PR openai#1976) - Similar to fixed websocket task cleanup (PR openai#1955) - asyncio best practices require awaiting cancelled tasks Solution: 1. Made _cleanup_tasks() async 2. Collect all real asyncio.Task objects that need to be awaited 3. Added await asyncio.gather() with return_exceptions=True to properly collect exceptions from cancelled tasks 4. Updated close() method to await _cleanup_tasks() Testing: - All existing voice/STT tests pass (17 passed) - Uses isinstance check to support mock objects in tests - Follows the same pattern as PR openai#1976 and PR openai#1955
Simplified the cleanup logic by removing isinstance checks for asyncio.Task. All tasks are created through asyncio.create_task(), so these type checks are unnecessary. This makes the code cleaner and avoids test-specific workarounds in production code, following the same pattern as PR openai#1976.
caaefe8 to
5a3b758
Compare
- Test cancellation and awaiting of all pending tasks - Test exception handling with return_exceptions=True - Test close() properly calls cleanup_tasks - All tests pass and verify proper resource cleanup
5a3b758 to
6ebc71e
Compare
✅ Added 3 unit tests: These tests cover _cleanup_tasks() and verify that: |
Summary
Fixes resource leak in OpenAISTTTranscriptionSession where tasks are cancelled but not properly awaited during cleanup.
Problem
The
_cleanup_tasks()method inOpenAISTTTranscriptionSessionwas only callingtask.cancel()on pending tasks (listener, process_events, stream_audio, connection) but not awaiting them. This could lead to:Evidence
.claude/bug-analysis/03-resource-leaks.mdSolution
_cleanup_tasks()asyncawait asyncio.gather()withreturn_exceptions=Trueto properly collect exceptions from cancelled tasksclose()method to await_cleanup_tasks()Testing
isinstancecheck to support mock objects in testsFiles Changed
src/agents/voice/models/openai_stt.py: Update task cleanup to properly await cancelled tasks