Repository navigation
fix(responses): run stream failure and success hooks on the iterating loop instead of blocking it #43270
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
fix(responses): run stream failure and success hooks on the iterating loop instead of blocking it #43270
Changes from all commits
a2ac961
f1407ac
0be4a64
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 |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| import time | ||
| import traceback | ||
| import uuid | ||
| from collections.abc import Awaitable, Callable, Iterable, Mapping, Sequence | ||
| from collections.abc import Awaitable, Callable, Coroutine, Iterable, Mapping, Sequence | ||
| from datetime import datetime | ||
| from functools import lru_cache | ||
| from types import MappingProxyType | ||
|
|
@@ -169,6 +169,26 @@ def _log_background_task_failure(task: asyncio.Task[object], *, task_name: str) | |
| verbose_logger.error("%s failed: %s", task_name, exception) | ||
|
|
||
|
|
||
| _PENDING_LOGGING_TASKS: Final[set[asyncio.Task[object]]] = set() # mutable-ok: strong refs to pending logging tasks | ||
|
|
||
|
|
||
| def _running_loop() -> asyncio.AbstractEventLoop | None: | ||
| try: | ||
| return asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| return None | ||
|
|
||
|
|
||
| def _spawn_logging_task( | ||
| running_loop: asyncio.AbstractEventLoop, coroutine: Coroutine[object, object, object], *, task_name: str | ||
| ) -> asyncio.Task[object]: | ||
| task: Final = running_loop.create_task(coroutine) | ||
| _PENDING_LOGGING_TASKS.add(task) | ||
| task.add_done_callback(_PENDING_LOGGING_TASKS.discard) | ||
| task.add_done_callback(lambda done: _log_background_task_failure(done, task_name=task_name)) | ||
| return task | ||
|
|
||
|
|
||
| _ERROR_CODE_HTTP_STATUS: Final[Mapping[str, int]] = MappingProxyType( | ||
| { | ||
| "server_error": 500, | ||
|
|
@@ -275,6 +295,8 @@ class BaseResponsesAPIStreamingIterator: | |
| This class contains shared logic for both synchronous and asynchronous iterators. | ||
| """ | ||
|
|
||
| _pending_logging_tasks: tuple[asyncio.Task[object], ...] = () | ||
|
|
||
| def __init__( | ||
| self, | ||
| response: httpx.Response, | ||
|
|
@@ -839,8 +861,21 @@ def _run_post_success_hooks(self, end_time: datetime): | |
| except Exception: | ||
| typed_call_type = None | ||
|
|
||
| running_loop: Final = _running_loop() | ||
| if running_loop is not None: | ||
| self._record_pending_logging_task( | ||
| _spawn_logging_task( | ||
| running_loop, | ||
| async_post_call_success_deployment_hook( | ||
| request_data=request_payload, | ||
| response=self.completed_response, | ||
| call_type=typed_call_type, | ||
| ), | ||
| task_name="Responses stream post-call success hook", | ||
| ) | ||
| ) | ||
| return | ||
| try: | ||
| # Call synchronously; async hook will be executed via asyncio.run in a new loop | ||
| run_async_function( | ||
| async_function=async_post_call_success_deployment_hook, | ||
| request_data=request_payload, | ||
|
|
@@ -861,28 +896,63 @@ def _handle_failure(self, exception: Exception): | |
| self._failure_handled = True | ||
|
|
||
| traceback_exception: Final = traceback.format_exc() | ||
| end_time: Final = datetime.now() | ||
| running_loop: Final = _running_loop() | ||
| if running_loop is not None: | ||
| self._record_pending_logging_task( | ||
| _spawn_logging_task( | ||
| running_loop, | ||
| self._run_failure_handlers_in_order(exception, traceback_exception, end_time), | ||
| task_name="Responses stream failure logging", | ||
| ) | ||
| ) | ||
| return | ||
| try: | ||
| run_async_function( | ||
| async_function=self.logging_obj.async_failure_handler, | ||
| exception=exception, | ||
| traceback_exception=traceback_exception, | ||
| start_time=self.start_time, | ||
| end_time=datetime.now(), | ||
| end_time=end_time, | ||
| ) | ||
| except Exception: | ||
| pass | ||
| self._submit_sync_failure_handler(exception, traceback_exception, end_time) | ||
|
|
||
| async def _run_failure_handlers_in_order( | ||
| self, exception: Exception, traceback_exception: str, end_time: datetime | ||
| ) -> None: | ||
| try: | ||
| await self.logging_obj.async_failure_handler( | ||
| exception=exception, | ||
| traceback_exception=traceback_exception, | ||
| start_time=self.start_time, | ||
| end_time=end_time, | ||
| ) | ||
| finally: | ||
| self._submit_sync_failure_handler(exception, traceback_exception, end_time) | ||
|
|
||
| def _submit_sync_failure_handler(self, exception: Exception, traceback_exception: str, end_time: datetime) -> None: | ||
| try: | ||
| executor.submit( | ||
| self.logging_obj.failure_handler, | ||
| exception, | ||
| traceback_exception, | ||
| self.start_time, | ||
| datetime.now(), | ||
| end_time, | ||
| ) | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| except Exception: | ||
| pass | ||
|
|
||
| def _record_pending_logging_task(self, task: asyncio.Task[object]) -> None: | ||
| self._pending_logging_tasks = (*self._pending_logging_tasks, task) | ||
|
|
||
| async def _await_pending_logging(self) -> None: | ||
| pending: Final = self._pending_logging_tasks | ||
| self._pending_logging_tasks = () | ||
| if pending: | ||
| await asyncio.wait(pending) | ||
|
|
||
| def _note_yielded_event(self, event: ResponsesAPIStreamingResponse) -> None: | ||
| self._yielded_first_chunk = True | ||
| if event.type not in PRE_OUTPUT_LIFECYCLE_EVENT_TYPES: | ||
|
|
@@ -970,6 +1040,13 @@ def __aiter__(self): | |
| return self | ||
|
|
||
| async def __anext__(self) -> ResponsesAPIStreamingResponse: | ||
| try: | ||
| return await self._next_event() | ||
| except Exception: | ||
| await self._await_pending_logging() | ||
| raise | ||
|
Comment on lines
+1045
to
+1047
Contributor
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.
Contributor
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. Base blocked the whole loop for the same duration, so this is parity. Awaiting also keeps failed-deployment attribution correct during router fallbacks
Contributor
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. You're right. The important distinction is that this Waiting before propagating the exception is intentional: it preserves the existing ordering and ensures the primary deployment's failure is logged before router fallback can reuse or mutate the logging object. The added regression test also codifies that attribution requirement. This can still delay the affected consumer if a callback never completes, but that is a per-stream delay rather than a proxy-wide event-loop stall. I’m withdrawing the original finding. |
||
|
|
||
| async def _next_event(self) -> ResponsesAPIStreamingResponse: | ||
| try: | ||
| self._check_max_streaming_duration() | ||
| while True: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.