Skip to content

FIX Harden prompt target cancellation cleanup - #2483

Open
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz-cancellation-hardening
Open

FIX Harden prompt target cancellation cleanup#2483
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz-cancellation-hardening

Conversation

@romanlutz

Copy link
Copy Markdown
Contributor

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.

  • Shield Hugging Face model loading so cancelling one send does not cancel the shared load task for other callers.
  • Remove and close partially configured OpenAI Realtime connections when setup is cancelled or fails.
  • Cancel and await unfinished Realtime receive tasks when text or audio sends fail, preventing orphaned listeners.

Tests and Documentation

  • Added unit coverage for cancellation during Realtime session configuration.
  • Added unit coverage for receive-task cleanup after response creation fails.
  • Added unit coverage confirming send cancellation preserves shared Hugging Face model loading.
  • Ran both focused target test modules: 120 passed.
  • Ran Ruff check and format checks, changed-file pre-commit hooks including ty, and git diff --check.
  • Documentation: N/A. This is an internal lifecycle cleanup fix with no public API change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@hannahwestra25 hannahwestra25 self-assigned this Aug 25, 2026
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")

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.

2 participants