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

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

@@ -6,21 +6,29 @@ public protocol POSCatalogSyncCoordinatorProtocol {
     /// Performs a full catalog sync for the specified site
     /// - Parameter siteID: The site ID to sync catalog for
     /// - Returns: The synced catalog containing products and variations
+    /// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
     func performFullSync(for siteID: Int64) async throws -> POSCatalog
 
     /// Determines if a full sync should be performed based on the age of the last sync
     /// - Parameters:
     ///   - siteID: The site ID to check
     ///   - maxAge: Maximum age before a sync is considered stale
     /// - Returns: True if a sync should be performed
-    func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) -> Bool
+    func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool
 }
 
-public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
+public enum POSCatalogSyncError: Error, Equatable {
+    case syncAlreadyInProgress(siteID: Int64)
+}
+
+public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
     private let syncService: POSCatalogFullSyncServiceProtocol
     private let settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol
     private let grdbManager: GRDBManagerProtocol
 
+    /// Tracks ongoing syncs by site ID to prevent duplicates
+    private var ongoingSyncs: Set<Int64> = []
+
     public init(syncService: POSCatalogFullSyncServiceProtocol,
                 settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol? = nil,
                 grdbManager: GRDBManagerProtocol) {
@@ -30,18 +38,31 @@ public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol
     }
 
     public func performFullSync(for siteID: Int64) async throws -> POSCatalog {
+        if ongoingSyncs.contains(siteID) {
+            DDLogInfo("⚠️ POSCatalogSyncCoordinator: Sync already in progress for site \(siteID)")
+            throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
+        }
+
+        // Mark sync as in progress
+        ongoingSyncs.insert(siteID)
+
+        // Ensure cleanup happens regardless of success or failure
+        defer {
+            ongoingSyncs.remove(siteID)
+        }
+
         DDLogInfo("🔄 POSCatalogSyncCoordinator starting full sync for site \(siteID)")
 
         let catalog = try await syncService.startFullSync(for: siteID)
 
-        // Record the sync timestamp
         settingsStore.setPOSLastFullSyncDate(Date(), for: siteID)
 
         DDLogInfo("✅ POSCatalogSyncCoordinator completed full sync for site \(siteID)")
+
         return catalog
     }
 
