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()