Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 64 additions & 14 deletions Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ public protocol POSCatalogSyncCoordinatorProtocol {
/// - 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

/// Performs an incremental sync if applicable based on sync conditions
/// - Parameters:
/// - siteID: The site ID to sync catalog for
/// - 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
}

public enum POSCatalogSyncError: Error, Equatable {
Expand All @@ -23,26 +31,23 @@ public enum POSCatalogSyncError: Error, Equatable {

public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
private let fullSyncService: POSCatalogFullSyncServiceProtocol
private let persistenceService: POSCatalogPersistenceServiceProtocol
private let incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol
private let grdbManager: GRDBManagerProtocol
private let maxIncrementalSyncAge: TimeInterval

/// Tracks ongoing syncs by site ID to prevent duplicates
/// Tracks ongoing full syncs by site ID to prevent duplicates
private var ongoingSyncs: Set<Int64> = []
/// Tracks ongoing incremental syncs by site ID to prevent duplicates
private var ongoingIncrementalSyncs: Set<Int64> = []
Comment on lines +38 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether we really want to be able to do a full sync and an incremental sync for the same site simultaneously.

  1. Start a full sync that takes 1 minute to download
  2. 10s later, start an incremental sync which takes 30s to download
  3. 40s after starting, the incremental sync changes are written to the db
  4. 1m after starting, the full sync write starts – it deletes everything in the db for that site, and puts new data in.

Let's tackle in a separate ticket, as it's an edge case, but WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know whether we really want to be able to do a full sync and an incremental sync for the same site simultaneously.

Yea good point, incremental sync should be skipped when a full sync is ongoing. Created an issue WOOMOB-1331.


public init(fullSyncService: POSCatalogFullSyncServiceProtocol,
grdbManager: GRDBManagerProtocol) {
self.fullSyncService = fullSyncService
self.persistenceService = POSCatalogPersistenceService(grdbManager: grdbManager)
self.grdbManager = grdbManager
}

//periphery:ignore - used for tests to inject persistence service
init(fullSyncService: POSCatalogFullSyncServiceProtocol,
persistenceService: POSCatalogPersistenceServiceProtocol,
grdbManager: GRDBManagerProtocol) {
incrementalSyncService: POSCatalogIncrementalSyncServiceProtocol,
grdbManager: GRDBManagerProtocol,
maxIncrementalSyncAge: TimeInterval = 300) {
self.fullSyncService = fullSyncService
self.persistenceService = persistenceService
self.incrementalSyncService = incrementalSyncService
self.grdbManager = grdbManager
self.maxIncrementalSyncAge = maxIncrementalSyncAge
}

public func performFullSync(for siteID: Int64) async throws {
Expand All @@ -61,7 +66,7 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {

DDLogInfo("🔄 POSCatalogSyncCoordinator starting full sync for site \(siteID)")

let catalog = try await fullSyncService.startFullSync(for: siteID)
_ = try await fullSyncService.startFullSync(for: siteID)

DDLogInfo("✅ POSCatalogSyncCoordinator completed full sync for site \(siteID)")
}
Expand Down Expand Up @@ -89,6 +94,40 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
return shouldSync
}

public func performIncrementalSyncIfApplicable(for siteID: Int64, forceSync: Bool) async throws {
if ongoingIncrementalSyncs.contains(siteID) {
DDLogInfo("⚠️ POSCatalogSyncCoordinator: Incremental sync already in progress for site \(siteID)")
throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
}

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")
}
}
Comment on lines +98 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth moving this to a shouldPerform function, even if it's only used privately in this class and not exposed.

I think we should probably use the same approach for both – I don't always enjoy the "if applicable/if needed" uncertainty, but then it is the job of this coordinator to manage when syncs happen, so it's not unreasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think are the use cases / benefits of a separate shouldPerform that is public? It'd be useful for the call site to handle the result, but I don't think we plan to show any UI for it. A private shouldPerform function could make the code more readable though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think giving both a private shouldPerform is the best approach for now.

We probably don't ever need to make them public, but some possible future reasons:

  • To log analytics when a background task runs about whether we synced, or why we didn't.
  • To show the status in settings "will sync" or make different buttons available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think giving both a private shouldPerform is the best approach for now.

Sounds good, we can make it public when the use case calls for it. I created an issue WOOMOB-1333 for making this change.


ongoingIncrementalSyncs.insert(siteID)

defer {
ongoingIncrementalSyncs.remove(siteID)
}

DDLogInfo("🔄 POSCatalogSyncCoordinator starting incremental sync for site \(siteID)")

try await incrementalSyncService.startIncrementalSync(for: siteID,
lastFullSyncDate: lastFullSyncDate,
lastIncrementalSyncDate: lastIncrementalSyncDate(for: siteID))

DDLogInfo("✅ POSCatalogSyncCoordinator completed incremental sync for site \(siteID)")
}

// MARK: - Private

private func lastFullSyncDate(for siteID: Int64) async -> Date? {
Expand All @@ -102,6 +141,17 @@ public actor POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol {
}
}

private func lastIncrementalSyncDate(for siteID: Int64) async -> Date? {
do {
return try await grdbManager.databaseConnection.read { db in
return try PersistedSite.filter(key: siteID).fetchOne(db)?.lastCatalogIncrementalSyncDate
}
} catch {
DDLogError("⛔️ POSCatalogSyncCoordinator: Error loading site \(siteID) for incremental sync date: \(error)")
return nil
}
}
Comment on lines +144 to +153
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this, removing the loadSite/updateSite functions from the persistence service, and saving the dates as part of the catalog, not afterwards.

It's worked out to be much simpler IMO, but I wanted to bring it to your attention, so let me know if you think we should go back


private func siteExistsInDatabase(siteID: Int64) -> Bool {
do {
return try grdbManager.databaseConnection.read { db in
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import Foundation
@testable import Yosemite

final class MockPOSCatalogIncrementalSyncService: POSCatalogIncrementalSyncServiceProtocol {
var startIncrementalSyncResult: Result<Void, Error> = .success(())

private(set) var startIncrementalSyncCallCount = 0
private(set) var lastSyncSiteID: Int64?
private(set) var lastFullSyncDate: Date?
private(set) var lastIncrementalSyncDate: Date?

private var syncContinuation: CheckedContinuation<Void, Never>?
private var shouldBlockSync = false

func startIncrementalSync(for siteID: Int64, lastFullSyncDate: Date, lastIncrementalSyncDate: Date?) async throws {
startIncrementalSyncCallCount += 1
lastSyncSiteID = siteID
self.lastFullSyncDate = lastFullSyncDate
self.lastIncrementalSyncDate = lastIncrementalSyncDate

if shouldBlockSync {
await withCheckedContinuation { continuation in
syncContinuation = continuation
}
}

switch startIncrementalSyncResult {
case .success:
return
case .failure(let error):
throw error
}
}
}

extension MockPOSCatalogIncrementalSyncService {
func blockNextSync() {
shouldBlockSync = true
}

func resumeBlockedSync() {
syncContinuation?.resume()
syncContinuation = nil
shouldBlockSync = false
}
}
Loading