diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 9cff68aa..a6002c20 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -82,8 +82,10 @@ final class AccountsManager { /// decides an older server copy has to be corrected. Without this the /// fallback would be pushed back over a deliberate choice another device /// made, which is exactly what not stamping it "now" is meant to prevent. - /// Persisted beside the context, or a relaunch would forget and publish it - /// on the next stale record it saw. + /// It is also what makes a repair yield to any context that arrives, rather + /// than winning on that borrowed rank and standing against the server + /// forever. Persisted beside the context, or a relaunch would forget and + /// publish it on the next stale record it saw. private(set) var storedContextIsRepair = false /// Instances the user has deleted here. @@ -719,18 +721,30 @@ final class AccountsManager { return false } + // A repair never outranks a context somebody chose. It borrowed the + // replaced selection's timestamp only so that a later choice still beats + // it — that borrowed rank was never meant to win an argument with an + // explicit value, by being newer or by settling a tie on + // `DataContext.tieBreakKey`. Leaving it in place instead would keep a + // local fallback standing against the server's value indefinitely, since + // a repair is also never pushed back: nothing would ever resolve the + // disagreement. Migrated contexts make the tie real rather than + // theoretical — they are all stamped at the epoch. + // + // Adopting is safe whatever the incoming value says: one naming an + // account that is gone has already been turned back into a repair above, + // so it stays unpublished and lands on the same target this repair did. + if storedContextIsRepair { + adoptSyncedContext(incoming, wasRepaired: wasRepaired) + return false + } + guard storedContext.isOutranked(by: incoming) else { - // The local choice is the newer one and survives untouched — a - // repair that lost the ordering never gets to clear what that - // choice still owes the server. + // The local choice is the newer one and survives untouched, keeping + // whatever it still owes the server. guard incoming.isOutranked(by: storedContext) else { return false } - // Newer, but not a choice. A repair carries the replaced selection's - // timestamp, so it outranks the server's older explicit value while - // having nothing of its own to say; publishing it would push an - // automatic fallback over that value. Nothing is uploaded here: the - // server's context stands remotely, the repair stays local, and the - // next explicit choice on any device re-converges everyone. - guard !storedContextIsRepair else { return false } + // The server holds the older copy of a deliberate choice, so it has + // to be pushed back over it. pendingContextUpload = true save() return true @@ -1095,6 +1109,13 @@ final class AccountsManager { } pendingContextUpload = defaults.bool(forKey: key("pendingContextUpload")) + // An install that repaired its context before this key existed reads + // back as "chosen", so that one repair can still be marked dirty by an + // older incoming context and published once. Accepted: no shipped + // release ever persisted a data context — the whole revamp ships as a + // single release — so the window covers only interim development and + // TestFlight builds, it is one-shot, and the next explicit choice + // heals it. storedContextIsRepair = defaults.bool(forKey: key("contextIsRepair")) purgeUnresolvedNamespace() diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index 8eb9e534..a3888ccc 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -410,9 +410,24 @@ final class AccountSyncCoordinator { isApplyingStagedChanges = true defer { isApplyingStagedChanges = false } - // Tombstone-first. A deletion that arrived in a later event than the + // Applied by category rather than in the order the records happened to + // be fetched, because each category depends on the ones before it. + // + // 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. + // + // Contexts last: repairing a context naming a deleted account reads the + // accounts and the instance a dominated record left behind, and a repair + // evaluated before that state exists lands on local past a signed-in + // sibling on the same server, with nothing to re-run it afterwards. + // + // Only within a batch, which is what a fetch delivers and what a + // deferred activation accumulates. A context applied in an *earlier* + // batch than the account record that says where the deleted account + // lived still repairs without it; the next explicit choice anywhere + // settles that, and re-repairing on every later record would undo + // choices made in between. let tombstones = staged.filter { change in switch AccountRecordName.parse(change.recordName) { case .accountDeletion: return change.kind == .save diff --git a/YatteeTests/AccountSyncTests.swift b/YatteeTests/AccountSyncTests.swift index 800cf595..e2520da4 100644 --- a/YatteeTests/AccountSyncTests.swift +++ b/YatteeTests/AccountSyncTests.swift @@ -723,7 +723,9 @@ struct AccountSyncTests { harness.recorder.clear() await harness.coordinator.activate() - #expect(harness.accounts.currentContext == .account(sibling.id)) + // The repair yields to the choice the server holds rather than pushing a + // fallback nobody made over it — and yielding sends nothing either. + #expect(harness.accounts.currentContext == .local) #expect(!harness.recorder.savedNames.contains("datacontext-current")) #expect(harness.coordinator.recordToSave(named: "datacontext-current") == nil) } @@ -761,6 +763,38 @@ struct AccountSyncTests { #expect(harness.accounts.currentContext == .account(sibling.id)) } + @Test("A context fetched ahead of the records it depends on is still applied last") + func stagedContextIsAppliedAfterTheAccountRecords() async { + let instances = InstancesManager() + let instance = Instance(type: .invidious, url: URL(string: "https://inv-\(UUID().uuidString).example.com")!) + instances.add(instance) + defer { instances.remove(instance) } + + let harness = Self.makeHarness(instances: instances) + let sibling = harness.accounts.addAccount(instanceID: instance.id) + harness.accounts.setStatus(.signedIn, for: sibling.id) + + // The order records come back in is CloudKit's, not ours: the same three + // as above, with the context enumerated first. Applied in that order the + // repair would run before anything knew the deleted account existed, let + // alone which server it lived on, and land on local past the sibling. + let deleted = Account(instanceID: instance.id) + harness.coordinator.stageFetched( + records: [ + harness.mapper.toCKRecord( + dataContext: StoredDataContext(context: .account(deleted.id), changedAt: Date()) + ), + harness.mapper.toCKRecord(account: deleted), + harness.mapper.toCKRecord(accountDeletionID: deleted.id) + ], + deletions: [] + ) + await harness.coordinator.activate() + + #expect(harness.accounts.deletedAccountInstanceIDs[deleted.id] == instance.id) + #expect(harness.accounts.currentContext == .account(sibling.id)) + } + @Test("Records fetched before the identity resolves are recovered by a bootstrap") func unresolvedIdentityForcesAnotherBootstrap() async { let defaults = Self.makeDefaults() diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 27aab0b6..490f8db4 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -981,26 +981,153 @@ struct DataContextRepairTests { let older = StoredDataContext(context: .local, changedAt: chosenAt.addingTimeInterval(-60)) #expect(!manager.applySyncedContext(older)) - // The repair only outranks the server's copy because it inherited that + // The repair only outranked the server's copy because it inherited that // timestamp. Publishing it would put a fallback nobody chose over an - // explicit choice — the server's context stands, and the next real - // selection anywhere re-converges everyone. - #expect(manager.currentContext == .account(sibling.id)) - #expect(manager.storedContext.changedAt == chosenAt) + // explicit choice, so the explicit value is taken as it stands instead — + // and nothing is sent. + #expect(manager.currentContext == .local) + #expect(manager.storedContext.changedAt == older.changedAt) #expect(!manager.pendingContextUpload) - // Still a repair after a relaunch, or the next stale record read from - // disk would publish it. + // What was adopted is a choice, so it publishes like one when this + // device later has the newer copy. let reloaded = makeManager(instances: instances, defaults: defaults) #expect(!reloaded.applySyncedContext(older)) #expect(!reloaded.pendingContextUpload) - // Choosing it deliberately is a different thing entirely, and publishes. reloaded.setContext(.account(sibling.id)) #expect(reloaded.applySyncedContext(older)) #expect(reloaded.pendingContextUpload) } + @Test("A repair read back from disk still yields rather than publishing") + func reloadedRepairStillYieldsToTheServersContext() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let defaults = UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + let manager = makeManager(instances: instances, defaults: defaults) + let selected = manager.addAccount(instanceID: instance.id) + let sibling = manager.addAccount(instanceID: instance.id) + manager.setStatus(.signedIn, for: sibling.id) + + manager.setContext(.account(selected.id)) + let chosenAt = manager.storedContext.changedAt + manager.confirmContextUpload(changedAt: chosenAt) + manager.applyRemoteTombstone(selected.id) + #expect(manager.currentContext == .account(sibling.id)) + + // Relaunched before any record arrives: without the repair surviving on + // disk this reads as a choice, and the stale record below would be + // pushed back over rather than adopted. + let reloaded = makeManager(instances: instances, defaults: defaults) + reloaded.setStatus(.signedIn, for: sibling.id) + #expect(reloaded.currentContext == .account(sibling.id)) + + let older = StoredDataContext(context: .local, changedAt: chosenAt.addingTimeInterval(-60)) + #expect(!reloaded.applySyncedContext(older)) + + #expect(reloaded.currentContext == .local) + #expect(!reloaded.pendingContextUpload) + } + + @Test("A repair yields to an explicit context it ties with") + func repairYieldsToAnExplicitContextOnATie() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let manager = makeManager(instances: instances) + let selected = manager.addAccount(instanceID: instance.id) + let sibling = manager.addAccount(instanceID: instance.id) + manager.setStatus(.signedIn, for: sibling.id) + + manager.setContext(.account(selected.id)) + let chosenAt = manager.storedContext.changedAt + manager.confirmContextUpload(changedAt: chosenAt) + manager.applyRemoteTombstone(selected.id) + #expect(manager.currentContext == .account(sibling.id)) + + // Exactly the same instant — which two migrated contexts always are, + // both being stamped at the epoch. The tie-break settles it against the + // incoming value, so the repair is neither outranked nor pushed back and + // would otherwise stand against the server's choice forever. + let tied = StoredDataContext(context: .local, changedAt: chosenAt) + #expect(!manager.applySyncedContext(tied)) + + #expect(manager.currentContext == .local) + #expect(!manager.pendingContextUpload) + } + + @Test("A repair yields to an older context naming an account that is still there") + func repairYieldsToAnOlderContextNamingALiveAccount() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let manager = makeManager(instances: instances) + let selected = manager.addAccount(instanceID: instance.id) + let sibling = manager.addAccount(instanceID: instance.id) + let other = manager.addAccount(instanceID: instance.id) + manager.setStatus(.signedIn, for: sibling.id) + + manager.setContext(.account(selected.id)) + let chosenAt = manager.storedContext.changedAt + manager.confirmContextUpload(changedAt: chosenAt) + manager.applyRemoteTombstone(selected.id) + #expect(manager.currentContext == .account(sibling.id)) + + let older = StoredDataContext( + context: .account(other.id), + changedAt: chosenAt.addingTimeInterval(-60) + ) + #expect(!manager.applySyncedContext(older)) + + // Somebody chose that account; the sibling was picked by this device's + // own statuses. Adopting it is what re-converges the devices, and it is + // still not a reason to send anything. + #expect(manager.currentContext == .account(other.id)) + #expect(!manager.pendingContextUpload) + } + + @Test("An older context naming the deleted account repairs again rather than landing on it") + func repairYieldsToAnOlderContextNamingTheDeletedAccount() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let manager = makeManager(instances: instances) + let selected = manager.addAccount(instanceID: instance.id) + let sibling = manager.addAccount(instanceID: instance.id) + manager.setStatus(.signedIn, for: sibling.id) + + manager.setContext(.account(selected.id)) + let chosenAt = manager.storedContext.changedAt + manager.confirmContextUpload(changedAt: chosenAt) + manager.applyRemoteTombstone(selected.id) + + // The server still holds the choice the tombstone killed. Yielding to it + // means yielding to what it repairs to, so the result is the repair + // again — and it is still a repair, so it still never publishes. + let older = StoredDataContext( + context: .account(selected.id), + changedAt: chosenAt.addingTimeInterval(-60) + ) + #expect(!manager.applySyncedContext(older)) + + #expect(manager.currentContext == .account(sibling.id)) + // Yielded rather than held onto: the borrowed timestamp goes back to + // what the server holds, so this device stops out-ranking it. + #expect(manager.storedContext.changedAt == older.changedAt) + #expect(manager.storedContextIsRepair) + #expect(!manager.pendingContextUpload) + } + @Test("A repair recomputes statuses before it picks a sibling") func repairRefreshesStatusesBeforeChoosing() { let instances = InstancesManager()