Create `SyncOrchestrator` #4176

jmartinesp · 2025-01-20T15:52:46Z

Content

Centralise the start/stop sync logic in a single component of the app:

Create SyncOrchestrator, which will act as the single point where sync start/stop can happen, based on several inputs (sync state, network available, app in foreground, app in call, app needing to sync an event for a notification).
Add the extra cases (app in call, syncing an event) to AppForegroundStateService.
Remove the start/stop sync logic from several parts of the code, replace them with toggling the inputs for the SyncOrchestrator.

Motivation and context

Having too many points where the sync could be started and stopped could cause issues where the sync was unexpectedly stopped when it should instead be running or a deadlock may happen for several start/stop calls.

It should help with #4150

Tests

Just play with the sync state with network online/offline and app foreground state, receiving notifications, joining calls, etc.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 24
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR

github-actions · 2025-01-20T15:57:26Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/UtojEU

...sh/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt

codecov · 2025-01-21T17:11:11Z

Codecov Report

Attention: Patch coverage is 75.96154% with 25 lines in your changes missing coverage. Please review.

Project coverage is 80.24%. Comparing base (6362e01) to head (27e841a).

Files with missing lines	Patch %	Lines
...oid/libraries/matrix/impl/timeline/RustTimeline.kt	0.00%	6 Missing ⚠️
...pnavstate/impl/DefaultAppForegroundStateService.kt	0.00%	6 Missing ⚠️
...tures/networkmonitor/impl/DefaultNetworkMonitor.kt	0.00%	4 Missing ⚠️
...n/io/element/android/appnav/di/SyncOrchestrator.kt	91.17%	2 Missing and 1 partial ⚠️
...ent/android/appnav/root/RootNavStateFlowFactory.kt	0.00%	2 Missing ⚠️
...io/element/android/appnav/di/MatrixSessionCache.kt	95.83%	0 Missing and 1 partial ⚠️
...atures/lockscreen/impl/DefaultLockScreenService.kt	0.00%	1 Missing ⚠️
...d/libraries/designsystem/utils/ForceOrientation.kt	0.00%	1 Missing ⚠️
...pnavstate/impl/DefaultAppNavigationStateService.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4176      +/-   ##
===========================================
+ Coverage    80.22%   80.24%   +0.02%     
===========================================
  Files         2043     2044       +1     
  Lines        54113    54153      +40     
  Branches      6570     6573       +3     
===========================================
+ Hits         43411    43457      +46     
+ Misses        8429     8421       -8     
- Partials      2273     2275       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

appnav/src/main/kotlin/io/element/android/appnav/SyncObserver.kt

bmarty · 2025-01-23T17:39:27Z

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

