Stop treating an unknown network state as offline - #78
Merged
aleksandar-apostolov merged 1 commit intoSep 9, 2026
Merged
Conversation
`shouldDisconnect` read every non-Available network state as offline, `Unknown` included -- the state before any connectivity callback has fired. When the platform reports no viable network at registration time the monitor never seeds a state, so a foreground signal arriving while the socket is still connecting evaluates to Recovery.Disconnect and cancels the caller's connect() rather than letting it fail on its own. Only a reported loss counts as offline now, `Disconnected` or `Unavailable`, which matches how an `Unknown` lifecycle state is already handled.
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
aleksandar-apostolov
marked this pull request as ready for review
September 3, 2026 14:25
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 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 |
|
gpunto
approved these changes
Sep 9, 2026
aleksandar-apostolov
deleted the
fix/and-1486-unknown-network-not-offline
branch
September 9, 2026 10:13
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
Fixes AND-1486
StreamConnectionRecoveryEvaluatorImplread every non-Availablenetwork state as offline,StreamNetworkState.Unknownincluded.Unknownis the state before any connectivity callback hasfired:
StreamNetworkMonitorCallback.onRegistered()seeds a state only when the platform reports aviable network, so on a device with none it stays
Unknown.A foreground signal arriving in that state while the socket was still
ConnectingsatisfiedshouldDisconnect, sorecoveryEffectcancelled the connect single-flight and disconnected thesocket — the caller's
connect()ended in cancellation instead of the socket or network error itshould have surfaced. Reachable once #77 lands: before it, the blocking attach in
StreamLifecycleMonitorImpl.start()completed ahead of the listener subscription, so the replayedON_RESUMEreached nobody.Implementation
Only a reported loss counts as offline now,
DisconnectedorUnavailable.Unknownno longercontributes to
shouldDisconnect, which matches the lifecycle side of the same expression: italready tests
== Backgroundrather than!= Foreground, so anUnknownlifecycle state hasnever been read as backgrounded.
shouldConnectis untouched. It keys offAvailable, soUnknownnever triggered a reconnect andstill doesn't.
Testing
:stream-android-core:testDebugUnitTestwith--rerun-tasks.Two new tests. One asserts no recovery for a
Connectingsocket with anUnknownnetwork state;reverting the guard reddens it and nothing else. One pins the
Unavailablearm of the new check,which had no coverage — every existing disconnect test uses
Disconnected.One pre-existing failure in the run:
StreamCompositeEventSerializationImplTest.serialize - neither core nor product returns failurepicks a constructor withdeclaredConstructors.first()and callsit with three arguments. It passes in isolation, fails in a full run, and
developfails the sameway in a clean worktree.
Detekt: 1309 weighted issues, matching the
developbaseline.Checklist