[POS][Local Catalog] Ensure we only run one sync at a time per site #16100

joshheald · 2025-09-05T17:51:36Z

Merge after: #16099
Part of: WOOMOB-1252

Description

This PR updates the POSCatalogSyncCoordinator to prevent simultaneous syncs for the same site, while allowing simultaneous syncs for different sites.

This may be over the top... but is potentially easier to manage long term than cancelling syncs.

It does this by:

Making the coordinator a singleton (via ServiceLocator)
Making it an actor so that performFullSync is safe to call concurrently
Adding a Set to hold sites being synced
Throwing an error when we call performFullSync for a site already being synced.

Steps to reproduce

Launch the app and switch to a store with a big catalog
Switch to another POS store
Switch back to the first store before its sync completes
Observe that the following message is shown in the console, and additional sync logs don't start:

POSCatalogSyncCoordinator: Site 247136746 not found in database, sync needed
⚠️ POSCatalogSyncCoordinator: Sync already in progress for site 247136746
ℹ️ POS catalog sync already in progress for site 247136746, skipping

Screenshots

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

joshheald · 2025-09-05T18:01:45Z

@jaclync I've realised the ServiceLocator pattern here is broken – if you log out and back in again with a different account, it'll still use the previous credentials, because the static var won't be replaced in that scenario.

I'll handle this on Monday, but it won't change the fundamentals of the PR – so I'm marking it ready for review anyway.

