FIX Harden prompt target cancellation cleanup - #2483
Open
Roman Lutz (romanlutz) wants to merge 1 commit into
Open
FIX Harden prompt target cancellation cleanup#2483Roman Lutz (romanlutz) wants to merge 1 commit into
Roman Lutz (romanlutz) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+861
to
+867
| async def _cancel_receive_task_async(self, *, receive_task: asyncio.Task[RealtimeTargetResult]) -> None: | ||
| """Cancel and retrieve an unfinished Realtime receive task.""" | ||
| if receive_task.done(): | ||
| return | ||
| receive_task.cancel() | ||
| await asyncio.gather(receive_task, return_exceptions=True) | ||
|
|
Contributor
There was a problem hiding this comment.
Suggested change
| async def _cancel_receive_task_async(self, *, receive_task: asyncio.Task[RealtimeTargetResult]) -> None: | |
| """Cancel and retrieve an unfinished Realtime receive task.""" | |
| if receive_task.done(): | |
| return | |
| receive_task.cancel() | |
| await asyncio.gather(receive_task, return_exceptions=True) | |
| async def _cancel_receive_task_async( | |
| self, | |
| *, | |
| receive_task: asyncio.Task[RealtimeTargetResult], | |
| ) -> None: | |
| """Cancel and retrieve a Realtime receive task.""" | |
| if not receive_task.done(): | |
| receive_task.cancel() | |
| await asyncio.gather(receive_task, return_exceptions=True) | |
so exceptions from tasks that are already completed are still retrieved
| logger.info(f"Disconnected from {self._endpoint} with conversation ID: {conversation_id}") | ||
| except Exception as e: | ||
| logger.warning(f"Error closing connection for {conversation_id}: {e}") | ||
| del self._existing_conversation[conversation_id] |
Contributor
There was a problem hiding this comment.
Could we shield and finish connection.close() before propagating cancellation? Since the connection is popped first, cancellation during close() can leave the socket open with no remaining handle for later cleanup.
async def cleanup_conversation_async(self, conversation_id: str) -> None:
connection = self._existing_conversation.pop(conversation_id, None)
if not connection:
return
close_task = asyncio.ensure_future(connection.close())
try:
await asyncio.shield(close_task)
except asyncio.CancelledError as cancellation_error:
try:
await close_task
except BaseException as close_error:
raise cancellation_error from close_error
raise
except Exception as e:
logger.warning(f"Error closing connection for {conversation_id}: {e}")
else:
logger.info(f"Disconnected from {self._endpoint} with conversation ID: {conversation_id}")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cancellation during provider setup or response handling can leave shared work cancelled or background resources running. This change makes target cleanup deterministic without changing the provider-attempt design.
Tests and Documentation
git diff --check.