fix(llc): ignore the ws channel ready future on connect - #2927
Conversation
`AdapterWebSocketChannel` reports a connect failure on three futures: `ready`, the stream, and `sink.done`. `_initWebSocketChannel` handled the last two but never listened to `ready`, so every failed connect left an errored future in the root zone. Apps routing `PlatformDispatcher.onError` into a crash reporter saw each one as a fatal crash, up to `maxReconnectAttempts` per outage. Closes #2921 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesWebSocket error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change prevents duplicate connection errors from being reported as uncaught application failures without changing reconnect or stream behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2927 +/- ##
=======================================
Coverage 74.17% 74.18%
=======================================
Files 437 437
Lines 28373 28374 +1
=======================================
+ Hits 21047 21048 +1
Misses 7326 7326 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Submit a pull request
Linear: FLU-
Closes: #2921
CLA
Description of the pull request
AdapterWebSocketChannelreports a connect failure on three futures:_initWebSocketChannelhandled (2) viaonError: _onConnectionErrorand (3) via the existingsink.done.ignore(), but never listened to (1). AFuturecompleted with an error and never listened to is reported toZone.current.handleUncaughtError— in a Flutter app,PlatformDispatcher.onError— so apps routing that into a crash reporter saw every failed connect as a fatal crash:Reconnect behaviour was never affected — the error is handled by
_onConnectionError, and the orphan is a silent duplicate that only shows up once a zone /PlatformDispatchererror handler is installed. That is why #2304 looked non-reproducible in a debug run. Report volume is high because_reconnectbuilds a fresh channel per attempt (up tomaxReconnectAttempts, default 6) and_ChatLifecycleManagercallsmaybeReconnect()on every foreground and connectivity change.The fix is one line, next to the existing one and for the same reason:
How this was verified
Against an unresolvable host, mirroring
_initWebSocketChannel+_subscribeToWebSocketChanneland counting errors that reach a zone handler:stream.onErrorstill receives its copy —.ignore()onreadyonly marks that future handled, it does not consume the stream's delivery._closeWebSocketChannelwas checked for the same failure class (closing a failed channel, a healthy one, one the server already dropped, and asink.addon a dead channel): zero orphans in all four, becausesink.close()hands back the same future asdone, which is already ignored. No change needed there.Credit to @jm-harman for the diagnosis in #2921.
Test coverage
Added
`connect` does not orphan the channel `ready` error— stubsreadyto complete with an error and asserts the SDK reads it, with apumpEventQueue()guard so a future regression that readsreadybut drops the result also fails. Reverting the fix makes the test fail. Fullstream_chatsuite passes (1626 tests).Screenshots / Videos
Not applicable — no UI change.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Deprecations