Skip to content

[Local Catalog] Update full/incremental sync to have consistent interface #16187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 7, 2025
Merged

[Local Catalog] Update full/incremental sync to have consistent interface #16187

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b30df8
Update full and incremental sync interface to be consistent and mark …
jaclync Oct 1, 2025
a6819b3
Refactor `POSCatalogSyncCoordinatorProtocol` full and incremental syn…
jaclync Oct 1, 2025
c22baf5
Merge branch 'trunk' into feat/WOOMOB-1333-catalog-sync-updates
jaclync Oct 7, 2025
149167f
Remove redundant `forceSync` parameter in full/incremental sync metho…
jaclync Oct 7, 2025
ea8efcb
Add back arguments for syncs with/without age checks.
jaclync Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 69 additions & 42 deletions Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,32 @@ import Storage
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
/// Performs a full catalog sync if applicable for the specified site
/// - Parameters:
/// - siteID: The site ID to check
/// - siteID: The site ID to sync catalog for
/// - maxAge: Maximum age before a sync is considered stale
/// - Returns: True if a sync should be performed
func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool
/// - forceSync: Whether to bypass age checks and always sync
/// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
func performFullSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async throws
Copy link
Contributor

@joshheald joshheald Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you anticipate forceSync doing more than bypassing age checks? If not, perhaps we can just pass maxAge: 0 when we want to force it.

Copy link
Contributor Author

@jaclync jaclync Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion - nope, it's just used to skip the age check now. In 149167f, I removed this parameter and added non-negative maxAge validation and only fetch last incremental date from database for age check for positive maxAge in shouldPerformIncrementalSync.


/// Performs an incremental sync if applicable based on sync conditions
/// - Parameters:
/// - siteID: The site ID to sync catalog for
/// - maxAge: Maximum age before a sync is considered stale
/// - forceSync: Whether to bypass age checks and always sync
/// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
//periphery:ignore - remove ignore comment when incremental sync is integrated with POS
func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws
func performIncrementalSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async throws
}

public extension POSCatalogSyncCoordinatorProtocol {
func performFullSync(for siteID: Int64) async throws {
try await performFullSyncIfApplicable(for: siteID, maxAge: .zero, forceSync: true)
}

func performIncrementalSync(for siteID: Int64) async throws {
try await performIncrementalSyncIfApplicable(for: siteID, maxAge: .zero, forceSync: true)
}
}

