From 5abd314c88ce6d8a5469e720eac5a1efb4eb0e22 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Sat, 5 Sep 2026 23:30:40 +0200 Subject: [PATCH] Let an incoming context settle what a repair cannot argue with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A repair is a fallback, not a choice: it borrows the timestamp of the selection it replaced so that a later choice still beats it, and it is never published. But that borrowed rank also won arguments it had no business winning. An incoming explicit context stamped at the same instant that loses the `DataContext.tieBreakKey` tie was neither adopted — the repair was not outranked — nor pushed back, because a repair pushes nothing. Neither side moved, and the fallback stood against the server's explicit value indefinitely. The tie is not theoretical: migrated contexts are all stamped at the epoch, and a repair keeps the timestamp of whatever it replaced, so two migrated-era values meet as equals. A stored repair now never outranks an incoming context — it is replaced by whatever arrived, older or tied, and nothing is ever enqueued for that. Adopting is safe whatever the incoming value names: one naming an account a tombstone dominates has already been turned back into a repair before the comparison, so it stays unpublished and lands on the same target this device's repair did. Two more from the same trace: - Staged records are applied by category, and contexts already come last so that a repair reads the accounts and the instance a dominated record left behind rather than falling to local past a signed-in sibling. That ordering was load-bearing and unremarked; it is now written down, along with what it does not cover — a context applied in an earlier batch than the account record that says where the deleted account lived still repairs without it, which the next explicit choice anywhere settles. - An install that repaired its context before the repair bit existed reads back as having chosen it, so that one repair can still be published once. Accepted and documented at the load site: no shipped release ever persisted a data context, the window is interim builds only, and the next explicit choice heals it. --- Yattee/Core/AccountsManager.swift | 45 ++++-- .../CloudKit/AccountSyncCoordinator.swift | 17 ++- YatteeTests/AccountSyncTests.swift | 36 ++++- YatteeTests/AccountsManagerTests.swift | 143 +++++++++++++++++- 4 files changed, 219 insertions(+), 22 deletions(-) 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()