refactor: integrate new initial sync - WPB-10801 #2527

johnxnguyen · 2025-02-12T14:47:13Z

WPB-10801 [iOS] Integrate new slow sync

Issue

In this PR we integrate the new initial sync by:

initializing and injecting SyncAgent into the ZMUserSession
replace calls to the SyncStatus with calls to SyncAgent

If the developer flag newInitialSync is off (the default), then the SyncAgent will execute legacy code paths via the SyncStatus. If it is on, it will run the new initial sync, then continue with the legacy incremental sync.

Integrating the new incremental sync will follow in a subsequent PR.

Testing

To check the legacy code paths are still in tact, install the app fresh and log in and use the app as normal. To test the new code paths, enabled the flag via the developer menu.

Issues to look for include infinite sync, getting stuck when logging in or registering a client (including deleting a client).

Checklist

Title contains a reference JIRA issue number like [WPB-XXX].
Description is filled and free of optional paragraphs.
Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

Make sure you use the API for UI elements that support large fonts.
All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
New UI elements have Accessibility strings for VoiceOver.

johnxnguyen · 2025-02-12T14:47:31Z

wire-ios-request-strategy/Sources/Protocols/ApplicationStatus.swift

    var synchronizationState: SynchronizationState { get }
    var operationState: OperationState { get }
    var clientRegistrationDelegate: ClientRegistrationDelegate { get }
    var requestCancellation: ZMRequestCancellation { get }

-    func requestResyncResources()


This was dead.

johnxnguyen · 2025-02-12T14:48:13Z

wire-ios-sync-engine/Source/SessionManager/SessionFactories.swift

Changes here are for injecting dependencies necessary to create the SyncAgent.

johnxnguyen · 2025-02-12T14:48:46Z

wire-ios-sync-engine/Source/Synchronization/ApplicationStatusDirectory.swift

-    public func requestResyncResources() {
-        syncStatus.resyncResources()
-    }
-
-    public func requestQuickSync() {
-        syncStatus.forceQuickSync()
-    }
-


This code was dead

johnxnguyen · 2025-02-12T14:50:27Z

wire-ios-sync-engine/Source/UserSession/SyncStatus.swift

