From 923e13565fb4abbceced8101458d733d0bd6c213 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Wed, 26 Aug 2026 08:06:52 +0200 Subject: [PATCH] Stop a fresh login from synthesizing a phantom migrated account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signing in wrote `usesAccountLogin`, which syncs through key-value storage independently of the account records. A device receiving that flag before the account synthesized a migrated account for a login that was never legacy — and the marker arriving later deliberately retracts nothing, so the credential-less duplicate was permanent and uploaded. The flag is pre-revamp state that migration only reads; accounts describe logins now. `needsCredentials` reads the account list for any instance that has one, so the badge it drives is unaffected. A tombstone now dominates contexts as well as accounts: an offline device can select an account another device deleted and publish that choice with a newer timestamp, which would have left every device pointing at an account that is never coming back. Cached private feeds survived an Apple-account switch two ways. A scope this device has seen before resolves while `CredentialIdentityScope` is still being constructed, so the change notification fires before any observer exists — startup now compares the persisted cache owner. And a purge asked for before the disk cache is read was undone by the load, which now carries it. The need to re-bootstrap after dropping records for want of a resolved identity is persisted. The engine may already have moved its token past those records, so a launch that lost the in-memory flag would neither replay nor bootstrap them. --- Yattee/Core/AccountsManager.swift | 18 ++++++ Yattee/Core/AppEnvironment.swift | 47 +++++++++++++++ .../CloudKit/AccountSyncCoordinator.swift | 59 ++++++++++--------- .../Services/CloudKit/AccountSyncStore.swift | 15 +++++ Yattee/Services/HomeInstanceCache.swift | 22 +++++++ Yattee/Views/Settings/EditSourceView.swift | 9 ++- YatteeTests/AccountSyncTests.swift | 49 ++++++++++++--- 7 files changed, 183 insertions(+), 36 deletions(-) diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index df973fd6..36dd21c1 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -448,6 +448,24 @@ final class AccountsManager { /// pushed back over the stale server copy. @discardableResult func applySyncedContext(_ incoming: StoredDataContext) -> Bool { + var incoming = incoming + + // A tombstone dominates the account everywhere, contexts included. An + // offline device can select an account another device has deleted and + // publish that choice with a newer timestamp; adopting it would leave + // every device pointing at an account that is never coming back. The + // choice's own timestamp is kept, so a later real selection still wins, + // and every device that holds the tombstone repairs it identically. + if let accountID = incoming.context.accountID, deletedAccountIDs.contains(accountID) { + incoming = StoredDataContext(context: .local, changedAt: incoming.changedAt) + + LoggingService.shared.info( + "Ignoring a synced context that names a deleted account", + category: .general, + details: "accountID=\(accountID)" + ) + } + // Before the user has ever chosen here there is nothing to order // against: the local value is a default stamped when this manager was // constructed, so a synced choice made yesterday would lose to it and a diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index a05ab153..b842fb52 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -460,10 +460,15 @@ final class AppEnvironment { // 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) + } self.mirrorContextToLegacySelection(resetWhenUnresolvable: true) } } + discardCachesFromAnotherIdentityIfNeeded() + // Turn any pre-revamp single-instance logins into accounts. Repeatable: // it runs again whenever instances arrive from iCloud. accounts.reconcileLegacyAccounts() @@ -650,6 +655,33 @@ final class AppEnvironment { HomeInstanceCache.shared.clearAllFeeds() } + private static let cacheOwnerKey = "accountScopedCacheOwner" + + /// Discards cached private data left behind by a different Apple account. + /// + /// The identity-change notification cannot cover this: a scope this device + /// has seen before resolves synchronously while `CredentialIdentityScope` is + /// 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 } + + let defaults = UserDefaults.standard + let previous = defaults.string(forKey: Self.cacheOwnerKey) + defaults.set(owner, forKey: Self.cacheOwnerKey) + + guard let previous, previous != owner else { return } + + clearAccountScopedCaches() + LoggingService.shared.info( + "Apple account changed while the app was closed — discarded cached account data", + category: .general + ) + } + /// Deletes an instance together with the accounts that live on it. /// /// The cascade has to happen here, while the instance is still resolvable: @@ -812,6 +844,21 @@ final class AppEnvironment { return true } + // Accounts describe the logins on an instance once there are any, so the + // pre-revamp flag is consulted only for instances that have none — + // fresh logins deliberately no longer set it. + let accounts = accountsManager.accounts(forInstance: instance.id) + if !accounts.isEmpty { + // An unresolved identity scope means "cannot tell yet". Reporting a + // missing credential on it would badge every account on every + // launch until the scope lands. + guard credentialIdentityScope.isResolved else { return false } + + return !accounts.contains { account in + accountCredentialsManager(for: account)?.isLoggedIn(for: account) ?? false + } + } + if instance.usesAccountLogin, let manager = credentialsManager(for: instance), !manager.isLoggedIn(for: instance) { diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index b8ada013..46835f01 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -78,10 +78,6 @@ final class AccountSyncCoordinator { /// no identity was resolved. private var lastResolvedNamespace: String? - /// Set when account records had to be dropped because no identity was - /// resolved yet. The engine's token has already moved past them, so the - /// identity that resolves next has to bootstrap again to recover them. - private var droppedRecordsWhileUnresolved = false private var isActivating = false private var activationRequested = false @@ -182,29 +178,7 @@ final class AccountSyncCoordinator { func identityDidChange() { identityGeneration += 1 - guard let current = namespace else { return } - let previouslyResolved = lastResolvedNamespace - lastResolvedNamespace = current - - // Records dropped for want of a resolved identity belonged to whichever - // Apple account was signed in at the time, and that cannot be - // reconstructed afterwards: a first launch and a sign-out/sign-in look - // identical from here. Rather than guess, every identity that could have - // owned them bootstraps again and recovers its own from its own zone. A - // redundant full fetch is the whole cost of that; guessing wrongly would - // show one Apple account's logins under another's. - if droppedRecordsWhileUnresolved { - droppedRecordsWhileUnresolved = false - - for candidate in Set([current, previouslyResolved].compactMap { $0 }) { - store.clearBootstrapCompletion(in: candidate) - } - - LoggingService.shared.logCloudKit( - "Account records arrived before the identity resolved — bootstrapping again to recover them" - ) - } - + guard namespace != nil else { return } Task { await activate() } } @@ -239,7 +213,7 @@ final class AccountSyncCoordinator { // Staging under a guessed identity is the one thing worse than // losing the records: they would be applied to, and uploaded from, // an account they never belonged to. - droppedRecordsWhileUnresolved = true + store.hasPendingIdentityRecovery = true LoggingService.shared.logCloudKit( "Dropping \(accountRecords.count + accountDeletions.count) account records fetched before the identity resolved" ) @@ -306,6 +280,9 @@ final class AccountSyncCoordinator { return } + recoverDroppedRecordsIfNeeded(in: namespace) + lastResolvedNamespace = namespace + await runBootstrapIfNeeded(in: namespace) guard store.isBootstrapCompleted(in: namespace) else { return } @@ -315,6 +292,32 @@ final class AccountSyncCoordinator { enqueueDirtyRecords() } + /// Makes every identity that could have owned dropped records fetch its zone + /// again. + /// + /// Which identity they belonged to cannot be reconstructed — a first launch + /// and a sign-out followed by a different sign-in look identical from here — + /// so instead of guessing, each candidate recovers its own records from its + /// own zone. A redundant full fetch is the whole cost; guessing wrongly + /// would show one Apple account's logins under another's. + /// + /// Only the identity signed in now and the last one seen in this process are + /// candidates. An identity that owned dropped records, is not signed in now, + /// and was last seen in an earlier process keeps its completed flag; nothing + /// here knows it exists. + private func recoverDroppedRecordsIfNeeded(in namespace: String) { + guard store.hasPendingIdentityRecovery else { return } + store.hasPendingIdentityRecovery = false + + for candidate in Set([namespace, lastResolvedNamespace].compactMap { $0 }) { + store.clearBootstrapCompletion(in: candidate) + } + + LoggingService.shared.logCloudKit( + "Account records arrived before the identity resolved — bootstrapping again to recover them" + ) + } + private func applyStagedChanges(in namespace: String) { let staged = store.stagedChanges(in: namespace) guard !staged.isEmpty else { return } diff --git a/Yattee/Services/CloudKit/AccountSyncStore.swift b/Yattee/Services/CloudKit/AccountSyncStore.swift index 475b1f67..8e2ee8ed 100644 --- a/Yattee/Services/CloudKit/AccountSyncStore.swift +++ b/Yattee/Services/CloudKit/AccountSyncStore.swift @@ -154,6 +154,21 @@ final class AccountSyncStore { defaults.removeObject(forKey: key("schemaVersion", in: namespace)) } + // MARK: - Identity recovery + + /// Whether account records were dropped because no identity was resolved to + /// stage them under. + /// + /// Deliberately not namespaced — the identity they belonged to is the one + /// thing that could not be established — and persisted, because the engine + /// may already have moved its change token past those records: a crash + /// before the identity resolved would otherwise leave them unreachable by + /// any later fetch. + var hasPendingIdentityRecovery: Bool { + get { defaults.bool(forKey: "accountSync.pendingIdentityRecovery") } + set { defaults.set(newValue, forKey: "accountSync.pendingIdentityRecovery") } + } + /// The account-category schema this build understands. Raising it makes /// every identity bootstrap once more, which is how a future release picks /// up record types this one ignores. diff --git a/Yattee/Services/HomeInstanceCache.swift b/Yattee/Services/HomeInstanceCache.swift index d46ecaf9..3e62f1dd 100644 --- a/Yattee/Services/HomeInstanceCache.swift +++ b/Yattee/Services/HomeInstanceCache.swift @@ -24,6 +24,10 @@ final class HomeInstanceCache { /// Whether the disk cache has been loaded. private var diskCacheLoaded = false + /// Set when feeds had to be dropped before the disk cache was read, so the + /// entries that load carry the same purge. + private var pendingFeedPurge = false + private init() {} // MARK: - Cache Key Management @@ -71,6 +75,11 @@ final class HomeInstanceCache { } else { LoggingService.shared.debug("No home instance cache found on disk", category: .general) } + + if pendingFeedPurge { + pendingFeedPurge = false + clearAllFeeds() + } } /// Refreshes content from network for a specific instance and content type. @@ -184,6 +193,19 @@ final class HomeInstanceCache { /// otherwise be served each other's. Popular and trending are public and /// stay. func clearAllFeeds() { + // A purge can be asked for before the disk cache has been read — the + // identity check runs at startup, the load is async. Dropping only what + // is in memory would then be undone by the load, so the requirement is + // remembered and applied to whatever it brings in. + guard diskCacheLoaded else { + pendingFeedPurge = true + LoggingService.shared.debug( + "HomeInstanceCache.clearAllFeeds: Deferred until the disk cache is loaded", + category: .general + ) + return + } + let keysToRemove = cache.keys.filter { $0.hasSuffix("_\(InstanceContentType.feed.rawValue)") } guard !keysToRemove.isEmpty else { return } diff --git a/Yattee/Views/Settings/EditSourceView.swift b/Yattee/Views/Settings/EditSourceView.swift index 8db4fc65..3a54a069 100644 --- a/Yattee/Views/Settings/EditSourceView.swift +++ b/Yattee/Views/Settings/EditSourceView.swift @@ -427,7 +427,14 @@ private struct EditRemoteServerContent: View { } appEnvironment.accountsManager.setStatus(.signedIn, for: account.id) - appEnvironment.instancesManager.setUsesAccountLogin(true, for: instance) + + // `usesAccountLogin` is deliberately not set. It is pre-revamp state + // that migration only reads, and it syncs through iCloud key-value + // storage independently of the account records — a device receiving the + // flag before the account would synthesize a migrated account for a + // login that was never legacy, and the marker that arrives later does + // not retract it. Accounts describe logins now; the flag only describes + // the world before they existed. // Signing in again keeps the same account, so nothing downstream sees a // change of context — but the credential behind it may now belong to diff --git a/YatteeTests/AccountSyncTests.swift b/YatteeTests/AccountSyncTests.swift index c93702a8..a9280ad0 100644 --- a/YatteeTests/AccountSyncTests.swift +++ b/YatteeTests/AccountSyncTests.swift @@ -612,8 +612,32 @@ struct AccountSyncTests { #expect(identityA.accounts.account(id: account.id) != nil) } + @Test("A context naming a deleted account is never adopted") + func tombstoneDominatesASyncedContext() async { + let harness = Self.makeHarness() + let account = harness.accounts.addAccount(instanceID: UUID()) + harness.accounts.removeAccount(account.id) + + // An offline device can select an account another device has already + // deleted, and publish that choice with a newer timestamp. + let stale = StoredDataContext( + context: .account(account.id), + changedAt: Date().addingTimeInterval(3_600) + ) + harness.coordinator.stageFetched( + records: [harness.mapper.toCKRecord(dataContext: stale)], + deletions: [] + ) + await harness.coordinator.activate() + + #expect(harness.accounts.currentContext == .local) + // The choice's own timestamp is kept, so a later real selection still + // outranks this repair on every device. + #expect(harness.accounts.storedContext.changedAt == stale.changedAt) + } + @Test("Records fetched before the identity resolves are recovered by a bootstrap") - func unresolvedIdentityForcesAnotherBootstrap() { + func unresolvedIdentityForcesAnotherBootstrap() async { let defaults = Self.makeDefaults() let store = AccountSyncStore(defaults: defaults) store.markBootstrapCompleted(in: "identity-A") @@ -640,14 +664,27 @@ struct AccountSyncTests { #expect(store.stagedChanges(in: "unresolved").isEmpty) // The engine's token has moved past those records, so the identity that - // resolves has to fetch the zone in full again to see them. + // resolves has to fetch the zone in full again to see them. The + // requirement is persisted, not held in memory: the token may already + // have advanced, and a launch that lost it would neither replay nor + // bootstrap. + #expect(store.hasPendingIdentityRecovery) + identity.value = "identity-A" - coordinator.identityDidChange() + let relaunched = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { identity.value }, + store: AccountSyncStore(defaults: defaults), + isCategoryEnabled: { true } + ) + await relaunched.activate() + #expect(!store.isBootstrapCompleted(in: "identity-A")) + #expect(!store.hasPendingIdentityRecovery) } @Test("A switch of Apple account never claims records dropped while unresolved") - func switchedIdentityDoesNotClaimDroppedRecords() { + func switchedIdentityDoesNotClaimDroppedRecords() async { let defaults = Self.makeDefaults() let store = AccountSyncStore(defaults: defaults) store.markBootstrapCompleted(in: "identity-A") @@ -672,10 +709,8 @@ struct AccountSyncTests { records: [AccountSyncTests.makeMapper().toCKRecord(account: Account(instanceID: UUID()))], deletions: [] ) - coordinator.identityDidChange() - identity.value = "identity-B" - coordinator.identityDidChange() + await coordinator.activate() // Neither identity is handed the other's records; both recover their // own from their own zone instead.