Skip to content

fix: don't double-wrap WebRTCError raised inside connect()#48

Merged
AdirAmsalem merged 6 commits intomainfrom
avoid-double-wrap-errors
Apr 20, 2026
Merged

fix: don't double-wrap WebRTCError raised inside connect()#48
AdirAmsalem merged 6 commits intomainfrom
avoid-double-wrap-errors

Conversation

@AdirAmsalem
Copy link
Copy Markdown
Contributor

@AdirAmsalem AdirAmsalem commented Apr 20, 2026

Summary

Errors surfaced through realtime.connect() were being reported twice and with a redundant nested cause. When a WebRTCError was raised from inside connect() (e.g. a server capacity rejection, or an initial image/prompt failure), the outer except Exception handler re-wrapped it into another WebRTCError and emitted it through the on_error callback a second time — once from the original raise site, once from the except block.

Callers now see a single, unwrapped error; on_error fires once per failure. Follow-up to #47, addressing review feedback.

Test plan

  • New regression test asserts no nested cause and no duplicate on_error invocation
  • Full realtime unit suite still passes (38/38)

Note

Low Risk
Small, localized error-handling change with added unit coverage; main risk is subtly altering callback firing order/behavior on edge-case connection failures.

Overview
Prevents WebRTCConnection.connect() from double-wrapping a WebRTCError (and from emitting on_error twice) by adding a per-connect() _on_error_fired guard and a dedicated except WebRTCError path that re-raises as-is.

Adds regression tests covering both a server-error/_handle_error race and direct WebRTCError raises (e.g., ack timeouts), asserting no nested cause and exactly one on_error callback invocation.

Reviewed by Cursor Bugbot for commit a40c2ad. Bugbot is set up for automated code reviews on this repo. Configure here.

The blanket `except Exception` handler in connect() was re-wrapping every
WebRTCError into another WebRTCError (message=str(original), cause=original)
and emitting it through on_error a second time — once from the original
raise site (e.g. _handle_error), once from the except block.

Catch WebRTCError separately and re-raise as-is so callers see a single,
unwrapped error with no redundant cause chain.
The prior fix skipped on_error for all WebRTCError exceptions in connect(),
which regressed direct-raise paths (e.g. ack timeouts) that don't flow
through _handle_error. Gate the skip on _connection_error being set so only
the _handle_error path (which already emitted on_error) is deduped.
_connection_error was doubling as a proxy for "on_error was already fired".
That coupling was implicit and would break silently if a future code path
set _connection_error without firing on_error. Replace with a dedicated
flag that expresses intent directly and is set at each call site.
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e9ab59b. Configure here.

Comment thread decart/realtime/webrtc_connection.py
The new except WebRTCError branch was missing the logger.error call that
the generic except Exception branch has, so client-side timeout paths
(ack timeouts from _send_initial_{prompt,image}_and_wait) had no log
entry. Server-originated errors were still logged by _handle_error.
@AdirAmsalem AdirAmsalem merged commit 3c6a145 into main Apr 20, 2026
9 checks passed
@AdirAmsalem AdirAmsalem deleted the avoid-double-wrap-errors branch April 20, 2026 11:23
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.

1 participant