Use Cron for periodic account attributes update

Signal/Provisioning/ProvisioningCoordinatorImpl.swift

@@ -730,7 +730,7 @@ class ProvisioningCoordinatorImpl: ProvisioningCoordinator {
             registrationRecoveryPassword: registrationRecoveryPassword,
             encryptedDeviceName: encryptedDeviceName,
             discoverableByPhoneNumber: phoneNumberDiscoverability,
-            hasSVRBackups: hasSVRBackups
+            capabilities: AccountAttributes.Capabilities(hasSVRBackups: hasSVRBackups),
         )
     }
 

Signal/Registration/RegistrationCoordinatorImpl.swift

@@ -4702,7 +4702,7 @@ public class RegistrationCoordinatorImpl: RegistrationCoordinator {
             registrationRecoveryPassword: inMemoryState.regRecoveryPw,
             encryptedDeviceName: nil, // This class only deals in primary devices, which have no name
             discoverableByPhoneNumber: inMemoryState.phoneNumberDiscoverability,
-            hasSVRBackups: hasSVRBackups
+            capabilities: AccountAttributes.Capabilities(hasSVRBackups: hasSVRBackups),
         )
     }
 

Signal/test/Registration/RegistrationCoordinatorTest.swift

@@ -3393,7 +3393,7 @@ public class RegistrationCoordinatorTest {
                 registrationRecoveryPassword: masterKey?.regRecoveryPw,
                 encryptedDeviceName: nil,
                 discoverableByPhoneNumber: .nobody,
-                hasSVRBackups: true
+                capabilities: AccountAttributes.Capabilities(hasSVRBackups: true),
             )
         }
 

SignalServiceKit/Environment/AppSetup.swift

@@ -409,6 +409,7 @@ extension AppSetup.GlobalsContinuation {
             ),
             appReadiness: appReadiness,
             appVersion: appVersion,
+            cron: cron,
             dateProvider: dateProvider,
             db: db,
             networkManager: networkManager,

SignalServiceKit/Jobs/Cron.swift

