Add stateScope for StateFlows that survive call.leave()#1618
Add stateScope for StateFlows that survive call.leave()#1618aleksandar-apostolov wants to merge 3 commits intodevelopfrom
stateScope for StateFlows that survive call.leave()#1618Conversation
Introduce a separate stateScope that lives for the entire Call object lifetime and is NOT cancelled on leave(). This allows StateFlows using stateIn() to survive leave() and continue receiving coordinator updates. The existing scope behavior is preserved - it's still cancelled on leave() to clean up RTC-related coroutines. Only the stateIn() flows in CallState, CallStats, and ParticipantState now use stateScope. This approach (Rahul's Approach 3) provides structural safety: - Existing cancellation behavior preserved (scope cancelled on leave) - If a stateIn is missed, it dies on leave - safe default, no zombies - StateFlows that need updates after leave() explicitly use stateScope
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
aleksandar-apostolov
added
the
pr:improvement
aleksandar-apostolov
changed the title
stateScope for StateFlows that survive call.leave()
Remove shutDownJobsGracefully() which cancelled scope and supervisorJob. This prevented rejoining after leave() since the scope was dead. RTC-related jobs are already manually cancelled in leave(): - sfuEvents, monitorPublisherPCStateJob, monitorSubscriberPCStateJob - leaveTimeoutAfterDisconnect, callStatsReportingJob RtcSession.cleanup() cancels its own supervisorJob, handling its coroutines.
WalkthroughThe pull request introduces a new long-lived Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (1)
351-424:⚠️ Potential issue | 🟡 Minor
livestreamFlowusesscope.launchinternally but is shared viastateScope— will break silently afterleave().The
channelFlowat line 351 launches coroutines onscope(lines 357, 366), which is cancelled onleave(). However,stateIn(stateScope, ...)at line 424 keeps the flow alive. Afterleave(), the innerscope.launchcalls silently fail, so the flow stops emitting new values while appearing active.Since this API is
@Deprecated, the degraded behavior is acceptable, but worth noting. If this flow is ever un-deprecated or copied, the inner launches should usestateScope(or thechannelFlow's own coroutine scope viathis.launch).🤖 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/CallState.kt` around lines 351 - 424, livestreamFlow launches coroutines on scope (via scope.launch) while the resulting flow is kept alive with stateIn(stateScope...), so after leave() those launched coroutines are cancelled and emissions stop silently; fix by switching the internal launches in livestreamFlow to run on the channelFlow's coroutine scope (use this.launch or plain launch inside channelFlow) or explicitly use stateScope for those launches so emissions continue while stateIn is active—update the two scope.launch blocks inside livestreamFlow (the one collecting _participants and the one subscribing to call) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`:
- Around line 169-177: The stateScope CoroutineScope is created with an
unparented SupervisorJob(), causing leaks; change its construction to parent the
SupervisorJob to the clientImpl.scope's Job (e.g., combine
clientImpl.scope.coroutineContext with
SupervisorJob(clientImpl.scope.coroutineContext[Job])) so it is cancelled when
the client is torn down, and also add an explicit cancellation in the Call
cleanup path (in cleanup() — after shutDownJobsGracefully() and related
teardown) by calling stateScope.cancel() to ensure final teardown of any
stateIn() coroutines; update references around the Call constructor where
stateScope is defined and ensure CallState continues to receive the same scope.
---
Outside diff comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 351-424: livestreamFlow launches coroutines on scope (via
scope.launch) while the resulting flow is kept alive with
stateIn(stateScope...), so after leave() those launched coroutines are cancelled
and emissions stop silently; fix by switching the internal launches in
livestreamFlow to run on the channelFlow's coroutine scope (use this.launch or
plain launch inside channelFlow) or explicitly use stateScope for those launches
so emissions continue while stateIn is active—update the two scope.launch blocks
inside livestreamFlow (the one collecting _participants and the one subscribing
to call) accordingly.
| /** | ||
| * A long-lived scope for StateFlows that need to survive leave() and receive coordinator updates. | ||
| * This scope lives for the entire lifetime of the Call object and is NOT cancelled on leave(). | ||
| * Used for stateIn() operators in CallState, CallStats, and ParticipantState. | ||
| */ | ||
| internal val stateScope = CoroutineScope(clientImpl.scope.coroutineContext + SupervisorJob()) | ||
|
|
||
| /** The call state contains all state such as the participant list, reactions etc */ | ||
| val state = CallState(client, this, user, scope) | ||
| val state = CallState(client, this, user, scope, stateScope) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for stateScope.cancel and related cancellation patterns
echo "=== Searching for stateScope.cancel or .close ==="
rg -n 'stateScope\.(cancel|close)' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
echo ""
echo "=== Searching for stateScope initialization and usage ==="
rg -n 'stateScope' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt | head -20
echo ""
echo "=== Looking at cleanup() and shutDownJobsGracefully() methods ==="
sed -n '1519,1540p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktRepository: GetStream/stream-video-android
Length of output: 1217
🏁 Script executed:
#!/bin/bash
# Check the internalLeave method to understand the full teardown sequence
echo "=== Examining internalLeave() method ==="
sed -n '985,1010p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
echo ""
echo "=== Check if there's a destroy or onCleared method that might cancel stateScope ==="
rg -n 'fun (destroy|onCleared|dispose|finalCleanup)' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktRepository: GetStream/stream-video-android
Length of output: 1000
🏁 Script executed:
#!/bin/bash
# Verify the exact SupervisorJob initialization
echo "=== Get the exact line 174 context ==="
sed -n '169,180p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktRepository: GetStream/stream-video-android
Length of output: 765
stateScope is never cancelled — resource leak when the Call is discarded.
SupervisorJob() creates an unparented job, leaving stateScope completely disconnected from the clientImpl.scope hierarchy. While the design comment correctly notes that stateScope should survive leave() to support reuse, there is no code path that ever cancels stateScope — not in cleanup(), not in shutDownJobsGracefully(), and not anywhere else. Every stateIn() coroutine launched on stateScope will run until process death, even if the Call object is discarded without being reused.
Two fixes to consider:
- Parent the
SupervisorJobtoclientImpl.scope's job so it gets cancelled when the client is torn down:
Proposed fix
- internal val stateScope = CoroutineScope(clientImpl.scope.coroutineContext + SupervisorJob())
+ internal val stateScope = CoroutineScope(
+ clientImpl.scope.coroutineContext + SupervisorJob(clientImpl.scope.coroutineContext[Job])
+ )- Additionally, explicitly cancel
stateScopeincleanup()for final teardown:
fun cleanup() {
session?.cleanup()
shutDownJobsGracefully()
callStatsReportingJob?.cancel()
mediaManager.cleanup()
session = null
scopeProvider.cleanup()
stateScope.cancel() // final teardown
}Fix (1) is the minimum — without it, stateScope outlives even the StreamVideo client itself.
Violates: "Keep concurrency deterministic—use structured coroutines and avoid global scope" and "Ensure cleanup/teardown paths handle cancellation and failure."
🤖 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 169 - 177, The stateScope CoroutineScope is created with an
unparented SupervisorJob(), causing leaks; change its construction to parent the
SupervisorJob to the clientImpl.scope's Job (e.g., combine
clientImpl.scope.coroutineContext with
SupervisorJob(clientImpl.scope.coroutineContext[Job])) so it is cancelled when
the client is torn down, and also add an explicit cancellation in the Call
cleanup path (in cleanup() — after shutDownJobsGracefully() and related
teardown) by calling stateScope.cancel() to ensure final teardown of any
stateIn() coroutines; update references around the Call constructor where
stateScope is defined and ensure CallState continues to receive the same scope.
There was a problem hiding this comment.
This is intentional
There was a problem hiding this comment.
@aleksandar-apostolov, understood — I've noted that the stateScope lifecycle design is intentional for this PR.
Since the goal is to make Call objects reusable after leave(), keeping stateScope alive across leave/rejoin cycles makes sense for maintaining StateFlow subscriptions that need to receive coordinator updates continuously. The separation between scope (cancelled on leave) and stateScope (survives leave) achieves the reusability objective.
🧠 Learnings used
Learnt from: aleksandar-apostolov
Repo: GetStream/stream-video-android PR: 1587
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt:795-798
Timestamp: 2025-12-19T14:10:16.577Z
Learning: In the `rejoin()` function in `stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`, the design invariant is that `rejoin()` should only be called when there is an active session (`this.session != null`). Therefore, using `this.session!!` is safe and intentional.
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep concurrency deterministic—use structured coroutines and avoid global scope
Learnt from: rahul-lohra
Repo: GetStream/stream-video-android PR: 1588
File: stream-video-android-core/api/stream-video-android-core.api:3561-3569
Timestamp: 2026-01-07T09:52:52.695Z
Learning: The `LocalCallMissedEvent` class in `stream-video-android-core` is only instantiated internally by the library maintainers. Clients only read these instances and never construct them directly, so breaking changes to the constructor are acceptable.
SDK Size Comparison 📏
|
Instead of passing stateScope as constructor parameter: - Add stateScope property to CallActions interface - CallState, CallStats access call.stateScope directly - ParticipantState accesses callActions.stateScope This is cleaner than passing scopes through constructors - each class gets the scope from its existing dependencies.
|
Approach needs to be revisited |
1 participant
Goal
Make Call objects reusable after
leave()by:stateScopefor StateFlows that need to survive leaveThis is an alternative approach to #1605 (implements Rahul's Approach 3) that provides structural safety.
Implementation
1. Add
stateScopefor StateFlows (survives leave)stateScopeinCall.kt- a long-lived scope that is NOT cancelled onleave()stateScopetoCallState,CallStats, andParticipantStatestateIn()calls to usestateScope:_pinnedParticipantslivestreamduration/durationInMsliveDurationInMsCallStats.localParticipantState.video2. Remove scope cancellation (allows rejoin)
shutDownJobsGracefully()which cancelledscopeandsupervisorJobleave():sfuEvents,monitorPublisherPCStateJob,monitorSubscriberPCStateJobleaveTimeoutAfterDisconnect,callStatsReportingJobRtcSession.cleanup()cancels its ownsupervisorJobKey benefits over #1605:
stateScopefor StateFlows,scopefor RTC workstateInis accidentally left onscope, it survives (since scope isn't cancelled)Testing
call.stateupdates work even without joining (viacall.get()or queries)./gradlew apiCheckto confirm API changes are intentional (CallState constructor signature)