-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(openai): fail generate_reply fast on conversation_already_has_act… #6818
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 |
|---|---|---|
|
|
@@ -2282,6 +2282,17 @@ def _handle_error(self, event: RealtimeErrorEvent) -> None: | |
| if not _is_fatal_error(event.error): | ||
| return | ||
|
|
||
| # a rejected response.create (e.g. the conversation already has an active response) draws | ||
| # an error, not a response.created, so nothing else settles the future generate_reply | ||
| # handed out. Fail it now with the provider code attached, instead of orphaning it until | ||
| # the 10s timeout turns it into a generic "generate_reply timed out". Fall through so the | ||
| # error still surfaces as an "error" event. | ||
| if (event_id := event.error.event_id) and ( | ||
|
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. maybe merge this and the above check under a single
Contributor
Author
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. Done — merged both waiter checks under a single if event_id := event.error.event_id:. Since the chat-ctx and response.create event-ids are separate namespaces (chat_ctx_* vs response_create_*) they can't collide, so the second check is now an elif, and the comments are trimmed to one line each. The response.create branch intentionally falls through instead of returning, so the error still hits the existing emit path (recoverable) or the fatal _is_fatal_error raise (terminal) — same reconnect-stopping behavior as before. |
||
| fut := self._response_created_futures.pop(event_id, None) | ||
| ): | ||
| if not fut.done(): | ||
| fut.set_exception(llm.RealtimeError(event.error.message, code=event.error.code)) | ||
|
|
||
| if event.error.message.startswith("Cancellation failed"): | ||
| return | ||
|
|
||
|
|
||
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.
simplify the docstring?
handle.exception()is worth to mention but maybe not the example here.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.
Trimmed to a short Note: — kept the handle.exception() guidance and the conversation_already_has_active_response code so callers know what to branch on, and dropped the full retry example.