wpmobilebot · 2025-09-05T18:03:11Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16100-cb8b923`
	Version	`23.2`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`cb8b923`
	Installation URL	30nq24jf0287g

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

jaclync

Leaving some initial thoughts on the ServiceLocator usage!

jaclync · 2025-09-08T05:19:27Z

WooCommerce/Classes/ViewRelated/MainTabBarController.swift

        // Configure POS catalog sync coordinator for local catalog syncing
        if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) {
-            posCatalogSyncCoordinator = createPOSCatalogSyncCoordinator()
+            posCatalogSyncCoordinator = ServiceLocator.posCatalogSyncCoordinator


Personally, I felt like the move to a ServiceLocator static singleton could open a can of worms - handling login/login states, unit testing caveats, etc., which add to the complexity of the sync coordinator. What do you think of alternatives, like keeping a dictionary from site ID to catalog sync coordinator, then resetting it in removeViewControllers, all within MainTabBarController? This way, each sync coordinator is associated with a single site ID, making it easier to maintain. The changes in POSCatalogSyncCoordinator could become keeping track of whether a full sync is ongoing, and guard on this state at the beginning of a full sync.

Yeah, perhaps it's wrong.

I thought it should be on service locator, looking ahead to background updates. We'll need to deal with the sync coordinator in the background and in the foreground, and there really ought to only be one at a time.

For example, we don't want there to be a full sync in progress, then the app be backgrounded, a BGAppRefreshTask run, and start another full sync from that task.

Yup, there should certainly just be one single sync coordinator, I was mostly wondering if we could implement the singleton in a different way from a "static singleton" in ServiceLocator. If you think ServiceLocator is the best option, let's go for it.

What do you think about this approach?

cb8b923

By exposing it from AuthenticatedState, we make a new one whenever we log in or switch sites... but it's still long-lived, staying around as long as we're logged in.

What do you think about this approach?

Looks nice, thanks for the updates! I like that its life cycle is now tied to the authenticated state. I left a separate comment about resetting it on logout.

By exposing it from AuthenticatedState, we make a new one whenever we log in or switch sites...

From my understanding and quick testing, switching stores shouldn't trigger a reinitialization of AuthenticatedState, just logging in.

jaclync · 2025-09-08T05:22:37Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift

+        if ongoingSyncs.contains(siteID) {
+            DDLogInfo("⚠️ POSCatalogSyncCoordinator: Sync already in progress for site \(siteID)")
+            throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
+        }


How about moving it to shouldPerformFullSync?

I think probably better to keep it where it is. There could be some gap between calling shouldPerform and perform, which would make this check fail.

…nto woomob-1252-ensure-only-one-sync-at-a-time-per-site

jaclync

Works as expected!

jaclync · 2025-09-10T01:40:26Z

WooCommerce/Classes/Yosemite/AuthenticatedState.swift

+                grdbManager: ServiceLocator.grdbManager
+            )
+        } else {
+            posCatalogSyncCoordinator = nil


❓ Do we also want to reset posCatalogSyncCoordinator in willLeave when the merchant logs out? Perhaps can call a reset / onLogout function to cancel ongoing syncs and reset the database (can also be a future task).

I'll add a ticket for this. We may just want to let syncs complete though.

We need to check what the current behaviour with Core Data is, and what our requirements are, esp legal/privacy for clearing the db. Jirka mentioned that for GDPR compliance we may need to clear the database on any logout, but I'm not sure about whether our data is really personal enough to require that.

We need to check what the current behaviour with Core Data is

The app has been deleting all stored data in all tables in the database on logout:

woocommerce-ios/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift

Line 271 in 3914fc5

ServiceLocator.storageManager.reset()

and

woocommerce-ios/Modules/Sources/Storage/CoreData/CoreDataManager.swift

Lines 152 to 163 in 3914fc5

public func reset(onCompletion: (() -> Void)?) {

/// Delete all objects in the background context to avoid discrepancy with the view context

/// The view context will get updated automatically once the changes are saved to the persistent container.

performAndSave({ storage in

let backgroundContext = storage as! NSManagedObjectContext

/// persist self to complete deleting objects

self.deleteAllStoredObjects(in: backgroundContext)

}, completion: {

DDLogVerbose("💣 [CoreDataManager] Stack Destroyed!")

onCompletion?()

}, on: .main)

}

thus I was assuming that we are going to reset the database in GRDB as well. I don't know much about the legal requirements, but generally as a user I expect all data to be cleared when logging out manually from an app. If it's a shared device, leaving store data after logging out might not be ideal even though there is no sensitve information.

What do you think are the use cases of keeping the data after logout? Is it for the site credentials use case where the merchant needs to log out and in for different sites? I think a proper multi-site support would be preferred. Since we're deleting all the data at the beginning of each full sync and a full sync is triggered after logging in to an eligible site, I'm not sure how the previous data before login could be used (maybe in case the latest full sync fails?). Would like to hear your perspectives.

OK, thanks for the detail, we should follow that existing approach.

What do you think are the use cases of keeping the data after logout? Is it for the site credentials use case where the merchant needs to log out and in for different sites? I think a proper multi-site support would be preferred.

This, and yes I agree that something where you could be logged in to multiple sites with site credentials at the same time would be better.

Since we're deleting all the data at the beginning of each full sync and a full sync is triggered after logging in to an eligible site, I'm not sure how the previous data before login could be used (maybe in case the latest full sync fails?). Would like to hear your perspectives.

This comes more into the existing site switching for JP users...

We're only deleting all data for full syncs while the products aren't site-scoped. When we resolve that, we'd only delete the site's data when you switch to it.

At that point, we wouldn't need to redo a full-sync in this scenario:

Log in with WP.com to some site for the first time – full sync site A

Switch to another site – full sync site B

Switch back to site A – no need to full sync here, we already have it.

See WOOMOB-1322 for resetting the database

Sounds good to be consistent with how other storage data are handled on logout 👍

jaclync · 2025-09-10T02:09:58Z

WooCommerce/Classes/ViewRelated/MainTabBarController.swift

        // Configure POS catalog sync coordinator for local catalog syncing
        if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) {
-            posCatalogSyncCoordinator = createPOSCatalogSyncCoordinator()
+            posCatalogSyncCoordinator = ServiceLocator.posCatalogSyncCoordinator


What do you think about this approach?

Looks nice, thanks for the updates! I like that its life cycle is now tied to the authenticated state. I left a separate comment about resetting it on logout.

By exposing it from AuthenticatedState, we make a new one whenever we log in or switch sites...

From my understanding and quick testing, switching stores shouldn't trigger a reinitialization of AuthenticatedState, just logging in.

joshheald · 2025-09-10T05:26:10Z

From my understanding and quick testing, switching stores shouldn't trigger a reinitialization of AuthenticatedState, just logging in.

Yes, you're right. It doesn't need to happen on switching store, because the credentials are the same.

Ensure we don’t sync the same site concurrently

7c8c180

joshheald added this to the 23.3 milestone Sep 5, 2025

joshheald added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. feature: POS labels Sep 5, 2025

joshheald marked this pull request as ready for review September 5, 2025 18:01

joshheald requested a review from jaclync September 5, 2025 18:01

jaclync reviewed Sep 8, 2025

View reviewed changes

Base automatically changed from woomob-1252-always-full-sync-if-site-not-in-database to trunk September 9, 2025 13:43

joshheald added 2 commits September 9, 2025 15:37

Merge branch 'woomob-1252-always-full-sync-if-site-not-in-database' i…

5085f75

…nto woomob-1252-ensure-only-one-sync-at-a-time-per-site

Use a session-scoped sync coordinator

cb8b923

joshheald enabled auto-merge September 9, 2025 16:45

joshheald requested a review from jaclync September 9, 2025 16:47

joshheald disabled auto-merge September 9, 2025 16:47

jaclync approved these changes Sep 10, 2025

View reviewed changes

joshheald merged commit 2777090 into trunk Sep 10, 2025
14 checks passed

joshheald deleted the woomob-1252-ensure-only-one-sync-at-a-time-per-site branch September 10, 2025 05:26

jaclync mentioned this pull request Sep 10, 2025

[Local Catalog] Incremental sync: persist incremental sync date in Site storage entity #16108

Merged

1 task

	public func reset(onCompletion: (() -> Void)?) {
	/// Delete all objects in the background context to avoid discrepancy with the view context
	/// The view context will get updated automatically once the changes are saved to the persistent container.
	performAndSave({ storage in
	let backgroundContext = storage as! NSManagedObjectContext
	/// persist self to complete deleting objects
	self.deleteAllStoredObjects(in: backgroundContext)
	}, completion: {
	DDLogVerbose("💣 [CoreDataManager] Stack Destroyed!")
	onCompletion?()
	}, on: .main)
	}

[POS][Local Catalog] Ensure we only run one sync at a time per site #16100

[POS][Local Catalog] Ensure we only run one sync at a time per site #16100

Uh oh!

Conversation

joshheald commented Sep 5, 2025

Description

Steps to reproduce

Screenshots

Uh oh!

joshheald commented Sep 5, 2025

Uh oh!

wpmobilebot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshheald Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshheald commented Sep 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wpmobilebot commented Sep 5, 2025 •

edited

Loading

joshheald Sep 9, 2025 •

edited

Loading