handle SFU_FULL errors by excluding failed SFUs and retrying with migration#1631
Open
PratimMallick wants to merge 5 commits intodevelopfrom
Open
handle SFU_FULL errors by excluding failed SFUs and retrying with migration#1631PratimMallick wants to merge 5 commits intodevelopfrom
PratimMallick wants to merge 5 commits intodevelopfrom
Conversation
…h migration - Track failed SFU IDs (edge names) and send them via migrating_from_list so the coordinator can exclude full/unreachable SFUs when assigning a new one - Add retry-then-migrate logic in RtcSession: after 2 consecutive connection failures to the same SFU, stop retrying and trigger migration to a new SFU - Use SFU edge name (ID) instead of URL for migrating_from, migrating_from_list, and from_sfu_id in ReconnectDetails - Clear failed SFU list on successful SFU connection establishment Made-with: Cursor
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
PratimMallick
changed the title
Contributor
SDK Size Comparison 📏
|
WalkthroughThese changes implement SFU edge failover and migration tracking. The modifications introduce a failed SFU ID tracking mechanism, automatic retry logic with migration triggers in RtcSession, and extend API contracts to support a list of previously accessed SFU locations via Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Call
participant RtcSession
participant SFU
Client->>Call: join/migrate()
Call->>Call: Check failedSfuIds
Call->>Call: Auto-populate migratingFromList from failedSfuIds
Call->>RtcSession: Create with sfuName, migratingFromList
RtcSession->>SFU: Attempt connection
alt Connection Succeeds
SFU->>RtcSession: Connected
RtcSession->>RtcSession: Reset sfuConnectionRetryCount
RtcSession->>Call: onSfuConnectionEstablished()
Call->>Call: clearFailedSfuIds()
else Connection Fails (Transient/WebSocket Loss)
SFU->>RtcSession: DisconnectedTemporarily
RtcSession->>RtcSession: Increment sfuConnectionRetryCount
alt Retry Limit Reached
RtcSession->>RtcSession: sfuConnectionRetryCount >= 2
RtcSession->>Call: migrate()
Call->>Call: addFailedSfuId(oldSfuName)
Call->>Call: Auto-populate migratingFromList from failedSfuIds
Call->>RtcSession: Create new RtcSession with updated migratingFromList
else Retry Remaining
RtcSession->>RtcSession: Log retry attempt
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (2)
1496-1513: Consider adding a comment toclearFailedSfuIdsfor consistency.While
clearFailedSfuIds()correctly relies onsynchronizedList's atomicclear()operation, the other methods use explicitsynchronizedblocks. Adding a brief comment explaining why explicit synchronization isn't needed here would improve clarity.📝 Suggested comment
/** Clears the failed SFU list (e.g. after a successful join). */ private fun clearFailedSfuIds() { + // clear() is atomic on synchronizedList; no explicit sync needed failedSfuIds.clear() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt` around lines 1496 - 1513, Add a brief explanatory comment to clearFailedSfuIds() noting that it omits an explicit synchronized block because failedSfuIds is a Collections.synchronizedList (or otherwise thread-safe) and its clear() is atomic/guarded, matching the intent of addFailedSfuId and getFailedSfuIdsSnapshot; reference failedSfuIds, addFailedSfuId, and getFailedSfuIdsSnapshot to show consistency with the other methods.
1528-1545: Variable shadowing may reduce readability.Line 1532 shadows the
migratingFromListparameter with a local variable of the same name. While intentional for auto-populating from the failed list, this can be confusing during code review or debugging.Consider renaming the local variable to make the intent clearer:
♻️ Suggested rename for clarity
- val migratingFromList = migratingFromList ?: getFailedSfuIdsSnapshot().takeIf { it.isNotEmpty() } + val resolvedMigratingFromList = migratingFromList ?: getFailedSfuIdsSnapshot().takeIf { it.isNotEmpty() } val result = clientImpl.joinCall( type, id, create = create != null, members = create?.memberRequestsFromIds(), custom = create?.custom, settingsOverride = create?.settings, startsAt = create?.startsAt, team = create?.team, ring = ring, notify = notify, location = location, migratingFrom = migratingFrom, - migratingFromList = migratingFromList, + migratingFromList = resolvedMigratingFromList, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt` around lines 1528 - 1545, The local variable migratingFromList inside the joinCall function is shadowing the migratingFromList parameter; rename the local to something like migratingFromFallback or defaultMigratingFromList and update its usage in the clientImpl.joinCall(...) call (the migratingFromList= argument) so the parameter name isn't shadowed; change any references in this function (e.g., the assignment from getFailedSfuIdsSnapshot() and the migratingFromList argument passed to clientImpl.joinCall) to use the new variable name.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (1)
634-674: Verify the interaction between socket auto-reconnection and migration trigger.The retry logic increments on
DisconnectedTemporarilyandWebSocketEventLoststates, triggering migration after 2 occurrences. However, these states may be emitted during the socket's internal reconnection attempts before it fully recovers.Consider whether the socket could naturally recover between disconnected states. If the socket emits
Connectedduring recovery, the counter resets correctly (line 636). If it keeps failing, migration is triggered as intended.One observation: after calling
disconnect()andmigrate(), the old state flow continues until the session is cleaned up. This appears to be handled correctly sincemigrate()eventually callsoldSession?.cleanup()which cancels the supervisor job.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt` around lines 634 - 674, The retry/migrate logic can prematurely trigger migrate while the socket is still performing internal reconnection attempts; update the handling around SfuSocketState to avoid this by (1) resetting sfuConnectionRetryCount when you see SfuSocketState.Connecting in addition to Connected (handle in the when branch for SfuSocketState.Connecting and/or before incrementing), and (2) add a simple time-window guard using a lastSfuDisconnectTimestamp to only count successive DisconnectedTemporarily/WebSocketEventLost occurrences within a short window (e.g., few seconds) before incrementing sfuConnectionRetryCount and calling call.migrate(); keep the existing disconnect via sfuConnectionModule.socketConnection.disconnect() and the call.migrate() invocation but only execute migrate when both retry count >= MAX_SFU_CONNECTION_RETRIES and the disconnects occurred within the configured time window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`:
- Around line 1496-1513: Add a brief explanatory comment to clearFailedSfuIds()
noting that it omits an explicit synchronized block because failedSfuIds is a
Collections.synchronizedList (or otherwise thread-safe) and its clear() is
atomic/guarded, matching the intent of addFailedSfuId and
getFailedSfuIdsSnapshot; reference failedSfuIds, addFailedSfuId, and
getFailedSfuIdsSnapshot to show consistency with the other methods.
- Around line 1528-1545: The local variable migratingFromList inside the
joinCall function is shadowing the migratingFromList parameter; rename the local
to something like migratingFromFallback or defaultMigratingFromList and update
its usage in the clientImpl.joinCall(...) call (the migratingFromList= argument)
so the parameter name isn't shadowed; change any references in this function
(e.g., the assignment from getFailedSfuIdsSnapshot() and the migratingFromList
argument passed to clientImpl.joinCall) to use the new variable name.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt`:
- Around line 634-674: The retry/migrate logic can prematurely trigger migrate
while the socket is still performing internal reconnection attempts; update the
handling around SfuSocketState to avoid this by (1) resetting
sfuConnectionRetryCount when you see SfuSocketState.Connecting in addition to
Connected (handle in the when branch for SfuSocketState.Connecting and/or before
incrementing), and (2) add a simple time-window guard using a
lastSfuDisconnectTimestamp to only count successive
DisconnectedTemporarily/WebSocketEventLost occurrences within a short window
(e.g., few seconds) before incrementing sfuConnectionRetryCount and calling
call.migrate(); keep the existing disconnect via
sfuConnectionModule.socketConnection.disconnect() and the call.migrate()
invocation but only execute migrate when both retry count >=
MAX_SFU_CONNECTION_RETRIES and the disconnects occurred within the configured
time window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce530746-8075-467a-97f1-41b30ae65d2b
⛔ Files ignored due to path filters (1)
stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/JoinCallRequest.ktis excluded by!**/generated/**
📒 Files selected for processing (4)
stream-video-android-core/api/stream-video-android-core.apistream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
- Only increment sfuConnectionRetryCount on DisconnectedTemporarily (actual connection failure), not on WebSocketEventLost (intermediate HealthMonitor transition state) - Use > instead of >= for retry threshold so the SDK performs 2 full retry attempts before triggering migration to a new SFU Made-with: Cursor
rahul-lohra
reviewed
Mar 16, 2026
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Outdated
Show resolved
Hide resolved
…FU_FULL simulation - Handle MIGRATE, REJOIN, DISCONNECT strategies immediately in stateJob (previously only handled in unreachable errorJob) - FAST/UNSPECIFIED strategies go through retry counter before escalating to migrate after MAX_SFU_CONNECTION_RETRIES - Remove dead errorJob (errors() flow never receives SFU errors since all are NetworkError routed through state machine) - Add simulateSfuFull() debug function to inject SFU_FULL (700) error - Add "Simulate SFU Full (700)" button in demo app debug menu - Add unit tests for all reconnect strategy behaviors Made-with: Cursor
|
PratimMallick
added
pr:improvement
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
2 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
When the SDK receives an SFU_FULL error (or any recoverable connection failure) from an SFU, it can enter a REJOIN loop — repeatedly requesting credentials from the coordinator and being assigned the same full SFU. This happens because the coordinator doesn't know which SFUs the client has already failed to connect to. Additionally, if an SFU becomes unreachable for any reason (network issues, timeouts), the client retries the same SFU indefinitely via the HealthMonitor without ever requesting a new one.
This change ensures the SDK:
Tracks SFUs it failed to connect to and tells the coordinator to exclude them when requesting new credentials.
Stops retrying an unreachable SFU after 2 attempts and triggers a migration to a new SFU.
Uses SFU IDs (edge_name) instead of URLs for migrating_from, migrating_from_list, and from_sfu_id in ReconnectDetails, aligning with the server-side expectations.
Implementation
Call.kt:
addFailedSfuId() / getFailedSfuIdsSnapshot() / clearFailedSfuIds() manage this list.
2.migrate() adds the current SFU's edgeName to the failed list before requesting new credentials, and passes it as migratingFrom.
3.joinRequest() automatically populates migratingFromList from the failed SFU snapshot if no explicit list is provided.
onSfuConnectionEstablished() clears the failed list once a connection succeeds (called from RtcSession).
4.All references to SFU URLs in migratingFrom, migratingFromList, and ReconnectDetails.from_sfu_id are replaced with SFU edge names.
RtcSession.kt:
1.Added sfuName (edge name) constructor parameter for identifying the current SFU.
2.Added sfuConnectionRetryCount that increments on DisconnectedTemporarily and WebSocketEventLost states.
3.After MAX_SFU_CONNECTION_RETRIES (2) consecutive failures, disconnects the current socket and calls call.migrate() to request a new SFU from the coordinator.
4.Resets the counter on Connected state.
StreamVideoClient.kt
Testing
SFU_FULL scenario:
Simulate or trigger an SFU_FULL error from the SFU. Verify the client adds the full SFU's edge name to the exclusion list, requests new credentials with migrating_from_list populated, and connects to a different SFU.
Unreachable SFU:
Disconnect or block the SFU endpoint. Verify the client retries twice, then triggers a migration instead of retrying indefinitely.
Successful connection clears state:
After migration to a new SFU succeeds, verify failedSfuIds is cleared so future requests are not unnecessarily constrained.
Normal rejoin unaffected:
Verify that a simple network reconnection (rejoin) does not add the current SFU to the exclusion list, since the client may want to reconnect to the same SFU.
Unit tests can mock SfuSocketState transitions and verify that migrate() is called after 2 DisconnectedTemporarily events, and that clearFailedSfuIds() is called on Connected.
☑️Contributor Checklist
General
developbranchCode & documentation
stream-video-examples)☑️Reviewer Checklist
🎉 GIF
Please provide a suitable gif that describes your work on this pull request
Summary by CodeRabbit
New Features
Improvements