-        RequestAvailableNotification.notifyNewRequestsAvailable(nil)
-        log("slow sync")
-        syncStateDelegate?.didStartSlowSync()
+        managedObjectContext.perform { [weak self] in


Previously, these code paths were always executed on the sync context (which is what managedObjectContext is). Now these methods may be called from async methods, so we need to ensure we switch to the right queue otherwise we'll get some core data errors.

johnxnguyen · 2025-02-12T14:51:16Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

-    // MARK: Network
-
-    public func requestResyncResources() {
-        applicationStatusDirectory.requestResyncResources()
-    }
-


This was dead code

johnxnguyen · 2025-02-12T14:52:09Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

-                    try await mlsService.repairOutOfSyncConversations()
-                } catch {
-                    WireLogger.mls.error("Repairing out of sync conversations failed: \(error)")
+        syncContext.perform { [weak self] in


Before we (bravely) assumed that this method would be invoked on the sync context, now it's not always true, so best to switch to it explicitly.

johnxnguyen · 2025-02-12T14:52:32Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

-        WireLogger.sync.debug("did finish quick sync")
-        processEvents()
+    private func didFinishIncrementalSync() {
+        syncContext.perform { [weak self] in


Changes here are to switch to the sync context.

johnxnguyen · 2025-02-12T14:54:30Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

-        if userClient.hasRegisteredMLSClient {
-            // Before the client was registered as an MLS client,
-            // They wouldn't have been able to migrate any conversations from Proteus to MLS.
-            // So we perform a slow sync to sync the conversations. This will ensure that
-            // the message protocol of each conversation is up-to-date.
-            // The client will then join any MLS groups they haven't joined yet.
-            syncStatus.forceSlowSync()
-        }


I deleted this because now we always trigger an initial sync (a few lines below)

johnxnguyen · 2025-02-12T15:25:06Z

WireDomain/Sources/WireDomain/Providers/InitialSyncBuilder.swift

I realized that I was missing a step in the pull resources sync to fetch the mls status, changes here are adding it.

johnxnguyen · 2025-02-12T15:25:34Z

WireDomain/Sources/WireDomain/Synchronization/PullResourcesSync.swift

Here we add the missing step I mentioned earlier.

netbe

looks good, adding some thoughts and concerns about processing buffered events in legacy

netbe · 2025-02-12T15:05:27Z

wire-ios-sync-engine/Source/SessionManager/SessionManager.swift

            } else if context.readMigrationNeedsSyncResourcesFlag() {
-                userSession.syncStatus.resyncResources()
+                userSession.triggerResourceSync()


nitpic: wouldn't it be?

Suggested change

userSession.triggerResourceSync()

userSession.triggerResourcesSync()

netbe · 2025-02-12T15:11:59Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

+
+            Task {
+                do {
+                    // Only sync if there is a self client, otherwise we'll perform a initial sync


Suggested change

// Only sync if there is a self client, otherwise we'll perform a initial sync

// Only sync if there is a self client, otherwise we'll perform an initial sync

netbe · 2025-02-12T15:12:27Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

+        }
+    }
+
+    public func triggerResourceSync() {


Suggested change

public func triggerResourceSync() {

public func triggerResourcesSync() {

netbe · 2025-02-12T15:19:03Z

.../Wire-iOS/Sources/Developer/DeveloperTools/DebugActions/DeveloperDebugActionsViewModel.swift

-            .init(title: "Resync resources", action: resyncResources),
-            .init(title: "Break next quick sync", action: breakNextQuickSync),
+            .init(title: "Trigger incremental sync", action: triggerIncrementalSync),
+            .init(title: "Trigger resource sync", action: triggerResourceSync),


Suggested change

.init(title: "Trigger resource sync", action: triggerResourceSync),

.init(title: "Trigger resources sync", action: triggerResourceSync),

netbe · 2025-02-12T15:26:59Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

+    func syncAgentDidStartInitialSync(_ syncAgent: SyncAgent) {
+        didStartInitialSync()
+    }
+
+    func syncAgentDidFinishInitialSync(_ syncAgent: SyncAgent) {
+        didFinishInitialSync()
+    }
+
+    func syncAgentDidStartLegacyInitialSync(_ syncAgent: SyncAgent) {
+        didStartInitialSync()
+    }
+
+    func syncAgentDidFinishLegacyInitialSync(_ syncAgent: SyncAgent) {
+        didFinishInitialSync()
+    }
+
+    func syncAgentDidStartLegacyIncrementalSync(_ syncAgent: SyncAgent) {
+        didStartIncrementalSync()
+    }

-    public func didStartSlowSync() {
+    func syncAgentDidFinishLegacyIncrementalSync(_ syncAgent: SyncAgent) {
+        didFinishIncrementalSync()
+    }


thought: might be good to make these methods async as we rework later the codebase we will be able to take advantage of it

netbe · 2025-02-12T15:30:37Z

wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift

+        syncContext.perform { [weak self] in
+            guard let self else { return }
+            WireLogger.sync.debug("did finish incremental sync")
+            processEvents()


issue: here we process buffered events in legacy code, isn't the new sync handling those?

suggestion: separate the implementation of didFinishIncrementalSync in distinct methods for legacy and new code. That way we can also really wait for the async function in the WaitingGroupTask

Not yet, this PR is only replacing the slow sync. I was planning to add the new quick sync in a next PR, but I could also add it here

as long as we don't forget, it's fine in the other PR. I'll approve as it is now

netbe

LGTM;)

github-actions · 2025-02-12T17:05:15Z

Test Results

1 files 2 suites 2m 8s ⏱️
1 789 tests 1 763 ✅ 26 💤 0 ❌
1 799 runs 1 773 ✅ 26 💤 0 ❌

Results for commit 94256c5.

♻️ This comment has been updated with latest results.

datadog-wireapp · 2025-02-12T17:06:59Z

Datadog Report

Branch report: refactor/integrate-sync-agent-wpb-10801
Commit report: 56a86c8
Test service: wire-ios-mono

✅ 0 Failed, 1687 Passed, 26 Skipped, 2m 8.16s Total Time

github-actions bot assigned johnxnguyen Feb 12, 2025

johnxnguyen commented Feb 12, 2025

View reviewed changes

netbe self-requested a review February 12, 2025 15:04

johnxnguyen commented Feb 12, 2025

View reviewed changes

netbe reviewed Feb 12, 2025

View reviewed changes

netbe approved these changes Feb 12, 2025

View reviewed changes

johnxnguyen marked this pull request as ready for review February 12, 2025 16:45

johnxnguyen changed the base branch from release/cycle-3.118 to develop February 14, 2025 08:04

johnxnguyen changed the base branch from develop to release/cycle-3.118 February 14, 2025 08:04

johnxnguyen added 11 commits February 14, 2025 12:10

inject SyncAgent to user session

5538c17

make user session delegate of sync agent

616cb74

make sync agent delegate of sync status

625c2f7

trigger syncs

57aed9a

delete dead code

028517d

fix todo

2bd6117

fix error

633f69c

fix log

4e5c087

remove dead code

880506e

run sync status methods on sync context

cc18864

fix wrong method call

2344b7d

johnxnguyen added 11 commits February 14, 2025 12:10

trigger sync only if there is a self client

aa02c53

trigger sync after client registration

1a92034

handle sync events on sync context

f1ed4f2

update label

f2fabfc

update todo

3b683cb

add missing resource step

6f2e08b

fix errors

2fac4d7

format

23be37a

fix tests

94437e2

fix typo

12338ef

format

6563468

johnxnguyen force-pushed the refactor/integrate-sync-agent-wpb-10801 branch from 94256c5 to 6563468 Compare February 14, 2025 11:11

johnxnguyen changed the base branch from release/cycle-3.118 to develop February 14, 2025 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: integrate new initial sync - WPB-10801 #2527

refactor: integrate new initial sync - WPB-10801 #2527

johnxnguyen commented Feb 12, 2025 •

edited by github-actions bot

Loading

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

johnxnguyen Feb 12, 2025

netbe left a comment

netbe Feb 12, 2025

netbe Feb 12, 2025

netbe Feb 12, 2025

netbe Feb 12, 2025

netbe Feb 12, 2025

netbe Feb 12, 2025

johnxnguyen Feb 12, 2025

netbe Feb 12, 2025

netbe left a comment

github-actions bot commented Feb 12, 2025 •

edited

Loading

datadog-wireapp bot commented Feb 12, 2025 •

edited

Loading

	userSession.triggerResourceSync()
	userSession.triggerResourcesSync()

	// Only sync if there is a self client, otherwise we'll perform a initial sync
	// Only sync if there is a self client, otherwise we'll perform an initial sync

	public func triggerResourceSync() {
	public func triggerResourcesSync() {

	.init(title: "Trigger resource sync", action: triggerResourceSync),
	.init(title: "Trigger resources sync", action: triggerResourceSync),

refactor: integrate new initial sync - WPB-10801 #2527

Are you sure you want to change the base?

refactor: integrate new initial sync - WPB-10801 #2527

Conversation

johnxnguyen commented Feb 12, 2025 • edited by github-actions bot Loading

Issue

Testing

Checklist

UI accessibility checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netbe 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

Choose a reason for hiding this comment

netbe left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 12, 2025 • edited Loading

Test Results

datadog-wireapp bot commented Feb 12, 2025 • edited Loading

Datadog Report

johnxnguyen commented Feb 12, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

datadog-wireapp bot commented Feb 12, 2025 •

edited

Loading