@@ -74,6 +74,7 @@ public class Cron {
         case fetchStaleGroup
         case fetchStaleProfiles
         case fetchSubscriptionConfig
+        case updateAttributes
     }
 
     init(

SignalServiceKit/Network/API/Requests/AccountAttributes/AccountAttributes.swift

@@ -85,7 +85,7 @@ public struct AccountAttributes: Codable {
         registrationRecoveryPassword: String?,
         encryptedDeviceName: String?,
         discoverableByPhoneNumber: PhoneNumberDiscoverability?,
-        hasSVRBackups: Bool
+        capabilities: Capabilities,
     ) {
         self.isManualMessageFetchEnabled = isManualMessageFetchEnabled
         self.registrationId = registrationId
@@ -96,7 +96,7 @@ public struct AccountAttributes: Codable {
         self.registrationRecoveryPassword = registrationRecoveryPassword
         self.encryptedDeviceName = encryptedDeviceName
         self.discoverableByPhoneNumber = discoverableByPhoneNumber.orAccountAttributeDefault.isDiscoverable
-        self.capabilities = Capabilities(hasSVRBackups: hasSVRBackups)
+        self.capabilities = capabilities
     }
 
     public struct Capabilities: Codable {

SignalServiceKit/Network/API/Requests/AccountAttributes/AccountAttributesGenerator.swift

@@ -28,12 +28,18 @@ public struct AccountAttributesGenerator {
     }
 
     func generateForPrimary(
-        aciRegistrationId: UInt32,
-        pniRegistrationId: UInt32,
+        capabilities: AccountAttributes.Capabilities,
         tx: DBReadTransaction
-    ) -> AccountAttributes {
+    ) throws -> AccountAttributes {
         owsAssertDebug(tsAccountManager.registrationState(tx: tx).isPrimaryDevice == true)
 
+        guard
+            let aciRegistrationId = tsAccountManager.getRegistrationId(for: .aci, tx: tx),
+            let pniRegistrationId = tsAccountManager.getRegistrationId(for: .pni, tx: tx)
+        else {
+            throw OWSGenericError("couldn't fetch registration IDs")
+        }
+
         let isManualMessageFetchEnabled = tsAccountManager.isManualMessageFetchEnabled(tx: tx)
 
         guard let profileKey = profileManager.localUserProfile(tx: tx)?.profileKey else {
@@ -42,7 +48,6 @@ public struct AccountAttributesGenerator {
         let udAccessKey = SMKUDAccessKey(profileKey: profileKey).keyData.base64EncodedString()
 
         let allowUnrestrictedUD = udManager.shouldAllowUnrestrictedAccessLocal(transaction: tx)
-        let hasSVRBackups = svrLocalStorage.getIsMasterKeyBackedUp(tx)
 
         let reglockToken: String?
         if
@@ -70,7 +75,7 @@ public struct AccountAttributesGenerator {
             registrationRecoveryPassword: registrationRecoveryPassword,
             encryptedDeviceName: nil,
             discoverableByPhoneNumber: phoneNumberDiscoverability,
-            hasSVRBackups: hasSVRBackups
+            capabilities: capabilities,
         )
     }
 }

SignalServiceKit/Network/API/Requests/AccountAttributes/AccountAttributesUpdaterImpl.swift

@@ -9,6 +9,7 @@ public class AccountAttributesUpdaterImpl: AccountAttributesUpdater {
     private let accountAttributesGenerator: AccountAttributesGenerator
     private let appReadiness: AppReadiness
     private let appVersion: AppVersion
+    private let cronStore: CronStore
     private let dateProvider: DateProvider
     private let db: any DB
     private let kvStore: KeyValueStore
@@ -18,10 +19,18 @@ public class AccountAttributesUpdaterImpl: AccountAttributesUpdater {
     private let syncManager: SyncManagerProtocol
     private let tsAccountManager: TSAccountManager
 
+    private enum Constants {
+        // We must refresh our registration recovery password periodically. We
+        // typically do this when updating to a new version, but we want to refresh
+        // it after 14 days if we haven't upgraded.
+        static let periodicRefreshInterval: TimeInterval = 14 * .day
+    }
+
     public init(
         accountAttributesGenerator: AccountAttributesGenerator,
         appReadiness: AppReadiness,
         appVersion: AppVersion,
+        cron: Cron,
         dateProvider: @escaping DateProvider,
         db: any DB,
         networkManager: NetworkManager,
@@ -33,6 +42,7 @@ public class AccountAttributesUpdaterImpl: AccountAttributesUpdater {
         self.accountAttributesGenerator = accountAttributesGenerator
         self.appReadiness = appReadiness
         self.appVersion = appVersion
+        self.cronStore = CronStore(uniqueKey: .updateAttributes)
         self.dateProvider = dateProvider
         self.db = db
         self.kvStore = KeyValueStore(collection: "AccountAttributesUpdater")
@@ -41,43 +51,66 @@ public class AccountAttributesUpdaterImpl: AccountAttributesUpdater {
         self.svrLocalStorage = svrLocalStorage
         self.syncManager = syncManager
         self.tsAccountManager = tsAccountManager
+        self.registerForCron(cron)
+    }
 
-        appReadiness.runNowOrWhenAppDidBecomeReadyAsync {
-            Task {
-                try await self.updateAccountAttributesIfNecessaryAttempt(authedAccount: .implicit())
-            }
-        }
-
-        NotificationCenter.default.addObserver(
-            self,
-            selector: #selector(reachabilityDidChange),
-            name: .reachabilityChanged,
-            object: nil
+    private func registerForCron(_ cron: Cron) {
+        cron.scheduleFrequently(
+            mustBeRegistered: true,
+            mustBeConnected: true,
+            operation: { () throws -> Bool in
+                let updateConfig = self.db.read { (tx) -> UpdateConfig? in
+                    guard let updateConfig = self.updateConfig(tx: tx) else {
+                        return nil
+                    }
+                    // We update periodically (according to Cron), whenever the capabilities
+                    // change (useful during testing or if capabilities are influenced by
+                    // RemoteConfig, DB migrations, etc.), and whenever requested explicitly.
+                    let shouldUpdate: Bool = (
+                        updateConfig.updateRequestToken != nil
+                        || Date() >= self.cronStore.mostRecentDate(tx: tx).addingTimeInterval(Constants.periodicRefreshInterval)
+                        || updateConfig.capabilities.requestParameters != self.oldCapabilities(tx: tx)
+                    )
+                    return shouldUpdate ? updateConfig : nil
+                }
+                guard let updateConfig else {
+                    return false
+                }
+                try await self.updateAccountAttributes(updateConfig: updateConfig, authedAccount: .implicit())
+                return true
+            },
+            handleResult: { result in
+                switch result {
+                case .failure(is NotRegisteredError), .success(false), .failure(is CancellationError):
+                    break
+                case .success(true):
+                    // Handled by updateAccountAttributes.
+                    break
+                case .failure(let error):
+                    Logger.warn("account attributes hit terminal error; stopping for now: \(error)")
+                    await self.db.awaitableWrite(block: self.updateMostRecentDate(tx:))
+                }
+            },
         )
     }
 
-    deinit {
-        NotificationCenter.default.removeObserver(self)
-    }
-
-    @objc
-    private func reachabilityDidChange() {
-        appReadiness.runNowOrWhenAppDidBecomeReadyAsync {
-            Task {
-                try await self.updateAccountAttributesIfNecessaryAttempt(authedAccount: .implicit())
-            }
-        }
+    private func updateMostRecentDate(tx: DBWriteTransaction) {
+        self.cronStore.setMostRecentDate(Date(), jitter: Constants.periodicRefreshInterval / 20, tx: tx)
     }
 
     public func updateAccountAttributes(authedAccount: AuthedAccount) async throws {
-        await db.awaitableWrite { tx in
+        let updateConfig = await db.awaitableWrite { (tx) -> UpdateConfig? in
             self.kvStore.setData(
                 Randomness.generateRandomBytes(16),
                 key: Keys.latestUpdateRequestToken,
                 transaction: tx,
             )
+            return self.updateConfig(tx: tx)
         }
-        try await self.updateAccountAttributesIfNecessaryAttempt(authedAccount: authedAccount)
+        guard let updateConfig else {
+            return
+        }
+        try await self.updateAccountAttributes(updateConfig: updateConfig, authedAccount: authedAccount)
     }
 
     public func scheduleAccountAttributesUpdate(authedAccount: AuthedAccount, tx: DBWriteTransaction) {
@@ -86,177 +119,95 @@ public class AccountAttributesUpdaterImpl: AccountAttributesUpdater {
             key: Keys.latestUpdateRequestToken,
             transaction: tx,
         )
+        let updateConfig = self.updateConfig(tx: tx)
+        guard let updateConfig else {
+            return
+        }
         tx.addSyncCompletion {
             Task {
-                try await self.updateAccountAttributesIfNecessaryAttempt(authedAccount: authedAccount)
+                try await self.updateAccountAttributes(updateConfig: updateConfig, authedAccount: authedAccount)
             }
         }
     }
 
-    // Performs a single attempt to update the account attributes.
-    //
-    // We need to update our account attributes in a variety of scenarios:
-    //
-    // * Every time the user upgrades to a new version.
-    // * Whenever the device capabilities change.
-    //   This is useful during development and internal testing when
-    //   moving between builds with different device capabilities.
-    // * Whenever another component of the system requests an attribute,
-    //   update e.g. during registration, after rotating the profile key, etc.
-    //
-    // The client will retry failed attempts:
-    //
-    // * On launch.
-    // * When reachability changes.
-    private func updateAccountAttributesIfNecessaryAttempt(authedAccount: AuthedAccount) async throws {
-        guard appReadiness.isAppReady else {
-            Logger.info("Aborting; app is not ready.")
-            return
+    private struct UpdateConfig {
+        var registrationState: TSRegistrationState
+        var updateRequestToken: Data?
+        var capabilities: AccountAttributes.Capabilities
+    }
+
+    private func updateConfig(tx: DBReadTransaction) -> UpdateConfig? {
+        let registrationState = self.tsAccountManager.registrationState(tx: tx)
+        guard registrationState.isRegistered else {
+            return nil
         }
 
-        let currentAppVersion = appVersion.currentAppVersion
+        // has non-nil value if isRegistered is true.
+        let hasBackedUpMasterKey = self.svrLocalStorage.getIsMasterKeyBackedUp(tx)
+        let capabilities = AccountAttributes.Capabilities(hasSVRBackups: hasBackedUpMasterKey)
+        let lastAttributeRequestToken = self.kvStore.getData(Keys.latestUpdateRequestToken, transaction: tx)
 
-        enum ShouldUpdate {
-            case no
-            case yes(
-                currentDeviceCapabilities: AccountAttributes.Capabilities,
-                lastAttributeRequestToken: Data?,
-                registrationState: TSRegistrationState,
-                aciRegistrationId: UInt32,
-                pniRegistrationId: UInt32
-            )
+        return UpdateConfig(
+            registrationState: registrationState,
+            updateRequestToken: lastAttributeRequestToken,
+            capabilities: capabilities,
+        )
+    }
+
+    private func oldCapabilities(tx: DBReadTransaction) -> [String: NSNumber]? {
+        return self.kvStore.getDictionary(
+            Keys.lastUpdateDeviceCapabilities,
+            keyClass: NSString.self,
+            objectClass: NSNumber.self,
+            transaction: tx,
+        ) as [String: NSNumber]?
+    }
+
+    /// Performs a single attempt to update the account attributes.
+    ///
+    /// This method assumes we have a priori knowledge that an update is
+    /// required; callers must check whether or not an update is required.
+    private func updateAccountAttributes(updateConfig: UpdateConfig, authedAccount: AuthedAccount) async throws {
+        let request: TSRequest
+        if updateConfig.registrationState.isPrimaryDevice == true {
+            let attributes = try db.read { tx in
+                return try accountAttributesGenerator
+                    .generateForPrimary(capabilities: updateConfig.capabilities, tx: tx)
+            }
+            request = AccountAttributesRequestFactory(tsAccountManager: tsAccountManager)
+                .updatePrimaryDeviceAttributesRequest(attributes, auth: authedAccount.chatServiceAuth)
+        } else {
+            request = AccountAttributesRequestFactory(tsAccountManager: tsAccountManager)
+                .updateLinkedDeviceCapabilitiesRequest(updateConfig.capabilities, auth: authedAccount.chatServiceAuth)
+        }
+        _ = try await networkManager.asyncRequest(request)
+
+        await db.awaitableWrite { tx in
+            self.updateMostRecentDate(tx: tx)
+            self.kvStore.setObject(updateConfig.capabilities.requestParameters, key: Keys.lastUpdateDeviceCapabilities, transaction: tx)
+            // Clear the update request unless a new update has been requested
+            // while this update was in flight.
+            if
+                let updateRequestToken = updateConfig.updateRequestToken,
+                updateRequestToken == self.kvStore.getData(Keys.latestUpdateRequestToken, transaction: tx)
+            {
+                self.kvStore.removeValue(forKey: Keys.latestUpdateRequestToken, transaction: tx)
+            }
         }
 
-        let shouldUpdate = db.read { tx -> ShouldUpdate in
-            let registrationState = self.tsAccountManager.registrationState(tx: tx)
-            let isRegistered = registrationState.isRegistered
-            let aciRegistrationId = self.tsAccountManager.getRegistrationId(for: .aci, tx: tx)
-            let pniRegistrationId = self.tsAccountManager.getRegistrationId(for: .pni, tx: tx)
+        // Fetch our profile (unclear why), but ignore the result and any errors
+        // because this is a best-effort fetch.
+        _ = try? await profileManager.fetchLocalUsersProfile(authedAccount: authedAccount)
 
-            guard
-                isRegistered,
-                let aciRegistrationId,
-                let pniRegistrationId
-            else {
-                return .no
-            }
-
-            // has non-nil value if isRegistered is true.
-            let hasBackedUpMasterKey = self.svrLocalStorage.getIsMasterKeyBackedUp(tx)
-            let currentDeviceCapabilities = AccountAttributes.Capabilities(hasSVRBackups: hasBackedUpMasterKey)
-
-            // Check if there's been a request for an attributes update.
-            let lastAttributeRequestToken = self.kvStore.getData(Keys.latestUpdateRequestToken, transaction: tx)
-            if lastAttributeRequestToken != nil {
-                return .yes(
-                    currentDeviceCapabilities: currentDeviceCapabilities,
-                    lastAttributeRequestToken: lastAttributeRequestToken,
-                    registrationState: registrationState,
-                    aciRegistrationId: aciRegistrationId,
-                    pniRegistrationId: pniRegistrationId
-                )
-            }
-
-            // Check if device capabilities have changed.
-            let lastUpdateDeviceCapabilities = self.kvStore.getDictionary(
-                Keys.lastUpdateDeviceCapabilities,
-                keyClass: NSString.self,
-                objectClass: NSNumber.self,
-                transaction: tx
-            ) as [String: NSNumber]?
-            if lastUpdateDeviceCapabilities != currentDeviceCapabilities.requestParameters {
-                return .yes(
-                    currentDeviceCapabilities: currentDeviceCapabilities,
-                    lastAttributeRequestToken: lastAttributeRequestToken,
-                    registrationState: registrationState,
-                    aciRegistrationId: aciRegistrationId,
-                    pniRegistrationId: pniRegistrationId
-                )
-            }
-            // Check if the app version has changed.
-            let lastUpdateAppVersion = self.kvStore.getString(Keys.lastUpdateAppVersion, transaction: tx)
-            if lastUpdateAppVersion != currentAppVersion {
-                return .yes(
-                    currentDeviceCapabilities: currentDeviceCapabilities,
-                    lastAttributeRequestToken: lastAttributeRequestToken,
-                    registrationState: registrationState,
-                    aciRegistrationId: aciRegistrationId,
-                    pniRegistrationId: pniRegistrationId
-                )
-            }
-            return .no
-        }
-        switch shouldUpdate {
-        case .no:
-            return
-        case let .yes(
-            currentDeviceCapabilities,
-            lastAttributeRequestToken,
-            registrationState,
-            aciRegistrationId,
-            pniRegistrationId
-        ):
-            Logger.info("Updating account attributes.")
-            let reportedDeviceCapabilities: AccountAttributes.Capabilities
-            if registrationState.isPrimaryDevice == true {
-                let attributes = db.read { tx in
-                    accountAttributesGenerator.generateForPrimary(
-                        aciRegistrationId: aciRegistrationId,
-                        pniRegistrationId: pniRegistrationId,
-                        tx: tx
-                    )
-                }
-
-                let request = AccountAttributesRequestFactory(
-                    tsAccountManager: tsAccountManager
-                ).updatePrimaryDeviceAttributesRequest(
-                    attributes,
-                    auth: authedAccount.chatServiceAuth
-                )
-                _ = try await networkManager.asyncRequest(request)
-
-                reportedDeviceCapabilities = attributes.capabilities
-            } else {
-                let request = AccountAttributesRequestFactory(
-                    tsAccountManager: tsAccountManager
-                ).updateLinkedDeviceCapabilitiesRequest(
-                    currentDeviceCapabilities,
-                    auth: authedAccount.chatServiceAuth
-                )
-                _ = try await networkManager.asyncRequest(request)
-
-                reportedDeviceCapabilities = currentDeviceCapabilities
-            }
-
-            // Kick off an async profile fetch (not awaited)
-            Task {
-                _ = try await profileManager.fetchLocalUsersProfile(authedAccount: authedAccount)
-            }
-
-            await db.awaitableWrite { tx in
-                self.kvStore.setString(currentAppVersion, key: Keys.lastUpdateAppVersion, transaction: tx)
-                self.kvStore.setObject(reportedDeviceCapabilities.requestParameters, key: Keys.lastUpdateDeviceCapabilities, transaction: tx)
-                // Clear the update request unless a new update has been requested
-                // while this update was in flight.
-                if
-                    let lastAttributeRequestToken,
-                    lastAttributeRequestToken == self.kvStore.getData(Keys.latestUpdateRequestToken, transaction: tx)
-                {
-                    self.kvStore.removeValue(forKey: Keys.latestUpdateRequestToken, transaction: tx)
-                }
-            }
-
-            // Primary devices should sync their configuration whenever they
-            // update their account attributes.
-            if registrationState.isRegisteredPrimaryDevice {
-                self.syncManager.sendConfigurationSyncMessage()
-            }
+        // Primary devices should sync their configuration whenever they
+        // update their account attributes.
+        if updateConfig.registrationState.isRegisteredPrimaryDevice {
+            self.syncManager.sendConfigurationSyncMessage()
         }
     }
 
     private enum Keys {
         static let latestUpdateRequestToken = "latestUpdateRequestDate"
         static let lastUpdateDeviceCapabilities = "lastUpdateDeviceCapabilities"
-        static let lastUpdateAppVersion = "lastUpdateAppVersion"
     }
 }