-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(realtime): bound background task cleanup #3704
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,7 @@ class _RealtimeSessionClosedSentinel: | |
|
|
||
|
|
||
| _REALTIME_SESSION_CLOSED_SENTINEL = _RealtimeSessionClosedSentinel() | ||
| _BACKGROUND_TASK_CLEANUP_TIMEOUT = 1.0 | ||
|
|
||
|
|
||
| def _serialize_tool_output(output: Any) -> str: | ||
|
|
@@ -192,6 +193,7 @@ def __init__( | |
| asyncio.Queue() | ||
| ) | ||
| self._event_iterator_waiters = 0 | ||
| self._closing = False | ||
| self._closed = False | ||
| self._stored_exception: BaseException | None = None | ||
| self._pending_tool_calls: dict[str, _PendingToolCall] = {} | ||
|
|
@@ -1265,6 +1267,8 @@ async def _run_output_guardrails(self, text: str, response_id: str) -> bool: | |
|
|
||
| def _enqueue_guardrail_task(self, text: str, response_id: str) -> None: | ||
| # Runs the guardrails in a separate task to avoid blocking the main loop | ||
| if self._closing or self._closed: | ||
| return | ||
|
|
||
| task = asyncio.create_task(self._run_output_guardrails(text, response_id)) | ||
| self._guardrail_tasks.add(task) | ||
|
|
@@ -1277,6 +1281,11 @@ def _on_guardrail_task_done(self, task: asyncio.Task[Any]) -> None: | |
| # Remove from tracking set | ||
| self._guardrail_tasks.discard(task) | ||
|
|
||
| if self._closing or self._closed: | ||
| if not task.cancelled(): | ||
| task.exception() | ||
| return | ||
|
|
||
| # Check for exceptions and propagate as events | ||
| if not task.cancelled(): | ||
| exception = task.exception() | ||
|
|
@@ -1291,11 +1300,8 @@ def _on_guardrail_task_done(self, task: asyncio.Task[Any]) -> None: | |
| ) | ||
| ) | ||
|
|
||
| def _cleanup_guardrail_tasks(self) -> None: | ||
| for task in self._guardrail_tasks: | ||
| if not task.done(): | ||
| task.cancel() | ||
| self._guardrail_tasks.clear() | ||
| async def _cleanup_guardrail_tasks(self, caller_task: asyncio.Task[Any] | None) -> None: | ||
| await self._cancel_and_wait_for_tasks(self._guardrail_tasks, "guardrail", caller_task) | ||
|
|
||
| def _enqueue_tool_call_task( | ||
| self, | ||
|
|
@@ -1307,6 +1313,9 @@ def _enqueue_tool_call_task( | |
| call_id_reserved: bool = False, | ||
| ) -> None: | ||
| """Run tool calls in the background to avoid blocking realtime transport.""" | ||
| if self._closing or self._closed: | ||
| return | ||
|
|
||
| handle_kwargs: dict[str, Any] = {"agent_snapshot": agent_snapshot} | ||
| if dispatch_snapshot is not None: | ||
| handle_kwargs["dispatch_snapshot"] = dispatch_snapshot | ||
|
|
@@ -1322,6 +1331,11 @@ def _enqueue_tool_call_task( | |
| def _on_tool_call_task_done(self, task: asyncio.Task[Any]) -> None: | ||
| self._tool_call_tasks.discard(task) | ||
|
|
||
| if self._closing or self._closed: | ||
| if not task.cancelled(): | ||
| task.exception() | ||
| return | ||
|
|
||
| if task.cancelled(): | ||
| return | ||
|
|
||
|
|
@@ -1364,11 +1378,38 @@ def _on_tool_call_task_done(self, task: asyncio.Task[Any]) -> None: | |
| ) | ||
| ) | ||
|
|
||
| def _cleanup_tool_call_tasks(self) -> None: | ||
| for task in self._tool_call_tasks: | ||
| async def _cleanup_tool_call_tasks(self, caller_task: asyncio.Task[Any] | None) -> None: | ||
| await self._cancel_and_wait_for_tasks(self._tool_call_tasks, "tool call", caller_task) | ||
|
|
||
| async def _cancel_and_wait_for_tasks( | ||
| self, | ||
| tasks: set[asyncio.Task[Any]], | ||
| label: str, | ||
| caller_task: asyncio.Task[Any] | None, | ||
| ) -> None: | ||
| tasks_to_wait: list[asyncio.Task[Any]] = [] | ||
|
|
||
| for task in list(tasks): | ||
| if task is caller_task: | ||
| tasks.discard(task) | ||
| continue | ||
| if not task.done(): | ||
| task.cancel() | ||
| self._tool_call_tasks.clear() | ||
| tasks_to_wait.append(task) | ||
|
|
||
| if tasks_to_wait: | ||
| _done, pending = await asyncio.wait( | ||
| tasks_to_wait, | ||
| timeout=_BACKGROUND_TASK_CLEANUP_TIMEOUT, | ||
| ) | ||
| if pending: | ||
| logger.warning( | ||
| "Timed out waiting for %d realtime %s background task(s) to stop.", | ||
| len(pending), | ||
| label, | ||
| ) | ||
|
|
||
| tasks.difference_update(tasks_to_wait) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a background task ignores cancellation past the timeout, this removes it from the tracking set before listener removal and Useful? React with 👍 / 👎. |
||
|
|
||
| def _wake_event_iterators(self) -> None: | ||
| for _ in range(self._event_iterator_waiters): | ||
|
|
@@ -1379,24 +1420,38 @@ async def _cleanup(self) -> None: | |
| if self._closed: | ||
| self._wake_event_iterators() | ||
| return | ||
| if self._closing: | ||
| self._wake_event_iterators() | ||
| return | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When two tasks call Useful? React with 👍 / 👎. |
||
|
|
||
| self._closing = True | ||
| caller_task = asyncio.current_task() | ||
|
|
||
| # Cancel and cleanup guardrail tasks | ||
| self._cleanup_guardrail_tasks() | ||
| self._cleanup_tool_call_tasks() | ||
| try: | ||
| # Cancel and cleanup guardrail tasks | ||
| await asyncio.gather( | ||
| self._cleanup_guardrail_tasks(caller_task), | ||
| self._cleanup_tool_call_tasks(caller_task), | ||
| ) | ||
|
|
||
| # Remove ourselves as a listener | ||
| self._model.remove_listener(self) | ||
| # Remove ourselves as a listener | ||
| self._model.remove_listener(self) | ||
|
|
||
| # Close the model connection | ||
| await self._model.close() | ||
| # Close the model connection | ||
| await self._model.close() | ||
|
|
||
| # Clear pending approval tracking | ||
| self._pending_tool_calls.clear() | ||
| self._pending_tool_outputs.clear() | ||
| # Clear pending approval tracking | ||
| self._pending_tool_calls.clear() | ||
| self._pending_tool_outputs.clear() | ||
|
|
||
| # Mark as closed | ||
| self._closed = True | ||
| self._wake_event_iterators() | ||
| # Mark as closed | ||
| self._closed = True | ||
| except BaseException: | ||
| self._closing = False | ||
| raise | ||
| else: | ||
| self._closing = False | ||
| self._wake_event_iterators() | ||
|
|
||
| def _dispatch_snapshot_from_settings( | ||
| self, | ||
|
|
||
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.
When the caller is one of the tracked background tasks, such as a function tool or output guardrail that captures the session and calls
await session.close(), this branch skips cancelling that current task so cleanup can finish. Afterclose()returns to the same coroutine, the rest of_handle_tool_callor_run_output_guardrailscan continue and enqueue events or send tool outputs/interrupts after_model.close()has already run, and any resulting exception is then swallowed by the_closedcallback path. The self-await guard should also abort or no-op the caller's remaining session work after close completes.Useful? React with 👍 / 👎.