-    public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) -> Bool {
+    public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool {
         if !siteExistsInDatabase(siteID: siteID) {
             DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) not found in database, sync needed")
             return true

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogSyncCoordinatorTests.swift

@@ -77,59 +77,59 @@ struct POSCatalogSyncCoordinatorTests {
 
     // MARK: - Should Sync Decision Tests
 
-    @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() {
+    @Test func shouldPerformFullSync_site_not_in_database_with_no_sync_history() async {
         // Given - site doesn't exist in database AND has no sync history
         mockSettingsStore.storedDates = [:]
         // Note: NOT creating site in database
 
         // When
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
 
         // Then - should sync because site doesn't exist in database
         #expect(shouldSync == true)
     }
 
-    @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() throws {
+    @Test func shouldPerformFullSync_returns_true_when_no_previous_sync() async throws {
         // Given - no previous sync date stored, but site exists in database
         // This is much less likely to happen, but could help at a migration point
         mockSettingsStore.storedDates = [:]
         try createSiteInDatabase(siteID: sampleSiteID)
 
         // When
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 3600)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 3600)
 
         // Then
         #expect(shouldSync == true)
         #expect(mockSettingsStore.getPOSLastFullSyncDateCallCount == 1)
     }
 
-    @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() throws {
+    @Test func shouldPerformFullSync_returns_true_when_sync_is_stale() async throws {
         // Given - previous sync was 2 hours ago, and site exists in database
         let twoHoursAgo = Date().addingTimeInterval(-2 * 60 * 60)
         mockSettingsStore.storedDates[sampleSiteID] = twoHoursAgo
         try createSiteInDatabase(siteID: sampleSiteID)
 
         // When - max age is 1 hour
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
 
         // Then
         #expect(shouldSync == true)
     }
 
-    @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() throws {
+    @Test func shouldPerformFullSync_returns_false_when_sync_is_fresh() async throws {
         // Given - previous sync was 30 minutes ago, and site exists in database
         let thirtyMinutesAgo = Date().addingTimeInterval(-30 * 60)
         mockSettingsStore.storedDates[sampleSiteID] = thirtyMinutesAgo
         try createSiteInDatabase(siteID: sampleSiteID)
 
         // When - max age is 1 hour
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
 
         // Then
         #expect(shouldSync == false)
     }
 
-    @Test func shouldPerformFullSync_handles_different_sites_independently() throws {
+    @Test func shouldPerformFullSync_handles_different_sites_independently() async throws {
         // Given
         let siteA: Int64 = 123
         let siteB: Int64 = 456
@@ -143,55 +143,126 @@ struct POSCatalogSyncCoordinatorTests {
         try createSiteInDatabase(siteID: siteB)
 
         // When
-        let shouldSyncA = sut.shouldPerformFullSync(for: siteA, maxAge: 2 * 60 * 60) // 2 hours
-        let shouldSyncB = sut.shouldPerformFullSync(for: siteB, maxAge: 2 * 60 * 60) // 2 hours
+        let shouldSyncA = await sut.shouldPerformFullSync(for: siteA, maxAge: 2 * 60 * 60) // 2 hours
+        let shouldSyncB = await sut.shouldPerformFullSync(for: siteB, maxAge: 2 * 60 * 60) // 2 hours
 
         // Then
         #expect(shouldSyncA == false) // Recent sync exists
         #expect(shouldSyncB == true)  // No previous sync
     }
 
-    @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() throws {
+    @Test func shouldPerformFullSync_with_zero_maxAge_always_returns_true() async throws {
         // Given - previous sync was just now, and site exists in database
         let justNow = Date()
         mockSettingsStore.storedDates[sampleSiteID] = justNow
         try createSiteInDatabase(siteID: sampleSiteID)
 
         // When - max age is 0 (always sync)
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 0)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 0)
 
         // Then
         #expect(shouldSync == true)
     }
 
     // MARK: - Database Check Tests
 
-    @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() {
+    @Test func shouldPerformFullSync_returns_true_when_site_not_in_database() async {
         // Given - site does not exist in database, but has recent sync date
         let recentSyncDate = Date().addingTimeInterval(-30 * 60) // 30 minutes ago
         mockSettingsStore.storedDates[sampleSiteID] = recentSyncDate
         // Note: not creating site in database so it won't exist
 
         // When - max age is 1 hour (normally wouldn't sync)
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
 
         // Then - should sync because site doesn't exist in database
         #expect(shouldSync == true)
     }
 
-    @Test func shouldPerformFullSync_respects_time_when_site_exists_in_database() throws {
+    @Test func shouldPerformFullSync_respects_time_when_site_exists_in_database() async throws {
         // Given - site exists in database with recent sync date
         let recentSyncDate = Date().addingTimeInterval(-30 * 60) // 30 minutes ago
         mockSettingsStore.storedDates[sampleSiteID] = recentSyncDate
         try createSiteInDatabase(siteID: sampleSiteID)
 
         // When - max age is 1 hour
-        let shouldSync = sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
+        let shouldSync = await sut.shouldPerformFullSync(for: sampleSiteID, maxAge: 60 * 60)
 
         // Then - should not sync because site exists and time hasn't passed
         #expect(shouldSync == false)
     }
 
+    // MARK: - Sync Tracking Tests
+
+    @Test func performFullSync_throws_error_when_sync_already_in_progress() async throws {
+        // Given - block the sync service so first sync will wait
+        let expectedCatalog = POSCatalog(products: [], variations: [])
+        mockSyncService.startFullSyncResult = expectedCatalog
+        mockSyncService.blockNextSync()
+
+        // Start first sync in a task (it will block waiting for continuation)
+        let firstSyncTask = Task {
+            try await sut.performFullSync(for: sampleSiteID)
+        }
+
+        // Give first sync a moment to start and get blocked
+        try await Task.sleep(nanoseconds: 10_000_000) // 10ms
+
+        // When - try to start second sync while first is blocked
+        do {
+            _ = try await sut.performFullSync(for: sampleSiteID)
+            #expect(Bool(false), "Should have thrown syncAlreadyInProgress error")
+        } catch let error as POSCatalogSyncError {
+            // Then
+            #expect(error == POSCatalogSyncError.syncAlreadyInProgress(siteID: sampleSiteID))
+        }
+
+        // Cleanup - resume the first sync and wait for it to complete
+        mockSyncService.resumeBlockedSync()
+        _ = try await firstSyncTask.value
+    }
+
+    @Test func performFullSync_allows_concurrent_syncs_for_different_sites() async throws {
+        // Given
+        let siteA: Int64 = 123
+        let siteB: Int64 = 456
+        let expectedCatalog = POSCatalog(products: [], variations: [])
+        mockSyncService.startFullSyncResult = expectedCatalog
+
+        // When - start syncs for different sites concurrently
+        async let syncA = sut.performFullSync(for: siteA)
+        async let syncB = sut.performFullSync(for: siteB)
+
+        // Then - both should complete successfully
+        let catalogA = try await syncA
+        let catalogB = try await syncB
+
+        #expect(catalogA.products.isEmpty)
+        #expect(catalogB.products.isEmpty)
+        #expect(mockSyncService.startFullSyncCallCount == 2)
+    }
+
+    @Test func sync_tracking_cleaned_up_on_error() async throws {
+        // Given
+        let expectedError = NSError(domain: "test", code: 1, userInfo: nil)
+        mockSyncService.startFullSyncError = expectedError
+
+        // When - sync fails
+        do {
+            _ = try await sut.performFullSync(for: sampleSiteID)
+            #expect(Bool(false), "Should have thrown error")
+        } catch {
+            // Expected error
+        }
+
+        // Then - subsequent sync should be allowed
+        mockSyncService.startFullSyncError = nil
+        mockSyncService.startFullSyncResult = POSCatalog(products: [], variations: [])
+
+        let catalog = try await sut.performFullSync(for: sampleSiteID)
+        #expect(catalog.products.isEmpty)
+    }
+
     // MARK: - Helper Methods
 
     private func createSiteInDatabase(siteID: Int64) throws {
@@ -207,6 +278,11 @@ struct POSCatalogSyncCoordinatorTests {
 final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol {
     var startFullSyncResult: POSCatalog = POSCatalog(products: [], variations: [])
     var startFullSyncError: Error?
+    var syncDelay: UInt64 = 0 // nanoseconds to delay before returning
+
+    // Controlled sync mechanism
+    private var syncContinuation: CheckedContinuation<Void, Never>?
+    private var shouldBlockSync = false
 
     private(set) var startFullSyncCallCount = 0
     private(set) var lastSyncSiteID: Int64?
@@ -215,10 +291,32 @@ final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol {
         startFullSyncCallCount += 1
         lastSyncSiteID = siteID
 
+        // If we should block, wait for continuation to be resumed
+        if shouldBlockSync {
+            await withCheckedContinuation { continuation in
+                syncContinuation = continuation
+            }
+        }
+
+        // Add delay if specified
+        if syncDelay > 0 {
+            try await Task.sleep(nanoseconds: syncDelay)
+        }
+
         if let error = startFullSyncError {
             throw error
         }
 
         return startFullSyncResult
     }
+
+    func blockNextSync() {
+        shouldBlockSync = true
+    }
+
+    func resumeBlockedSync() {
+        syncContinuation?.resume()
+        syncContinuation = nil
+        shouldBlockSync = false
+    }
 }

WooCommerce/Classes/ServiceLocator/ServiceLocator.swift

@@ -112,6 +112,10 @@ final class ServiceLocator {
     ///
     private static var _startupWaitingTimeTracker: AppStartupWaitingTimeTracker = AppStartupWaitingTimeTracker()
 
+    /// POS catalog sync coordinator
+    ///
+    private static var _posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol?
+
     // MARK: - Getters
 
     /// Provides the access point to the analytics.
@@ -299,6 +303,28 @@ final class ServiceLocator {
     static var startupWaitingTimeTracker: AppStartupWaitingTimeTracker {
         _startupWaitingTimeTracker
     }
+
+    /// Provides access point to the `POSCatalogSyncCoordinator`.
+    ///
+    static var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
+        guard featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) else {
+            fatalError("POSCatalogSyncCoordinator accessed when pointOfSaleLocalCatalogi1 feature flag is disabled")
+        }
+
+        guard let coordinator = _posCatalogSyncCoordinator else {
+            guard let credentials = stores.sessionManager.defaultCredentials,
+                  let syncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: grdbManager)
+            else {
+                fatalError("Failed to create POSCatalogSyncCoordinator due to missing credentials")
+            }
+
+            let coordinator = POSCatalogSyncCoordinator(syncService: syncService, grdbManager: grdbManager)
+            _posCatalogSyncCoordinator = coordinator
+            return coordinator
+        }
+
+        return coordinator
+    }
 }
 
 

WooCommerce/Classes/ViewRelated/MainTabBarController.swift

@@ -768,7 +768,7 @@ private extension MainTabBarController {
 
         // Configure POS catalog sync coordinator for local catalog syncing
         if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) {
-            posCatalogSyncCoordinator = createPOSCatalogSyncCoordinator()
+            posCatalogSyncCoordinator = ServiceLocator.posCatalogSyncCoordinator
         }
 
         // Configure hub menu tab coordinator once per logged in session potentially with multiple sites.
@@ -792,15 +792,6 @@ private extension MainTabBarController {
         OrdersSplitViewWrapperController(siteID: siteID)
     }
 
-    func createPOSCatalogSyncCoordinator() -> POSCatalogSyncCoordinatorProtocol? {
-        guard let credentials = ServiceLocator.stores.sessionManager.defaultCredentials,
-              let syncService = POSCatalogFullSyncService(credentials: credentials, grdbManager: ServiceLocator.grdbManager)
-        else {
-            return nil
-        }
-
-        return POSCatalogSyncCoordinator(syncService: syncService, grdbManager: ServiceLocator.grdbManager)
-    }
 
     func triggerPOSCatalogSyncIfNeeded(for siteID: Int64) async {
         guard let coordinator = posCatalogSyncCoordinator else {
@@ -809,14 +800,16 @@ private extension MainTabBarController {
 
         // Check if sync is needed (older than 24 hours)
         let maxAge: TimeInterval = 24 * 60 * 60
-        guard coordinator.shouldPerformFullSync(for: siteID, maxAge: maxAge) else {
+        guard await coordinator.shouldPerformFullSync(for: siteID, maxAge: maxAge) else {
             return
         }
 
         // Perform background sync
         Task.detached {
             do {
                 _ = try await coordinator.performFullSync(for: siteID)
+            } catch POSCatalogSyncError.syncAlreadyInProgress {
+                DDLogInfo("ℹ️ POS catalog sync already in progress for site \(siteID), skipping")
             } catch {
                 DDLogError("⚠️ POS catalog sync failed: \(error)")
             }