-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix: Properly await cancelled tasks in voice result streaming cleanup #1978
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
…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
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 the VoiceStreamResult class by properly awaiting cancelled tasks during cleanup and preventing CancelledError from masking real exceptions.
Key Changes:
- Made
_cleanup_tasks()async to properly await cancelled tasks usingasyncio.gather()withreturn_exceptions=True - Enhanced
_check_errors()to checktask.cancelled()before callingtask.exception()to avoid CancelledError - Updated the
stream()method to await the now-async_cleanup_tasks()
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
The task.cancelled() check on line 281 already ensures that CancelledError will never be raised when calling task.exception(). The try-except block was redundant and added unnecessary complexity. Thanks to reviewer comment for catching this.
Summary
Fixes resource leak and error handling issue in VoiceStreamResult where tasks are cancelled but not properly awaited during cleanup, and CancelledError could mask real exceptions.
Problem
The
_cleanup_tasks()method inVoiceStreamResulthad two issues:Resource leak: Only calling
task.cancel()without awaiting could lead to:Error masking:
_check_errors()callingtask.exception()on cancelled tasks would raiseCancelledError, masking the real exceptionEvidence
.claude/bug-analysis/03-resource-leaks.mdSolution
_cleanup_tasks()asyncawait asyncio.gather()withreturn_exceptions=Trueto properly collect exceptions from cancelled tasks_check_errors()to checktask.cancelled()before callingtask.exception()to avoid CancelledErrorstream()async generator to await_cleanup_tasks()Testing
Files Changed
src/agents/voice/result.py: Fix task cleanup and error checking