Bug 1168892 - Implement a status response for synchronizers. r=nalexa…

…nder

ClientTests/MockProfile.swift

@@ -11,9 +11,9 @@ import Sync
 import XCTest
 
 public class MockSyncManager: SyncManager {
-    public func syncClients() -> Success { return succeed() }
+    public func syncClients() -> SyncResult { return deferResult(.Completed) }
     public func syncClientsAndTabs() -> Success { return succeed() }
-    public func syncHistory() -> Success { return succeed() }
+    public func syncHistory() -> SyncResult { return deferResult(.Completed) }
     public func onAddedAccount() -> Success {
         return succeed()
     }

Providers/Profile.swift

@@ -13,20 +13,17 @@ import XCGLogger
 // TODO: same comment as for SyncAuthState.swift!
 private let log = XCGLogger.defaultInstance()
 
-public class NoAccountError: SyncError {
-    public var description: String {
-        return "No account configured."
-    }
-}
-
 public protocol SyncManager {
-    func syncClients() -> Success
+    func syncClients() -> SyncResult
     func syncClientsAndTabs() -> Success
-    func syncHistory() -> Success
+    func syncHistory() -> SyncResult
     func onRemovedAccount(account: FirefoxAccount?) -> Success
     func onAddedAccount() -> Success
 }
 
+typealias EngineIdentifier = String
+typealias SyncFunction = (SyncDelegate, Prefs, Ready) -> SyncResult
+
 class ProfileFileAccessor: FileAccessor {
     init(profile: Profile) {
         let profileDirName = "profile.\(profile.localName())"
@@ -308,77 +305,100 @@ public class BrowserProfile: Profile {
             return done
         }
 
-        private func syncClientsWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> Success {
+        private func syncClientsWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> SyncResult {
             log.debug("Syncing clients to storage.")
             let clientSynchronizer = ready.synchronizer(ClientsSynchronizer.self, delegate: delegate, prefs: prefs)
             return clientSynchronizer.synchronizeLocalClients(self.profile.remoteClientsAndTabs, withServer: ready.client, info: ready.info)
         }
 
-        private func syncTabsWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> Success {
+        private func syncTabsWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> SyncResult {
             let storage = self.profile.remoteClientsAndTabs
             let tabSynchronizer = ready.synchronizer(TabsSynchronizer.self, delegate: delegate, prefs: prefs)
             return tabSynchronizer.synchronizeLocalTabs(storage, withServer: ready.client, info: ready.info)
         }
 
-        private func syncHistoryWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> Success {
+        private func syncHistoryWithDelegate(delegate: SyncDelegate, prefs: Prefs, ready: Ready) -> SyncResult {
             log.debug("Syncing history to storage.")
             let historySynchronizer = ready.synchronizer(HistorySynchronizer.self, delegate: delegate, prefs: prefs)
             return historySynchronizer.synchronizeLocalHistory(self.profile.history, withServer: ready.client, info: ready.info)
         }
 
-        func ignoreContinuableErrorInDeferred(deferred: Success) -> Success {
-            return deferred.bind() { result in
-                if let failure = result.failureValue where failure is ContinuableError {
-                    log.debug("Got continuable error \(failure); pretending that nothing failed.")
-                    return succeed()
-                }
-                return deferred
-            }
-        }
-
-        func doSync(label: String, synchronizers: (SyncDelegate, Prefs, Ready) -> Success ...) -> Success {
+        /**
+         * Returns nil if there's no account.
+         */
+        private func withSyncInputs<T>(label: EngineIdentifier? = nil, function: (SyncDelegate, Prefs, Ready) -> Deferred<Result<T>>) -> Deferred<Result<T>>? {
             if let account = profile.account {
-                log.info("Syncing \(label).")
+                if let label = label {
+                    log.info("Syncing \(label).")
+                }
 
                 let authState = account.syncAuthState
                 let syncPrefs = profile.prefs.branch("sync")
 
                 let readyDeferred = SyncStateMachine.toReady(authState, prefs: syncPrefs)
                 let delegate = profile.getSyncDelegate()
 
-                // Run them sequentially, ignoring continuable errors.
-                // TODO: find a better way to do this. We want to report if tab sync is disabled, for
-                // example.
                 return readyDeferred >>== { ready in
-                    let tasks = synchronizers.map { f in
-                        { self.ignoreContinuableErrorInDeferred(f(delegate, syncPrefs, ready)) }
-                    }
+                    function(delegate, syncPrefs, ready)
+                }
+            }
+
+            log.warning("No account; can't sync.")
+            return nil
+        }
+
+        /**
+         * Runs the single provided synchronization function and returns its status.
+         */
+        private func sync(label: EngineIdentifier, function: (SyncDelegate, Prefs, Ready) -> SyncResult) -> SyncResult {
+            return self.withSyncInputs(label: label, function: function) ??
+                   deferResult(.NotStarted(.NoAccount))
+        }
 
-                    return walk(tasks, { $0() })
+        /**
+         * Runs each of the provided synchronization functions with the same inputs.
+         * Returns an array of IDs and SyncStatuses the same length as the input.
+         */
+        private func syncSeveral(synchronizers: (EngineIdentifier, SyncFunction)...) -> Deferred<Result<[(EngineIdentifier, SyncStatus)]>> {
+            typealias Pair = (EngineIdentifier, SyncStatus)
+            let combined: (SyncDelegate, Prefs, Ready) -> Deferred<Result<[Pair]>> = { delegate, syncPrefs, ready in
+                let thunks = synchronizers.map { (i, f) in
+                    return { () -> Deferred<Result<Pair>> in
+                        log.debug("Syncing \(i)…")
+                        return f(delegate, syncPrefs, ready) >>== { deferResult((i, $0)) }
+                    }
                 }
+                return accumulate(thunks)
             }
 
-            log.warning("No account; can't sync \(label).")
-            return deferResult(NoAccountError())
+            return self.withSyncInputs(label: nil, function: combined) ??
+                   deferResult(synchronizers.map { ($0.0, .NotStarted(.NoAccount)) })
         }
 
         func syncEverything() -> Success {
-            return self.doSync("everything", synchronizers:
-                self.syncClientsWithDelegate,
-                self.syncTabsWithDelegate,
-                self.syncHistoryWithDelegate)
+            return self.syncSeveral(
+                ("clients", self.syncClientsWithDelegate),
+                ("tabs", self.syncTabsWithDelegate),
+                ("history", self.syncHistoryWithDelegate)
+            ) >>> succeed
         }
 
-        func syncClients() -> Success {
-            return self.doSync("clients", synchronizers: syncClientsWithDelegate)
+        func syncClients() -> SyncResult {
+            // TODO: recognize .NotStarted.
+            return self.sync("clients", function: syncClientsWithDelegate)
         }
 
         func syncClientsAndTabs() -> Success {
-            return self.doSync("clients and tabs", synchronizers: self.syncClientsWithDelegate, self.syncTabsWithDelegate)
+            // TODO: recognize .NotStarted.
+            return self.syncSeveral(
+                ("clients", self.syncClientsWithDelegate),
+                ("tabs", self.syncTabsWithDelegate)
+            ) >>> succeed
         }
 
-        func syncHistory() -> Success {
-            return self.doSync("history", synchronizers: syncHistoryWithDelegate)
+        func syncHistory() -> SyncResult {
+            // TODO: recognize .NotStarted.
+            return self.sync("history", function: syncHistoryWithDelegate)
         }
     }
 }

Sync/Synchronizers/ClientsSynchronizer.swift

@@ -227,12 +227,17 @@ public class ClientsSynchronizer: BaseSingleCollectionSynchronizer, Synchronizer
         }
     }
 
-    // TODO: return whether or not the sync should continue.
-    public func synchronizeLocalClients(localClients: RemoteClientsAndTabs, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> Success {
+    public func synchronizeLocalClients(localClients: RemoteClientsAndTabs, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> SyncResult {
         log.debug("Synchronizing clients.")
 
-        if !self.canSync() {
-            return deferResult(FatalError(message: "clients not mentioned in meta/global. Server wiped?"))
+        if let reason = self.reasonToNotSync() {
+            switch reason {
+            case .EngineRemotelyNotEnabled:
+                // This is a hard error for us.
+                return deferResult(FatalError(message: "clients not mentioned in meta/global. Server wiped?"))
+            default:
+                return deferResult(SyncStatus.NotStarted(reason))
+            }
         }
 
         let keys = self.scratchpad.keys?.value
@@ -248,12 +253,17 @@ public class ClientsSynchronizer: BaseSingleCollectionSynchronizer, Synchronizer
         if !self.remoteHasChanges(info) {
             log.debug("No remote changes for clients. (Last fetched \(self.lastFetched).)")
             return maybeUploadOurRecord(false, ifUnmodifiedSince: nil, toServer: clientsClient)
+                >>> { deferResult(.Completed) }
         }
 
+        // TODO: some of the commands we process might involve wiping collections or the
+        // entire profile. We should model this as an explicit status, and return it here
+        // instead of .Completed.
         return clientsClient.getSince(self.lastFetched)
             >>== { response in
                 return self.wipeIfNecessary(localClients)
                     >>> { self.applyStorageResponse(response, toLocalClients: localClients, withServer: clientsClient) }
-        }
+            }
+            >>> { deferResult(.Completed) }
     }
 }

Sync/Synchronizers/HistorySynchronizer.swift

@@ -202,9 +202,9 @@ public class HistorySynchronizer: BaseSingleCollectionSynchronizer, Synchronizer
            >>> succeed
     }
 
-    public func synchronizeLocalHistory(history: SyncableHistory, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> Success {
-        if !self.canSync() {
-            return deferResult(EngineNotEnabledError(engine: self.collection))
+    public func synchronizeLocalHistory(history: SyncableHistory, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> SyncResult {
+        if let reason = self.reasonToNotSync() {
+            return deferResult(.NotStarted(reason))
         }
 
         let keys = self.scratchpad.keys?.value
@@ -241,6 +241,7 @@ public class HistorySynchronizer: BaseSingleCollectionSynchronizer, Synchronizer
                 // to the last successfully applied record timestamp, no matter where we fail.
                 // There's no need to do the upload before bumping -- the storage of local changes is stable.
                >>> { self.uploadOutgoingFromStorage(history, lastTimestamp: 0, withServer: historyClient) }
+               >>> { return deferResult(.Completed) }
         }
 
         log.error("Couldn't make history factory.")

Sync/Synchronizers/Synchronizer.swift

@@ -43,25 +43,43 @@ public protocol Synchronizer {
     init(scratchpad: Scratchpad, delegate: SyncDelegate, basePrefs: Prefs)
 
     /**
-     * Return true if the current state of this synchronizer -- particularly prefs and scratchpad --
-     * allow a sync to occur.
+     * Return a reason if the current state of this synchronizer -- particularly prefs and scratchpad --
+     * prevent a routine sync from occurring.
      */
-    func canSync() -> Bool
+    func reasonToNotSync() -> SyncNotStartedReason?
 }
 
-public class FatalError: SyncError {
-    let message: String
-    init(message: String) {
-        self.message = message
-    }
+/**
+ * We sometimes wish to return something more nuanced than simple success or failure.
+ * For example, refusing to sync because the engine was disabled isn't success (nothing was transferred!)
+ * but it also isn't an error.
+ *
+ * To do this we model real failures -- something went wrong -- as failures in the Result, and
+ * everything else in this status enum. This will grow as we return more details from a sync to allow
+ * for batch scheduling, success-case backoff and so on.
+ */
+public enum SyncStatus {
+    case Completed                 // TODO: we pick up a bunch of info along the way. Pass it along.
+    case NotStarted(SyncNotStartedReason)
+}
 
-    public var description: String {
-        return self.message
-    }
+
+
+public typealias SyncResult = Deferred<Result<SyncStatus>>
+
+public enum SyncNotStartedReason {
+    case NoAccount
+    case Offline
+    case Backoff(remainingSeconds: Int)
+    case EngineRemotelyNotEnabled(collection: String)
+    case EngineFormatOutdated(needs: Int)
+    case EngineFormatTooNew(expected: Int)   // This'll disappear eventually; we'll wipe the server and upload m/g.
+    case StorageFormatOutdated(needs: Int)
+    case StorageFormatTooNew(expected: Int)  // This'll disappear eventually; we'll wipe the server and upload m/g.
+    case StateMachineNotReady                // Because we're not done implementing.
 }
 
-// These won't interrupt a multi-engine sync.
-public class ContinuableError: SyncError {
+public class FatalError: SyncError {
     let message: String
     init(message: String) {
         self.message = message
@@ -72,13 +90,6 @@ public class ContinuableError: SyncError {
     }
 }
 
-public class EngineNotEnabledError: ContinuableError {
-    init(engine: String) {
-        super.init(message: "Engine \(engine) not enabled in meta/global.")
-        log.debug("\(engine) sync disabled remotely.")
-    }
-}
-
 public protocol SingleCollectionSynchronizer {
     func remoteHasChanges(info: InfoCollections) -> Bool
 }
@@ -119,10 +130,26 @@ public class BaseSingleCollectionSynchronizer: SingleCollectionSynchronizer {
         return info.modified(self.collection) > self.lastFetched
     }
 
-    public func canSync() -> Bool {
-        if let engineMeta = self.scratchpad.global?.value.engines?[collection] {
-            return engineMeta.version == self.storageVersion
+    public func reasonToNotSync() -> SyncNotStartedReason? {
+        if let global = self.scratchpad.global?.value {
+            // There's no need to check the global storage format here; the state machine will already have
+            // done so.
+            if let engineMeta = self.scratchpad.global?.value.engines?[collection] {
+                if engineMeta.version > self.storageVersion {
+                    return .EngineFormatOutdated(needs: engineMeta.version)
+                }
+                if engineMeta.version < self.storageVersion {
+                    return .EngineFormatTooNew(expected: engineMeta.version)
+                }
+            } else {
+                return .EngineRemotelyNotEnabled(collection: self.collection)
+            }
+        } else {
+            // But a missing meta/global is a real problem.
+            return .StateMachineNotReady
         }
-        return false
+
+        // Success!
+        return nil
     }
 }

Sync/Synchronizers/TabsSynchronizer.swift

@@ -20,7 +20,7 @@ public class TabsSynchronizer: BaseSingleCollectionSynchronizer, Synchronizer {
         return TabsStorageVersion
     }
 
-    public func synchronizeLocalTabs(localTabs: RemoteClientsAndTabs, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> Success {
+    public func synchronizeLocalTabs(localTabs: RemoteClientsAndTabs, withServer storageClient: Sync15StorageClient, info: InfoCollections) -> SyncResult {
         func onResponseReceived(response: StorageResponse<[Record<TabsPayload>]>) -> Success {
 
             func afterWipe() -> Success {
@@ -59,14 +59,14 @@ public class TabsSynchronizer: BaseSingleCollectionSynchronizer, Synchronizer {
             return afterWipe()
         }
 
-        if !self.canSync() {
-            return deferResult(EngineNotEnabledError(engine: self.collection))
+        if let reason = self.reasonToNotSync() {
+            return deferResult(SyncStatus.NotStarted(reason))
         }
 
         if !self.remoteHasChanges(info) {
             // Nothing to do.
             // TODO: upload local tabs if they've changed or we're in a fresh start.
-            return succeed()
+            return deferResult(.Completed)
         }
 
         let keys = self.scratchpad.keys?.value
@@ -76,6 +76,7 @@ public class TabsSynchronizer: BaseSingleCollectionSynchronizer, Synchronizer {
 
             return tabsClient.getSince(self.lastFetched)
                 >>== onResponseReceived
+                >>> { deferResult(.Completed) }
         }
 
         log.error("Couldn't make tabs factory.")