From b67b3472f6d5375924a965745500073b349178d3 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Wed, 9 Sep 2026 23:59:59 +0200 Subject: [PATCH] Let a purge be answered once and a re-seed be answered until it sticks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two answers to a deleted zone were sharing one lifecycle, and only one of them deserved it. A re-seed only adds, so it is held open until the engine has actually serialized the work and redone for as long as it has not. A purge removes, and replaying it removes more than it was asked to — the account added while the app kept running, the category switched back on, the reset that runs its own upload afterwards. It is done the moment its durable half is, and the comment that called both idempotent now says which is which. The note itself was never scoped to an identity. A marker written under one Apple account replayed against the next one's namespace, answering a wipe that never happened there, and a purge would have taken that account's staged records and its outbox with it. The account change forgets it now, the way it forgets everything else it holds. Retiring the marker trusted whichever state update arrived next, which is not the same as one that carries the work: an update handed over before the recovery registered anything can still reach the main actor after it, describing a state in which none of it exists. Deliveries are counted where the engine hands them over, and only a later one can retire what was armed. A refused re-seed waited for the next launch. The accounts it could not speak for announce themselves when they load, and the recovery is answered then — outside the callback tree that announcement comes from. A purge left two ways back for what it dropped: a pending zone save would have recreated the zone on the next send, and the pre-engine pending-change keys would have been rediscovered on a later launch. It clears both, and it now runs after everything that puts work into the engine rather than before. The account half fences before it destroys, like its upload twin, so a purge arriving during the identity reload gap answers with nothing rather than with somebody else's records. And the reset that promises a re-upload is refused while sync is off, where nothing would have performed one. Re-entering the engine from inside its own callback is what started all of this, and nothing in the code said so. A task-local marks the callback tree and the two entry points assert on it in debug builds — a plain Task inherits it, which is the mistake; Task.detached escapes it, which is the fix. --- .../CloudKit/AccountSyncCoordinator.swift | 33 ++- .../CloudKit/CloudKitSyncEngine.swift | 231 +++++++++++++--- .../CloudKit/CloudKitZoneRecovery.swift | 114 +++++++- .../Settings/DeveloperSettingsView.swift | 4 + YatteeTests/CloudKitZoneRecoveryTests.swift | 255 +++++++++++++++++- 5 files changed, 576 insertions(+), 61 deletions(-) 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) + } }