diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 972a8d86..7960bd1d 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -684,6 +684,13 @@ final class AccountsManager { /// migrated account would simply recreate it, because `usesAccountLogin` /// remains true on the instance forever. func reconcileLegacyAccounts() { + // Nothing may be synthesized into the unresolved namespace: the app + // cannot yet tell whose accounts these are, and the marker it would + // write there is permanent while the namespace itself is discarded. + // Nothing is lost by waiting — resolution runs + // `reloadForCurrentIdentity()`, which reconciles under the namespace + // that will actually be read. + guard identityScope.isResolved else { return } guard let instancesManager else { return } var didChange = false @@ -742,6 +749,11 @@ final class AccountsManager { /// previous release wrote the key to iCloud but never imported it back — on /// a fresh install the only copy lives in KVS. func migrateLegacyContextIfNeeded() { + // A one-way migration is spent the moment it runs, so running it + // against the unresolved namespace would consume it against storage + // that is thrown away — and mark a context chosen that the identity + // owning the choice never sees. + guard identityScope.isResolved else { return } guard !hasStoredContext else { return } guard let legacy = legacySubscriptionAccount() else { return } @@ -792,16 +804,41 @@ final class AccountsManager { identityScope.current ?? "unresolved" } + /// Whether the identity everything here is keyed by is known. + /// + /// Exposed because the corrections other services make — rewriting the + /// legacy selection, resetting a context whose account is "missing" — are + /// only meaningful once this is true. While it is false the app is looking + /// at a namespace it will discard, and acting on what it finds there + /// corrects real state from state that was never authoritative. + var isIdentityResolved: Bool { identityScope.isResolved } + + /// The namespace the in-memory state was loaded from. + /// + /// `namespace` moves the instant the identity scope does, but the reload + /// that refills memory runs on a main-queue observer afterwards. In that + /// window everything held here still belongs to the previous identity, so + /// writes are fenced on this rather than on `namespace` — otherwise one + /// Apple account's accounts would be persisted, and uploaded, under + /// another's. Kept out of observation: it is a storage fence, not state + /// anything renders. + @ObservationIgnored private(set) var loadedNamespace = "" + private func key(_ name: String) -> String { "accounts.\(namespace).\(name)" } private var hasStoredContext: Bool { get { defaults.bool(forKey: key("hasContext")) } - set { defaults.set(newValue, forKey: key("hasContext")) } + set { + guard loadedNamespace == namespace else { return } + defaults.set(newValue, forKey: key("hasContext")) + } } private func load() { + loadedNamespace = namespace + if let data = defaults.data(forKey: key("accounts")), let decoded = try? JSONDecoder().decode([Account].self, from: data) { accounts = decoded @@ -837,9 +874,53 @@ final class AccountsManager { } pendingContextUpload = defaults.bool(forKey: key("pendingContextUpload")) + + purgeUnresolvedNamespace() + } + + /// Every key `save()` writes, for purging a namespace wholesale. + private static let storedKeyNames = [ + "accounts", "context", "hasContext", "migrated", "pendingUploads", + "pendingDeletions", "pendingMarkerUploads", "deletedAccounts", + "deletedInstances", "pendingContextUpload" + ] + + /// Deletes whatever was written while the identity was still unresolved. + /// + /// That namespace is a holding pen no resolved identity ever reads back, + /// and what accumulates there is actively wrong: a snapshot from before a + /// tombstone was applied, outboxes owing uploads to a zone nothing will + /// ever send them to. With iCloud Drive switched off the scope dips to + /// unresolved on every foreground activation, so it is written in ordinary + /// use rather than only in exotic states. + /// + /// Run from `load()`, which covers both a launch that resolves immediately + /// (where no reload ever runs) and one that resolves later. Repeatable, so + /// there is nothing to remember about having done it. + private func purgeUnresolvedNamespace() { + guard identityScope.isResolved else { return } + + for name in Self.storedKeyNames { + defaults.removeObject(forKey: "accounts.unresolved.\(name)") + } } private func save() { + // The namespace moved and the reload has not run yet, so this is the + // previous identity's state being written under the new identity's + // keys. Dropped rather than deferred: the reload that follows loads + // whatever storage already holds for the new identity, and everything + // dropped here is either that identity's — and still on disk — or the + // previous one's, which its own namespace still holds. + guard loadedNamespace == namespace else { + LoggingService.shared.warning( + "Dropping an account save made while the identity namespace was reloading", + category: .general, + details: "loaded=\(loadedNamespace), current=\(namespace)" + ) + return + } + if let data = try? JSONEncoder().encode(accounts) { defaults.set(data, forKey: key("accounts")) } @@ -879,6 +960,12 @@ final class AccountsManager { load() reconcileLegacyAccounts() + // Both migrations are held back while the scope is unresolved, and this + // is where a launch that resolved late gets them. Reconciliation only + // runs the context migration when it changed something, so a namespace + // whose accounts were already migrated would otherwise keep its legacy + // selection until the next relaunch. Idempotent via `hasStoredContext`. + migrateLegacyContextIfNeeded() NotificationCenter.default.post(name: .accountsDidChange, object: nil) // The context belongs to the namespace too, so this reload replaced it. diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 8855e1fa..6b27cb5a 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -94,6 +94,18 @@ final class AppEnvironment { } } + /// Drops the fingerprints recorded while the identity was unresolved. + /// + /// Nothing legitimate is written under that key — `refreshAccountStatuses` + /// refuses to read the Keychain until the scope resolves — so anything left + /// there is a digest of a credential read under a scope the app could not + /// vouch for, and comparing a real credential against it would report a + /// replacement that never happened. + private func purgeUnresolvedCredentialFingerprints() { + guard credentialIdentityScope.isResolved else { return } + UserDefaults.standard.removeObject(forKey: "accountCredentialFingerprints.unresolved") + } + private func saveCredentialFingerprints() { let stored = credentialFingerprints.reduce(into: [String: String]()) { result, entry in result[entry.key.uuidString] = entry.value @@ -476,6 +488,7 @@ final class AppEnvironment { // Fingerprints are recorded per identity, like everything else // keyed to one. self.loadCredentialFingerprints() + self.purgeUnresolvedCredentialFingerprints() self.refreshAccountStatuses() // Compared against the recorded owner rather than cleared @@ -488,6 +501,7 @@ final class AppEnvironment { } loadCredentialFingerprints() + purgeUnresolvedCredentialFingerprints() discardCachesFromAnotherIdentityIfNeeded(atLaunch: true) // Turn any pre-revamp single-instance logins into accounts. Repeatable: @@ -651,6 +665,14 @@ final class AppEnvironment { clearAccountScopedCaches() } + // The context in memory belongs to whichever namespace was loaded, and + // an unresolved scope is not one whose accounts anybody has looked at + // yet. Mirroring from it would rewrite the legacy selection to `.local` + // on every activation dip — several times a day with iCloud Drive off — + // and the user would find their picker reset. The observer refires when + // the scope comes back, so the mirror catches up rather than being lost. + guard credentialIdentityScope.isResolved else { return } + let legacy: SubscriptionAccount switch accountsManager.currentContext { diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index dd60185e..78cdab7b 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -52,6 +52,13 @@ final class AccountSyncCoordinator { /// lost by not having one. private var enqueueChanges: EnqueueChanges? + /// Drops the conflict-resolved account records the engine holds for upload. + /// That cache is keyed by record name only, and the names are deterministic + /// across identities, so it is emptied when the identity moves rather than + /// left holding the previous one's records. Their stamps would refuse them + /// anyway; this is what keeps them from lingering in memory until they do. + private var discardCachedRecords: (@MainActor () -> Void)? + /// The private database, for the bootstrap fetch that runs outside the /// engine's change token. private weak var database: CKDatabase? @@ -82,6 +89,11 @@ final class AccountSyncCoordinator { private var isActivating = false private var activationRequested = false + /// Set while staged changes are being applied. Applying posts the very + /// notifications this coordinator retries deferred activations from, so + /// without it a run would ask itself to run again. + private var isApplyingStagedChanges = false + private nonisolated(unsafe) var identityObserver: NSObjectProtocol? private nonisolated(unsafe) var accountsObserver: NSObjectProtocol? private nonisolated(unsafe) var contextObserver: NSObjectProtocol? @@ -151,12 +163,14 @@ final class AccountSyncCoordinator { // Local edits mark themselves dirty in AccountsManager; these turn that // into engine registrations without every call site knowing about sync. + // They are also how `reloadForCurrentIdentity()` announces itself, which + // is what releases records held back by the reload fence. accountsObserver = NotificationCenter.default.addObserver( forName: .accountsDidChange, object: nil, queue: .main ) { [weak self] _ in - MainActor.assumeIsolated { self?.enqueueDirtyRecords() } + MainActor.assumeIsolated { self?.accountStateDidChange() } } contextObserver = NotificationCenter.default.addObserver( @@ -164,7 +178,7 @@ final class AccountSyncCoordinator { object: nil, queue: .main ) { [weak self] _ in - MainActor.assumeIsolated { self?.enqueueDirtyRecords() } + MainActor.assumeIsolated { self?.accountStateDidChange() } } } @@ -187,22 +201,52 @@ final class AccountSyncCoordinator { func identityDidChange() { identityGeneration += 1 + // Records already built for the previous identity are still sitting in + // the engine's conflict cache, which is not namespaced and would serve + // them at send time behind this coordinator's back. + discardCachedRecords?() + guard namespace != nil else { return } Task { await activate() } } + /// Re-runs an activation that was held back because `AccountsManager` had + /// not reloaded yet. + /// + /// Only meaningful while something is staged and unapplied — activation + /// itself posts the notifications this is called from, so it would + /// otherwise ask for a run on the way out of every run it performs. + private func accountStateDidChange() { + enqueueDirtyRecords() + + guard !isApplyingStagedChanges else { return } + guard let namespace, isManagerLoaded(for: namespace) else { return } + guard !store.stagedChanges(in: namespace).isEmpty else { return } + + Task { await activate() } + } + // MARK: - Engine attachment /// Connects the coordinator to a live sync engine. - func attach(database: CKDatabase?, enqueueChanges: @escaping EnqueueChanges) { + /// + /// - Parameter discardingCachedRecords: empties whatever account records the + /// engine is holding from conflict resolution, for when the identity moves. + func attach( + database: CKDatabase?, + discardingCachedRecords discardCachedRecords: (@MainActor () -> Void)? = nil, + enqueueChanges: @escaping EnqueueChanges + ) { self.database = database self.enqueueChanges = enqueueChanges + self.discardCachedRecords = discardCachedRecords } /// Disconnects when the engine is torn down. The durable state — staging, /// outbox, bootstrap flag — is deliberately left untouched. func detach() { enqueueChanges = nil + discardCachedRecords = nil database = nil } @@ -313,6 +357,22 @@ final class AccountSyncCoordinator { guard store.isBootstrapCompleted(in: namespace) else { return } guard namespace == self.namespace else { return } + // Applying is a write into `AccountsManager`'s memory, and during the + // reload gap that memory still belongs to the identity being left. The + // damage is not hypothetical: an incoming account whose deterministic ID + // matches a tombstone the *previous* identity holds would be read as a + // resurrection and deleted from the new identity's zone, and a tombstone + // applied against the previous identity's list removes that account — + // taking its Keychain credential with it. Staging is durable, so this + // defers rather than drops; `accountStateDidChange()` retries it when the + // reload announces itself. + guard isManagerLoaded(for: namespace) else { + LoggingService.shared.logCloudKit( + "Account records stay staged: the accounts in memory still belong to the previous identity" + ) + return + } + applyStagedChanges(in: namespace) enqueueDirtyRecords() } @@ -347,6 +407,9 @@ final class AccountSyncCoordinator { let staged = store.stagedChanges(in: namespace) guard !staged.isEmpty else { return } + isApplyingStagedChanges = true + defer { isApplyingStagedChanges = false } + // Tombstone-first. A deletion that arrived in a later event than the // account record it kills must still be applied before it, or the // account becomes visible — and selectable — in between. @@ -553,6 +616,7 @@ final class AccountSyncCoordinator { func enqueueDirtyRecords() { guard let enqueueChanges, let namespace else { return } guard isCategoryEnabled(), store.isBootstrapCompleted(in: namespace) else { return } + guard isManagerLoaded(for: namespace) else { return } var changes: [CKSyncEngine.PendingRecordZoneChange] = [] @@ -584,10 +648,50 @@ final class AccountSyncCoordinator { /// Registers every account record for upload, for a zone with nothing in it /// — first enable, or a zone recreated after being deleted elsewhere. func uploadEverything() { + // Marking everything dirty commits the *in-memory* list to this + // identity's zone, so it must not run against a snapshot loaded under + // another one. + guard let namespace, isManagerLoaded(for: namespace) else { return } + accountsManager.markAllRecordsForUpload() enqueueDirtyRecords() } + /// Whether the account manager's in-memory state belongs to the namespace + /// this coordinator uploads into. + /// + /// The identity scope moves before `AccountsManager` reloads — the reload + /// runs from a main-queue observer — so in that window `namespace` already + /// names the new identity while the accounts, the outboxes and the selected + /// context in memory are still the previous one's. Engine callbacks can + /// land there, and building records from that snapshot would publish one + /// Apple account's accounts into another's zone. Observer ordering is not + /// contractual, so this fences rather than assuming an order; the outbox is + /// durable, and the reload re-enqueues whatever is still owed. + private func isManagerLoaded(for namespace: String) -> Bool { + accountsManager.loadedNamespace == namespace + } + + /// Whether an account-category record belongs to the identity that is + /// current now — and may therefore be uploaded, cached for retry, or have + /// its confirmation acted on. + /// + /// Judged on the record's own stamp rather than on anything kept beside it. + /// A side table keyed by record name cannot answer this: the names are + /// deterministic, so two identities' copies of the "same" record collide in + /// it, and the second send silently relabels the first one's. The engine + /// also delivers delegate events from a previous identity to the same + /// instance, and an async conflict handler can resume long after the + /// identity moved — both arrive holding a record, which is the one thing + /// that still knows where it came from. + /// + /// An unstamped record fails closed: nothing this build sends is unstamped, + /// so it is not ours to act on. + func belongsToCurrentIdentity(_ record: CKRecord) -> Bool { + guard let namespace, isManagerLoaded(for: namespace) else { return false } + return record[AccountRecordName.identityNamespaceField] as? String == namespace + } + /// Builds the record for a pending save at send time. /// /// Returning `nil` withdraws the change from the engine — used both for @@ -596,7 +700,19 @@ final class AccountSyncCoordinator { /// change is registered again as soon as what it waits on is confirmed. func recordToSave(named recordName: String) -> CKRecord? { guard let namespace, isCategoryEnabled(), store.isBootstrapCompleted(in: namespace) else { return nil } + guard isManagerLoaded(for: namespace) else { return nil } + // Stamped at the moment of building, which is the only moment that says + // for certain whose zone this copy of the record is going into. It + // travels with the record from here on: to the server, back in the + // confirmation, and into the engine's retry cache. + let record = buildRecord(named: recordName) + record?[AccountRecordName.identityNamespaceField] = namespace as CKRecordValue + + return record + } + + private func buildRecord(named recordName: String) -> CKRecord? { switch AccountRecordName.parse(recordName) { case let .account(id): guard !accountsManager.isDeleted(id), let account = accountsManager.account(id: id) else { return nil } @@ -626,11 +742,33 @@ final class AccountSyncCoordinator { /// Clears outbox entries the server has confirmed, and releases whatever was /// waiting on them. + /// + /// A confirmation only ever clears state for the identity that sent the + /// record. The names are deterministic, so the identity that took over + /// during the send has an outbox entry under the same name; honouring the + /// previous identity's confirmation would clear that entry without the + /// server having confirmed anything — and, for a tombstone, register the + /// deletion of the new identity's live account record. func didSave(records: [CKRecord]) { var didSaveAccountRecord = false + var didIgnoreConfirmation = false for record in records { - switch AccountRecordName.parse(record.recordID.recordName) { + let recordName = record.recordID.recordName + + guard !AccountRecordName.isAccountCategory(recordName) || belongsToCurrentIdentity(record) else { + // Left pending rather than confirmed: the outbox is durable and + // the record is registered again for whichever identity still + // owes it. Nothing is enqueued from here — least of all the + // deletion a tombstone's confirmation releases. + didIgnoreConfirmation = true + LoggingService.shared.logCloudKit( + "Ignoring a confirmation for \(recordName) sent under a different Apple account" + ) + continue + } + + switch AccountRecordName.parse(recordName) { case let .account(id): accountsManager.confirmUpload(id, updatedAt: record["updatedAt"] as? Date) didSaveAccountRecord = true @@ -658,7 +796,13 @@ final class AccountSyncCoordinator { // Whatever was held behind these can go now — a marker waiting on its // account, or a context the user changed while the previous one was // still in flight, which confirmation deliberately left dirty. - if didSaveAccountRecord { + // + // An ignored confirmation needs the same call, for the opposite reason: + // the send it belonged to consumed the engine's pending change for that + // record name, and the current identity's reload — which is what would + // otherwise re-register it — has already been and gone. Without this the + // entry stays owed in a durable outbox that nothing is reading. + if didSaveAccountRecord || didIgnoreConfirmation { enqueueDirtyRecords() } } @@ -675,26 +819,15 @@ final class AccountSyncCoordinator { /// Resolves a conflict on an account-category record, and applies the /// outcome locally. - /// - /// The local apply matters here in a way it does not for the other - /// categories: CloudKit never delivers back a record this device saved, so - /// a conflict the server won would otherwise leave stale state on screen - /// until some unrelated future change to the same record. func resolveConflict(local: CKRecord, server: CKRecord) async -> CKRecord { let resolved: CKRecord switch server.recordType { case RecordType.account: resolved = await conflictResolver.resolveAccountConflict(local: local, server: server) - if let account = try? mapper.toAccount(from: resolved), !accountsManager.isDeleted(account.id) { - accountsManager.applySyncedAccount(account) - } case RecordType.dataContext: resolved = await conflictResolver.resolveDataContextConflict(local: local, server: server) - if let context = try? mapper.toStoredDataContext(from: resolved) { - accountsManager.applySyncedContext(context) - } case RecordType.legacyAccountMigration, RecordType.accountDeletion: resolved = await conflictResolver.resolveWriteOnceConflict(local: local, server: server) @@ -703,9 +836,52 @@ final class AccountSyncCoordinator { resolved = server } + // The merge can take fields from the server's copy, including its stamp, + // so provenance is restored from the local side: this record is a retry + // of the record *this device built*, and that is whose zone it is going + // into. Stamping it with whatever is current instead would launder a + // resolution begun under the previous Apple account into the one that + // took over while it was being computed. + inheritProvenance(from: local, to: resolved) + + applyResolvedRecord(resolved) + return resolved } + /// Copies the identity stamp of the record a resolution started from onto + /// its outcome. + private func inheritProvenance(from local: CKRecord, to resolved: CKRecord) { + guard resolved !== local else { return } + resolved[AccountRecordName.identityNamespaceField] = local[AccountRecordName.identityNamespaceField] + } + + /// Writes a resolution's outcome into memory. + /// + /// Matters here in a way it does not for the other categories: CloudKit + /// never delivers back a record this device saved, so a conflict the server + /// won would otherwise leave stale state on screen until some unrelated + /// future change to the same record. Fenced on the record's own provenance — + /// the resolution suspends, and the identity can move across that suspension. + private func applyResolvedRecord(_ resolved: CKRecord) { + guard belongsToCurrentIdentity(resolved) else { return } + + switch resolved.recordType { + case RecordType.account: + guard let account = try? mapper.toAccount(from: resolved), + !accountsManager.isDeleted(account.id) else { return } + accountsManager.applySyncedAccount(account) + + case RecordType.dataContext: + guard let context = try? mapper.toStoredDataContext(from: resolved) else { return } + accountsManager.applySyncedContext(context) + + default: + // Write-once records carry no local state to correct. + break + } + } + // MARK: - Helpers private func recordID(for type: SyncableRecordType) -> CKRecord.ID { diff --git a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift index 5d342b39..75a390ad 100644 --- a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift +++ b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift @@ -399,7 +399,12 @@ final class CloudKitSyncEngine: @unchecked Sendable { // to complete before anything is uploaded, or a device upgrading // from the previous release would publish its unmerged local list // over records it has not seen yet. - accountSync?.attach(database: database) { [weak self] changes in + accountSync?.attach( + database: database, + discardingCachedRecords: { [weak self] in + self?.discardConflictResolvedAccountRecords() + } + ) { [weak self] changes in self?.addPendingChanges(changes) } await accountSync?.activate() @@ -1759,7 +1764,17 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { // A conflict-resolved record (carrying the server change tag) takes precedence if let resolved = conflictResolvedRecords[recordName] { - return resolved + if mayUseResolvedRecord(resolved) { + return resolved + } + + // An account record built under another identity can never become + // valid here — including one resolved after the identity moved but + // before the accounts in memory caught up, which the invalidation on + // the change itself cannot have seen. Dropping it costs at most one + // more conflict round-trip; serving it would upload another Apple + // account's data. + conflictResolvedRecords.removeValue(forKey: recordName) } if let record = await materializeRecord(named: recordName) { @@ -1771,6 +1786,52 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { return nil } + /// Whether a conflict-resolved record may be cached for retry, and served + /// when that retry is sent. + /// + /// This cache is keyed by record name alone, and account record names are + /// deterministic across Apple accounts — so the name says nothing about + /// whose record this is, and the record's own stamp has to be asked. Both + /// ends need the question: the resolution runs asynchronously and can finish + /// after the identity moved (too late for the invalidation that change + /// triggers), and the send can come later still. Falling through to + /// materialization is safe — the coordinator holds the change there, and the + /// durable outbox re-registers it. Every other category is unaffected: + /// nothing else here is namespaced by Apple account. + private func mayUseResolvedRecord(_ record: CKRecord) -> Bool { + guard AccountRecordName.isAccountCategory(record.recordID.recordName) else { return true } + return accountSync?.belongsToCurrentIdentity(record) ?? false + } + + /// Puts an account record's engine change back when a conflict is abandoned. + /// + /// The send consumed the pending change, and these paths return without + /// registering another — but the account outbox is durable and still owes + /// the record, and unlike the SwiftData categories nothing re-derives it + /// from a model change. It would sit there until an unrelated edit, an + /// identity reload or a relaunch happened to re-register it. The coordinator + /// decides what is actually still owed; a record that is not simply does + /// nothing here. Other categories keep the existing behaviour. + private func restoreAccountOutboxIfNeeded(for recordName: String) { + guard AccountRecordName.isAccountCategory(recordName) else { return } + accountSync?.enqueueDirtyRecords() + } + + /// Drops the account records held from conflict resolution, for when the + /// identity moves. Called by the coordinator, which is what observes that. + private func discardConflictResolvedAccountRecords() { + let stale = conflictResolvedRecords.keys.filter { AccountRecordName.isAccountCategory($0) } + guard !stale.isEmpty else { return } + + for recordName in stale { + conflictResolvedRecords.removeValue(forKey: recordName) + } + + LoggingService.shared.logCloudKit( + "Discarded \(stale.count) conflict-resolved account records after the Apple account changed" + ) + } + // MARK: - Scope-Aware Local Lookups /// Bare-ID lookups can return an entity from a different source scope when @@ -1935,6 +1996,7 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { guard let serverRecord = error.serverRecord else { LoggingService.shared.logCloudKitError("No server record in conflict error for \(recordName)", error: error) conflictResolvedRecords.removeValue(forKey: recordName) + restoreAccountOutboxIfNeeded(for: recordName) return } @@ -1946,6 +2008,7 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { ) conflictResolvedRecords.removeValue(forKey: recordName) retryCount.removeValue(forKey: recordName) + restoreAccountOutboxIfNeeded(for: recordName) return } retryCount[recordName] = attempts + 1 @@ -1954,6 +2017,20 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { // The resolved record keeps the server change tag, so the retry // is accepted as an update. let resolved = await resolveConflict(local: record, server: serverRecord) + + // The resolution suspends, and the Apple account can move across + // that suspension: this handler may have begun for an identity that + // is no longer the one this cache would serve the retry to. Neither + // the record nor its retry is this identity's to send. + guard mayUseResolvedRecord(resolved) else { + conflictResolvedRecords.removeValue(forKey: recordName) + retryCount.removeValue(forKey: recordName) + LoggingService.shared.logCloudKit( + "Dropping the conflict resolution for \(recordName) — it was built under a different Apple account" + ) + return + } + conflictResolvedRecords[recordName] = resolved syncEngine?.state.add(pendingRecordZoneChanges: [.saveRecord(recordID)]) LoggingService.shared.logCloudKit("Resolved \(serverRecord.recordType) conflict for \(recordName), retrying (attempt \(attempts + 1))") diff --git a/Yattee/Services/CloudKit/RecordTypes.swift b/Yattee/Services/CloudKit/RecordTypes.swift index 3472c1e7..1907ea9c 100644 --- a/Yattee/Services/CloudKit/RecordTypes.swift +++ b/Yattee/Services/CloudKit/RecordTypes.swift @@ -197,6 +197,16 @@ enum AccountRecordName { static let accountDeletionPrefix = "accountDeletion-" static let dataContext = "datacontext-current" + /// The field every outbound account-category record carries, naming the + /// identity that built it. + /// + /// Record names here are deterministic, so they are identical under every + /// Apple account and prove nothing about whose record this is — provenance + /// has to travel inside the record itself. Written and read only by the + /// account sync coordinator for that comparison; the mappers never look at + /// it, so it cannot reach the `Account` model. + static let identityNamespaceField = "identityNamespace" + static func account(_ id: UUID) -> String { accountPrefix + id.uuidString } static func legacyMigration(_ instanceID: UUID) -> String { legacyMigrationPrefix + instanceID.uuidString } static func accountDeletion(_ accountID: UUID) -> String { accountDeletionPrefix + accountID.uuidString } diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index aca9f34c..2fdb790c 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -224,6 +224,23 @@ final class CredentialIdentityScope { state = fixedScope.map { State.resolved($0) } ?? .unresolved } + /// Test seam: moves a pinned scope after construction. + /// + /// - Parameter announcing: whether to take the real `setState` path, which + /// posts the change notification everything namespaced by identity + /// reloads on. `false` mutates the state silently, which is the only way + /// to reproduce the window between the scope moving and those observers + /// running — the window writes have to be fenced against. + func setFixedScope(_ scope: String?, announcing: Bool = true) { + let newState = scope.map { State.resolved($0) } ?? .unresolved + + if announcing { + setState(newState) + } else { + state = newState + } + } + // MARK: - Resolution /// Re-reads the Apple account and restores the cached scope if there is diff --git a/Yattee/Services/Subscriptions/DataContextValidator.swift b/Yattee/Services/Subscriptions/DataContextValidator.swift index 8529d993..948a89d8 100644 --- a/Yattee/Services/Subscriptions/DataContextValidator.swift +++ b/Yattee/Services/Subscriptions/DataContextValidator.swift @@ -165,6 +165,10 @@ final class DataContextValidator { /// must never reach here — those are all "unknown", and treating them as /// removal is how a user loses their selection during a slow sync. func correctIfContextRemoved() { + // An unresolved identity has not loaded anybody's accounts yet, so an + // account "missing" from that state proves nothing — least of all a + // deletion, which is the only thing allowed to move the context here. + guard accountsManager.isIdentityResolved else { return } guard case let .account(id) = accountsManager.currentContext else { return } guard accountsManager.account(id: id) == nil else { return } diff --git a/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift b/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift index e02a03dc..68a35945 100644 --- a/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift +++ b/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift @@ -117,6 +117,20 @@ final class SubscriptionAccountValidator { /// Validates the current account and auto-corrects if invalid. /// Call this on app launch and when configuration changes. func validateAndCorrectIfNeeded() { + // Availability is decided from account statuses, and every status is + // "cannot tell yet" while the credential identity scope is unresolved. + // This runs on every foreground activation, so correcting from that + // would overwrite the user's selection with `.local` on a state that + // was never an answer. The observation loop re-runs when the scope + // resolves and the statuses become real. + if accountsManager?.isIdentityResolved == false { + LoggingService.shared.debug( + "Leaving the subscription account alone — the credential identity is still unresolved", + category: .general + ) + return + } + guard !isCurrentAccountValid else { LoggingService.shared.debug( "Current subscription account is valid: \(settingsManager.subscriptionAccount.type)", diff --git a/YatteeTests/AccountSyncTests.swift b/YatteeTests/AccountSyncTests.swift index e8731201..a54c8dc6 100644 --- a/YatteeTests/AccountSyncTests.swift +++ b/YatteeTests/AccountSyncTests.swift @@ -63,6 +63,16 @@ struct AccountSyncTests { let category: CategorySwitch let mapper: CloudKitRecordMapper let scope: String + + /// Confirms records the way the server does: what comes back is the + /// copy the sending identity built, stamped with that identity. + @MainActor + func confirm(_ records: [CKRecord]) { + for record in records { + record[AccountRecordName.identityNamespaceField] = scope as CKRecordValue + } + coordinator.didSave(records: records) + } } /// A coordinator with its bootstrap already satisfied. @@ -535,7 +545,7 @@ struct AccountSyncTests { #expect(harness.coordinator.recordToSave(named: "legacymigrated-\(instanceID.uuidString)") == nil) harness.recorder.clear() - harness.coordinator.didSave(records: [harness.mapper.toCKRecord(account: account)]) + harness.confirm([harness.mapper.toCKRecord(account: account)]) #expect(!harness.accounts.pendingUploads.contains(account.id)) #expect(harness.recorder.savedNames.contains("legacymigrated-\(instanceID.uuidString)")) @@ -553,7 +563,7 @@ struct AccountSyncTests { #expect(!harness.accounts.canUploadContext) #expect(harness.coordinator.recordToSave(named: "datacontext-current") == nil) - harness.coordinator.didSave(records: [harness.mapper.toCKRecord(account: account)]) + harness.confirm([harness.mapper.toCKRecord(account: account)]) #expect(harness.accounts.canUploadContext) #expect(harness.coordinator.recordToSave(named: "datacontext-current") != nil) } @@ -573,7 +583,7 @@ struct AccountSyncTests { // The record deletion is registered only once the durable half landed. harness.recorder.clear() - harness.coordinator.didSave(records: [harness.mapper.toCKRecord(accountDeletionID: account.id)]) + harness.confirm([harness.mapper.toCKRecord(accountDeletionID: account.id)]) #expect(!harness.accounts.pendingDeletions.contains(account.id)) #expect(harness.recorder.deletedNames.contains("account-\(account.id.uuidString)")) @@ -601,7 +611,7 @@ struct AccountSyncTests { let sent = harness.mapper.toCKRecord(account: account) harness.accounts.updateDisplayName("Renamed", for: account.id) - harness.coordinator.didSave(records: [sent]) + harness.confirm([sent]) #expect(harness.accounts.pendingUploads.contains(account.id)) } @@ -629,7 +639,7 @@ struct AccountSyncTests { #expect(!harness.recorder.savedNames.contains("legacymigrated-\(kept.instanceID.uuidString)")) harness.recorder.clear() - harness.coordinator.didSave(records: [harness.mapper.toCKRecord(account: kept)]) + harness.confirm([harness.mapper.toCKRecord(account: kept)]) #expect(harness.recorder.savedNames.contains("legacymigrated-\(kept.instanceID.uuidString)")) } @@ -763,6 +773,254 @@ struct AccountSyncTests { #expect(!store.isBootstrapCompleted(in: "identity-B")) } + @Test("Nothing is uploaded from a snapshot loaded under another identity") + func reloadGapBlocksUploads() { + let defaults = Self.makeDefaults() + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: "identity-A") + + // The manager still holds what it loaded while the scope was + // unresolved; the coordinator is already namespaced by the identity + // that resolved. That is the window between the scope moving and the + // manager's reload observer running, and engine callbacks land in it. + let accounts = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: nil), + defaults: defaults + ) + let account = accounts.addAccount(instanceID: UUID()) + accounts.setContext(.account(account.id)) + + let recorder = Recorder() + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { "identity-A" }, + store: store, + isCategoryEnabled: { true } + ) + coordinator.attach(database: nil) { recorder.record($0) } + + coordinator.enqueueDirtyRecords() + #expect(recorder.changes.isEmpty) + + // Nothing is built at send time either — a change registered before the + // scope moved is withdrawn rather than filled in from the wrong state. + #expect(coordinator.recordToSave(named: "account-\(account.id.uuidString)") == nil) + + coordinator.uploadEverything() + #expect(recorder.changes.isEmpty) + + // And the outbox of the identity that owns the namespace is untouched: + // marking everything dirty would have committed another identity's list + // to this one's zone. + for name in ["accounts", "context", "hasContext", "pendingUploads", "pendingContextUpload"] { + #expect(defaults.object(forKey: "accounts.identity-A.\(name)") == nil) + } + } + + @Test("Staged records are never applied to another identity's accounts") + func reloadGapDefersStagedChanges() async throws { + let defaults = Self.makeDefaults() + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: "identity-B") + + // identity-A's list, still in memory: one live account, and a tombstone + // for another whose ID identity-B happens to reuse — migrated accounts + // take their instance's UUID, so the collision is ordinary rather than + // exotic. + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let accounts = AccountsManager(identityScope: scope, defaults: defaults) + let livingAccount = accounts.addAccount(instanceID: UUID()) + let sharedID = accounts.addAccount(instanceID: UUID()).id + accounts.removeAccount(sharedID) + + // The scope moved; the reload that refills memory has not run yet. + scope.setFixedScope("identity-B", announcing: false) + + let recorder = Recorder() + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { scope.current }, + store: store, + isCategoryEnabled: { true } + ) + coordinator.attach(database: nil) { recorder.record($0) } + + var deletedAccounts: [(UUID, Account?)] = [] + coordinator.onAccountDeleted = { deletedAccounts.append(($0, $1)) } + + let mapper = Self.makeMapper() + coordinator.stageFetched( + records: [ + mapper.toCKRecord(accountDeletionID: livingAccount.id), + mapper.toCKRecord(account: Account(id: sharedID, instanceID: UUID())) + ], + deletions: [] + ) + await coordinator.activate() + + // Nothing applied: identity-B's tombstone would have removed + // identity-A's account and taken its credential with it, and + // identity-B's account would have been read as a resurrection of + // identity-A's tombstone and deleted from identity-B's zone. + #expect(accounts.account(id: livingAccount.id) != nil) + #expect(deletedAccounts.isEmpty) + #expect(recorder.changes.isEmpty) + #expect(!store.stagedChanges(in: "identity-B").isEmpty) + + // Deferred, not dropped. The reload announces itself, and that is what + // releases them — against the right list this time. + accounts.reloadForCurrentIdentity() + try await Task.sleep(for: .milliseconds(200)) + + #expect(store.stagedChanges(in: "identity-B").isEmpty) + #expect(accounts.account(id: sharedID) != nil) + #expect(!recorder.deletedNames.contains("account-\(sharedID.uuidString)")) + // The tombstone applies to identity-B, which never had that account — + // so the credential removal it triggers carries no account of + // identity-A's. + #expect(deletedAccounts.map(\.0) == [livingAccount.id]) + #expect(deletedAccounts.first?.1 == nil) + } + + @Test("A confirmation from the previous identity clears nothing") + func confirmationsAreTiedToTheIdentityThatSentThem() async throws { + let defaults = Self.makeDefaults() + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: "identity-A") + store.markBootstrapCompleted(in: "identity-B") + + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let accounts = AccountsManager(identityScope: scope, defaults: defaults) + let identity = Identity("identity-A") + + let recorder = Recorder() + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { identity.value }, + store: store, + isCategoryEnabled: { true } + ) + coordinator.attach(database: nil) { recorder.record($0) } + + // A deletion built and sent under identity-A, in flight when the Apple + // account changes. + let sharedID = accounts.addAccount(instanceID: UUID()).id + accounts.removeAccount(sharedID) + coordinator.enqueueDirtyRecords() + #expect(recorder.savedNames.contains("accountDeletion-\(sharedID.uuidString)")) + let sentByA = try #require(coordinator.recordToSave(named: "accountDeletion-\(sharedID.uuidString)")) + + scope.setFixedScope("identity-B") + try await Task.sleep(for: .milliseconds(200)) + identity.value = "identity-B" + + // identity-B owes the same record name — the IDs are deterministic — and + // has had no answer from the server about it. It even builds and sends + // its own copy, which is what makes anything kept *beside* the record, + // keyed by that shared name, unable to tell the two apart. + accounts.applySyncedAccount(Account(id: sharedID, instanceID: UUID())) + accounts.removeAccount(sharedID) + let sentByB = try #require(coordinator.recordToSave(named: "accountDeletion-\(sharedID.uuidString)")) + recorder.clear() + + coordinator.didSave(records: [sentByA]) + + // Still owed, and no deletion was registered off the back of it: + // releasing the account record's deletion here would delete identity-B's + // record on the strength of identity-A's confirmation. + #expect(accounts.pendingDeletions.contains(sharedID)) + #expect(recorder.deletedNames.isEmpty) + + // identity-B's own copy is its own confirmation. + recorder.clear() + coordinator.didSave(records: [sentByB]) + + #expect(!accounts.pendingDeletions.contains(sharedID)) + #expect(recorder.deletedNames.contains("account-\(sharedID.uuidString)")) + } + + @Test("A skipped confirmation puts back what the current identity still owes") + func skippedConfirmationRestoresTheOutbox() async throws { + let defaults = Self.makeDefaults() + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: "identity-A") + store.markBootstrapCompleted(in: "identity-B") + + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let accounts = AccountsManager(identityScope: scope, defaults: defaults) + let identity = Identity("identity-A") + + let recorder = Recorder() + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { identity.value }, + store: store, + isCategoryEnabled: { true } + ) + coordinator.attach(database: nil) { recorder.record($0) } + + let accountID = accounts.addAccount(instanceID: UUID()).id + let sentByA = try #require(coordinator.recordToSave(named: "account-\(accountID.uuidString)")) + + scope.setFixedScope("identity-B") + try await Task.sleep(for: .milliseconds(200)) + identity.value = "identity-B" + + // identity-B's own upload, registered by the reload that has already + // happened. identity-A's send consumed the engine's pending change for + // that record name, so after its confirmation is skipped nothing else + // is left to re-register the debt. + let owed = accounts.addAccount(instanceID: UUID()).id + recorder.clear() + + coordinator.didSave(records: [sentByA]) + + #expect(accounts.pendingUploads.contains(owed)) + #expect(recorder.savedNames.contains("account-\(owed.uuidString)")) + #expect(recorder.deletedNames.isEmpty) + } + + @Test("A change of identity invalidates the records the engine cached") + func identityChangeDropsCachedRecords() throws { + let defaults = Self.makeDefaults() + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: "identity-A") + + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let accounts = AccountsManager(identityScope: scope, defaults: defaults) + let identity = Identity("identity-A") + + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { identity.value }, + store: store, + isCategoryEnabled: { true } + ) + + var didDiscardCachedRecords = false + coordinator.attach( + database: nil, + discardingCachedRecords: { didDiscardCachedRecords = true } + ) { _ in } + + // The engine serves conflict-resolved records ahead of anything the + // coordinator builds, and that cache is keyed by record name alone — + // which every identity shares. The record's own stamp is what tells them + // apart. + let accountID = accounts.addAccount(instanceID: UUID()).id + let builtByA = try #require(coordinator.recordToSave(named: "account-\(accountID.uuidString)")) + #expect(coordinator.belongsToCurrentIdentity(builtByA)) + + identity.value = "identity-B" + #expect(!coordinator.belongsToCurrentIdentity(builtByA)) + + // And an unstamped record — one this build never sent — is nobody's. + #expect(!coordinator.belongsToCurrentIdentity(Self.makeMapper().toCKRecord(account: Account(instanceID: UUID())))) + + coordinator.identityDidChange() + #expect(didDiscardCachedRecords) + } + /// A mutable identity, for driving scope changes deterministically. private final class Identity { var value: String? diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 7106cf4a..2f05d1c0 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -339,6 +339,156 @@ struct AccountsManagerTests { #expect(relaunched.currentContext == .local) } + /// Every key the manager persists, spelled out here rather than read from + /// it: a purge that stopped covering one of them would otherwise be + /// invisible to these tests. + private static let storedKeyNames = [ + "accounts", "context", "hasContext", "migrated", "pendingUploads", + "pendingDeletions", "pendingMarkerUploads", "deletedAccounts", + "deletedInstances", "pendingContextUpload" + ] + + private func unresolvedKeysAreAbsent(in defaults: UserDefaults) -> Bool { + Self.storedKeyNames.allSatisfy { defaults.object(forKey: "accounts.unresolved.\($0)") == nil } + } + + private func makeLegacyInstance() -> Instance { + Instance( + type: .invidious, + url: URL(string: "https://legacy-\(UUID().uuidString).example.com")!, + usesAccountLogin: true + ) + } + + @Test("An unresolved identity never migrates a legacy login") + func unresolvedIdentityDoesNotMigrate() { + let defaults = makeDefaults() + let instances = InstancesManager() + let instance = makeLegacyInstance() + instances.add(instance) + defer { instances.remove(instance) } + + let unresolved = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: nil), + instancesManager: instances, + defaults: defaults + ) + unresolved.reconcileLegacyAccounts() + + // Synthesizing here would file the account — and the permanent marker + // that suppresses synthesis everywhere else — under a namespace no + // identity ever reads back. + #expect(unresolved.accounts.isEmpty) + #expect(unresolved.migratedInstanceIDs.isEmpty) + #expect(unresolved.pendingUploads.isEmpty) + #expect(unresolved.pendingMarkerUploads.isEmpty) + #expect(unresolvedKeysAreAbsent(in: defaults)) + + // Held back, not lost: the same instance migrates as soon as there is + // an identity to migrate it for. + let resolved = makeManager(defaults: defaults, instances: instances) + resolved.reconcileLegacyAccounts() + #expect(resolved.accounts(forInstance: instance.id).count == 1) + #expect(resolved.migratedInstanceIDs.contains(instance.id)) + } + + @Test("A scope that dips to unresolved and returns writes nothing in between") + func scopeRoundTripLeavesTheUnresolvedNamespaceEmpty() async throws { + let defaults = makeDefaults() + let instances = InstancesManager() + let instance = makeLegacyInstance() + instances.add(instance) + defer { instances.remove(instance) } + + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let manager = AccountsManager( + identityScope: scope, + instancesManager: instances, + defaults: defaults + ) + manager.reconcileLegacyAccounts() + + let account = try #require(manager.accounts(forInstance: instance.id).first) + manager.setContext(.account(account.id)) + + // With iCloud Drive switched off this happens on every foreground + // activation — the scope drops and comes back as the same account. + scope.setFixedScope(nil) + try await Task.sleep(for: .milliseconds(50)) + #expect(manager.accounts.isEmpty) + + // Instances keep arriving over iCloud while this lasts, and each + // arrival asks for reconciliation. + NotificationCenter.default.post(name: .instancesDidChange, object: nil) + try await Task.sleep(for: .milliseconds(50)) + #expect(manager.accounts.isEmpty) + #expect(manager.migratedInstanceIDs.isEmpty) + // Checked here as well as after the flip back: once an identity is + // resolved the purge would hide a write made while it was not. + #expect(unresolvedKeysAreAbsent(in: defaults)) + + scope.setFixedScope("identity-A") + try await Task.sleep(for: .milliseconds(50)) + + #expect(manager.accounts(forInstance: instance.id).map(\.id) == [account.id]) + #expect(manager.currentContext == .account(account.id)) + #expect(unresolvedKeysAreAbsent(in: defaults)) + } + + @Test("Resolving an identity purges what was stored while it was unknown") + func resolutionPurgesTheUnresolvedNamespace() { + let defaults = makeDefaults() + + let identityA = makeManager(defaults: defaults) + let account = identityA.addAccount(instanceID: UUID(), displayName: "Main") + + // The blob measured on the device: a snapshot written before the + // identity was known, contradicting the one that owns the storage. + let unresolved = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: nil), + defaults: defaults + ) + let stale = unresolved.addAccount(instanceID: UUID()) + unresolved.setContext(.account(stale.id)) + #expect(defaults.data(forKey: "accounts.unresolved.accounts") != nil) + + let relaunched = makeManager(defaults: defaults) + + #expect(unresolvedKeysAreAbsent(in: defaults)) + #expect(relaunched.account(id: account.id)?.displayName == "Main") + #expect(relaunched.account(id: stale.id) == nil) + } + + @Test("A save made while the namespace is reloading is dropped") + func saveDuringTheReloadGapIsDropped() async throws { + let defaults = makeDefaults() + let scope = CredentialIdentityScope(fixedScope: "identity-A") + let manager = AccountsManager(identityScope: scope, defaults: defaults) + let account = manager.addAccount(instanceID: UUID()) + + // The scope moves before the observer that reloads this manager runs, + // so `namespace` already names the new identity while everything in + // memory is still the previous one's. + scope.setFixedScope("identity-B", announcing: false) + manager.setContext(.account(account.id)) + + // identity-B must not be handed identity-A's selection... + #expect(defaults.data(forKey: "accounts.identity-B.context") == nil) + #expect(!defaults.bool(forKey: "accounts.identity-B.hasContext")) + + // ...and identity-A's stored copy must not have moved either: the write + // was aimed at keys that no longer describe what is in memory. + let storedA = try #require(defaults.data(forKey: "accounts.identity-A.context")) + #expect(try JSONDecoder().decode(StoredDataContext.self, from: storedA).context == .local) + + // The announced flip is the real path, and it reloads cleanly. + scope.setFixedScope("identity-A") + try await Task.sleep(for: .milliseconds(50)) + + #expect(manager.accounts.map(\.id) == [account.id]) + #expect(manager.currentContext == .local) + } + @Test("A migrated context loses to any explicit choice") func migratedContextLosesToExplicitChoice() { let manager = makeManager()