public enum POSCatalogSyncError: Error, Equatable {
Expand All @@ -33,7 +40,6 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
private let fullSyncService: POSCatalogFullSyncServiceProtocol
private let incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol
private let grdbManager: GRDBManagerProtocol
private let maxIncrementalSyncAge: TimeInterval
private let catalogSizeLimit: Int
private let catalogSizeChecker: POSCatalogSizeCheckerProtocol

Expand All @@ -45,18 +51,20 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
public init(fullSyncService: POSCatalogFullSyncServiceProtocol,
incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol,
grdbManager: GRDBManagerProtocol,
maxIncrementalSyncAge: TimeInterval = 300,
catalogSizeLimit: Int? = nil,
catalogSizeChecker: POSCatalogSizeCheckerProtocol) {
self.fullSyncService = fullSyncService
self.incrementalSyncService = incrementalSyncService
self.grdbManager = grdbManager
self.maxIncrementalSyncAge = maxIncrementalSyncAge
self.catalogSizeLimit = catalogSizeLimit ?? Constants.defaultSizeLimitForPOSCatalog
self.catalogSizeChecker = catalogSizeChecker
}

public func performFullSync(for siteID: Int64) async throws {
public func performFullSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async throws {
guard await shouldPerformFullSync(for: siteID, maxAge: maxAge, forceSync: forceSync) else {
return
}

if ongoingSyncs.contains(siteID) {
DDLogInfo("⚠️ POSCatalogSyncCoordinator: Sync already in progress for site \(siteID)")
throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
Expand All @@ -81,12 +89,13 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
/// - Parameters:
/// - siteID: The site ID to check
/// - maxAge: Maximum age before a sync is considered stale
/// - forceSync: Whether to bypass age checks and always sync
/// - Returns: True if a sync should be performed
public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool {
return await shouldPerformFullSync(for: siteID, maxAge: maxAge, maxCatalogSize: catalogSizeLimit)
private func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async -> Bool {
await shouldPerformFullSync(for: siteID, maxAge: maxAge, forceSync: forceSync, maxCatalogSize: catalogSizeLimit)
}

private func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, maxCatalogSize: Int) async -> Bool {
private func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool, maxCatalogSize: Int) async -> Bool {
guard await isCatalogSizeWithinLimit(for: siteID, maxCatalogSize: maxCatalogSize) else {
return false
}
Expand All @@ -101,52 +110,48 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
return true
}

let age = Date().timeIntervalSince(lastSyncDate)
let shouldSync = age > maxAge
if !forceSync {
let age = Date().timeIntervalSince(lastSyncDate)
let shouldSync = age > maxAge

if shouldSync {
DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " +
"(max: \(Int(maxAge))s), sync needed")
} else {
DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " +
"(max: \(Int(maxAge))s), sync not needed")
if shouldSync {
DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " +
"(max: \(Int(maxAge))s), sync needed")
} else {
DDLogInfo("📋 POSCatalogSyncCoordinator: Last sync for site \(siteID) was \(Int(age))s ago " +
"(max: \(Int(maxAge))s), sync not needed")
}

return shouldSync
}

return shouldSync
return true
}

/// Performs an incremental sync if applicable based on sync conditions
/// - Parameters:
/// - siteID: The site ID to sync catalog for
/// - maxAge: Maximum age before a sync is considered stale
/// - forceSync: Whether to bypass age checks and always sync
/// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
public func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws {
try await performIncrementalSyncIfApplicable(for: siteID, forceSync: forceSync, maxCatalogSize: catalogSizeLimit)
public func performIncrementalSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async throws {
try await performIncrementalSyncIfApplicable(for: siteID, maxAge: maxAge, forceSync: forceSync, maxCatalogSize: catalogSizeLimit)
}

private func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool, maxCatalogSize: Int) async throws {
private func performIncrementalSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool, maxCatalogSize: Int) async throws {
guard await shouldPerformIncrementalSync(for: siteID, maxAge: maxAge, forceSync: forceSync, maxCatalogSize: maxCatalogSize) else {
return
}

if ongoingIncrementalSyncs.contains(siteID) {
DDLogInfo("⚠️ POSCatalogSyncCoordinator: Incremental sync already in progress for site \(siteID)")
throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
}

guard await isCatalogSizeWithinLimit(for: siteID, maxCatalogSize: maxCatalogSize) else {
return
}

guard let lastFullSyncDate = await lastFullSyncDate(for: siteID) else {
DDLogInfo("📋 POSCatalogSyncCoordinator: No full sync performed yet for site \(siteID), skipping incremental sync")
return
}

if !forceSync, let lastIncrementalSyncDate = await lastIncrementalSyncDate(for: siteID) {
let age = Date().timeIntervalSince(lastIncrementalSyncDate)

if age <= maxIncrementalSyncAge {
return DDLogInfo("📋 POSCatalogSyncCoordinator: Last incremental sync for site \(siteID) was \(Int(age))s ago, sync not needed")
}
}

ongoingIncrementalSyncs.insert(siteID)

defer {
Expand All @@ -162,6 +167,28 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
DDLogInfo("✅ POSCatalogSyncCoordinator completed incremental sync for site \(siteID)")
}

private func shouldPerformIncrementalSync(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool, maxCatalogSize: Int) async -> Bool {
guard await isCatalogSizeWithinLimit(for: siteID, maxCatalogSize: maxCatalogSize) else {
return false
}

guard await lastFullSyncDate(for: siteID) != nil else {
DDLogInfo("📋 POSCatalogSyncCoordinator: No full sync performed yet for site \(siteID), skipping incremental sync")
return false
}

if !forceSync, let lastIncrementalSyncDate = await lastIncrementalSyncDate(for: siteID) {
let age = Date().timeIntervalSince(lastIncrementalSyncDate)

if age <= maxAge {
DDLogInfo("📋 POSCatalogSyncCoordinator: Last incremental sync for site \(siteID) was \(Int(age))s ago, sync not needed")
return false
}
}

return true
}

// MARK: - Private

/// Checks if the catalog size is within the specified sync limit
Expand Down
Loading
Loading