@@ -147,6 +138,9 @@ class LoggedInFlowNode @AssistedInject constructor(

    override fun onBuilt() {
        super.onBuilt()
+
+        syncOrchestrator.start()


I think this code will not be run if the app is killed and a Push is received. In this case the sync will not start?

I think you're right, and I completely overlooked this... This complicates things, as we would have to use SyncOrchestrator in the AppScope and that means holding as many as MatrixClients there are in MatrixClientsHolder, maybe following a similar approach 🫤 .

bmarty

Thanks! Please see my comments.

bmarty · 2025-01-23T17:43:25Z

...es/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt

-                        }
-                    }
+                // Make sure we mark the call as ended in the app state
+                if (appForegroundStateService.isInCall.value) {


Nit: I do not think it is useful to read the value before changing it, or do I miss something here?

bmarty · 2025-01-23T17:45:28Z

features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt

@@ -199,7 +199,7 @@ class LoginFlowNode @AssistedInject constructor(

    @Composable
    override fun View(modifier: Modifier) {
-        activity = LocalContext.current as? Activity
+        activity = LocalActivity.current


Maybe stick to the same line of code than for other classes:
val activity = requireNotNull(LocalActivity.current)

ganfra

Some remarks

ganfra · 2025-01-24T13:20:24Z

.../matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/sync/SyncOrchestrator.kt

+/**
+ * Observes the app state and network state to start/stop the sync service.
+ */
+interface SyncOrchestrator {


Not sure this class belongs the the matrix api, it's more an app thing to me

ganfra · 2025-01-24T13:20:33Z

...api/src/main/kotlin/io/element/android/libraries/matrix/api/sync/SyncOrchestratorProvider.kt

+/**
+ * Provides a [SyncOrchestrator] for a given [SessionId].
+ */
+fun interface SyncOrchestratorProvider {


ganfra · 2025-01-24T13:56:02Z

appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt

@@ -29,13 +30,14 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold
 @ContributesBinding(AppScope::class)
 class MatrixClientsHolder @Inject constructor(


Maybe we should rename this class so it's a bit clearer it's not just about MatrixClients?
Also we might want to create a class instead of using Pair?

I don't think we need the DefaultSyncOrchestratorProvider, we should let MatrixClientsHolder (with a new name :P) implements the SyncOrchestratorProvider

jmartinesp · 2025-01-27T10:30:36Z

@ganfra all the points should have been fixed in the latest commit.

ganfra

LGTM, thanks!

…ugh the whole app. The decision is based on several inputs: sync state, network available, app in foreground, app in call, app needing to sync an event for a notification.

… connectivity

…fail instead This prevents an issue when using the offline mode of the SDK, which made the wrong UI states to be shown when the `SyncState` is `Idle` (that is, after the service being manually stopped).

…they're not easily mistaken with internet connectivity instead

bmarty

A new bunch of comments, I have not run the code yet

bmarty · 2025-02-05T11:19:47Z

appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt

-@ContributesBinding(AppScope::class)
-class MatrixClientsHolder @Inject constructor(
+@ContributesBinding(AppScope::class, boundType = MatrixClientProvider::class)
+@ContributesBinding(AppScope::class, boundType = SyncOrchestratorProvider::class)


TIL we can have multiple @ContributesBinding annotations

Well, we don't have them anymore after the latest changes 😅 .

bmarty · 2025-02-05T11:21:50Z

appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt

-    private val sessionIdsToMatrixClient = ConcurrentHashMap<SessionId, MatrixClient>()
+    private val syncOrchestratorFactory: DefaultSyncOrchestrator.Factory,
+) : MatrixClientProvider, SyncOrchestratorProvider {
+    private val sessionIdsToMatrixClient = ConcurrentHashMap<SessionId, InMemoryMatrixSession>()


Maybe rename this val now that it will contain InMemoryMatrixSession?

bmarty · 2025-02-05T11:29:32Z

appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt

+    /**
+     * Stop observing the app state and network state.
+     */
+    override fun stop() {


Since this method is only used for test, you should remove it.
The background coroutine scope can be used from the TestScope, and can be provided in the constructor parameter.

bmarty · 2025-02-05T11:31:54Z

...sh/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt

+
+        // Start the sync if it wasn't already started
+        syncOrchestratorProvider.getSyncOrchestrator(notifiableEvent.sessionId)?.start() ?: return@withContext
+


I am wondering if matrixClientProvider.getOrRestore could also take care of starting the syncOrchestrator. Maybe the sync orchestrator should always be started? In this case the interface SyncOrchestrator could just be private?

Good call, this simplified the code quite a bit!

bmarty · 2025-02-05T11:39:29Z

appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt

+            baseCoroutineScope.launch(dispatchers.io) {
+                try {
+                    initialSyncMutex.lock()
+                    syncService.startSync()


Why do we need a first startSync here? The combine below is not enough to decide what to do depending on the status of all the observed flows?

I was mimicking what we had before in LoggedInFlowNode for the airgapped case. After the changes in the SDK it's probably not needed anymore.

bmarty · 2025-02-05T11:40:16Z

appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt

+            initialSyncMutex.lock()
+
+            combine(
+                // small debounce to avoid spamming startSync when the state is changing quickly in case of error.


Maybe the debounced can be moved after the combine block (before .distinctUntilChanged())?

This causes issues in the tests, since it's preventing some intermediate states from being generated. I'd rather keep it were it is, if possible.

...rc/main/kotlin/io/element/android/services/appnavstate/test/FakeAppForegroundStateService.kt

… the existing loop when offline mode is enabled for the Client in the SDK

… to it, remove the existing provider interface.

sonarqubecloud · 2025-02-05T12:46:00Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

bmarty reviewed Jan 20, 2025

View reviewed changes

...sh/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt Outdated Show resolved Hide resolved

jmartinesp added the PR-Feature For a new feature label Jan 22, 2025

jmartinesp force-pushed the refactor/sync-observer branch from 396fe48 to 801dd62 Compare January 22, 2025 11:12

jmartinesp commented Jan 22, 2025

View reviewed changes

appnav/src/main/kotlin/io/element/android/appnav/SyncObserver.kt Outdated Show resolved Hide resolved

jmartinesp marked this pull request as ready for review January 22, 2025 11:19

jmartinesp requested a review from a team as a code owner January 22, 2025 11:19

jmartinesp requested review from ganfra and removed request for a team January 22, 2025 11:19

jmartinesp changed the title ~~Create SyncObserver~~ Create SyncOrchestrator Jan 22, 2025

jmartinesp requested a review from bmarty January 22, 2025 15:29

bmarty reviewed Jan 23, 2025

View reviewed changes

ganfra reviewed Jan 24, 2025

View reviewed changes

ganfra approved these changes Jan 27, 2025

View reviewed changes

ganfra mentioned this pull request Feb 3, 2025

Let the SDK provide the "network is available information" #4215

Merged

9 tasks

jmartinesp added 5 commits February 5, 2025 10:03

Create SyncOrchestrator to centralise the sync start/stop flow thro…

85f7e5e

…ugh the whole app. The decision is based on several inputs: sync state, network available, app in foreground, app in call, app needing to sync an event for a notification.

Fix errors after rebase

64f5a30

Make network monitor return network connectivity status, not internet…

fa41a0a

… connectivity

Don't stop the SyncService when network connection is lost, let it …

4218140

…fail instead This prevents an issue when using the offline mode of the SDK, which made the wrong UI states to be shown when the `SyncState` is `Idle` (that is, after the service being manually stopped).

Rename NetworkStatus.Online/Offline to Connected/Disconnected so …

64abdf6

…they're not easily mistaken with internet connectivity instead

jmartinesp force-pushed the refactor/sync-observer branch from 3033b67 to 64abdf6 Compare February 5, 2025 10:30

jmartinesp requested a review from ganfra February 5, 2025 10:32

bmarty requested changes Feb 5, 2025

View reviewed changes

jmartinesp added 4 commits February 5, 2025 13:24

Remove DefaultSyncOrchestrator.stop()

68aa4a4

Start the SyncOrchestrator as soon as the Client is created

d1f2240

Remove the force initial sync, the orchestrator should handle that in…

d1afcb4

… the existing loop when offline mode is enabled for the Client in the SDK

Remove SyncOrchestrator interface, rename DefaultSyncOrchestrator…

27e841a

… to it, remove the existing provider interface.

jmartinesp requested a review from bmarty February 5, 2025 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `SyncOrchestrator` #4176

Create `SyncOrchestrator` #4176

jmartinesp commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

bmarty Jan 23, 2025

jmartinesp Jan 24, 2025

bmarty left a comment

bmarty Jan 23, 2025

bmarty Jan 23, 2025

ganfra left a comment

ganfra Jan 24, 2025

ganfra Jan 24, 2025

ganfra Jan 24, 2025

jmartinesp commented Jan 27, 2025

ganfra left a comment

bmarty left a comment

bmarty Feb 5, 2025

jmartinesp Feb 5, 2025

bmarty Feb 5, 2025

bmarty Feb 5, 2025

bmarty Feb 5, 2025

jmartinesp Feb 5, 2025

bmarty Feb 5, 2025

jmartinesp Feb 5, 2025 •

edited

Loading

bmarty Feb 5, 2025

jmartinesp Feb 5, 2025

sonarqubecloud bot commented Feb 5, 2025

		@@ -29,13 +30,14 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold
		@ContributesBinding(AppScope::class)
		class MatrixClientsHolder @Inject constructor(


		// Start the sync if it wasn't already started
		syncOrchestratorProvider.getSyncOrchestrator(notifiableEvent.sessionId)?.start() ?: return@withContext

Create SyncOrchestrator #4176

Are you sure you want to change the base?

Create SyncOrchestrator #4176

Conversation

jmartinesp commented Jan 20, 2025 • edited Loading

Content

Motivation and context

Tests

Tested devices

Checklist

github-actions bot commented Jan 20, 2025 • edited Loading

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Jan 27, 2025

ganfra left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 5, 2025

Quality Gate passed

Create `SyncOrchestrator` #4176

Create `SyncOrchestrator` #4176

jmartinesp commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

jmartinesp Feb 5, 2025 •

edited

Loading