diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index f22e6506..3cf7eda1 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -73,6 +73,15 @@ final class AccountSyncCoordinator { /// enough to remove it. var onAccountDeleted: (@MainActor (UUID, Account?) -> Void)? + /// Called whenever the accounts in memory are readable for the current + /// identity — the state in which this coordinator can say what they owe. + /// + /// Anything that had to be refused during the reload gap gets its second + /// chance here rather than waiting for the next launch. Fired on every + /// announcement, not only on the transition, so a listener that cares must + /// be idempotent and cheap to refuse. + var onAccountsBecameReadable: (@MainActor () -> Void)? + // MARK: - Identity /// Bumped on every observed change of identity. Async work captures it and @@ -234,9 +243,19 @@ final class AccountSyncCoordinator { guard !isApplyingStagedChanges else { return } guard let namespace, isManagerLoaded(for: namespace) else { return } + + // Announced before the staging check below returns: the accounts being + // readable is the news, and it is news whether or not anything is + // staged. + onAccountsBecameReadable?() + guard !store.stagedChanges(in: namespace).isEmpty else { return } - Task { await activate() } + // Detached: this observer runs inside the sync-engine delegate's + // callback tree when `handleFetchedChanges` applies account records and + // posts `.accountsDidChange`, and an unstructured task would inherit + // that scope — where a call back into the engine is a trap. + Task.detached { [weak self] in await self?.activate() } } // MARK: - Engine attachment @@ -777,12 +796,14 @@ final class AccountSyncCoordinator { // empty belongs to the identity actually signed in. guard !debugSimulationActive, let namespace else { return false } - store.clearStagedChanges(in: namespace) - - // Same fence as `uploadEverything()`, for the same reason: during the - // reload gap the outbox in memory is still the previous identity's, and - // emptying it would drop uploads the purge said nothing about. + // Fenced before anything is destroyed, the way `uploadEverything()` is: + // during the reload gap the accounts in memory are still the previous + // identity's, and dropping their staging and their outbox here would + // answer this purge with someone else's data. The purge is answered + // again when they arrive. guard isManagerLoaded(for: namespace) else { return false } + + store.clearStagedChanges(in: namespace) accountsManager.clearPendingUploads() return true } diff --git a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift index 448e7815..5aec4cab 100644 --- a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift +++ b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift @@ -53,6 +53,19 @@ private enum ApplyRecordResult { case failed } +/// Whether the current task is running inside a `CKSyncEngineDelegate` +/// callback. +/// +/// CloudKit traps, in release builds, on a call back into the engine from +/// inside one of its own event callbacks — `fetchChanges()` or `sendChanges()` +/// — and reads "inside the callback" from exactly this kind of task-local, so a +/// plain `Task { }` spawned there inherits the trap while a `Task.detached` +/// escapes it. That is what killed the app a second after the push in the S14 +/// campaign, and it is invisible until it happens on someone's device. +enum CloudKitDelegateScope { + @TaskLocal static var isHandlingEvent = false +} + /// Main CloudKit sync engine that manages bidirectional sync between local SwiftData and iCloud. @MainActor @Observable @@ -133,6 +146,18 @@ final class CloudKitSyncEngine: @unchecked Sendable { /// not finished answering it. private let zoneRecovery = CloudKitZoneRecoveryMarker() + /// Counts state updates as the engine hands them over, before they reach + /// the main actor — see ``ZoneRecoveryDeliveryCounter``. + private nonisolated let stateUpdateDeliveries = ZoneRecoveryDeliveryCounter() + + /// Whether the last answer to a zone deletion could not speak for the + /// accounts, and is owed a second attempt once they arrive. + /// + /// Distinct from the marker still being pending: a re-seed that registered + /// everything keeps its marker until the engine serializes the work, and + /// redoing *that* would only disarm it and start over. + private var zoneRecoveryAwaitsAccounts = false + /// UserDefaults key for the record schema version this app last synced with. private let lastSchemaVersionKey = "cloudKitLastSchemaVersion" @@ -233,8 +258,34 @@ final class CloudKitSyncEngine: @unchecked Sendable { /// Drives the account category: accounts, migration markers, deletion /// tombstones and the selected data context. Set after initialization, and /// nil in tests that only exercise the SwiftData categories. - var accountSync: AccountSyncCoordinator? - + var accountSync: AccountSyncCoordinator? { + didSet { wireZoneRecoveryRetry() } + } + + /// Asks the accounts to say when they become readable again, so a recovery + /// that could not speak for them is answered as soon as they arrive. + /// + /// Without this the marker simply waits: the accounts finish loading, and + /// nothing looks at it again until the next `setupSyncEngine()` — which, + /// on a device left running, may be the next launch. + private func wireZoneRecoveryRetry() { + accountSync?.onAccountsBecameReadable = { [weak self] in + // Only a recovery that was *refused*, never one that is merely + // waiting for the engine to serialize what it registered. This + // notification fires for every account edit, and redoing an armed + // re-seed on each one would disarm it and start it over. + guard let self, self.zoneRecoveryAwaitsAccounts else { return } + + // Detached, not a plain `Task`: the notification behind this is + // posted from inside the sync-engine delegate's callback tree when + // fetched account records are applied, and an unstructured task + // would inherit that scope and carry the recovery back into it. + Task.detached { [weak self] in + await self?.resumeZoneRecoveryIfNeeded() + } + } + } + // MARK: - Initialization init( @@ -413,12 +464,6 @@ final class CloudKitSyncEngine: @unchecked Sendable { } await accountSync?.activate() - // A zone deletion noticed in an earlier run may not have been - // answered in full — the marker outlives whatever interrupted it, - // and this is the first point where the engine exists and the - // accounts are loaded, outside any delegate callback. - await resumeZoneRecoveryIfNeeded() - if accountChanged { LoggingService.shared.logCloudKit("CloudKit sync engine initialized with new account (fresh sync state)") } else { @@ -430,10 +475,21 @@ final class CloudKitSyncEngine: @unchecked Sendable { loadDeferredItems() updatePendingCount() - + + // A zone deletion noticed in an earlier run may not have been + // answered in full — the marker outlives whatever interrupted it, + // and this is the first point where the engine exists and the + // accounts are loaded, outside any delegate callback. + // + // Deliberately last: everything above puts work *into* the engine — + // the buffered changes, the legacy pending-change migration, the + // deferred playlist items — and a resumed purge has to be able to + // drop what it is not allowed to resend, including all of that. + await resumeZoneRecoveryIfNeeded() + // Perform initial sync await sync() - + } catch { LoggingService.shared.logCloudKitError("Failed to setup sync engine", error: error) syncError = error @@ -503,6 +559,13 @@ final class CloudKitSyncEngine: @unchecked Sendable { // Clear deferred playlist items (they belong to the old account context) clearDeferredItems() + // The zone a recovery was answering belonged to the account being left. + // Replayed against the next one's namespace it answers a wipe that + // never happened there — and a pending purge would drop that identity's + // staged accounts and its outbox on the way in. + zoneRecovery.forget() + zoneRecoveryAwaitsAccounts = false + // Clear any existing sync engine syncEngine = nil updatePendingCount() @@ -600,8 +663,29 @@ final class CloudKitSyncEngine: @unchecked Sendable { isSyncEnabled && (settingsManager?.syncInstances ?? true) } + /// Fails the build's own tests, and a developer's run, if the engine is + /// about to be re-entered from inside one of its delegate callbacks. + /// + /// Release behaviour is unchanged: CloudKit's own trap is what users would + /// meet, and adding a second one here would only change which line the + /// crash names. This is here so the trap is met by whoever wrote the call + /// instead — a plain `Task { }` in a delegate path is enough to inherit it, + /// and nothing about the call site says so. + private nonisolated func assertNotInsideDelegateEvent(_ call: String) { + #if DEBUG + if CloudKitDelegateScope.isHandlingEvent { + assertionFailure( + "\(call) was reached from inside a CKSyncEngine delegate callback. " + + "Register changes in the engine's state instead, or escape the callback with Task.detached." + ) + } + #endif + } + /// Manually trigger a full sync. func sync() async { + assertNotInsideDelegateEvent("sync()") + guard isSyncEnabled, let syncEngine else { LoggingService.shared.logCloudKit("Sync skipped: sync disabled or engine not ready") return @@ -1319,6 +1403,8 @@ final class CloudKitSyncEngine: @unchecked Sendable { /// Fetch remote changes from CloudKit. Used when the app returns to foreground /// to catch any changes that may have been missed (e.g. dropped push notifications). func fetchRemoteChanges() async { + assertNotInsideDelegateEvent("fetchRemoteChanges()") + guard isSyncEnabled, let syncEngine else { if !isSyncEnabled { LoggingService.shared.logCloudKit("fetchRemoteChanges skipped: sync disabled") @@ -1557,7 +1643,18 @@ final class CloudKitSyncEngine: @unchecked Sendable { } /// Clear all sync state and reset. For testing/debugging only. + /// + /// Refused while sync is off. Everything after the zone deletion runs + /// through `setupSyncEngine()` and `performInitialUpload()`, both of which + /// stop at the same disabled toggle — so with sync off this would delete + /// the zone, recreate it empty, and then tell the user their local data had + /// been re-uploaded into it. func resetSync() async throws { + guard isSyncEnabled else { + LoggingService.shared.logCloudKit("Reset refused: iCloud sync is turned off, nothing would be re-uploaded") + throw CloudKitError.syncDisabled + } + guard let zoneManager else { return } // Delete zone (and all records) @@ -1815,6 +1912,17 @@ final class CloudKitSyncEngine: @unchecked Sendable { extension CloudKitSyncEngine: CKSyncEngineDelegate { nonisolated func handleEvent(_ event: CKSyncEngine.Event, syncEngine: CKSyncEngine) async { + // Marks the whole callback tree, so anything that re-enters the engine + // from inside it says so in a debug build instead of trapping on + // someone's device. Inherited by every plain `Task { }` started below + // it — which is precisely the mistake worth catching — and escaped by + // `Task.detached`, which is the fix. + await CloudKitDelegateScope.$isHandlingEvent.withValue(true) { + await handleDelegateEvent(event) + } + } + + private nonisolated func handleDelegateEvent(_ event: CKSyncEngine.Event) async { // Events that require async processing must be awaited directly (not in a fire-and-forget Task) // so that CKSyncEngine waits for processing to complete before advancing the sync state token. // Otherwise the state is saved before changes are applied, and on next launch CKSyncEngine @@ -1838,20 +1946,27 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { // what tells staged account records they may be applied. await accountSync?.zoneFetchCompleted(zoneID: changes.zoneID, error: changes.error) + case .stateUpdate(let update): + // Numbered here, before the hop to the main actor: a recovery holds + // the main actor while it registers its work, so an update handed + // over before that can still run its body afterwards — carrying a + // serialization in which none of the work exists. + let delivery = stateUpdateDeliveries.next() + await MainActor.run { + let encoder = PropertyListEncoder() + let stateSize = (try? encoder.encode(update.stateSerialization))?.count ?? 0 + LoggingService.shared.logCloudKit("Sync state updated and saved (\(stateSize) bytes)") + saveSyncState(update.stateSerialization) + // The state just written may be what a zone recovery registered + // itself into, so the note that it still owes that work can go + // — and not one moment earlier. + zoneRecovery.stateDidPersist(deliveredAt: delivery) + updatePendingCount() + } + default: await MainActor.run { switch event { - case .stateUpdate(let update): - let encoder = PropertyListEncoder() - let stateSize = (try? encoder.encode(update.stateSerialization))?.count ?? 0 - LoggingService.shared.logCloudKit("Sync state updated and saved (\(stateSize) bytes)") - saveSyncState(update.stateSerialization) - // The state just written is what a zone recovery registered - // itself into, so the note that it still owes that work can - // go — and not one moment earlier. - zoneRecovery.stateDidPersist() - updatePendingCount() - case .accountChange: LoggingService.shared.logCloudKit("iCloud account change event received - reinitializing sync engine") Task.detached { [weak self] in @@ -2289,10 +2404,12 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { /// Answers a deletion of our zone, using nothing but durable storage and /// the engine's own state. /// - /// Safe to run twice: re-registering pending changes collapses in the - /// engine state, and marking the account outbox dirty is idempotent. That - /// is what lets an interrupted recovery simply be redone on the next - /// launch. + /// The two responses are not equally repeatable, and their markers are + /// retired on different terms because of it — see ``ZoneRecoveryRetirement``. + /// A re-seed only adds, so it is safe to run twice and is held open until + /// the engine has written it down. A purge removes, and running it a second + /// time would remove work registered in between, so it is retired the + /// moment its durable half succeeds. private func answerZoneDeletion(_ response: ZoneDeletionResponse) async { // Durable first. Everything below can be interrupted, and a relaunch // that finds the zone recreated and no marker cannot tell a wiped zone @@ -2313,32 +2430,53 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { // having created the zone this session and would do nothing, and // the engine saves the zone ahead of the records that need it. requestZoneRecreation() - finish(zoneRecovery, ifComplete: await enqueueFullUpload()) + finish(response, ifComplete: await enqueueFullUpload()) case .discard: LoggingService.shared.logCloudKit( "Zone '\(RecordType.zoneName)' was purged by the account owner - dropping what it held, nothing is resent" ) - finish(zoneRecovery, ifComplete: discardWhatTheZoneHeld()) + finish(response, ifComplete: discardWhatThePurgedZoneHeld()) } } - /// Lets the marker be retired once the engine serializes its state — but - /// only if the recovery reached every category. The account half can refuse - /// while the identity it belongs to has not loaded, and a recovery that - /// left the accounts out is one the next launch has to redo. - private func finish(_ marker: CloudKitZoneRecoveryMarker, ifComplete complete: Bool) { + /// Retires the marker, or arms it to be retired, once the recovery reached + /// every category. + /// + /// The account half can refuse while the identity it belongs to has not + /// loaded, and a recovery that left the accounts out is one that has to be + /// answered again — by the retry that runs when the accounts arrive, or by + /// the next launch. + /// + /// When it did reach everything, *how* the marker goes depends on the + /// response: a re-seed waits for the engine to serialize the work it + /// registered, a purge is already done. See ``ZoneRecoveryRetirement``. + private func finish(_ response: ZoneDeletionResponse, ifComplete complete: Bool) { guard complete else { + zoneRecoveryAwaitsAccounts = true LoggingService.shared.logCloudKit( - "Zone recovery could not reach the account records yet - keeping the marker for the next launch" + "Zone recovery could not reach the account records yet - keeping the marker for the accounts to arrive" ) return } - marker.readyToClear() + + zoneRecoveryAwaitsAccounts = false + + switch response.retirement { + case .whenStatePersists: + zoneRecovery.readyToClear(afterDelivery: stateUpdateDeliveries.current) + case .immediately: + zoneRecovery.retireNow() + } } /// Re-registers the recovery a previous run recorded but may not have - /// finished. Called during setup, outside any delegate callback. + /// finished. + /// + /// Called during setup, and again whenever the accounts this device could + /// not speak for become readable. Never from inside a delegate callback: + /// the recovery is safe there, but the retry's trigger is a notification + /// the delegate tree posts, and the hop out of it is what keeps that true. private func resumeZoneRecoveryIfNeeded() async { guard let response = zoneRecovery.pending else { return } @@ -2364,15 +2502,33 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { /// - Returns: whether the account category could be cleared too. It cannot /// while the accounts in memory belong to another identity, and a purge /// half-answered has to be answered again. - private func discardWhatTheZoneHeld() -> Bool { + private func discardWhatThePurgedZoneHeld() -> Bool { if let syncEngine { let pending = syncEngine.state.pendingRecordZoneChanges if !pending.isEmpty { syncEngine.state.remove(pendingRecordZoneChanges: pending) } + + // A pending `.saveZone` would put the purged zone back on the next + // send — from an earlier `.zoneNotFound`, or from a re-seed this + // purge overtook. The zone comes back lazily when the user next + // changes something, which is a new decision rather than a resend. + let pendingZones = syncEngine.state.pendingDatabaseChanges + if !pendingZones.isEmpty { + syncEngine.state.remove(pendingDatabaseChanges: pendingZones) + } } pendingChangesBuffer.removeAll() clearDeferredItems() + + // Written by a build older than the state-driven engine and consumed by + // `migrateLegacyPendingChanges(into:)`. A resumed purge runs after that + // migration, so these are normally already spent — but a purge answered + // before the migration ever ran would otherwise leave them to be + // rediscovered and sent on a later launch. + UserDefaults.standard.removeObject(forKey: pendingSaveRecordNamesKey) + UserDefaults.standard.removeObject(forKey: pendingDeleteRecordNamesKey) + let accountsCleared = accountSync?.discardWhatTheZoneHeld() ?? true updatePendingCount() return accountsCleared @@ -3063,6 +3219,7 @@ enum SyncStatus: Equatable { enum CloudKitError: Error, LocalizedError, Sendable { case iCloudNotAvailable(status: CKAccountStatus) case notAuthenticated + case syncDisabled case quotaExceeded case zoneNotFound case recordNotFound @@ -3079,6 +3236,8 @@ enum CloudKitError: Error, LocalizedError, Sendable { "iCloud is not available (status: \(status))" case .notAuthenticated: "Not signed into iCloud. Sign in to Settings to enable sync." + case .syncDisabled: + "iCloud sync is turned off. Turn it on before resetting iCloud data." case .quotaExceeded: "iCloud storage quota exceeded. Free up space in iCloud settings." case .zoneNotFound: diff --git a/Yattee/Services/CloudKit/CloudKitZoneRecovery.swift b/Yattee/Services/CloudKit/CloudKitZoneRecovery.swift index 6b53331e..73193777 100644 --- a/Yattee/Services/CloudKit/CloudKitZoneRecovery.swift +++ b/Yattee/Services/CloudKit/CloudKitZoneRecovery.swift @@ -45,6 +45,72 @@ enum ZoneDeletionResponse: String, Sendable { } } +/// When the marker for a recovery may be retired. +enum ZoneRecoveryRetirement: Sendable { + /// Only once the engine has serialized the state the recovery registered, + /// so anything interrupted in between is redone on the next launch. + case whenStatePersists + + /// As soon as the recovery's own durable half has been done. + case immediately +} + +extension ZoneDeletionResponse { + /// The lifecycle this response's marker follows. + /// + /// The two do not share one, because being redone does not cost them the + /// same thing. + /// + /// A re-seed only adds: registering the same records twice collapses in the + /// engine's state, and marking the account outbox dirty is a set union. It + /// can therefore afford to be held until the engine has actually written + /// the work down, and to be repeated for as long as it has not. + /// + /// A discard removes, and a replay removes more than it was asked to: it + /// drops every pending change the engine holds *now*, including work + /// registered after the purge was answered — an account added while the app + /// kept running, a category switched back on, a re-run of `setupSyncEngine`. + /// Its durable half — this identity's staged records and its account outbox + /// — is the answer the purge actually needs, and it is complete the moment + /// that half succeeds. So the marker goes then, and the engine-state + /// clearing beside it is one-shot. + var retirement: ZoneRecoveryRetirement { + switch self { + case .reseed: .whenStatePersists + case .discard: .immediately + } + } +} + +/// Counts sync-engine state updates as they are delivered, off the main actor. +/// +/// A state update is handed to the delegate on the engine's own thread and only +/// then hops to the main actor. An update delivered *before* a recovery +/// registered its work can still run its main-actor body after — the recovery +/// holds the main actor while it registers — and the serialization it carries +/// predates that work entirely. Retiring a marker on one of those would forget +/// work the engine has not written down. Counting at delivery is what tells the +/// two apart. +final class ZoneRecoveryDeliveryCounter: @unchecked Sendable { + private let lock = NSLock() + private var count: UInt64 = 0 + + /// Records a delivery and returns its sequence number. + func next() -> UInt64 { + lock.lock() + defer { lock.unlock() } + count += 1 + return count + } + + /// How many deliveries have happened so far. + var current: UInt64 { + lock.lock() + defer { lock.unlock() } + return count + } +} + /// One category of local data a re-seed publishes. enum ReseedCategory: String, CaseIterable, Sendable { case accounts @@ -99,6 +165,10 @@ final class CloudKitZoneRecoveryMarker { /// and only waiting for the engine to serialize its state. private var awaitingStatePersistence = false + /// How many state updates the engine had delivered when the work was + /// registered. Only a later one can carry it. + private var armedAfterDelivery: UInt64 = 0 + init(defaults: UserDefaults = .standard, key: String = "cloudKitZoneNeedsReseed") { self.defaults = defaults self.key = key @@ -115,14 +185,29 @@ final class CloudKitZoneRecoveryMarker { awaitingStatePersistence = false } - /// Says the recovery is fully registered with the engine, so the next + /// Says the recovery is fully registered with the engine, so a *later* /// serialization of that state may retire the marker. /// /// Deliberately separate from `begin`: a recovery that could not register /// everything — accounts belonging to an identity that has not loaded yet — /// keeps its marker and is redone on the next launch. - func readyToClear() { + /// + /// - Parameter sequence: the number of state updates delivered so far. An + /// update already delivered when this is called was serialized before the + /// work was registered, however late its main-actor half runs. + func readyToClear(afterDelivery sequence: UInt64) { awaitingStatePersistence = true + armedAfterDelivery = sequence + } + + /// Retires the marker now, without waiting for the engine to write anything + /// down. + /// + /// For the response whose answer is its own durable half — see + /// ``ZoneRecoveryRetirement``. Redoing that one would destroy work that was + /// registered after it, so it is not held open for a redo. + func retireNow() { + clear() } /// Retires the marker once the engine has persisted the state the recovery @@ -130,10 +215,29 @@ final class CloudKitZoneRecoveryMarker { /// /// State updates arrive for every other reason too, and one of those must /// not retire a marker whose work has not been redone — that is exactly the - /// state a relaunch after an interrupted recovery starts in. - func stateDidPersist() { - guard awaitingStatePersistence else { return } + /// state a relaunch after an interrupted recovery starts in. Nor may one + /// that was already on its way when the work was registered: it is carrying + /// a serialization taken before any of it existed. + /// + /// - Parameter sequence: the delivery number of the state update being + /// handled, taken when the engine handed it over. + func stateDidPersist(deliveredAt sequence: UInt64) { + guard awaitingStatePersistence, sequence > armedAfterDelivery else { return } + clear() + } + + /// Forgets the marker entirely, without treating its work as done. + /// + /// For the Apple account moving underneath us: the marker names a decision + /// about *that* identity's zone, and replaying it against the next one's + /// namespace answers a wipe that never happened there. + func forget() { + clear() + } + + private func clear() { awaitingStatePersistence = false + armedAfterDelivery = 0 defaults.removeObject(forKey: key) } } diff --git a/Yattee/Views/Settings/DeveloperSettingsView.swift b/Yattee/Views/Settings/DeveloperSettingsView.swift index 39681fdc..86a89521 100644 --- a/Yattee/Views/Settings/DeveloperSettingsView.swift +++ b/Yattee/Views/Settings/DeveloperSettingsView.swift @@ -202,6 +202,10 @@ struct DeveloperSettingsView: View { } label: { Label(String(localized: "settings.advanced.data.resetICloud"), systemImage: "icloud.slash") } + // The reset promises the local data comes back. With sync off + // nothing would put it there, so the offer is withdrawn rather + // than made and broken. + .disabled(appEnvironment?.cloudKitSync.isSyncEnabled != true) Spacer() } } diff --git a/YatteeTests/CloudKitZoneRecoveryTests.swift b/YatteeTests/CloudKitZoneRecoveryTests.swift index cd333689..f6e41fba 100644 --- a/YatteeTests/CloudKitZoneRecoveryTests.swift +++ b/YatteeTests/CloudKitZoneRecoveryTests.swift @@ -13,10 +13,27 @@ import Testing @Suite("CloudKit Zone Recovery") @MainActor struct CloudKitZoneRecoveryTests { - private func makeDefaults() -> UserDefaults { - UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + /// A defaults suite of its own per test, removed again when the test ends. + /// + /// A named suite outlives the process that made it, so leaving them behind + /// accumulates one plist per test run in the simulator's preferences. + private final class TemporaryDefaults { + let suiteName = "yattee.tests.\(UUID().uuidString)" + let defaults: UserDefaults + + init() { + defaults = UserDefaults(suiteName: suiteName)! + } + + deinit { + defaults.removePersistentDomain(forName: suiteName) + } } + private let temporary = TemporaryDefaults() + + private func makeDefaults() -> UserDefaults { temporary.defaults } + // MARK: - Reason → behaviour @Suite("Deletion reason") @@ -36,6 +53,18 @@ struct CloudKitZoneRecoveryTests { func encryptedDataResetDiscards() { #expect(ZoneDeletionResponse(reason: .encryptedDataReset) == .discard) } + + @Test("A re-seed is redone until the engine has written it down") + func reseedWaitsForPersistence() { + #expect(ZoneDeletionResponse.reseed.retirement == .whenStatePersists) + } + + @Test("A purge is done as soon as it has dropped what it owed") + func discardRetiresImmediately() { + // Replaying it later would drop work registered *after* the purge + // was answered, which is not what a purge asked for. + #expect(ZoneDeletionResponse.discard.retirement == .immediately) + } } // MARK: - Re-seed composition @@ -55,6 +84,17 @@ struct CloudKitZoneRecoveryTests { } } + // MARK: - Delivery counter + + @Test("Deliveries are numbered from one, and never repeat") + func deliveriesAreNumbered() { + let counter = ZoneRecoveryDeliveryCounter() + #expect(counter.current == 0) + #expect(counter.next() == 1) + #expect(counter.next() == 2) + #expect(counter.current == 2) + } + // MARK: - Marker lifecycle @Test("Nothing is pending until a deletion is recorded") @@ -80,11 +120,36 @@ struct CloudKitZoneRecoveryTests { marker.begin(.reseed) // The engine serializes its state for all sorts of reasons; one that // arrives before the re-seed is registered must leave the marker alone. - marker.stateDidPersist() + marker.stateDidPersist(deliveredAt: 1) #expect(marker.pending == .reseed) - marker.readyToClear() - marker.stateDidPersist() + marker.readyToClear(afterDelivery: 1) + marker.stateDidPersist(deliveredAt: 2) + #expect(marker.pending == nil) + } + + @Test("A state update already on its way when the work was registered does not retire it") + func stateUpdateInFlightDoesNotRetire() { + let defaults = makeDefaults() + let marker = CloudKitZoneRecoveryMarker(defaults: defaults) + let deliveries = ZoneRecoveryDeliveryCounter() + + marker.begin(.reseed) + + // The engine hands over an update — its serialization is taken now, + // before the recovery registers anything. + let inFlight = deliveries.next() + + // The recovery registers its work and arms the marker. The update above + // has still not reached the main actor. + marker.readyToClear(afterDelivery: deliveries.current) + + // ...and now it does. It cannot possibly carry the work. + marker.stateDidPersist(deliveredAt: inFlight) + #expect(marker.pending == .reseed) + + // The next one can. + marker.stateDidPersist(deliveredAt: deliveries.next()) #expect(marker.pending == nil) } @@ -100,12 +165,12 @@ struct CloudKitZoneRecoveryTests { // A state update in the new process, before the recovery is redone, // must not retire the marker it inherited. - relaunched.stateDidPersist() + relaunched.stateDidPersist(deliveredAt: 1) #expect(relaunched.pending == .reseed) relaunched.begin(.reseed) - relaunched.readyToClear() - relaunched.stateDidPersist() + relaunched.readyToClear(afterDelivery: 1) + relaunched.stateDidPersist(deliveredAt: 2) #expect(relaunched.pending == nil) } @@ -114,27 +179,74 @@ struct CloudKitZoneRecoveryTests { let defaults = makeDefaults() let marker = CloudKitZoneRecoveryMarker(defaults: defaults) - marker.begin(.discard) - marker.readyToClear() - marker.stateDidPersist() - marker.stateDidPersist() + marker.begin(.reseed) + marker.readyToClear(afterDelivery: 0) + marker.stateDidPersist(deliveredAt: 1) + marker.stateDidPersist(deliveredAt: 2) #expect(marker.pending == nil) } + @Test("A purge is retired the moment it is answered, without waiting for the engine") + func discardRetiresWithoutStatePersistence() { + let defaults = makeDefaults() + let marker = CloudKitZoneRecoveryMarker(defaults: defaults) + + marker.begin(.discard) + marker.retireNow() + + // No state update was needed, and a relaunch finds nothing to replay — + // which is the point: replaying it would drop whatever was registered + // after the purge was answered. + #expect(marker.pending == nil) + #expect(CloudKitZoneRecoveryMarker(defaults: defaults).pending == nil) + } + + @Test("Retiring a purge outright does not arm it for a later state update") + func retiringNowLeavesNothingArmed() { + let defaults = makeDefaults() + let marker = CloudKitZoneRecoveryMarker(defaults: defaults) + + marker.begin(.reseed) + marker.readyToClear(afterDelivery: 0) + marker.retireNow() + #expect(marker.pending == nil) + + // A second wipe, unanswered so far, must not be retired by a state + // update that the first one armed. + marker.begin(.discard) + marker.stateDidPersist(deliveredAt: 99) + #expect(marker.pending == .discard) + } + @Test("A fresh deletion is not retired by the previous one's state update") func newDeletionRearms() { let defaults = makeDefaults() let marker = CloudKitZoneRecoveryMarker(defaults: defaults) marker.begin(.reseed) - marker.readyToClear() + marker.readyToClear(afterDelivery: 0) // A second wipe arrives before the first one's state was serialized. marker.begin(.discard) - marker.stateDidPersist() + marker.stateDidPersist(deliveredAt: 1) #expect(marker.pending == .discard) } + @Test("The marker is forgotten when the Apple account moves, not carried across") + func forgettingDropsThePendingResponse() { + let defaults = makeDefaults() + let marker = CloudKitZoneRecoveryMarker(defaults: defaults) + + marker.begin(.discard) + // What an iCloud account change does: the decision named the zone of + // the identity being left, and answering it under the next one's + // namespace would drop that identity's staged accounts and outbox. + marker.forget() + + #expect(marker.pending == nil) + #expect(CloudKitZoneRecoveryMarker(defaults: defaults).pending == nil) + } + @Test("A marker written by a build that meant something else is ignored") func unknownMarkerValueIsIgnored() { let defaults = makeDefaults() @@ -176,4 +288,119 @@ struct CloudKitZoneRecoveryTests { #expect(relaunched.pendingUploads.isEmpty) #expect(relaunched.account(id: account.id) != nil) } + + // MARK: - The coordinator's half + + private struct CoordinatorHarness { + let accounts: AccountsManager + let store: AccountSyncStore + let coordinator: AccountSyncCoordinator + let namespace: String + } + + /// A coordinator whose namespace may be steered away from the one the + /// accounts were loaded under — the reload gap, which is the state both + /// account-side fences exist for. + private func makeCoordinatorHarness( + namespace: String, + loadedAs loaded: String = "identity-A" + ) -> CoordinatorHarness { + let defaults = makeDefaults() + let accounts = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: loaded), + instancesManager: nil, + defaults: defaults + ) + let store = AccountSyncStore(defaults: defaults) + store.markBootstrapCompleted(in: namespace) + + let coordinator = AccountSyncCoordinator( + accountsManager: accounts, + namespaceProvider: { namespace }, + store: store, + isCategoryEnabled: { true } + ) + coordinator.attach(database: nil) { _ in } + + return CoordinatorHarness( + accounts: accounts, + store: store, + coordinator: coordinator, + namespace: namespace + ) + } + + private func makeStagedRecord(named name: String) -> CKRecord { + CKRecord( + recordType: RecordType.account, + recordID: CKRecord.ID(recordName: name, zoneID: RecordType.createZone().zoneID) + ) + } + + @Test("A purge answered during the reload gap destroys nothing") + func discardIsFencedBeforeItDestroys() { + // The identity moved but the accounts in memory are still the previous + // one's — the outbox and the staging both belong to somebody else. + let harness = makeCoordinatorHarness(namespace: "identity-B", loadedAs: "identity-A") + harness.store.stage( + records: [makeStagedRecord(named: "account-staged")], + deletions: [], + in: harness.namespace + ) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Main") + #expect(harness.accounts.pendingUploads.contains(account.id)) + + #expect(harness.coordinator.discardWhatTheZoneHeld() == false) + + // Refused, and — the point of the fence order — refused before + // anything was dropped. + #expect(!harness.store.stagedChanges(in: harness.namespace).isEmpty) + #expect(harness.accounts.pendingUploads.contains(account.id)) + } + + @Test("A purge answered with the accounts loaded drops the staging and the outbox") + func discardClearsWhenTheAccountsAreReadable() { + let harness = makeCoordinatorHarness(namespace: "identity-A", loadedAs: "identity-A") + harness.store.stage( + records: [makeStagedRecord(named: "account-staged")], + deletions: [], + in: harness.namespace + ) + let account = harness.accounts.addAccount(instanceID: UUID(), displayName: "Main") + + #expect(harness.coordinator.discardWhatTheZoneHeld() == true) + + #expect(harness.store.stagedChanges(in: harness.namespace).isEmpty) + #expect(harness.accounts.pendingUploads.isEmpty) + // The account itself is this device's own, and stays. + #expect(harness.accounts.account(id: account.id) != nil) + } + + @Test("A re-seed refused during the reload gap says so") + func uploadEverythingIsRefusedDuringTheReloadGap() { + let harness = makeCoordinatorHarness(namespace: "identity-B", loadedAs: "identity-A") + #expect(harness.coordinator.uploadEverything() == false) + } + + @Test("The accounts becoming readable is announced, so a refused recovery can be retried") + func readableAccountsAreAnnounced() { + let harness = makeCoordinatorHarness(namespace: "identity-A", loadedAs: "identity-A") + var announcements = 0 + harness.coordinator.onAccountsBecameReadable = { announcements += 1 } + + NotificationCenter.default.post(name: .accountsDidChange, object: nil) + // At least: the notification is global, and a test running beside this + // one can post it too. + #expect(announcements >= 1) + } + + @Test("Nothing is announced while the accounts belong to another identity") + func unreadableAccountsAreNotAnnounced() { + let harness = makeCoordinatorHarness(namespace: "identity-B", loadedAs: "identity-A") + var announcements = 0 + harness.coordinator.onAccountsBecameReadable = { announcements += 1 } + + NotificationCenter.default.post(name: .accountsDidChange, object: nil) + #expect(announcements == 0) + } }