Skip to content

fix: route publisher negotiation errors to reconnect instead of leaking them - #1226

Open
RaphaelFakhri wants to merge 3 commits into
livekit:mainfrom
RaphaelFakhri:fix/negotiation-error-unhandled
Open

RaphaelFakhri wants to merge 3 commits into
livekit:mainfrom
RaphaelFakhri:fix/negotiation-error-unhandled

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Summary

Fixes #1093.

Transport.negotiate is a debounced function. The debouncer discards the future that createAndSendOffer() returns, so a rejection (for example setLocalDescription failing with "The order of m-lines in subsequent offer doesn't match") became an unhandled error. The try/catch in Engine.negotiate never saw it, because publisher.negotiate(null) is not awaited and returns immediately. As a result, the reconnect that the catch block was meant to trigger never ran, and apps that install PlatformDispatcher.onError saw the raw NegotiationError.

This change:

  • Adds Transport.onNegotiationError, and the debounced callback catches errors from createAndSendOffer() and reports them through it.
  • Sets the callback in Engine to the previous catch-block logic: set fullReconnectOnNext for a NegotiationError, then call handleReconnect(negotiationFailed).
  • Removes the dead try/catch from Engine.negotiate.

Test plan

  • New test/core/transport_negotiate_test.dart:
    • a peer connection whose setLocalDescription rejects reports a NegotiationError to onNegotiationError and leaks no uncaught zone error;
    • a normal negotiation sends one offer and reports nothing.
  • Before the change the first test fails with an uncaught NegotiationError. After the change it passes.
  • flutter analyze, dart format . --set-exit-if-changed, import_sorter and the full flutter test suite (440 tests) pass.
  • Added a .changes entry.

@CLAassistant

CLAassistant commented Sep 29, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

devin-ai-integration[bot]

This comment was marked as resolved.

@RaphaelFakhri

Copy link
Copy Markdown
Author

Valid finding, fixed in the new commit. setRemoteDescription now sends the offer deferred by renegotiate through the same error-reporting path as the debounced negotiate, so a rejected deferred offer reaches onNegotiationError and triggers the reconnect. Direct createAndSendOffer calls, such as the one in resumeConnection, still propagate their errors to the caller. Two tests cover this: one starts with a local offer, sets renegotiate, and rejects the post-answer offer; the other checks that a direct call still throws.

This branch has not been deployed

No deployments
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.

NegotiationError from createAndSendOffer escapes to zone error handler (debounced negotiate discards inner Future)

2 participants