Wire StreamClient into StreamVideoClient as coordinator owner#1738
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
97f4003 to
fa2eeb2
Compare
633b493 to
415d6d7
Compare
c761587 to
6ecf46f
Compare
Eagerly inject core v5 StreamClient into StreamVideoClient's primary
constructor, replace the four coordinatorSocket.{events,state,errors,state}
collectors with a single StreamClientListener subscription that casts to
VideoEvent and forwards to the existing fireEvent(...) dispatch pipeline.
connectAsync() delegates to streamClient.connect(); cleanup() calls
streamClient.disconnect() (bridged via runBlocking) before scope.cancel().
Deletes setupGuestUser, createGuestUser, guestUserJob, and the
corresponding await() sites. Also lands the minimum StreamVideoBuilder
wiring (streamTokenProvider dispatch on user.type + streamClient factory
seam) so the two files compile as one unit; builder tests are extended
separately.
Call.kt referenced clientImpl.guestUserJob?.await() at the join call site.
Replaced the await with a KDoc comment noting that StreamTokenManager now
single-flights the guest JWT transparently through core's auth interceptor.
TODO: streamClientListener.onState currently no-ops; the handleStreamState
routing lands alongside the ConnectionState sealed-interface reshape.
New StreamVideoBuilderDispatchTest exercises the `when (user.type)` dispatch in StreamVideoBuilder.build() through the streamClientFactory seam: - Authenticated -> IntegrationStreamTokenProvider - Guest -> GuestStreamTokenProvider - Anonymous -> IllegalStateException on token load - localCoordinatorAddress "10.0.2.2:3030" threads through into args.resolvedWssUrl as "ws://10.0.2.2:3030/video/connect" The pre-existing StreamVideoBuilderTest stays @ignore'd (integration-style suite depending on network authData). The new lightweight class runs on the JVM with mockk Context + ConnectivityManager stubs.
Three surgical edits that finish unwiring the legacy coordinator socket from the live code path: 1. CoordinatorConnectionModule narrows its ConnectionModuleDeclaration generic to Unit and turns `socketConnection` into a `Unit = Unit` vestigial override. The CoordinatorSocketConnection field is deleted; the class file itself stays on disk until the legacy socket interfaces are removed later in v2. 2. StreamLifecycleObserver.kt moved from `lifecycle/` to `lifecycle/legacy/`. Coordinator-side lifecycle observation now flows through core's StreamLifecycleMonitor. Legacy consumers (Coordinator/SFU socket files kept for their own later migration) have their imports updated to `lifecycle.legacy.StreamLifecycleObserver`. The SFU migrates separately; the moved file survives until then. 3. IntegrationTestBase updated to read the connection ID from streamClient.connectionState instead of the deleted socketConnection. Legacy coordinator test @ignore roster: - CoordinatorSocketTest.kt / CoordinatorSocketStateServiceTest.kt do NOT exist under `src/test/.../socket/coordinator/` on this branch — nothing to annotate. - TokenManagerImplTest.kt exercises TokenManagerImpl behaviours shared with the SFU (still in-tree). Preserved unannotated so SFU coverage is not lost. @ignore baseline unchanged at 21.
Core's default StreamClient factory touches WifiManager during eager construction, which layoutlib's BridgeContext does not implement. All Compose snapshot tests were failing at setup with "AssertionError: Unsupported Service: wifi". Expose the streamClientFactory seam on StreamVideoBuilder via a public @InternalStreamVideoApi setter and have StreamPreviewDataUtils install a NoOpStreamClient before build().
Guest users (iOS loadGuestUserInfo parity): - GuestStreamTokenProvider now syncs the createGuest JWT into the legacy Retrofit auth path (CoordinatorAuthInterceptor) via onTokenIssued and flips the auth type anonymous->jwt, matching the legacy setupGuestUser sequence. Previously the token only reached core's WS path, leaving every guest REST call unauthenticated (401 loop). - Builder now auto-connects guests too (only anonymous never connects); core's token manager drives createGuest inside connect(). - Server-issued guest identity is adopted into ClientState.user through BindableWritableUserRepository, including iOS's name-preservation quirk for server-decorated display names. Anonymous users (iOS connectUser parity): - build() no longer throws; anonymous clients are REST-only with stream-auth-type anonymous restored. - connectAsync()/connectIfNotAlreadyConnected() fail fast without touching the network.
Forked test JVMs do not inherit org.gradle.jvmargs and run on Gradle's 512m default. The suite (Robolectric framework sandbox, MockK inline instrumentation, OkHttp mock servers in a single reused executor) now exceeds that on CI: the executor died with OutOfMemoryError across OkHttp/Robolectric/test-worker threads. The earlier intermittent 'Can't instantiate proxy' MockK failures were the same memory pressure surfacing sooner.
2g still produced OutOfMemoryError on CI (7 occurrences across OkHttp/Robolectric/test-worker threads in a single run). The single reused executor accumulates Robolectric sandboxes and MockK inline recordings across the whole suite; 3g gives the headroom the runner needs.
3g still hit OutOfMemoryError on CI while the same suite passes locally under coverage - CI runs the root :testCoverage with org.gradle.parallel=true, so core's executor competes with Paparazzi and every other module's test JVM for runner memory. Growing the heap worsens the overcommit. Bound the executor's accumulation (Robolectric sandboxes, MockK recordings) by forking every 200 tests and return to 2g.
Every successful StreamVideoBuilder.build() ends in StreamVideo.install(client); install() replaces the previous singleton without cleaning it up, so each test leaked a full client (live ClientScope, OkHttp pools, notification manager) for the executor's lifetime. Four leaked clients per fork plus kover instrumentation exhausted the CI executor heap regardless of size - the same reason StreamVideoBuilderTest and ClientAndAuthTest run in the single-fork isolatedTest task. removeClient() cleans up the retained client; unmockkAll() drops the MockK inline recordings.
The core unit-test executor dies with OutOfMemoryError only on CI (512m, 2g, 3g, fork-cycling, and test teardown all failed to stop it) while the same suite passes locally under coverage. Write the heap dump and GC log into the reports directory so the unit-tests-results artifact carries them; temporary until the retainer is identified.
The shared DispatcherRule installed one virtual-time dispatcher as both Main and DispatcherProvider.IO. Integration tests await responses from the live coordinator; while a runTest body is suspended on a real network round trip, the test scheduler fast-forwards virtual time, and core StreamClient's health monitor — a delay-driven loop on IO that reschedules itself unconditionally — spins millions of iterations, each logging at the client's DEBUG level. Robolectric's ShadowLog statically buffers every android.util.Log line, so the executor heap fills with LogItems (2 GB / 95% of heap, 5.6M items in the CI heap dump) and the fork dies with OOM. CI-only because the coordinator answers in milliseconds locally, bounding the loop. DispatcherRule now takes a separate ioDispatcher and IntegrationTestBase overrides it to real Dispatchers.IO: virtual time stays on Main for determinism, IO runs on the same clock as the network the tests talk to. Health pings return to real intervals — a handful per test.
With integration-test IO on real dispatchers, CallState work (participant flow propagation, the speaking-while-muted reset job) no longer runs on the virtual clock, so advanceTimeBy/instant reads race it. Await the expected state with a real-time timeout instead (awaitRealTime helper on IntegrationTestBase).
The executor OOM is resolved (ShadowLog flood from virtual-time IO in integration tests); the heap-dump/GC-log flags served their purpose and forkEvery(200) never triggered (the suite has fewer than 200 classes).
…order Same instant-read race as the sibling sorting test: updateParticipant propagates through the call scope (real IO in integration tests), so a fixed delay plus an instant value read is racy on slow runners.
…m coverage Sonar's new-code gate (80%) failed at 70.3% almost entirely because the moved lifecycle/legacy/StreamLifecycleObserver.kt counts as uncovered new code; exclude the quarantined package (it is deleted once the remaining legacy socket consumers migrate). Cover the genuinely new lines: the StreamClientListener event/error dispatch, the connect branch of connectIfNotAlreadyConnected, AnonymousStreamTokenProvider.loadToken's fail-fast, and ClientState.updateUser.
11e6329 to
d03a473
Compare
|
WalkthroughThe PR moves coordinator connection management into ChangesStreamClient coordinator migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Builder
participant StreamVideoClient
participant StreamClient
participant ClientState
Builder->>StreamClient: construct via factory
Builder->>StreamVideoClient: inject StreamClient
Builder->>ClientState: bind guest identity updates
StreamVideoClient->>StreamClient: subscribe and connect
StreamClient-->>StreamVideoClient: connection state and video events
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt (1)
232-236: 📐 Maintainability & Code Quality | 🔵 TrivialThe empty
onStatewith the deferred TODO is intentional and tracked for the ConnectionState rewrite in#1739. I can open a tracking issue to ensurehandleStreamState(state)wiring isn't lost — let me know.🤖 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/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt` around lines 232 - 236, Keep the intentionally empty onState implementation unchanged for now, preserving the TODO documenting replacement with this@StreamVideoClient.state.handleStreamState(state) after the ConnectionState rewrite. Do not add interim state-handling logic; track the follow-up wiring separately if needed.Source: Linters/SAST tools
🤖 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.
Inline comments:
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt`:
- Around line 132-138: Move the testSessionId assignment in IntegrationTestBase
from before connection establishment to immediately after
connectAsync().await(); read the connected connectionId from
streamClient.connectionState only after the connection completes so
waitForConnectionId() does not retain an initial null value.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt`:
- Around line 232-236: Keep the intentionally empty onState implementation
unchanged for now, preserving the TODO documenting replacement with
this@StreamVideoClient.state.handleStreamState(state) after the ConnectionState
rewrite. Do not add interim state-handling logic; track the follow-up wiring
separately if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 581095bc-d265-4731-adce-a3278715664f
📒 Files selected for processing (26)
build.gradle.ktsstream-video-android-core/build.gradle.ktsstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoBuilder.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/internal/module/CoordinatorConnectionModule.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/lifecycle/legacy/StreamLifecycleObserver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocket.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/v2/AnonymousStreamTokenProvider.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/v2/GuestStreamTokenProvider.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/BindableWritableUserRepositoryTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallStateTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/ClientStateUpdateUserTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoBuilderDispatchTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/DispatcherRule.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/TestBase.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/socket/coordinator/v2/AnonymousStreamTokenProviderTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/socket/coordinator/v2/GuestStreamTokenProviderTest.ktstream-video-android-previewdata/src/main/kotlin/io/getstream/video/android/mock/NoOpStreamClient.ktstream-video-android-previewdata/src/main/kotlin/io/getstream/video/android/mock/StreamPreviewDataUtils.kt
| clientImpl.testSessionId = runBlocking { | ||
| (client as StreamVideoClient).coordinatorConnectionModule.socketConnection.connectionId().value | ||
| // Derive the WS connectionId from core's StreamClient state (StreamClient | ||
| // replaced the coordinator-socket connectionId() flow). Null before Connected. | ||
| ( | ||
| clientImpl.streamClient.connectionState.value | ||
| as? io.getstream.android.core.api.model.connection.StreamConnectionState.Connected | ||
| )?.connectionId |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant test base file and related state usage.
git ls-files 'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt' \
'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/*' \
'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/**/IntegrationTestState*' \
'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/**/IntegrationTest*' \
| sed 's#^`#FILE` #'
echo '--- IntegrationTestBase outline ---'
ast-grep outline stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt --view expanded || true
echo '--- nearby lines ---'
nl -ba stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt | sed -n '110,180p'
echo '--- search for IntegrationTestState and testSessionId ---'
rg -n "IntegrationTestState|testSessionId|connectAsync\(\)\.await|connectionState\.value" stream-video-android-core/src/test/kotlin/io/getstream/video/android/core -SRepository: GetStream/stream-video-android
Length of output: 2149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read any related state/test setup files if present.
for f in \
$(git ls-files 'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/**/IntegrationTestState*.kt' \
'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/**/IntegrationTest*.kt' \
'stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/**/Test*.kt' | head -n 20); do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,240p'
doneRepository: GetStream/stream-video-android
Length of output: 319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test base and shared test-state files.
echo '--- IntegrationTestBase.kt ---'
nl -ba stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt | sed -n '120,190p'
echo '--- files mentioning IntegrationTestState/testSessionId/connectAsync ---'
rg -n "IntegrationTestState|testSessionId|connectAsync\(\)\.await|connectionState\.value|connectionId" stream-video-android-core/src/test/kotlin/io/getstream/video/android/core -SRepository: GetStream/stream-video-android
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt
echo '--- lines 60-170 ---'
sed -n '60,170p' "$FILE"
echo '--- lines 170-260 ---'
sed -n '170,260p' "$FILE"
echo '--- all occurrences of IntegrationTestState / testSessionId in file ---'
grep -nE 'IntegrationTestState|testSessionId|connectAsync\(\)\.await|connectionState\.value|connectionId' "$FILE" || trueRepository: GetStream/stream-video-android
Length of output: 8743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- references to testSessionId in repository ---'
rg -n "testSessionId" . -S
echo '--- references to connectionState / Connected in stream-video-android-core ---'
rg -n "connectionState\.value|StreamConnectionState\.Connected|connectionId" stream-video-android-core -SRepository: GetStream/stream-video-android
Length of output: 10548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt
python3 - <<'PY'
from pathlib import Path
p = Path("stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt")
text = p.read_text()
start = text.index(" if (IntegrationTestState.client == null) {")
end = text.index(" IntegrationTestState.client = client")
print(text[start:end+len(" IntegrationTestState.client = client")])
PYRepository: GetStream/stream-video-android
Length of output: 1593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact initialization/connection sequence in IntegrationTestBase.
python3 - <<'PY'
from pathlib import Path
text = Path("stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/base/IntegrationTestBase.kt").read_text().splitlines()
for i in range(124, 156):
print(f"{i+1}: {text[i]}")
PYRepository: GetStream/stream-video-android
Length of output: 1987
Set testSessionId after connecting. The cached client keeps the initial null snapshot here, so waitForConnectionId() can fall back to null later. Move this assignment after connectAsync().await().
🤖 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/base/IntegrationTestBase.kt`
around lines 132 - 138, Move the testSessionId assignment in IntegrationTestBase
from before connection establishment to immediately after
connectAsync().await(); read the connected connectionId from
streamClient.connectionState only after the connection completes so
waitForConnectionId() does not retain an initial null value.



Goal
Closes AND-1282
Replaces our custom coordinator socket with
stream-android-core'sStreamClientas the owner of the coordinator WebSocket connection.StreamClientnow handles the socket lifecycle, token refresh, HTTP interceptors, and reconnection. Guest and authenticated user handling is unified through a single token-provider abstraction.Depends on #1737 (adapter bridging). Merge after parent.
Merge order: adapters (#1737) → this PR → ConnectionState rewrite (#1739).
Implementation
StreamVideoClientnow owns aStreamClientinstance.Constructed eagerly in the primary constructor and disposed in
cleanup(). Tests inject a mock via a constructor parameter — no moreLazy<>workarounds needed.Event flow now goes through core:
StreamClientfires WebSocket events, our listener forwards them to the existingfireEvent(...)pipeline. Existing subscribers keep working unchanged.StreamVideoBuilderpicks the right token provider based on user type.Regular users get
IntegrationStreamTokenProvider(wrapping the integration'sTokenProvider). Guest users getGuestStreamTokenProvider(which callscreateGuestinternally). Anonymous users get a REST-only client that never opens a coordinator socket; explicitconnectattempts fail fast.Guest and anonymous flows mirror the iOS SDK (
StreamVideo.loadGuestUserInfo/initialConnectIfRequired):createGuestinsideconnect().CoordinatorAuthInterceptor) the moment it is issued, and auth type flipsanonymous→jwt— REST and WS stay on one token until HTTP is unified through core in a later PR.ClientState.user, including iOS's name-preservation quirk for decorated display names.createGuestagain, single-flighted byStreamTokenManager.Guest user machinery deleted.
setupGuestUser,createGuestUser,guestUserJob: Deferred<Unit>?, and everyapiCall { guestUserJob?.await() }synchronization site are gone. Guest token acquisition happens insideGuestStreamTokenProvider.loadToken(userId)— first invocation callscreateGuest, subsequent calls return the cached JWT via core'sStreamTokenManager(which provides single-flight dedup automatically). This removes the race conditions we've been patching in the guest flow over the last few weeks.Coordinator module surgery.
CoordinatorConnectionModule.socketConnectionfield is removed. The legacyCoordinatorSocketConnectionclass stays on disk (nothing references it), scheduled for deletion in a future cleanup PR.StreamLifecycleObservermoved tolifecycle/legacy/package.SfuSocketstill references it, so it can't be deleted yet — the SFU migration will remove it.Tests:
StreamVideoClientTestextended with assertions thatStreamClientis subscribed at init,connect()invoked onconnectAsync(),disconnect()invoked oncleanup().StreamVideoBuilderDispatchTestcovers the token-provider dispatch: Anonymous → REST-only provider (no throw), Authenticated → Integration provider, Guest → Guest provider,localCoordinatorAddress→ws://....GuestStreamTokenProviderTestcovers token sync into the REST auth path and the iOS name-preservation quirk;BindableWritableUserRepositoryTestcovers pre-bind buffering.StreamVideoClientTest.Testing
stream-video-android-core:testDebugUnitTestsuite green in ~2m30s.@Ignorecount unchanged at 21.spotlessCheckgreen.apiCheckshows zero public API diff — theConnectionStatebreaking change is in the follow-up PR.Heads-up for future test authors:
StreamVideoBuilder.build()constructs the connection module before the token-provider dispatch, and the module hitsgetSystemService(CONNECTIVITY_SERVICE) as ConnectivityManager— undermockk<Context>(relaxed = true)that throwsClassCastException. Tests using the builder in-process must stubCONNECTIVITY_SERVICE. SeeStreamVideoBuilderDispatchTest.builderFor(...)for the pattern.Intermediate-state note
On this PR alone,
ClientState.connectiononly updates viaConnectedEvent; disconnect transitions become visible when #1739 wireshandleStreamState. Merge the stack together.Summary by CodeRabbit