From b37ca40d37c0b50d9caed5d6c777891f53523be8 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Wed, 26 Aug 2026 23:53:29 +0200 Subject: [PATCH] Settle account ties and stop discarding caches on every launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two devices editing an account in the same instant — or migrating one, since every migrated account is stamped at the epoch — each kept its own label and considered the matter closed. Equal timestamps are now broken by the label, on the incoming side and in the conflict resolver alike, the same rule the context already uses. Cached feeds were thrown away on every ordinary launch: recomputing statuses posts a notification, and the observer ran before the value it compares against had been seeded. The seed now happens first. Cache ownership is checked once at launch and compared against a recorded owner afterwards. An unresolved identity at launch means the owner cannot be established at all — a switch to an unseen Apple account while offline looks exactly like that — so the caches are discarded; mid-session it is a transient, and with iCloud Drive off it happens on every activation, so it no longer clears anything. Whether a Keychain item syncs is fixed when it is written, so a login stored while sync was off stayed on this device while its account record travelled, leaving every other device with an account whose credential never arrives. Turning sync on rewrites them. Reseeding a deleted zone marks the account records dirty even while the category is off, so re-enabling it restores them instead of finding an empty outbox. --- Yattee/Core/AccountsManager.swift | 4 +- Yattee/Core/AppEnvironment.swift | 87 +++++++++++-------- Yattee/Models/Account.swift | 12 +++ .../CloudKit/CloudKitConflictResolver.swift | 16 +++- .../CloudKit/CloudKitSyncEngine.swift | 5 +- .../AccountCredentialsManager.swift | 6 ++ .../AccountKeychainCredentials.swift | 29 +++++++ .../InvidiousCredentialsManager.swift | 4 + .../Credentials/PipedCredentialsManager.swift | 4 + .../Views/Settings/iCloudSettingsView.swift | 13 +++ YatteeTests/AccountSyncTests.swift | 21 +++++ 11 files changed, 158 insertions(+), 43 deletions(-) diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index f699c92d..10ed8555 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -344,8 +344,8 @@ final class AccountsManager { if let index = accounts.firstIndex(where: { $0.id == incoming.id }) { let existing = accounts[index] - guard incoming.updatedAt > existing.updatedAt else { - guard existing.updatedAt > incoming.updatedAt else { return false } + guard existing.isOutranked(by: incoming) else { + guard incoming.isOutranked(by: existing) else { return false } // The server holds the older copy. Marked dirty rather than // merely re-registered with the engine, so the correction // survives a crash before it is sent. diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index b753b19e..61845d7f 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -389,28 +389,6 @@ final class AppEnvironment { // is global rather than account-keyed, so it would otherwise keep // serving the previous account's videos. // - // Accounts and instances arriving are triggers too, not redundant ones: - // a context naming an account that has not synced yet — or one whose - // instance has not — cannot be mapped onto the legacy shape, and these - // are what restore the mapping once it can be made rather than leaving - // that moment permanent. The same arrivals are what make a synced - // account's credential findable, so statuses are recomputed with them: - // an account that appears mid-session would otherwise sit at "sign in - // required" until the app was relaunched. - for name: Notification.Name in [.dataContextDidChange, .accountsDidChange, .instancesDidChange] { - contextMirrorObservers.append( - NotificationCenter.default.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in - MainActor.assumeIsolated { - guard let self else { return } - if name != .dataContextDidChange { - self.refreshAccountStatuses() - } - self.mirrorContextToLegacySelection() - } - } - ) - } - // A deletion that arrives from another device takes the credential with // it. Only a deletion does this — an account merely missing from a // fetch keeps everything, because a fetch is not an enumeration. @@ -450,19 +428,16 @@ final class AppEnvironment { guard let self else { return } self.refreshAccountStatuses() - // Cleared unconditionally, because the identity moving means - // everything cached belongs to the Apple account that just went - // away — even when the new namespace happens to select the same - // context and the change comparison would find nothing. - self.clearAccountScopedCaches() - if let owner = self.credentialIdentityScope.current { - UserDefaults.standard.set(owner, forKey: Self.cacheOwnerKey) - } + // Compared against the recorded owner rather than cleared + // outright: a scope that merely dropped to unresolved and came + // back is the same Apple account, and with iCloud Drive off + // that happens on every activation. + self.discardCachesFromAnotherIdentityIfNeeded() self.mirrorContextToLegacySelection(resetWhenUnresolvable: true) } } - discardCachesFromAnotherIdentityIfNeeded() + discardCachesFromAnotherIdentityIfNeeded(atLaunch: true) // Turn any pre-revamp single-instance logins into accounts. Repeatable: // it runs again whenever instances arrive from iCloud. @@ -474,6 +449,28 @@ final class AppEnvironment { // not look like a change and throw away caches that are still right. lastCachedContext = dataContextValidator.effectiveContext + // Accounts and instances arriving are triggers too, not redundant ones: + // a context naming an account that has not synced yet — or one whose + // instance has not — cannot be mapped onto the legacy shape, and these + // are what restore the mapping once it can be made rather than leaving + // that moment permanent. The same arrivals are what make a synced + // account's credential findable, so statuses are recomputed with them: + // an account that appears mid-session would otherwise sit at "sign in + // required" until the app was relaunched. + for name: Notification.Name in [.dataContextDidChange, .accountsDidChange, .instancesDidChange] { + contextMirrorObservers.append( + NotificationCenter.default.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in + MainActor.assumeIsolated { + guard let self else { return } + if name != .dataContextDidChange { + self.refreshAccountStatuses() + } + self.mirrorContextToLegacySelection() + } + } + ) + } + // Resolve the credential identity scope if this Apple account has never // been seen on this device. Until it lands, account credentials report // "can't tell yet" rather than "signed out". @@ -663,12 +660,28 @@ final class AppEnvironment { /// being constructed, which is before any observer here exists. So an Apple /// account switched while the app was closed would otherwise reach the new /// user with the previous one's feeds still on disk. - private func discardCachesFromAnotherIdentityIfNeeded() { - // Unresolved: no conclusion either way, and the observer covers it once - // the scope lands. - guard let owner = credentialIdentityScope.current else { return } - + /// - Parameter atLaunch: whether this is the startup check. It decides what + /// an *unresolved* identity means. At launch it means the cached data's + /// owner cannot be confirmed at all — which is exactly the state a switch + /// to an unseen Apple account while offline produces — so the safe answer + /// is to discard. Mid-session it is a transient: with iCloud Drive off the + /// scope drops to unresolved and re-resolves on every activation, and + /// clearing on that would throw the caches away several times a day. + private func discardCachesFromAnotherIdentityIfNeeded(atLaunch: Bool = false) { let defaults = UserDefaults.standard + + guard let owner = credentialIdentityScope.current else { + guard atLaunch, defaults.string(forKey: Self.cacheOwnerKey) != nil else { return } + + clearAccountScopedCaches() + defaults.removeObject(forKey: Self.cacheOwnerKey) + LoggingService.shared.info( + "Cannot confirm who the cached account data belongs to — discarded it", + category: .general + ) + return + } + let previous = defaults.string(forKey: Self.cacheOwnerKey) defaults.set(owner, forKey: Self.cacheOwnerKey) @@ -676,7 +689,7 @@ final class AppEnvironment { clearAccountScopedCaches() LoggingService.shared.info( - "Apple account changed while the app was closed — discarded cached account data", + "Apple account changed — discarded the previous account's cached data", category: .general ) } diff --git a/Yattee/Models/Account.swift b/Yattee/Models/Account.swift index 98264126..e709fb85 100644 --- a/Yattee/Models/Account.swift +++ b/Yattee/Models/Account.swift @@ -71,4 +71,16 @@ struct Account: Identifiable, Codable, Hashable, Sendable { /// Whether this account was produced by the legacy migration rather than an /// explicit sign-in. var isMigrated: Bool { id == instanceID } + + /// Whether `other` should replace this copy. + /// + /// Ordered by the app-managed `updatedAt`, with the label settling an exact + /// tie. The tie is reachable — two devices can migrate the same account in + /// the same instant, and every migrated account is stamped at the epoch — and + /// without a tie-break each device would keep its own label and consider the + /// matter closed, which is a divergence neither would ever notice. + func isOutranked(by other: Account) -> Bool { + if other.updatedAt != updatedAt { return other.updatedAt > updatedAt } + return (other.displayName ?? "") > (displayName ?? "") + } } diff --git a/Yattee/Services/CloudKit/CloudKitConflictResolver.swift b/Yattee/Services/CloudKit/CloudKitConflictResolver.swift index 540f4079..d0a5862e 100644 --- a/Yattee/Services/CloudKit/CloudKitConflictResolver.swift +++ b/Yattee/Services/CloudKit/CloudKitConflictResolver.swift @@ -342,8 +342,11 @@ actor CloudKitConflictResolver { /// modification date: an edit made offline has to stay orderable against /// a concurrent edit elsewhere, and a migrated account carries an /// epoch-old date precisely so any real edit outranks it. - /// - `instanceID` and `dateAdded` are immutable identity, taken from - /// whichever side wins only so a malformed record cannot strip them. + /// - An exact tie is settled by the label, so two devices that edited in the + /// same instant — or migrated the same account, all of which are stamped + /// at the epoch — converge instead of each keeping its own. This has to + /// match `Account.isOutranked`, or a record could win the upload and still + /// be refused on arrival. func resolveAccountConflict( local: CKRecord, server: CKRecord @@ -351,10 +354,17 @@ actor CloudKitConflictResolver { let localUpdated = (local["updatedAt"] as? Date) ?? Date.distantPast let serverUpdated = (server["updatedAt"] as? Date) ?? Date.distantPast + let localWins: Bool + if localUpdated != serverUpdated { + localWins = localUpdated > serverUpdated + } else { + localWins = ((local["displayName"] as? String) ?? "") > ((server["displayName"] as? String) ?? "") + } + // IMPORTANT: Always start with server record to preserve recordChangeTag let resolved = server - if localUpdated > serverUpdated { + if localWins { resolved["displayName"] = local["displayName"] resolved["updatedAt"] = local["updatedAt"] } diff --git a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift index 7fba874d..7d7210d5 100644 --- a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift +++ b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift @@ -1565,8 +1565,11 @@ final class CloudKitSyncEngine: @unchecked Sendable { /// recreating the zone after it was deleted elsewhere. The account outbox /// normally tracks only what changed, which is not enough when the server /// has nothing at all. + /// Deliberately not gated on the category being enabled: the zone can be + /// deleted while account sync is off, and marking the records dirty is what + /// lets re-enabling restore them. The outbox is durable and nothing is sent + /// while the category is off, so priming it costs nothing. func uploadAllAccounts() { - guard canSyncAccounts else { return } uploadProgress?.currentCategory = "Accounts" accountSync?.uploadEverything() } diff --git a/Yattee/Services/Credentials/AccountCredentialsManager.swift b/Yattee/Services/Credentials/AccountCredentialsManager.swift index d88d4c94..e7043589 100644 --- a/Yattee/Services/Credentials/AccountCredentialsManager.swift +++ b/Yattee/Services/Credentials/AccountCredentialsManager.swift @@ -41,6 +41,12 @@ protocol AccountCredentialsManager: AnyObject, Observable { /// key needs only the account's ID. func deleteCredential(forAccountID accountID: UUID) + /// Rewrites stored credentials so they carry the current iCloud sync + /// setting. Whether a Keychain item syncs is decided when it is written, so + /// enabling sync later would otherwise leave existing logins local — and + /// every other device with an account whose credential never arrives. + func resyncCredentialsWithCurrentSetting() + /// The login identifier the user typed. Kept in the Keychain beside the /// credential so it never enters synced metadata. func setUsername(_ username: String, for account: Account) diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 5f0958f4..8f26ee01 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -111,6 +111,35 @@ final class AccountKeychainCredentials { HomeInstanceCache.shared.clear(instanceID: account.instanceID, contentType: .feed) } + /// Rewrites the stored credentials so they carry the current sync setting. + /// + /// `kSecAttrSynchronizable` is fixed when an item is written, so a login + /// stored while iCloud sync was off stays local forever. Enabling sync + /// afterwards uploads the account metadata, and every other device would + /// then show an account whose credential never arrives. Called when the + /// user turns sync on. + func resyncCredentialsWithCurrentSetting() { + guard let scope = identityScope.current else { return } + + for accountID in loginsByScope[scope] ?? [] { + let credentialKey = KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) + let usernameKey = KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID) + let syncEnabled = shouldSyncToiCloud + + if let credential = store.value(forKey: credentialKey) { + _ = store.set(credential, forKey: credentialKey, synchronizable: syncEnabled) + } + if let username = store.value(forKey: usernameKey) { + _ = store.set(username, forKey: usernameKey, synchronizable: syncEnabled) + } + } + + LoggingService.shared.info( + "Rewrote stored \(credentialNoun)s with the current iCloud sync setting", + category: .keychain + ) + } + func deleteCredential(forAccountID accountID: UUID) { guard let scope = currentScope() else { return } diff --git a/Yattee/Services/Credentials/InvidiousCredentialsManager.swift b/Yattee/Services/Credentials/InvidiousCredentialsManager.swift index 808eecda..da60d1dd 100644 --- a/Yattee/Services/Credentials/InvidiousCredentialsManager.swift +++ b/Yattee/Services/Credentials/InvidiousCredentialsManager.swift @@ -289,6 +289,10 @@ extension InvidiousCredentialsManager: AccountCredentialsManager { accountCredentials.deleteCredential(forAccountID: accountID) } + func resyncCredentialsWithCurrentSetting() { + accountCredentials.resyncCredentialsWithCurrentSetting() + } + func setUsername(_ username: String, for account: Account) { accountCredentials.setUsername(username, for: account) } diff --git a/Yattee/Services/Credentials/PipedCredentialsManager.swift b/Yattee/Services/Credentials/PipedCredentialsManager.swift index 9c46a9b4..bc03d086 100644 --- a/Yattee/Services/Credentials/PipedCredentialsManager.swift +++ b/Yattee/Services/Credentials/PipedCredentialsManager.swift @@ -218,6 +218,10 @@ extension PipedCredentialsManager: AccountCredentialsManager { accountCredentials.deleteCredential(forAccountID: accountID) } + func resyncCredentialsWithCurrentSetting() { + accountCredentials.resyncCredentialsWithCurrentSetting() + } + func setUsername(_ username: String, for account: Account) { accountCredentials.setUsername(username, for: account) } diff --git a/Yattee/Views/Settings/iCloudSettingsView.swift b/Yattee/Views/Settings/iCloudSettingsView.swift index 4bcfc34c..6f812964 100644 --- a/Yattee/Views/Settings/iCloudSettingsView.swift +++ b/Yattee/Views/Settings/iCloudSettingsView.swift @@ -528,6 +528,18 @@ struct iCloudSettingsView: View { settingsManager?.replaceWithiCloudData() instancesManager?.replaceWithiCloudData() mediaSourcesManager?.replaceWithiCloudData() + + resyncStoredLogins() + } + + /// Rewrites stored logins so they carry the sync setting that has just + /// changed. Whether a Keychain item syncs is fixed when it is written, so a + /// login stored while sync was off would stay on this device while its + /// account record travelled — every other device showing an account whose + /// credential never arrives. + private func resyncStoredLogins() { + appEnvironment?.invidiousCredentialsManager.resyncCredentialsWithCurrentSetting() + appEnvironment?.pipedCredentialsManager.resyncCredentialsWithCurrentSetting() } private func disableiCloudSync() { @@ -547,6 +559,7 @@ struct iCloudSettingsView: View { Task { await appEnvironment?.accountSyncCoordinator.activate() } + resyncStoredLogins() case .subscriptions: settingsManager?.syncSubscriptions = true // Upload existing subscriptions to CloudKit diff --git a/YatteeTests/AccountSyncTests.swift b/YatteeTests/AccountSyncTests.swift index b49ca4dc..e8731201 100644 --- a/YatteeTests/AccountSyncTests.swift +++ b/YatteeTests/AccountSyncTests.swift @@ -267,6 +267,27 @@ struct AccountSyncTests { #expect(try mapper.toStoredDataContext(from: serverWins).context == .account(accountID)) } + @Test("Two accounts edited in the same instant converge") + func equalAccountTimestampsBreakTheTieTheSameWayEverywhere() async throws { + let mapper = AccountSyncTests.makeMapper() + let resolver = CloudKitConflictResolver() + let id = UUID() + let instanceID = UUID() + let sameMoment = Date(timeIntervalSince1970: 500) + + let lower = Account(id: id, instanceID: instanceID, displayName: "A", updatedAt: sameMoment) + let higher = Account(id: id, instanceID: instanceID, displayName: "B", updatedAt: sameMoment) + + #expect(lower.isOutranked(by: higher)) + #expect(!higher.isOutranked(by: lower)) + + let resolved = await resolver.resolveAccountConflict( + local: mapper.toCKRecord(account: higher), + server: mapper.toCKRecord(account: lower) + ) + #expect(try mapper.toAccount(from: resolved).displayName == "B") + } + @Test("Two migrated contexts converge instead of diverging") func equalTimestampsBreakTheTieTheSameWayEverywhere() async throws { let mapper = AccountSyncTests.makeMapper()