[WIP]Client event reporting 2#1702
Draft
rahul-lohra wants to merge 65 commits into
Draft
Conversation
WalkthroughThis PR introduces three interconnected systems to improve observability, testability, and structured error handling in video calls. It adds fault-injection capabilities for testing failure scenarios, replaces all string-based call-leave reasons with a typed enum hierarchy, and integrates a comprehensive client-side event reporting system throughout the call lifecycle. ChangesFault Injection, Call Lifecycle Tracking, and Analytics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt (1)
291-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLeave the active call before cancelling the client scope.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktLines 1345-1356 do the finalonCallLeave(...),sendLeaveEvent(...), andcleanup()work by launching onclientImpl.scope. Cancellingscopefirst means the newCLIENT_CLEANUPpath can skip both telemetry and the actualCall.cleanup()/RtcSession.cleanup()teardown.🤖 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 291 - 319, The cleanup sequence cancels client scope before invoking Call.leave, which prevents Call.onCallLeave/sendLeaveEvent/cleanup (which use clientImpl.scope) from running; in StreamVideoClient.cleanup re-order operations so you call state.activeCall.value?.leave(...) (using Call.leave with CallLeaveReason.SdkDriven CLIENT_CLEANUP) before calling scope.cancel(), then clear calls/evict destroyedCalls and stop the foreground call service (using callServiceConfigRegistry and ServiceIntentBuilder) and finally release audioExecutionContext and call coordinatorAnalytics.endObserver(); ensure you still wrap service stop in safeCall as before and keep references to activeCall and callConfig to avoid race conditions.stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt (1)
1097-1103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid using an empty string as the legacy leave reason.
CallLeaveReason.Custom("")will usually be serialized as an explicit blank value, which creates a useless telemetry bucket distinct from "no message provided". Usenullinstead, or map the legacy overload to a concrete default reason.Suggested fix
override fun leave( call: Call, onSuccess: (suspend (Call) -> Unit)?, onError: (suspend (Exception) -> Unit)?, ) { - leave(call, CallLeaveReason.Custom(""), onSuccess, onError) + leave(call, CallLeaveReason.Custom(), onSuccess, onError) }🤖 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-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt` around lines 1097 - 1103, The legacy overload of leave currently calls leave(call, CallLeaveReason.Custom(""), ...) which creates a blank serialized reason; change the overload to pass null (or a concrete default like CallLeaveReason.Custom("user_left")) instead of an empty string so the downstream serializer treats it as no message or a meaningful default; update the override in StreamCallActivity.leave to call leave(call, null, onSuccess, onError) (or use the chosen default) and ensure CallLeaveReason.Custom("") is removed from this call site.
🟡 Minor comments (6)
demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt-60-75 (1)
60-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the incoming
modifieron the rootColumn
FailureInjectorUiaccepts amodifier, but the rootColumnstill usesModifierinstead, so callers can’t control layout from the parent.Proposed fix
- Column( - modifier = Modifier + Column( + modifier = modifier .fillMaxSize() .background(VideoTheme.colors.baseSheetPrimary), ) {🤖 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 `@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt` around lines 60 - 75, FailureInjectorUi currently ignores the incoming modifier by using Modifier on the root Column; update the Column to use the passed-in modifier combined with the existing modifiers (e.g., modifier.fillMaxSize().background(...)) so callers can control layout from the parent; reference: FailureInjectorUi, the parameter modifier, and the root Column.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt-441-445 (1)
441-445:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a non-backend leave reason for the disconnect timeout.
This branch is driven by the local
leaveAfterDisconnectSecondstimer, butstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/CallAnalyticsHooks.ktLines 44-51 maps everyCallLeaveReason.BackendtoBACKEND_LEAVE. That will report SDK-driven timeout exits as server-ended calls in the new telemetry.SdkDrivenorRetryExhaustedis a better fit here.🤖 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/Call.kt` around lines 441 - 445, The disconnect-timer branch is currently calling leave(...) with CallLeaveReason.Backend (BackendCause.LEAVE_TIMEOUT_AFTER_DISCONNECT), which will be mapped to BACKEND_LEAVE in telemetry; change the leave invocation in Call.kt (the code that constructs message and calls leave(...)) to use a non-backend reason such as CallLeaveReason.SdkDriven (or CallLeaveReason.RetryExhausted) while keeping the existing message and context (connection=conn) so telemetry correctly reports an SDK-driven timeout rather than a backend-initiated leave.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt-904-908 (1)
904-908:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the retry attempt on successful WS connects.
Failures and timeouts already report
telemetryModel.retryAttempt, but the success path hardcodesretryCount = 0. A reconnect that succeeds on its second or later try will be recorded as a first-attempt success.Suggested fix
- call.callAnalyticsHooks.wsHook.onWsCompleted( - success = true, - retryCount = 0, - ) + call.callAnalyticsHooks.wsHook.onWsCompleted( + success = true, + retryCount = telemetryModel?.retryAttempt ?: 0, + )🤖 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/call/RtcSession.kt` around lines 904 - 908, The success branch for SfuSocketState.Connected currently calls call.callAnalyticsHooks.wsHook.onWsCompleted with retryCount = 0 which discards actual retry attempts; change that call to pass the real retry count (e.g. telemetryModel.retryAttempt or the field where retries are tracked) instead of 0 so reconnects that succeed after retries are recorded correctly, keeping the rest of the parameters unchanged in the onWsCompleted invocation.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketStateService.kt-33-37 (1)
33-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake retry counters instance-scoped (remove shared companion state)
retryAttempts/lastRetryAttemptsare stored inCoordinatorSocketStateService’sinternal companion object, so they become global/shared across allCoordinatorSocketStateService(and thus all sockets/tests). Updates (retryAttempts += 1/ resets and assignments) are unsynchronized, so concurrent reconnect/connect paths can overwrite each other. These counters are only written in this file (no reads elsewhere), so either move them to per-instance/per-connection state or remove if truly unused—don’t keep shared mutable state.🤖 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/socket/coordinator/CoordinatorSocketStateService.kt` around lines 33 - 37, The retry counters retryAttempts and lastRetryAttempts are currently in CoordinatorSocketStateService's internal companion object (shared/global and unsynchronized); move them to be instance-scoped private properties on CoordinatorSocketStateService (remove from the companion) and update all references in this file to use the instance properties; if concurrent access from different coroutines/threads is possible keep thread-safety by using AtomicInteger or synchronizing updates where retryAttempts += 1 / resets occur; ensure no other files rely on the companion variables (remove them once migrated).stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt-248-253 (1)
248-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't stringify a nullable error message here.
"${error.message}"turns a missing message into the literal"null", so this path will report bogus leave reasons for exceptions without a message. Passerror.messagethrough as-is.Suggested fix
error.call.leave( CallLeaveReason.SdkDriven( SdkCause.STREAM_CALL_ACTIVITY_EXCEPTION, - "${error.message}", + error.message, ), )🤖 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-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt` around lines 248 - 253, The code is stringifying a possibly-null exception message in StreamCallActivity by using "${error.message}" when calling error.call.leave with CallLeaveReason.SdkDriven and SdkCause.STREAM_CALL_ACTIVITY_EXCEPTION; change it to pass error.message directly (no string interpolation) so a null stays null rather than becoming the literal "null", ensuring the leave call receives the nullable String as intended.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/CallAnalyticsHooks.kt-54-54 (1)
54-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the leftover debug log artifact.
"noob isAnyStageInProgress:..."looks like temporary debugging and will leak into SDK logs on a hot leave path. Please drop it or replace it with a sanitized development-only message.As per coding guidelines, "Monitor logging verbosity; rely on
StreamVideoImpl.developmentModefor guardrails".🤖 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/analytics/CallAnalyticsHooks.kt` at line 54, Remove the leftover debug log string in CallAnalyticsHooks.kt (the logger.d call that prints "noob isAnyStageInProgress:..."); either delete that log entirely or replace it with a sanitized message and guard it behind the developmentMode check on StreamVideoImpl so it only logs in dev builds (use the same pattern as other dev-only logs in this class).
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (1)
683-686: ⚡ Quick winRemove the temporary
nooblogs from RTC hot paths.These new logs run on every socket transition, connect, cleanup, and rejoin path, so they'll add noise in the busiest RTC flows without giving durable signal. Keep only purposeful logs here or guard them behind the existing dev-only controls. As per coding guidelines, "Monitor logging verbosity; rely on
StreamVideoImpl.developmentModefor guardrails".Also applies to: 714-715, 886-888, 1133-1135, 1985-1993
🤖 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/call/RtcSession.kt` around lines 683 - 686, Remove the temporary "noob" debug logs that run on RTC hot paths (instances where logger.d logs include "noob" and reference sfuSocketState, call.state.connection.value, and sfuName) — specifically replace or remove the logger.d calls at the shown locations (including the occurrences around the symbols sfuSocketState, call.state.connection, and sfuName and the other mentioned blocks). If the information must remain for development, wrap those debug logs behind the existing development-mode guard (StreamVideoImpl.developmentMode) so they only execute in dev builds; otherwise delete them to avoid noisy hot-path logging.
🤖 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 `@demo-app/src/main/kotlin/io/getstream/video/android/App.kt`:
- Around line 84-89: The current injectFault() only sets failureInjector on
StreamVideo.instanceOrNull(), which can be null at cold start so later-created
SDK instances never receive the injector; change injectFault() to also observe
StreamVideo.instanceState (or hook into the SDK creation path) and set
state.failureInjector = FailureInjectorImpl() whenever a new instance appears so
every future StreamVideo instance gets the injector; reference the injectFault()
function, FailureInjectorImpl, StreamVideo.instanceOrNull(), and
StreamVideo.instanceState when implementing the observer/creation hook and
ensure you reapply the injector on instance creation events.
In
`@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.kt`:
- Around line 26-76: The mutable map faultCounts in FailureInjectorImpl is not
thread-safe; guard all accesses/updates (reads, writes, decrement) to
faultCounts used by
enable/disable/setEnabled/isEnabled/setCount/getCount/clear/throwDebugFault/sendFailResult
to avoid races—either replace faultCounts with a thread-safe structure (e.g.,
ConcurrentHashMap<FailureKey, AtomicInteger> with atomic increments/decrements)
or serialize access using a Mutex/synchronized block around each method that
touches faultCounts; pick one approach and apply it consistently to every method
that reads or mutates faultCounts so counts cannot be lost under concurrent
Compose/UI and async flows.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionAnalyticsObserver.kt`:
- Around line 45-73: The collector is only observing the peer connection "state"
but reads ICE state via a snapshot (session.value?...iceState?.value) and also
launches hook calls with scope.launch inside the collect, breaking structured
concurrency; change the publisher and subscriber collectors to combine (or
flatMapLatest) both the state and the iceState flows exposed on the peer
connection so ICE-only transitions are observed, replace the snapshot access
with the collected iceState value, and remove the nested scope.launch so
hook.onPeerConnectionStateChanged is invoked directly from the collector
coroutine (or within the same structured scope) so it is canceled with
publisherJob/subscriberJob; update code around session, publisherJob,
subscriberJob, getStage and hook.onPeerConnectionStateChanged
(PeerConnectionRole.PUBLISH / SUBSCRIBE) accordingly.
- Around line 50-56: The publisher/subscriber collectors currently call
hook.onPeerConnectionStateChanged(...) inside scope.launch which allows those
invocations to outlive peerConnectionObserverJob and introduces nondeterministic
ordering; remove the inner scope.launch and invoke
PeerConnectionHook.onPeerConnectionStateChanged(...) directly inside the collect
{ } blocks (e.g., where PeerConnectionRole.PUBLISH /
PeerConnectionRole.SUBSCRIBE and iceState/peerConnectionState are read) so the
calls are confined to the lifecycle of peerConnectionObserverJob and canceled by
stop().
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/Stage.kt`:
- Line 19: The enum Stage is currently public by default; mark it internal to
avoid leaking it into the core public API by changing its declaration to use the
internal visibility modifier (i.e., internal enum class Stage { NOT_STARTED,
IN_PROGRESS }) and then update any usages/imports of Stage within the analytics
plumbing to compile with the reduced visibility; ensure no public API or other
modules depend on Stage before making the change.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallLeaveReason.kt`:
- Around line 21-27: The CallLeaveReason sealed interface is annotated
`@InternalStreamVideoApi` but is exposed by the public API
ActivityCallOperationsWithCallLeaveReason.leave(...); either remove the internal
annotation and promote CallLeaveReason (and its nested types/enums referenced in
lines 64-108) to the supported public API, or stop leaking it by replacing the
public method signature with a public-safe type (e.g., a new public LeaveReason
DTO or an interface in the ui-core public package) and keep the internal
implementation internal; update references to CallLeaveReason,
ActivityCallOperationsWithCallLeaveReason.leave, and any related enums/classes
so the public surface only exposes publicly annotated types.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/ClientEventReporter.kt`:
- Around line 240-253: The completion event is using the live joinStageAttemptId
instead of the snapshot stored on the PostCallFlightSession, so completed events
can be attached to a newer attempt; update the buildRequest calls in
ClientEventReporter (the blocks constructing completion events) to use
session.joinStageAttemptIdSnapshot when present (e.g. joinStageAttemptId =
session.joinStageAttemptIdSnapshot ?: joinStageAttemptId) and apply the same
change to the other completion path referenced (lines around 374-384) so the
event tags the actual initiating join attempt.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/TelemetryModel.kt`:
- Around line 26-34: The AnalyticsFailureCodes enum has a wrong message for
REQUEST_TIMEOUT and a duplicated serialized code for SFU_REQUEST_TIMEOUT; update
AnalyticsFailureCodes so REQUEST_TIMEOUT's human message reflects a generic
timeout (e.g., "Request timed out") and change SFU_REQUEST_TIMEOUT's code string
from "REQUEST_TIMEOUT" to a unique value (e.g., "SFU_REQUEST_TIMEOUT") while
keeping its message ("SFU connection timed out"); modify the entries for
REQUEST_TIMEOUT and SFU_REQUEST_TIMEOUT in the AnalyticsFailureCodes enum
accordingly.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/faultinjector/FailureInjector.kt`:
- Around line 36-40: The sendFailResult contract currently always returns
io.getstream.result.Result.Failure, forcing callers to perform enabled/count
checks externally; change the sendFailResult(FailureKey) signature to return
io.getstream.result.Result.Failure? (nullable) or alternatively have
sendFailResult perform the enabled/count logic internally and return Failure? so
implementations can represent "disabled" or "exhausted" directly; update
implementations of FailureInjector, and adjust usages to handle a nullable
result, referencing the FailureInjector.sendFailResult, FailureInjector.setCount
and FailureInjector.getCount symbols and the FailureKey type when making the
change.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt`:
- Around line 225-248: The debug fault injection in debugFaultInjectors is being
executed on real SFU connects and must be guarded by the existing
development-only check; wrap the body (or the call site in connectUser()) with
the same suspendDebugOnly/developmentMode guard used by simulateNetworkError so
the faultInjector.throwDebugFault(...) calls only run in development mode.
Locate debugFaultInjectors and ensure it only executes its faultInjector checks
when StreamVideo.instanceOrNull()?.state?.developmentMode (or via
suspendDebugOnly) is true, preventing debug-only injectors from running in
production.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.kt`:
- Around line 167-169: Replace the current full-object debug log in
SfuSocketConnection.connect which prints the entire connectData (and the stray
"noob" tag) with a redacted or minimal-safe log entry: either remove the
logger.d line entirely or log only non-sensitive, whitelisted fields from the
JoinRequest (e.g., log connectData.roomId or an anonymized callId/hash) before
calling internalSocket.connect(connectData); ensure no JWTs, ICE tokens, or full
request payloads are ever logged.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt`:
- Around line 250-263: The collector in scope.launch reads the plain MutableMap
calls without synchronization, which can race with mutations from call(...),
onCallCleanUp(...), and cleanup(); fix by taking a thread-safe snapshot or
making the cache concurrent before iterating: either wrap reading of calls into
a synchronized/Mutex-protected block (acquire the same lock used by mutators)
and build watchedCalls = calls.values.map { it.cid } inside that critical
section, or change the calls map to a ConcurrentHashMap and read its values
safely, then pass the resulting immutable list of CIDs into Filters.`in` and
queryCalls(...).
In
`@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioControlActions.kt`:
- Around line 73-79: The onLeaveRoom callback currently is invoked parameterless
which drops the CallLeaveReason created in the default branch; change the
onLeaveRoom API to accept a CallLeaveReason (or add a new onLeaveRoomWithReason
callback) and pass the constructed CallLeaveReason into it instead of ignoring
it. Locate the invocation in AudioControlActions (the onLeaveRoom?.invoke()
call) and replace it with onLeaveRoom?.invoke(reason) where reason is the
CallLeaveReason(UserActionCause.CANCELLED_BY_SELF, "Cancelled the call"), and
update the onLeaveRoom declaration/clients to accept the CallLeaveReason type so
custom leave flows receive the same telemetry. Ensure any new callback name or
signature is propagated to callers and tests.
In
`@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioRoomContent.kt`:
- Around line 144-149: The leave reason currently embeds raw exception text
("Error in Pip: ${e.message}") when calling call.leave with
CallLeaveReason.SdkDriven and SdkCause.PIP_ERROR; replace that dynamic message
with a fixed sanitized string like "Failed to enter PiP" and move the exception
details to a local-only debug/log statement (e.g., Log.e or a local logger or
debug-only println) so the telemetry payload contains no raw platform exception
data; update the call.leave invocation in AudioRoomContent.kt (the
CallLeaveReason.SdkDriven / SdkCause.PIP_ERROR site) accordingly.
In
`@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/activecall/CallContent.kt`:
- Around line 194-199: The current call.leave invocation is passing raw
exception text (e.message) into CallLeaveReason.SdkDriven which leaks
un-sanitized diagnostics; update the CallContent.kt code that calls
call.leave(...) with CallLeaveReason.SdkDriven(SdkCause.PIP_ERROR, ...) to use a
stable, non-sensitive reason string (e.g., "pip_failed" or "pip_error") and do
NOT embed the platform exception message; if you need the exception for local
debugging, log it locally with a debug logger (e.g., processLogger.debug or
similar) but keep the telemetry/leave reason strictly sanitized.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/ActivityCallOperationsWithCallLeaveReason.kt`:
- Around line 23-33: The new default implementation of
ActivityCallOperationsWithCallLeaveReason.leave(call, callLeaveReason,
onSuccess, onError) is a silent no-op which will drop leave requests if
implementations don't override it; change the default to delegate to the
existing leave(call, onSuccess, onError) overload so behavior is preserved
(i.e., call the older leave method from the new overload and pass through
onSuccess/onError), ensuring any existing implementations continue to work.
---
Outside diff comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt`:
- Around line 291-319: The cleanup sequence cancels client scope before invoking
Call.leave, which prevents Call.onCallLeave/sendLeaveEvent/cleanup (which use
clientImpl.scope) from running; in StreamVideoClient.cleanup re-order operations
so you call state.activeCall.value?.leave(...) (using Call.leave with
CallLeaveReason.SdkDriven CLIENT_CLEANUP) before calling scope.cancel(), then
clear calls/evict destroyedCalls and stop the foreground call service (using
callServiceConfigRegistry and ServiceIntentBuilder) and finally release
audioExecutionContext and call coordinatorAnalytics.endObserver(); ensure you
still wrap service stop in safeCall as before and keep references to activeCall
and callConfig to avoid race conditions.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt`:
- Around line 1097-1103: The legacy overload of leave currently calls
leave(call, CallLeaveReason.Custom(""), ...) which creates a blank serialized
reason; change the overload to pass null (or a concrete default like
CallLeaveReason.Custom("user_left")) instead of an empty string so the
downstream serializer treats it as no message or a meaningful default; update
the override in StreamCallActivity.leave to call leave(call, null, onSuccess,
onError) (or use the chosen default) and ensure CallLeaveReason.Custom("") is
removed from this call site.
---
Minor comments:
In `@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt`:
- Around line 60-75: FailureInjectorUi currently ignores the incoming modifier
by using Modifier on the root Column; update the Column to use the passed-in
modifier combined with the existing modifiers (e.g.,
modifier.fillMaxSize().background(...)) so callers can control layout from the
parent; reference: FailureInjectorUi, the parameter modifier, and the root
Column.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/CallAnalyticsHooks.kt`:
- Line 54: Remove the leftover debug log string in CallAnalyticsHooks.kt (the
logger.d call that prints "noob isAnyStageInProgress:..."); either delete that
log entirely or replace it with a sanitized message and guard it behind the
developmentMode check on StreamVideoImpl so it only logs in dev builds (use the
same pattern as other dev-only logs in this class).
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`:
- Around line 441-445: The disconnect-timer branch is currently calling
leave(...) with CallLeaveReason.Backend
(BackendCause.LEAVE_TIMEOUT_AFTER_DISCONNECT), which will be mapped to
BACKEND_LEAVE in telemetry; change the leave invocation in Call.kt (the code
that constructs message and calls leave(...)) to use a non-backend reason such
as CallLeaveReason.SdkDriven (or CallLeaveReason.RetryExhausted) while keeping
the existing message and context (connection=conn) so telemetry correctly
reports an SDK-driven timeout rather than a backend-initiated leave.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt`:
- Around line 904-908: The success branch for SfuSocketState.Connected currently
calls call.callAnalyticsHooks.wsHook.onWsCompleted with retryCount = 0 which
discards actual retry attempts; change that call to pass the real retry count
(e.g. telemetryModel.retryAttempt or the field where retries are tracked)
instead of 0 so reconnects that succeed after retries are recorded correctly,
keeping the rest of the parameters unchanged in the onWsCompleted invocation.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketStateService.kt`:
- Around line 33-37: The retry counters retryAttempts and lastRetryAttempts are
currently in CoordinatorSocketStateService's internal companion object
(shared/global and unsynchronized); move them to be instance-scoped private
properties on CoordinatorSocketStateService (remove from the companion) and
update all references in this file to use the instance properties; if concurrent
access from different coroutines/threads is possible keep thread-safety by using
AtomicInteger or synchronizing updates where retryAttempts += 1 / resets occur;
ensure no other files rely on the companion variables (remove them once
migrated).
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt`:
- Around line 248-253: The code is stringifying a possibly-null exception
message in StreamCallActivity by using "${error.message}" when calling
error.call.leave with CallLeaveReason.SdkDriven and
SdkCause.STREAM_CALL_ACTIVITY_EXCEPTION; change it to pass error.message
directly (no string interpolation) so a null stays null rather than becoming the
literal "null", ensuring the leave call receives the nullable String as
intended.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt`:
- Around line 683-686: Remove the temporary "noob" debug logs that run on RTC
hot paths (instances where logger.d logs include "noob" and reference
sfuSocketState, call.state.connection.value, and sfuName) — specifically replace
or remove the logger.d calls at the shown locations (including the occurrences
around the symbols sfuSocketState, call.state.connection, and sfuName and the
other mentioned blocks). If the information must remain for development, wrap
those debug logs behind the existing development-mode guard
(StreamVideoImpl.developmentMode) so they only execute in dev builds; otherwise
delete them to avoid noisy hot-path logging.
🪄 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: 4a610ec6-c6c3-408d-8d41-f2aa8f0f9e53
⛔ Files ignored due to path filters (43)
stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/apis/ProductvideoApi.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/infrastructure/Serializer.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallEndedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallLevelEventPayload.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallStatsParticipantCounts.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallStatsParticipantSession.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallStatsReportReadyEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CallStatsSessionResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ChatPreferencesResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ClientEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CoordinatesResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/CreateDeviceRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/DeviceResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/FeedsPreferencesResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ICEServer.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/IndividualRecordingSettingsRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/IndividualRecordingSettingsResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/JoinCallRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/LocationResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/PushPreferencesResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/QueryCallSessionParticipantStatsResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/QueryCallSessionStatsRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/QueryCallSessionStatsResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/RawRecordingSettingsRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/RawRecordingSettingsResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ReportClientEventRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ReportClientEventResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ResolveSipAuthRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ResolveSipAuthResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/ResolveSipInboundRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/SFULocationResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/SIPChallengeRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/SIPTrunkResponse.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/SipInboundCredentials.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/SortParamRequest.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserBannedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserDeactivatedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserDeletedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserPresenceChangedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserReactivatedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserResponseCommonFields.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/UserUnbannedEvent.ktis excluded by!**/generated/**stream-video-android-core/src/main/kotlin/io/getstream/android/video/generated/models/VideoEvent.ktis excluded by!**/generated/**
📒 Files selected for processing (45)
demo-app/src/main/kotlin/io/getstream/video/android/App.ktdemo-app/src/main/kotlin/io/getstream/video/android/CallActivity.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/join/CallJoinScreen.ktdemo-app/src/main/res/values/strings.xmlstream-video-android-core/api/stream-video-android-core.apistream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ActiveStateGate.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/CallLeaveReason.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.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/ExternalCallRejectionHandler.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/analytics/CallAnalyticsHooks.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/JoinRequestHooks.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/MediaPermissionHook.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionAnalyticsObserver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionHook.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/Stage.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/WsHook.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/ClientCallEventData.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/ClientEventReporter.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/ClientEventReporterErrorMappers.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/reporting/TelemetryModel.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/faultinjector/FailureInjector.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/faultinjector/FailureKey.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/faultinjector/NoOpFailureInjector.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/telecom/IncomingCallTelecomAction.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketStateService.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-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/StreamCallActivityComposeDelegate.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioControlActions.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioRoomContent.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/activecall/CallContent.ktstream-video-android-ui-core/api/stream-video-android-ui-core.apistream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/AbstractCallActivity.ktstream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/ActivityCallOperations.ktstream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/ActivityCallOperationsWithCallLeaveReason.ktstream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
Comment on lines
+84
to
+89
| injectFault() | ||
| } | ||
|
|
||
| private fun injectFault() { | ||
| val faultInjector = FailureInjectorImpl() | ||
| StreamVideo.instanceOrNull()?.state?.failureInjector = faultInjector |
There was a problem hiding this comment.
Install the injector for SDK instances created later.
StreamVideo.instanceOrNull() can still be null here on cold starts without a persisted user, and this file already documents that deferred SDK init is a supported path. In that case the injector is never attached, so the new failure-injection menu silently stops working after login. Please hook this into SDK creation, or observe StreamVideo.instanceState and reapply it whenever a new instance appears.
🤖 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 `@demo-app/src/main/kotlin/io/getstream/video/android/App.kt` around lines 84 -
89, The current injectFault() only sets failureInjector on
StreamVideo.instanceOrNull(), which can be null at cold start so later-created
SDK instances never receive the injector; change injectFault() to also observe
StreamVideo.instanceState (or hook into the SDK creation path) and set
state.failureInjector = FailureInjectorImpl() whenever a new instance appears so
every future StreamVideo instance gets the injector; reference the injectFault()
function, FailureInjectorImpl, StreamVideo.instanceOrNull(), and
StreamVideo.instanceState when implementing the observer/creation hook and
ensure you reapply the injector on instance creation events.
Comment on lines
+26
to
+76
| private val faultCounts = mutableMapOf<FailureKey, Int>() | ||
|
|
||
| override fun enable(key: FailureKey) { | ||
| if ((faultCounts[key] ?: 0) == 0) faultCounts[key] = 1 | ||
| } | ||
|
|
||
| override fun disable(key: FailureKey) { | ||
| faultCounts[key] = 0 | ||
| } | ||
|
|
||
| override fun setEnabled(key: FailureKey, enabled: Boolean) { | ||
| if (enabled) enable(key) else disable(key) | ||
| } | ||
|
|
||
| override fun isEnabled(key: FailureKey): Boolean { | ||
| return (faultCounts[key] ?: 0) > 0 | ||
| } | ||
|
|
||
| override fun setCount(key: FailureKey, count: Int) { | ||
| faultCounts[key] = count | ||
| } | ||
|
|
||
| override fun getCount(key: FailureKey): Int { | ||
| return faultCounts[key] ?: 0 | ||
| } | ||
|
|
||
| override fun clear() { | ||
| faultCounts.clear() | ||
| } | ||
|
|
||
| override fun throwDebugFault(key: FailureKey) { | ||
| val count = faultCounts[key] ?: 0 | ||
| if (count > 0) { | ||
| faultCounts[key] = count - 1 | ||
| throw when (key) { | ||
| FailureKey.FAIL_LOCATION -> HttpException( | ||
| Response.error<String>( | ||
| 100, | ||
| okhttp3.ResponseBody.create(null, ""), | ||
| ), | ||
| ) | ||
| else -> RuntimeException("Failure injected: $key") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun sendFailResult(key: FailureKey): io.getstream.result.Result.Failure { | ||
| val count = faultCounts[key] ?: 0 | ||
| if (count > 0) { | ||
| faultCounts[key] = count - 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show FailureInjectorImpl implementation fully (bounded)
FILE="demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.kt"
echo "== $FILE (relevant excerpt) =="
nl -ba "$FILE" | sed -n '1,220p'
# 2) Find all usages of FailureInjectorImpl / FailureInjector interface
echo "== Searching for FailureInjectorImpl / FailureInjector usage =="
rg -n --hidden --glob '!*build/*' "FailureInjectorImpl|FailureInjector" demo-app/src/main/kotlin || true
# 3) Specifically find calls to throwDebugFault / sendFailResult and show call sites
echo "== Call sites for throwDebugFault / sendFailResult =="
rg -n --hidden --glob '!*build/*' "throwDebugFault\(|sendFailResult\(" demo-app/src/main/kotlin || true
# 4) Also check non-demo modules if FailureInjectorImpl is referenced elsewhere
echo "== Call sites across repository (optional) =="
rg -n --hidden --glob '!*build/*' "FailureInjectorImpl|throwDebugFault\(|sendFailResult\(" || true
# 5) Check for coroutine/thread contexts around join/socket/RTC flows mentioning these fault methods
echo "== Looking for join/socket/RTC fault injection references =="
rg -n --hidden --glob '!*build/*' "join|socket|rtc|RT|WebSocket" demo-app/src/main/kotlin/io/getstream/video/android/ui demo-app/src/main/kotlin/io/getstream/video/android || trueRepository: GetStream/stream-video-android
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.kt"
echo "== File length =="
wc -l "$FILE" || true
echo "== $FILE (lines 1-220) =="
cat -n "$FILE" | sed -n '1,220p'
echo "== Call sites for throwDebugFault / sendFailResult (demo-app) =="
rg -n --hidden --glob '!*build/*' "throwDebugFault\(|sendFailResult\(" demo-app/src/main/kotlin || true
echo "== References to FailureInjector / FailureInjectorImpl (demo-app) =="
rg -n --hidden --glob '!*build/*' "FailureInjectorImpl|FailureInjector" demo-app/src/main/kotlin || true
echo "== References across repository (only these symbols) =="
rg -n --hidden --glob '!*build/*' "FailureInjectorImpl|FailureInjector|throwDebugFault\(|sendFailResult\(" || trueRepository: GetStream/stream-video-android
Length of output: 10317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant call-site code around the referenced line numbers
sed -n '200,280p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt | nl -ba | sed -n '1,120p' || true
sed -n '420,520p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt | nl -ba | sed -n '1,140p' || true
sed -n '760,880p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt | nl -ba | sed -n '1,160p' || true
sed -n '1,160p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt | nl -ba | sed -n '1,200p' || true
# Demo UI wiring: how FailureInjectorImpl is created and used
sed -n '60,120p' demo-app/src/main/kotlin/io/getstream/video/android/App.kt | nl -ba | sed -n '1,140p' || true
sed -n '180,280p' demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt | nl -ba | sed -n '1,200p' || true
sed -n '80,260p' demo-app/src/main/kotlin/io/getstream/video/android/ui/join/CallJoinScreen.kt | nl -ba | sed -n '1,220p' || trueRepository: GetStream/stream-video-android
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SfuSocket.kt excerpt around throwDebugFault calls =="
# show a bit around the previously reported line numbers
sed -n '200,270p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt
echo
echo "== StreamVideoClient.kt excerpt around sendFailResult calls =="
sed -n '430,490p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt
echo
sed -n '800,860p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt
echo
echo "== ClientState.kt excerpt about failureInjector =="
sed -n '1,160p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
echo
echo "== demo-app App.kt excerpt where FailureInjectorImpl is created =="
sed -n '60,120p' demo-app/src/main/kotlin/io/getstream/video/android/App.kt
echo
echo "== demo-app FailureInjectorUi.kt excerpt where a FailureInjector is provided =="
sed -n '1,320p' demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt | head -n 260
echo
echo "== demo-app CallJoinScreen.kt excerpt wiring FailureInjectorUi =="
sed -n '180,260p' demo-app/src/main/kotlin/io/getstream/video/android/ui/join/CallJoinScreen.ktRepository: GetStream/stream-video-android
Length of output: 29260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt"
echo "== Occurrences of debugFaultInjectors( =="
rg -n "debugFaultInjectors\(" "$FILE"
# show around each occurrence
echo "== Context around debugFaultInjectors call sites =="
# use rg line numbers then sed ranges
python3 - <<'PY'
import re, subprocess, os, sys, textwrap, json
file_path = "stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt"
out = subprocess.check_output(["rg","-n","debugFaultInjectors\\(",file_path], text=True)
for line in out.strip().splitlines():
m = re.match(r".*:(\d+):", line)
if not m:
continue
ln = int(m.group(1))
start = max(1, ln-40)
end = ln+60
print(f"\n--- {file_path}:{ln} (showing {start}-{end}) ---")
# use sed to print range
subprocess.run(["sed","-n",f"{start},{end}p",file_path], check=False)
PYRepository: GetStream/stream-video-android
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt"
echo "== SfuSocket.kt first 120 lines =="
sed -n '1,140p' "$FILE"
echo
echo "== SfuSocket.kt lines 140-280 =="
sed -n '140,280p' "$FILE"
echo
echo "== Search for userScope / coroutine scope usage in SfuSocket.kt =="
rg -n "userScope|CoroutineScope|launch\(|withContext\(|Dispatchers\." "$FILE"Repository: GetStream/stream-video-android
Length of output: 13748
Make FailureInjectorImpl.faultCounts thread-safe
faultCounts is an unsynchronized mutableMapOf that’s mutated by Compose UI actions (clear, setCount) and by core async flows (SfuSocket.debugFaultInjectors via throwDebugFault, and StreamVideoClient via sendFailResult). These concurrent reads/writes/decrements can race and lose updates, making failure counts nondeterministic—guard all accesses with a Mutex/synchronized block or switch to an atomic/thread-safe structure.
🤖 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
`@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.kt`
around lines 26 - 76, The mutable map faultCounts in FailureInjectorImpl is not
thread-safe; guard all accesses/updates (reads, writes, decrement) to
faultCounts used by
enable/disable/setEnabled/isEnabled/setCount/getCount/clear/throwDebugFault/sendFailResult
to avoid races—either replace faultCounts with a thread-safe structure (e.g.,
ConcurrentHashMap<FailureKey, AtomicInteger> with atomic increments/decrements)
or serialize access using a Mutex/synchronized block around each method that
touches faultCounts; pick one approach and apply it consistently to every method
that reads or mutates faultCounts so counts cannot be lost under concurrent
Compose/UI and async flows.
Comment on lines
+45
to
+73
| session.filterNotNull() | ||
| .flatMapLatest { it.publisher.filterNotNull() } | ||
| .flatMapLatest { it.state.filterNotNull() } | ||
| .collect { state -> | ||
| publisherStage = getStage(state) | ||
| scope.launch { | ||
| hook.onPeerConnectionStateChanged( | ||
| role = PeerConnectionRole.PUBLISH, | ||
| iceState = session.value?.publisher?.value?.iceState?.value, | ||
| peerConnectionState = state, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| subscriberJob?.cancel() | ||
| subscriberJob = launch { | ||
| session.filterNotNull() | ||
| .flatMapLatest { it.subscriber.filterNotNull() } | ||
| .flatMapLatest { it.state.filterNotNull() } | ||
| .collect { state -> | ||
| subscriberStage = getStage(state) | ||
| scope.launch { | ||
| hook.onPeerConnectionStateChanged( | ||
| role = PeerConnectionRole.SUBSCRIBE, | ||
| iceState = session.value?.subscriber?.value?.iceState?.value, | ||
| peerConnectionState = state, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PeerConnectionAnalyticsObserver collectors =="
sed -n '40,76p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionAnalyticsObserver.kt
echo
echo "== RtcSession / StreamPeerConnection flow sources =="
fd 'RtcSession\.kt|StreamPeerConnection\.kt' stream-video-android-core/src/main/kotlin -x sh -c '
echo "--- $1 ---"
rg -n -C2 "iceState|state|StateFlow|PeerConnectionState|IceConnectionState" "$1"
' sh {}Repository: GetStream/stream-video-android
Length of output: 21521
Collect ICE state directly + avoid unstructured coroutine launches in the observer
In stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionAnalyticsObserver.kt (lines 45-73), the publisher/subscriber collectors only collect from it.state; iceState is passed as a snapshot (session.value?.publisher?.value?.iceState?.value / session.value?.subscriber?.value?.iceState?.value) rather than being collected itself. Any ICE-only transitions between state emissions won’t be observed by the hook, even though StreamPeerConnection.kt exposes state and iceState as separate flows.
Also, scope.launch { hook.onPeerConnectionStateChanged(...) } is nested inside the collect blocks, which breaks structured concurrency; ensure the hook work is launched in the same structured coroutine scope as the collector (and is canceled on teardown).
🤖 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/analytics/PeerConnectionAnalyticsObserver.kt`
around lines 45 - 73, The collector is only observing the peer connection
"state" but reads ICE state via a snapshot (session.value?...iceState?.value)
and also launches hook calls with scope.launch inside the collect, breaking
structured concurrency; change the publisher and subscriber collectors to
combine (or flatMapLatest) both the state and the iceState flows exposed on the
peer connection so ICE-only transitions are observed, replace the snapshot
access with the collected iceState value, and remove the nested scope.launch so
hook.onPeerConnectionStateChanged is invoked directly from the collector
coroutine (or within the same structured scope) so it is canceled with
publisherJob/subscriberJob; update code around session, publisherJob,
subscriberJob, getStage and hook.onPeerConnectionStateChanged
(PeerConnectionRole.PUBLISH / SUBSCRIBE) accordingly.
Comment on lines
+50
to
+56
| scope.launch { | ||
| hook.onPeerConnectionStateChanged( | ||
| role = PeerConnectionRole.PUBLISH, | ||
| iceState = session.value?.publisher?.value?.iceState?.value, | ||
| peerConnectionState = state, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Observer implementation =="
sed -n '40,100p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionAnalyticsObserver.kt
echo
echo "== Hook implementation =="
sed -n '23,44p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/PeerConnectionHook.kt
echo
echo "== Stop path =="
sed -n '65,80p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/analytics/CallAnalyticsHooks.ktRepository: GetStream/stream-video-android
Length of output: 3261
Remove the extra scope.launch inside the publisher/subscriber collectors.
stop()cancels onlypeerConnectionObserverJob, but these hook invocations are launched on the sharedscope, so they can outlivestop()and fire after analytics shutdown; usingscope.launchalso introduces nondeterministic publisher/subscriber ordering. SincePeerConnectionHook.onPeerConnectionStateChanged(...)is non-suspend, call it directly incollect {}.
Suggested fix
.collect { state ->
publisherStage = getStage(state)
- scope.launch {
- hook.onPeerConnectionStateChanged(
- role = PeerConnectionRole.PUBLISH,
- iceState = session.value?.publisher?.value?.iceState?.value,
- peerConnectionState = state,
- )
- }
+ hook.onPeerConnectionStateChanged(
+ role = PeerConnectionRole.PUBLISH,
+ iceState = session.value?.publisher?.value?.iceState?.value,
+ peerConnectionState = state,
+ )
}
@@
.collect { state ->
subscriberStage = getStage(state)
- scope.launch {
- hook.onPeerConnectionStateChanged(
- role = PeerConnectionRole.SUBSCRIBE,
- iceState = session.value?.subscriber?.value?.iceState?.value,
- peerConnectionState = state,
- )
- }
+ hook.onPeerConnectionStateChanged(
+ role = PeerConnectionRole.SUBSCRIBE,
+ iceState = session.value?.subscriber?.value?.iceState?.value,
+ peerConnectionState = state,
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scope.launch { | |
| hook.onPeerConnectionStateChanged( | |
| role = PeerConnectionRole.PUBLISH, | |
| iceState = session.value?.publisher?.value?.iceState?.value, | |
| peerConnectionState = state, | |
| ) | |
| } | |
| hook.onPeerConnectionStateChanged( | |
| role = PeerConnectionRole.PUBLISH, | |
| iceState = session.value?.publisher?.value?.iceState?.value, | |
| peerConnectionState = state, | |
| ) |
🤖 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/analytics/PeerConnectionAnalyticsObserver.kt`
around lines 50 - 56, The publisher/subscriber collectors currently call
hook.onPeerConnectionStateChanged(...) inside scope.launch which allows those
invocations to outlive peerConnectionObserverJob and introduces nondeterministic
ordering; remove the inner scope.launch and invoke
PeerConnectionHook.onPeerConnectionStateChanged(...) directly inside the collect
{ } blocks (e.g., where PeerConnectionRole.PUBLISH /
PeerConnectionRole.SUBSCRIBE and iceState/peerConnectionState are read) so the
calls are confined to the lifecycle of peerConnectionObserverJob and canceled by
stop().
|
|
||
| package io.getstream.video.android.core.analytics | ||
|
|
||
| enum class Stage { NOT_STARTED, IN_PROGRESS } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make this enum's visibility explicit.
Stage is public by default right now, so this telemetry state machine becomes part of the supported core API surface. If it is only used by the new analytics plumbing, mark it internal here instead of exporting it accidentally. As per coding guidelines, "Prefer explicit visibility modifiers; limit internal leakage across modules".
🤖 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/analytics/Stage.kt`
at line 19, The enum Stage is currently public by default; mark it internal to
avoid leaking it into the core public API by changing its declaration to use the
internal visibility modifier (i.e., internal enum class Stage { NOT_STARTED,
IN_PROGRESS }) and then update any usages/imports of Stage within the analytics
plumbing to compile with the reduced visibility; ensure no public API or other
modules depend on Stage before making the change.
Comment on lines
+250
to
+263
| scope.launch(CoroutineName("init#coordinatorSocket.connectionState.collect")) { | ||
| coordinatorConnectionModule.socketConnection.state().collect { it -> | ||
| // If the socket is reconnected then we have a new connection ID. | ||
| // We need to re-watch every watched call with the new connection ID | ||
| // (otherwise the WS events will stop) | ||
| val watchedCalls = calls | ||
| if (it is VideoSocketState.Connected && watchedCalls.isNotEmpty()) { | ||
| val filter = Filters.`in`("cid", watchedCalls.values.map { it.cid }).toMap() | ||
| queryCalls(filters = filter, watch = true).also { | ||
| if (it is Failure) { | ||
| logger.e { "Failed to re-watch calls (${it.value}" } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Protect calls before iterating it from the socket collector.
This collector walks a plain MutableMap from scope, while call(...), onCallCleanUp(...), and cleanup() mutate the same map elsewhere in this class. A reconnect racing with call creation/removal can throw here or build a partial CID filter. Snapshot the values under a shared lock or switch the cache itself to a concurrent map before watchedCalls.values.map { it.cid }.
🤖 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 250 - 263, The collector in scope.launch reads the plain MutableMap
calls without synchronization, which can race with mutations from call(...),
onCallCleanUp(...), and cleanup(); fix by taking a thread-safe snapshot or
making the cache concurrent before iterating: either wrap reading of calls into
a synchronized/Mutex-protected block (acquire the same lock used by mutators)
and build watchedCalls = calls.values.map { it.cid } inside that critical
section, or change the calls map to a ConcurrentHashMap and read its values
safely, then pass the resulting immutable list of CIDs into Filters.`in` and
queryCalls(...).
Comment on lines
73
to
+79
| onLeaveRoom?.invoke() ?: let { | ||
| call.leave() | ||
| call.leave( | ||
| CallLeaveReason.UserAction( | ||
| UserActionCause.CANCELLED_BY_SELF, | ||
| "Cancelled the call", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Thread the typed leave reason through onLeaveRoom.
This only reports CallLeaveReason in the default branch. Any caller that provides onLeaveRoom still gets a parameterless callback, so custom leave flows silently drop the new leave-reason telemetry. Please pass the reason through the callback API (or add a parallel callback that carries it) so overrides stay aligned with this PR’s contract.
🤖 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-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioControlActions.kt`
around lines 73 - 79, The onLeaveRoom callback currently is invoked
parameterless which drops the CallLeaveReason created in the default branch;
change the onLeaveRoom API to accept a CallLeaveReason (or add a new
onLeaveRoomWithReason callback) and pass the constructed CallLeaveReason into it
instead of ignoring it. Locate the invocation in AudioControlActions (the
onLeaveRoom?.invoke() call) and replace it with onLeaveRoom?.invoke(reason)
where reason is the CallLeaveReason(UserActionCause.CANCELLED_BY_SELF,
"Cancelled the call"), and update the onLeaveRoom declaration/clients to accept
the CallLeaveReason type so custom leave flows receive the same telemetry.
Ensure any new callback name or signature is propagated to callers and tests.
Comment on lines
+144
to
+149
| call.leave( | ||
| CallLeaveReason.SdkDriven( | ||
| SdkCause.PIP_ERROR, | ||
| "Error in Pip: ${e.message}", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Don’t send raw exception text in the leave reason.
e.message is unstructured platform data, and this payload is now part of client event reporting. That can leak device/app-specific details into telemetry. Prefer a fixed sanitized message such as "Failed to enter PiP" and keep any detailed exception text local-only if you still need it for debugging.
As per coding guidelines, "Sanitize logs to avoid dumping JWTs, ICE tokens, or call IDs in verbose logs".
🤖 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-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/audio/AudioRoomContent.kt`
around lines 144 - 149, The leave reason currently embeds raw exception text
("Error in Pip: ${e.message}") when calling call.leave with
CallLeaveReason.SdkDriven and SdkCause.PIP_ERROR; replace that dynamic message
with a fixed sanitized string like "Failed to enter PiP" and move the exception
details to a local-only debug/log statement (e.g., Log.e or a local logger or
debug-only println) so the telemetry payload contains no raw platform exception
data; update the call.leave invocation in AudioRoomContent.kt (the
CallLeaveReason.SdkDriven / SdkCause.PIP_ERROR site) accordingly.
Comment on lines
+194
to
+199
| call.leave( | ||
| CallLeaveReason.SdkDriven( | ||
| SdkCause.PIP_ERROR, | ||
| "Error in Pip: ${e.message}", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Sanitize the PiP failure reason before reporting it.
This has the same issue as the audio-room path: e.message is raw exception text, and pushing it into CallLeaveReason sends unsanitized diagnostics off-device. Use a stable sanitized reason and avoid embedding the platform exception message in telemetry.
As per coding guidelines, "Sanitize logs to avoid dumping JWTs, ICE tokens, or call IDs in verbose logs".
🤖 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-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/activecall/CallContent.kt`
around lines 194 - 199, The current call.leave invocation is passing raw
exception text (e.message) into CallLeaveReason.SdkDriven which leaks
un-sanitized diagnostics; update the CallContent.kt code that calls
call.leave(...) with CallLeaveReason.SdkDriven(SdkCause.PIP_ERROR, ...) to use a
stable, non-sensitive reason string (e.g., "pip_failed" or "pip_error") and do
NOT embed the platform exception message; if you need the exception for local
debugging, log it locally with a debug logger (e.g., processLogger.debug or
similar) but keep the telemetry/leave reason strictly sanitized.
Comment on lines
+23
to
+33
| public interface ActivityCallOperationsWithCallLeaveReason : ActivityCallOperations { | ||
|
|
||
| @StreamCallActivityDelicateApi | ||
| public fun leave( | ||
| call: Call, | ||
| callLeaveReason: CallLeaveReason, | ||
| onSuccess: (suspend (Call) -> Unit)? = null, | ||
| onError: (suspend (Exception) -> Unit)? = null, | ||
| ) { | ||
| // Do nothing | ||
| } |
There was a problem hiding this comment.
Don't make the new leave(..., CallLeaveReason, ...) overload a silent no-op.
Any implementation that adopts this interface but forgets to override the new method will now drop leave requests entirely. If you need a compatibility default here, delegate to leave(call, onSuccess, onError) so the old behavior is preserved.
Suggested fix
public interface ActivityCallOperationsWithCallLeaveReason : ActivityCallOperations {
`@StreamCallActivityDelicateApi`
public fun leave(
call: Call,
callLeaveReason: CallLeaveReason,
onSuccess: (suspend (Call) -> Unit)? = null,
onError: (suspend (Exception) -> Unit)? = null,
) {
- // Do nothing
+ leave(call, onSuccess, onError)
}
}🤖 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-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/ActivityCallOperationsWithCallLeaveReason.kt`
around lines 23 - 33, The new default implementation of
ActivityCallOperationsWithCallLeaveReason.leave(call, callLeaveReason,
onSuccess, onError) is a silent no-op which will drop leave requests if
implementations don't override it; change the default to delegate to the
existing leave(call, onSuccess, onError) overload so behavior is preserved
(i.e., call the older leave method from the new overload and pass through
onSuccess/onError), ensuring any existing implementations continue to work.
Contributor
SDK Size Comparison 📏
|
| ): SfuConnectionResult { | ||
| logger.i { "[connectInternal] #sfu; #track; reconnect=${reconnectDetails?.strategy}" } | ||
| logger.i { "noob [connectInternal] #sfu; #track; reconnect=${reconnectDetails?.strategy}" } | ||
| call.callAnalyticsHooks.wsHook.onWsInitiated(sfuName, reconnectDetails != null) |
Contributor
There was a problem hiding this comment.
can we move this inside sfu layer ?
…reporting-feedback
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
Describe why we are making this change
Implementation
Describe the implementation
🎨 UI Changes
Add relevant screenshots
Add relevant videos
Testing
Explain how this change can be tested (or why it can't be tested)
Provide a patch below if it is necessary for testing
Provide the patch summary here
Summary by CodeRabbit
New Features
Bug Fixes