Improve Sync-related database cleaning logic

DuckDuckGo.xcodeproj/project.pbxproj

@@ -10287,7 +10287,7 @@
 			repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
 			requirement = {
 				kind = exactVersion;
-				version = 75.0.3;
+				version = 75.0.4;
 			};
 		};
 		AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {

DuckDuckGo/AppDelegate/AppDelegate.swift

@@ -284,15 +284,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel
         let syncDataProviders = SyncDataProviders(bookmarksDatabase: BookmarkDatabase.shared.db)
         let syncService = DDGSync(dataProvidersSource: syncDataProviders, errorEvents: SyncErrorHandler(), log: OSLog.sync)
         syncService.initializeIfNeeded(isInternalUser: internalUserDecider?.isInternalUser ?? false)
-
-        syncStateCancellable = syncService.authStatePublisher
-            .prepend(syncService.authState)
-            .map { $0 == .inactive }
-            .removeDuplicates()
-            .sink { isSyncDisabled in
-                LocalBookmarkManager.shared.updateBookmarkDatabaseCleanupSchedule(shouldEnable: isSyncDisabled)
-                syncDataProviders.credentialsAdapter.updateDatabaseCleanupSchedule(shouldEnable: isSyncDisabled)
-            }
+        syncDataProviders.setUpDatabaseCleaners(syncService: syncService)
 
         // This is also called in applicationDidBecomeActive, but we're also calling it here, since
         // syncService can be nil when applicationDidBecomeActive is called during startup, if a modal
@@ -303,14 +295,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel
 
         self.syncDataProviders = syncDataProviders
         self.syncService = syncService
-
-        bookmarksManager.bookmarkDatabaseCleaner.isSyncActive = { [weak self] in
-            self?.syncService?.authState == .active
-        }
-
-        syncDataProviders.credentialsAdapter.databaseCleaner.isSyncActive = { [weak self] in
-            self?.syncService?.authState == .active
-        }
     }
 
     private func subscribeToEmailProtectionStatusNotifications() {

DuckDuckGo/Bookmarks/Model/BookmarkManager.swift

@@ -45,9 +45,6 @@ protocol BookmarkManager: AnyObject {
     func moveFavorites(with objectUUIDs: [String], toIndex: Int?, completion: @escaping (Error?) -> Void)
     func importBookmarks(_ bookmarks: ImportedBookmarks, source: BookmarkImportSource) -> BookmarkImportResult
 
-    func cleanUpBookmarksDatabase()
-    func updateBookmarkDatabaseCleanupSchedule(shouldEnable: Bool)
-
     // Wrapper definition in a protocol is not supported yet
     var listPublisher: Published<BookmarkList?>.Publisher { get }
     var list: BookmarkList? { get }
@@ -65,12 +62,6 @@ final class LocalBookmarkManager: BookmarkManager {
         self.faviconManagement = faviconManagement
     }
 
-    let bookmarkDatabaseCleaner = BookmarkDatabaseCleaner(
-        bookmarkDatabase: BookmarkDatabase.shared.db,
-        errorEvents: BookmarksCleanupErrorHandling(),
-        log: .bookmarks
-    )
-
     @Published private(set) var list: BookmarkList?
     var listPublisher: Published<BookmarkList?>.Publisher { $list }
 
@@ -79,19 +70,6 @@ final class LocalBookmarkManager: BookmarkManager {
 
     // MARK: - Bookmarks
 
-    func updateBookmarkDatabaseCleanupSchedule(shouldEnable: Bool) {
-        bookmarkDatabaseCleaner.cleanUpDatabaseNow()
-        if shouldEnable {
-            bookmarkDatabaseCleaner.scheduleRegularCleaning()
-        } else {
-            bookmarkDatabaseCleaner.cancelCleaningSchedule()
-        }
-    }
-
-    func cleanUpBookmarksDatabase() {
-        bookmarkDatabaseCleaner.cleanUpDatabaseNow()
-    }
-
     func loadBookmarks() {
         bookmarkStore.loadAll(type: .topLevelEntities) { [weak self] (topLevelEntities, error) in
             guard error == nil, let topLevelEntities = topLevelEntities else {

DuckDuckGo/Fire/Model/Fire.swift

@@ -38,6 +38,7 @@ final class Fire {
     let pinnedTabsManager: PinnedTabsManager
     let bookmarkManager: BookmarkManager
     let syncService: DDGSyncing?
+    let syncDataProviders: SyncDataProviders?
     let tabCleanupPreparer = TabCleanupPreparer()
     let secureVaultFactory: AutofillVaultFactory
     let tld: TLD
@@ -96,6 +97,7 @@ final class Fire {
          tld: TLD,
          bookmarkManager: BookmarkManager = LocalBookmarkManager.shared,
          syncService: DDGSyncing? = nil,
+         syncDataProviders: SyncDataProviders? = nil,
          secureVaultFactory: AutofillVaultFactory = AutofillSecureVaultFactory
     ) {
         self.webCacheManager = cacheManager
@@ -108,6 +110,7 @@ final class Fire {
         self.pinnedTabsManager = pinnedTabsManager ?? WindowControllersManager.shared.pinnedTabsManager
         self.bookmarkManager = bookmarkManager
         self.syncService = syncService ?? (NSApp.delegate as? AppDelegate)?.syncService
+        self.syncDataProviders = syncDataProviders ?? (NSApp.delegate as? AppDelegate)?.syncDataProviders
         self.secureVaultFactory = secureVaultFactory
         self.tld = tld
 
@@ -519,7 +522,7 @@ final class Fire {
 
     private func burnDeletedBookmarks() {
         if syncService?.authState == .inactive {
-            bookmarkManager.cleanUpBookmarksDatabase()
+            syncDataProviders?.bookmarksAdapter.databaseCleaner.cleanUpDatabaseNow()
         }
     }
 }

DuckDuckGo/Sync/CredentialsCleanupErrorHandling.swift

@@ -25,7 +25,7 @@ public class CredentialsCleanupErrorHandling: EventMapping<CredentialsCleanupErr
 
     public init() {
         super.init { event, _, _, _ in
-            if event.cleanupError is CredentialsCleanupErrorHandling {
+            if event.cleanupError is CredentialsCleanupCancelledError {
                 Pixel.fire(.debug(event: .credentialsCleanupAttemptedWhileSyncWasEnabled))
             } else {
                 let processedErrors = CoreDataErrorsParser.parse(error: event.cleanupError as NSError)

DuckDuckGo/Sync/SyncBookmarksAdapter.swift

@@ -16,6 +16,7 @@
 //  limitations under the License.
 //
 
+import Bookmarks
 import Combine
 import Common
 import DDGSync
@@ -25,6 +26,24 @@ import SyncDataProviders
 final class SyncBookmarksAdapter {
 
     private(set) var provider: BookmarksProvider?
+    let databaseCleaner: BookmarkDatabaseCleaner
+
+    init(database: CoreDataDatabase) {
+        databaseCleaner = BookmarkDatabaseCleaner(
+            bookmarkDatabase: database,
+            errorEvents: BookmarksCleanupErrorHandling(),
+            log: .bookmarks
+        )
+    }
+
+    func cleanUpDatabaseAndUpdateSchedule(shouldEnable: Bool) {
+        databaseCleaner.cleanUpDatabaseNow()
+        if shouldEnable {
+            databaseCleaner.scheduleRegularCleaning()
+        } else {
+            databaseCleaner.cancelCleaningSchedule()
+        }
+    }
 
     func setUpProviderIfNeeded(database: CoreDataDatabase, metadataStore: SyncMetadataStore) {
         guard provider == nil else {

DuckDuckGo/Sync/SyncCredentialsAdapter.swift

@@ -39,7 +39,7 @@ final class SyncCredentialsAdapter {
         )
     }
 
-    func updateDatabaseCleanupSchedule(shouldEnable: Bool) {
+    func cleanUpDatabaseAndUpdateSchedule(shouldEnable: Bool) {
         databaseCleaner.cleanUpDatabaseNow()
         if shouldEnable {
             databaseCleaner.scheduleRegularCleaning()

DuckDuckGo/Sync/SyncDataProviders.swift

@@ -17,6 +17,7 @@
 //
 
 import BrowserServicesKit
+import Combine
 import Common
 import DDGSync
 import Persistence
@@ -44,10 +45,36 @@ final class SyncDataProviders: DataProvidersSource {
         return providers.compactMap { $0 as? DataProviding }
     }
 
+    func setUpDatabaseCleaners(syncService: DDGSync) {
+        bookmarksAdapter.databaseCleaner.isSyncActive = { [weak syncService] in
+            syncService?.authState == .active
+        }
+
+        credentialsAdapter.databaseCleaner.isSyncActive = { [weak syncService] in
+            syncService?.authState == .active
+        }
+
+        let syncAuthStateDidChangePublisher = syncService.authStatePublisher
+            .dropFirst()
+            .map { $0 == .inactive }
+            .removeDuplicates()
+
+        syncAuthStateDidChangeCancellable = syncAuthStateDidChangePublisher
+            .sink { [weak self] isSyncDisabled in
+                self?.bookmarksAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: isSyncDisabled)
+                self?.credentialsAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: isSyncDisabled)
+            }
+
+        if syncService.authState == .inactive {
+            bookmarksAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: true)
+            credentialsAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: true)
+        }
+    }
+
     init(bookmarksDatabase: CoreDataDatabase, secureVaultFactory: AutofillVaultFactory = AutofillSecureVaultFactory) {
         self.bookmarksDatabase = bookmarksDatabase
         self.secureVaultFactory = secureVaultFactory
-        bookmarksAdapter = SyncBookmarksAdapter()
+        bookmarksAdapter = SyncBookmarksAdapter(database: bookmarksDatabase)
         credentialsAdapter = SyncCredentialsAdapter(secureVaultFactory: secureVaultFactory)
     }
 
@@ -74,6 +101,7 @@ final class SyncDataProviders: DataProvidersSource {
 
     private var isSyncMetadaDatabaseLoaded: Bool = false
     private var syncMetadata: SyncMetadataStore?
+    private var syncAuthStateDidChangeCancellable: AnyCancellable?
 
     private let syncMetadataDatabase: SyncMetadataDatabase = SyncMetadataDatabase()
     private let bookmarksDatabase: CoreDataDatabase

LocalPackages/NetworkProtectionUI/Package.swift

@@ -15,7 +15,7 @@ let package = Package(
             targets: ["NetworkProtectionUI"])
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "75.0.3"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "75.0.4"),
         .package(path: "../SwiftUIExtensions")
     ],
     targets: [

UnitTests/HomePage/Mocks/MockBookmarkManager.swift

@@ -20,9 +20,6 @@ import Foundation
 @testable import DuckDuckGo_Privacy_Browser
 
 class MockBookmarkManager: BookmarkManager {
-    func cleanUpBookmarksDatabase() {}
-
-    func updateBookmarkDatabaseCleanupSchedule(shouldEnable: Bool) {}
 
     func isUrlFavorited(url: URL) -> Bool {
         return false