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

Author: joshheald

Modules/Sources/Yosemite/Base/StoresManager.swift

@@ -65,6 +65,10 @@ public protocol StoresManager {
     ///
     var site: AnyPublisher<Site?, Never> { get }
 
+    /// Provides access to the session-scoped POS catalog sync coordinator
+    ///
+    var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? { get }
+
     /// Indicates if we need a Default StoreID, or there's one already set.
     ///
     var needsDefaultStore: Bool { get }

Modules/Sources/Yosemite/Model/Mocks/MockStoresManager.swift

@@ -253,6 +253,10 @@ public class MockStoresManager: StoresManager {
     public func shouldAuthenticateAdminPage(for site: Site) -> Bool {
         return false
     }
+
+    public var posCatalogSyncCoordinator: (any POSCatalogSyncCoordinatorProtocol)? {
+        nil
+    }
 }
 
 private extension MockStoresManager {

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

@@ -5,21 +5,29 @@ import GRDB
 public protocol POSCatalogSyncCoordinatorProtocol {
     /// Performs a full catalog sync for the specified site
     /// - Parameter siteID: The site ID to sync catalog for
+    /// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
     func performFullSync(for siteID: Int64) async throws
 
     /// 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 fullSyncService: 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(fullSyncService: POSCatalogFullSyncServiceProtocol,
                 settingsStore: SiteSpecificAppSettingsStoreMethodsProtocol? = nil,
                 grdbManager: GRDBManagerProtocol) {
@@ -29,17 +37,29 @@ public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol
     }
 
     public func performFullSync(for siteID: Int64) async throws {
+        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 fullSyncService.startFullSync(for: siteID)
 
-        // Record the sync timestamp
         settingsStore.setPOSLastFullSyncDate(Date(), for: siteID)
 
         DDLogInfo("✅ POSCatalogSyncCoordinator completed full sync for site \(siteID)")
     }
 
-    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

@@ -46,7 +46,7 @@ struct POSCatalogSyncCoordinatorTests {
         mockSyncService.startFullSyncResult = .success(expectedCatalog)
 
         // When
-        _ = try await sut.performFullSync(for: sampleSiteID)
+        try await sut.performFullSync(for: sampleSiteID)
         let afterSync = Date()
 
         // Then
@@ -66,7 +66,7 @@ struct POSCatalogSyncCoordinatorTests {
 
         // When/Then
         await #expect(throws: expectedError) {
-            _ = try await sut.performFullSync(for: sampleSiteID)
+            try await sut.performFullSync(for: sampleSiteID)
         }
 
         // Should not store timestamp on failure
@@ -75,59 +75,59 @@ struct POSCatalogSyncCoordinatorTests {
 
     // MARK: - Should Sync Decision Tests
 
-    @Test func shouldPerformFullSync_returns_true_when_site_is_not_in_database_with_no_sync_history() {
+    @Test func shouldPerformFullSync_returns_true_when_site_is_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_site_is_in_database_with_no_previous_sync() throws {
+    @Test func shouldPerformFullSync_returns_true_when_site_is_in_database_with_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
@@ -141,55 +141,121 @@ 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 = .success(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 = .success(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
+        try await syncA
+        try await syncB
+        #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.startFullSyncResult = .failure(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.startFullSyncResult = .success(POSCatalog(products: [], variations: []))
+
+        try await sut.performFullSync(for: sampleSiteID)
+    }
+
     // MARK: - Helper Methods
 
     private func createSiteInDatabase(siteID: Int64) throws {
@@ -204,6 +270,11 @@ struct POSCatalogSyncCoordinatorTests {
 
 final class MockPOSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol {
     var startFullSyncResult: Result<POSCatalog, Error> = .success(POSCatalog(products: [], variations: []))
+    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?
@@ -212,11 +283,33 @@ 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)
+        }
+
         switch startFullSyncResult {
         case .success(let catalog):
             return catalog
         case .failure(let error):
             throw error
         }
     }
+
+    func blockNextSync() {
+        shouldBlockSync = true
+    }
+
+    func resumeBlockedSync() {
+        syncContinuation?.resume()
+        syncContinuation = nil
+        shouldBlockSync = false
+    }
 }

WooCommerce/Classes/ServiceLocator/ServiceLocator.swift

@@ -299,6 +299,17 @@ final class ServiceLocator {
     static var startupWaitingTimeTracker: AppStartupWaitingTimeTracker {
         _startupWaitingTimeTracker
     }
+
+    /// Provides access point to the `POSCatalogSyncCoordinator`.
+    /// Returns nil if feature flag is disabled or user is not authenticated.
+    ///
+    static var posCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol? {
+        guard featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) else {
+            return nil
+        }
+
+        return stores.posCatalogSyncCoordinator
+    }
 }