Recover initial join when connect safety-timeout leaves no reconnect#1741
Merged
Conversation
… reconnect When RtcSession.connectInternal hits its own safety-timeout, the socket is left in a non-terminal state that stateJob ignores, so no Call.reconnect is ever launched and _join's didReconnectSucceed() would block forever. - Add SfuConnectionResult.Failure.reconnectTriggered so the join flow can tell whether a recovery loop has already been started by stateJob. - On the safety-timeout path, tear down the abandoned (still-in-flight) socket so a late Connected/JoinResponse can't resurface and resurrect the session. - In Call._join, trigger a REJOIN ourselves when a recoverable failure reports no reconnect was triggered; otherwise await the existing loop. - Rename canTriggersReconnect() to triggersStateJobReconnect() for clarity. - demo-app: align connectionTimeoutInMs with the SDK default (5s). Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
WalkthroughReconnect handling in ChangesRecoverable reconnect handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Call
participant RtcSession
participant SfuSocket
Call->>RtcSession: connectInternal()
RtcSession->>SfuSocket: connect, await terminal state
alt safety timeout, non-terminal
RtcSession->>SfuSocket: disconnect()
RtcSession-->>Call: Failure(recoverable=true, reconnectTriggered=false)
else terminal disconnected
RtcSession-->>Call: Failure(recoverable, reconnectTriggered via triggersStateJobReconnect)
end
Call->>Call: check reconnectTriggered
alt reconnectTriggered=false
Call->>Call: reconnect(WEBSOCKET_RECONNECT_STRATEGY_REJOIN)
else reconnectTriggered=true
Call->>Call: await existing reconnect outcome
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt (1)
361-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the safety-timeout reconnect contract too.
This test now covers socket teardown; add
reconnectTriggered=falseso it also locks the contract that makes_joinself-trigger REJOIN for this path.Proposed test assertion
assertTrue("Expected recoverable=true for a safety timeout", result.recoverable) + assertFalse( + "Expected reconnectTriggered=false for a safety timeout", + result.reconnectTriggered, + ) assertEquals( AnalyticsCallAbortReason.REQUEST_TIMEOUT, result.abortReason,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt` around lines 361 - 368, The safety-timeout test in RtcSessionTest2 currently verifies recoverable and disconnect behavior but does not lock the reconnect contract; update the same test around the `_join`/`join` flow to also assert that `reconnectTriggered` is false for this path. Use the existing result object in the safety-timeout scenario and add the missing assertion alongside the current `recoverable`, `abortReason`, and `mockSocketConnection.disconnect()` checks so the REJOIN self-trigger behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt`:
- Around line 361-368: The safety-timeout test in RtcSessionTest2 currently
verifies recoverable and disconnect behavior but does not lock the reconnect
contract; update the same test around the `_join`/`join` flow to also assert
that `reconnectTriggered` is false for this path. Use the existing result object
in the safety-timeout scenario and add the missing assertion alongside the
current `recoverable`, `abortReason`, and `mockSocketConnection.disconnect()`
checks so the REJOIN self-trigger behavior is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e2b610c-b328-4b9b-9bd8-09a10690a7d2
📒 Files selected for processing (5)
demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/JoinRecoverableFailureTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt
Contributor
SDK Size Comparison 📏
|
rahul-lohra
reviewed
Jul 8, 2026
rahul-lohra
reviewed
Jul 8, 2026
rahul-lohra
reviewed
Jul 8, 2026
aleksandar-apostolov
changed the title
Address PR review: reconnect-orchestration state should not live on the
SfuConnectionResult.Failure DTO. Replace the boolean flag with a typed error
so callers of connectInternal branch on the failure kind.
- Add sealed SfuConnectException with Timeout (connect safety-timeout tore down
a stuck socket; no recovery started) and Disconnected (terminal SFU state).
- connectInternal now returns these typed errors instead of Exception("msg").
- Drop SfuConnectionResult.Failure.hasReconnectStarted; _join self-triggers a
REJOIN only when the error is SfuConnectException.Timeout, else awaits the
loop stateJob already started.
- Update tests to construct/assert the typed errors.
Co-authored-by: Cursor <cursoragent@cursor.com>
rahul-lohra
reviewed
Jul 10, 2026
This was referenced Jul 10, 2026
* fix: Refactor with SfuConnectFailureCause code * fix: Add kdoc * fix: Replace Singletone shared Call.testInstanceProvider.rtcSessionCreator with Call..unitTestRtcSessionFactory
Contributor
|
All concerns resolved 👍 |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Follow-up to #1740 and AN-1294 That PR added a safety-timeout to
RtcSession.connectInternalso the SFU connect wait can no longer suspend forever. But there is still a gap on the initial join path: when the safety-timeout fires, the socket is left in a non-terminal state thatstateJobdoes not react to, so noCall.reconnectis ever launched._jointhen callsdidReconnectSucceed()and waits for a recovery loop that will never start — blocking indefinitely.This PR closes that gap and makes the abandoned connect attempt safe to leave behind.
Implementation
SfuConnectionResult.Failure.reconnectTriggered— new flag that tells the join flow whether a recovery loop has already been launched bystateJob:truefor terminal disconnect statesstateJobreacts to (aCall.reconnectis already in flight — the caller must not start another).falsefor the connect safety-timeout, which leaves the socket in a statestateJobignores (nothing will drive recovery on its own).connectInternalnow callssocketConnection.disconnect()before returning. This prevents a lateConnected/JoinResponsefrom an abandoned attempt resurfacing throughstateJoband resurrecting a dead session ("zombie socket").Call._joinself-triggers recovery — on a recoverable failure withreconnectTriggered == false,_joinnow launches aREJOINitself before awaiting the outcome (a full REJOIN, not FAST, because the initial join never completed — there is no negotiated media path to resume, so FAST would just burn attempts onPARTICIPANT_NOT_FOUND). WhenreconnectTriggered == trueit defers to the existing loop as before.SfuSocketState.Disconnected.canTriggersReconnect()→triggersStateJobReconnect()for clarity (it describes which statesstateJobacts on).connectionTimeoutInMswith the SDK default (5_000L) instead of the hardcoded12_000L.Testing
./gradlew :stream-video-android-core:testDebugUnitTest --tests "*.JoinRecoverableFailureTest" --tests "*.RtcSessionTest2"— passing./gradlew :stream-video-android-core:spotlessApply :demo-app:spotlessApply— cleanNew/updated unit tests:
JoinRecoverableFailureTestrecoverable failure with reconnect already triggered awaits the existing loop— asserts_joindoes not start its ownreconnectwhenreconnectTriggered == true.recoverable failure with no reconnect triggered starts a REJOIN itself— asserts_joinlaunches aREJOINwhenreconnectTriggered == false.RtcSessionTest2— safety-timeout test now verifies the abandoned socket isdisconnect()-ed.Manual: reproduced a silent connect hang (OkHttp
callTimeout/readTimeoutset to0locally + Charles breakpoint on the SFU WS upgrade). Confirmed the safety-timeout fires, the socket is torn down,_joinself-triggers a REJOIN, and the flow ends in a bounded failure instead of hanging.☑️Contributor Checklist
General
developbranchCode & documentation
SfuConnectionResult.Failure)stream-video-examples)☑️Reviewer Checklist
🎉 GIF
No UI change — skipping.
Summary by CodeRabbit