Improve Sync-related database cleaning logic (#1933)

Task/Issue URL: https://app.asana.com/0/414235014887631/1205315938732113/f Description: Only clean database when sync is inactive at app startup. Delay database cleaning until app becomes active in case it starts in background. Update credentialsDatabaseCleanupFailed pixel name to remove noise caused by users remaining on current version.
duckduckgo · Aug 23, 2023 · 4074976 · 4074976
1 parent b97241f
commit 4074976
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 38 deletions.
diff --git a/Core/BookmarksCleanupErrorHandling.swift b/Core/BookmarksCleanupErrorHandling.swift
@@ -26,11 +26,15 @@ public class BookmarksCleanupErrorHandling: EventMapping<BookmarksCleanupError>
 
     public init() {
         super.init { event, _, _, _ in
-            let domainEvent = Pixel.Event.bookmarksCleanupFailed
-            let processedErrors = CoreDataErrorsParser.parse(error: event.cleanupError as NSError)
-            let params = processedErrors.errorPixelParameters
+            if event.cleanupError is BookmarksCleanupCancelledError {
+                Pixel.fire(pixel: .bookmarksCleanupAttemptedWhileSyncWasEnabled)
+            } else {
+                let domainEvent = Pixel.Event.bookmarksCleanupFailed
+                let processedErrors = CoreDataErrorsParser.parse(error: event.cleanupError as NSError)
+                let params = processedErrors.errorPixelParameters
 
-            Pixel.fire(pixel: domainEvent, error: event.cleanupError, withAdditionalParameters: params)
+                Pixel.fire(pixel: domainEvent, error: event.cleanupError, withAdditionalParameters: params)
+            }
         }
     }
 

diff --git a/Core/CredentialsCleanupErrorHandling.swift b/Core/CredentialsCleanupErrorHandling.swift
@@ -26,11 +26,15 @@ public class CredentialsCleanupErrorHandling: EventMapping<CredentialsCleanupErr
 
     public init() {
         super.init { event, _, _, _ in
-            let domainEvent = Pixel.Event.credentialsDatabaseCleanupFailed
-            let processedErrors = CoreDataErrorsParser.parse(error: event.cleanupError as NSError)
-            let params = processedErrors.errorPixelParameters
+            if event.cleanupError is CredentialsCleanupCancelledError {
+                Pixel.fire(pixel: .credentialsCleanupAttemptedWhileSyncWasEnabled)
+            } else {
+                let domainEvent = Pixel.Event.credentialsDatabaseCleanupFailed
+                let processedErrors = CoreDataErrorsParser.parse(error: event.cleanupError as NSError)
+                let params = processedErrors.errorPixelParameters
 
-            Pixel.fire(pixel: domainEvent, error: event.cleanupError, withAdditionalParameters: params)
+                Pixel.fire(pixel: domainEvent, error: event.cleanupError, withAdditionalParameters: params)
+            }
         }
     }
 

diff --git a/Core/PixelEvent.swift b/Core/PixelEvent.swift
@@ -855,7 +855,7 @@ extension Pixel.Event {
         case .bookmarksCleanupFailed: return "m_d_bookmarks_cleanup_failed"
         case .bookmarksCleanupAttemptedWhileSyncWasEnabled: return "m_d_bookmarks_cleanup_attempted_while_sync_was_enabled"
 
-        case .credentialsDatabaseCleanupFailed: return "m_d_credentials_database_cleanup_failed"
+        case .credentialsDatabaseCleanupFailed: return "m_d_credentials_database_cleanup_failed_2"
         case .credentialsCleanupAttemptedWhileSyncWasEnabled: return "m_d_credentials_cleanup_attempted_while_sync_was_enabled"
 
         case .invalidPayload(let configuration): return "m_d_\(configuration.rawValue)_invalid_payload".lowercased()

diff --git a/Core/SyncBookmarksAdapter.swift b/Core/SyncBookmarksAdapter.swift
@@ -45,7 +45,7 @@ public final class SyncBookmarksAdapter {
         }
     }
 
-    public func updateDatabaseCleanupSchedule(shouldEnable: Bool) {
+    public func cleanUpDatabaseAndUpdateSchedule(shouldEnable: Bool) {
         databaseCleaner.cleanUpDatabaseNow()
         if shouldEnable {
             databaseCleaner.scheduleRegularCleaning()

diff --git a/Core/SyncCredentialsAdapter.swift b/Core/SyncCredentialsAdapter.swift
@@ -38,11 +38,11 @@ public final class SyncCredentialsAdapter {
             secureVaultFactory: secureVaultFactory,
             secureVaultErrorReporter: secureVaultErrorReporter,
             errorEvents: CredentialsCleanupErrorHandling(),
-            log: .passwordManager
+            log: .generalLog
         )
     }
 
-    public func updateDatabaseCleanupSchedule(shouldEnable: Bool) {
+    public func cleanUpDatabaseAndUpdateSchedule(shouldEnable: Bool) {
         databaseCleaner.cleanUpDatabaseNow()
         if shouldEnable {
             databaseCleaner.scheduleRegularCleaning()

diff --git a/Core/SyncDataProviders.swift b/Core/SyncDataProviders.swift
@@ -18,6 +18,7 @@
 //
 
 import BrowserServicesKit
+import Combine
 import Common
 import DDGSync
 import Persistence
@@ -46,6 +47,37 @@ public class SyncDataProviders: DataProvidersSource {
         return providers.compactMap { $0 as? DataProviding }
     }
 
+    public func setUpDatabaseCleanersIfNeeded(syncService: DDGSync) {
+        guard !isDatabaseCleanersSetUp else {
+            return
+        }
+
+        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?.credentialsAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: isSyncDisabled)
+                self?.bookmarksAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: isSyncDisabled)
+            }
+
+        if syncService.authState == .inactive {
+            credentialsAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: true)
+            bookmarksAdapter.cleanUpDatabaseAndUpdateSchedule(shouldEnable: true)
+        }
+
+        isDatabaseCleanersSetUp = true
+    }
+
     public init(
         bookmarksDatabase: CoreDataDatabase,
         secureVaultFactory: AutofillVaultFactory = AutofillSecureVaultFactory,
@@ -80,7 +112,9 @@ public class SyncDataProviders: DataProvidersSource {
     }
 
     private var isSyncMetadaDatabaseLoaded: Bool = false
+    private var isDatabaseCleanersSetUp: Bool = false
     private var syncMetadata: SyncMetadataStore?
+    private var syncAuthStateDidChangeCancellable: AnyCancellable?
 
     private let syncMetadataDatabase: CoreDataDatabase = SyncMetadataDatabase.make()
     private let bookmarksDatabase: CoreDataDatabase

diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj
@@ -8816,7 +8816,7 @@
 			repositoryURL = "https://github.com/DuckDuckGo/BrowserServicesKit";
 			requirement = {
 				kind = exactVersion;
-				version = 75.0.3;
+				version = 75.0.4;
 			};
 		};
 		C14882EB27F211A000D59F0C /* XCRemoteSwiftPackageReference "SwiftSoup" */ = {

diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -15,8 +15,8 @@
         "repositoryURL": "https://github.com/DuckDuckGo/BrowserServicesKit",
         "state": {
           "branch": null,
-          "revision": "9bf7b16196ac4a78bf9189841d9823047b5c141b",
-          "version": "75.0.3"
+          "revision": "02e87697d9e8d897bde0a913b7ed5b0943cbe993",
+          "version": "75.0.4"
         }
       },
       {
@@ -156,7 +156,7 @@
       },
       {
         "package": "TrackerRadarKit",
-        "repositoryURL": "https://github.com/duckduckgo/TrackerRadarKit.git",
+        "repositoryURL": "https://github.com/duckduckgo/TrackerRadarKit",
         "state": {
           "branch": null,
           "revision": "4684440d03304e7638a2c8086895367e90987463",

diff --git a/DuckDuckGo/AppDelegate.swift b/DuckDuckGo/AppDelegate.swift
@@ -59,7 +59,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
     private var showKeyboardIfSettingOn = true
     private var lastBackgroundDate: Date?
 
-    private(set) var syncService: DDGSyncing!
+    private(set) var syncService: DDGSync!
     private(set) var syncDataProviders: SyncDataProviders!
     private var syncDidFinishCancellable: AnyCancellable?
     private var syncStateCancellable: AnyCancellable?
@@ -69,16 +69,16 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
     // swiftlint:disable:next function_body_length cyclomatic_complexity
     func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
 
-        #if targetEnvironment(simulator)
+#if targetEnvironment(simulator)
         if ProcessInfo.processInfo.environment["UITESTING"] == "true" {
             // Disable hardware keyboards.
             let setHardwareLayout = NSSelectorFromString("setHardwareLayout:")
             UITextInputMode.activeInputModes
-                // Filter `UIKeyboardInputMode`s.
+            // Filter `UIKeyboardInputMode`s.
                 .filter({ $0.responds(to: setHardwareLayout) })
                 .forEach { $0.perform(setHardwareLayout, with: nil) }
         }
-        #endif
+#endif
 
         // Can be removed after a couple of versions
         cleanUpMacPromoExperiment2()
@@ -113,13 +113,13 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         }
 
         removeEmailWaitlistState()
-                
+
         Database.shared.loadStore { context, error in
             guard let context = context else {
 
                 let parameters = [PixelParameters.applicationState: "\(application.applicationState.rawValue)",
                                   PixelParameters.dataAvailability: "\(application.isProtectedDataAvailable)"]
-                        
+
                 switch error {
                 case .none:
                     fatalError("Could not create database stack: Unknown Error")
@@ -192,23 +192,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         // MARK: Sync initialisation
 
         syncDataProviders = SyncDataProviders(bookmarksDatabase: bookmarksDatabase, secureVaultErrorReporter: SecureVaultErrorReporter.shared)
-        syncService = DDGSync(dataProvidersSource: syncDataProviders, errorEvents: SyncErrorHandler(), log: .syncLog)
+        let syncService = DDGSync(dataProvidersSource: syncDataProviders, errorEvents: SyncErrorHandler(), log: .syncLog)
         syncService.initializeIfNeeded(isInternalUser: InternalUserStore().isInternalUser)
-        syncStateCancellable = syncService.authStatePublisher
-            .prepend(syncService.authState)
-            .map { $0 == .inactive }
-            .removeDuplicates()
-            .sink { [weak self] isSyncDisabled in
-                self?.syncDataProviders.credentialsAdapter.updateDatabaseCleanupSchedule(shouldEnable: isSyncDisabled)
-                self?.syncDataProviders.bookmarksAdapter.updateDatabaseCleanupSchedule(shouldEnable: isSyncDisabled)
-            }
-        syncDataProviders.bookmarksAdapter.databaseCleaner.isSyncActive = { [weak self] in
-            self?.syncService.authState == .active
-        }
-        syncDataProviders.credentialsAdapter.databaseCleaner.isSyncActive = { [weak self] in
-            self?.syncService.authState == .active
-        }
-
+        self.syncService = syncService
 
         let storyboard: UIStoryboard = UIStoryboard(name: "Main", bundle: Bundle.main)
 
@@ -275,6 +261,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         guard !testing else { return }
 
         syncService.initializeIfNeeded(isInternalUser: InternalUserStore().isInternalUser)
+        syncDataProviders.setUpDatabaseCleanersIfNeeded(syncService: syncService)
 
         if !(overlayWindow?.rootViewController is AuthenticationViewController) {
             removeOverlay()