From afc016c5ced21eaa54c8bcc4ad073e773fd25d6a Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Thu, 3 Sep 2026 23:58:42 +0200 Subject: [PATCH] Repair a deleted account's context onto its sibling login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting the selected account dropped the user to local data even when the same server held another login they were signed in to — usually the only other thing they could have meant. The repair now prefers a signed-in account on the deleted one's instance, and the synced-tombstone branch of applySyncedContext does the same, so a choice arriving from a device that had not heard about the deletion lands there too. The instance of a deleted account is recorded beside the tombstone: by the time a context is repaired the account record itself is gone, and the sibling cannot be found without it. The repair keeps the replaced selection's timestamp, clears any queued upload and is still never published. That is what makes picking by status safe: statuses are device-local, so two devices can repair the same deletion differently, and neither overwrites the other. --- Yattee/Core/AccountsManager.swift | 84 +++++++++-- YatteeTests/AccountsManagerTests.swift | 195 ++++++++++++++++++++++++- 2 files changed, 267 insertions(+), 12 deletions(-) diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 4c2fbe70..c0521816 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -89,6 +89,14 @@ final class AccountsManager { /// rather than enumerating the zone. private(set) var deletedAccountIDs: Set = [] + /// Which instance each deleted account lived on. + /// + /// Kept beside the tombstones for one purpose: repairing a context that + /// names a deleted account looks for another login on the same server, and + /// by then the account record itself is gone. Device-local and never + /// published, like the statuses that repair consults. + private(set) var deletedAccountInstanceIDs: [UUID: UUID] = [:] + var currentContext: DataContext { storedContext.context } // MARK: - Initialization @@ -271,7 +279,7 @@ final class AccountsManager { } if storedContext.context == .account(accountID) { - correctContextAfterDeletion() + correctContextAfterDeletion(onInstance: account.instanceID) } save() @@ -286,16 +294,21 @@ final class AccountsManager { /// which is what stops a stale device's queued edit from resurrecting the /// account here. private func forgetAccount(_ accountID: UUID) { + let instanceID = account(id: accountID)?.instanceID + accounts.removeAll { $0.id == accountID } accountStatuses[accountID] = nil pendingUploads.remove(accountID) deletedAccountIDs.insert(accountID) + if let instanceID { + deletedAccountInstanceIDs[accountID] = instanceID + } // Selecting a deleted account would leave the UI pointing at nothing. // A deletion the app can point at is one of the few things allowed to // correct the context. if storedContext.context == .account(accountID) { - correctContextAfterDeletion() + correctContextAfterDeletion(onInstance: instanceID) } } @@ -305,11 +318,19 @@ final class AccountsManager { /// it replaces and is never published. Stamping it "now" would let an /// automatic fallback outrank a deliberate choice another device made /// between the deletion and this device hearing about it — and publishing - /// it would push that fallback over their choice. Every device that sees - /// the deletion performs the same repair, so they converge without anyone - /// writing anything. - private func correctContextAfterDeletion() { - storedContext = StoredDataContext(context: .local, changedAt: storedContext.changedAt) + /// it would push that fallback over their choice. + /// + /// The devices no longer converge on the same *value* the way they did when + /// this always chose local: a sibling is picked by status, and statuses are + /// device-local, so a device that has no credential for the sibling repairs + /// to local while another lands on the sibling. That is acceptable precisely + /// because the repair is never uploaded — nothing overwrites anybody — and + /// the next explicit choice re-converges everyone. + private func correctContextAfterDeletion(onInstance instanceID: UUID?) { + storedContext = StoredDataContext( + context: repairTarget(onInstance: instanceID), + changedAt: storedContext.changedAt + ) // Including any upload the deleted choice had queued: sending it now // would publish this fallback under that choice's timestamp, which is // exactly what not stamping it "now" is meant to prevent. @@ -317,6 +338,28 @@ final class AccountsManager { NotificationCenter.default.post(name: .dataContextDidChange, object: nil) } + /// Where a context whose account was deleted should land. + /// + /// Another signed-in login on the same server is far closer to what the user + /// was doing than their local data, and on an instance holding two logins it + /// is usually the only other thing they meant. Only `signedIn` qualifies: a + /// sibling this device cannot use would repair the context onto something + /// that immediately falls back to local anyway, without the user's choice + /// surviving anywhere. + private func repairTarget(onInstance instanceID: UUID?) -> DataContext { + guard let instanceID else { return .local } + + let sibling = accounts(forInstance: instanceID) + .filter { status(for: $0.id) == .signedIn } + .filter { capabilities(for: $0)?.isSelectableContext == true } + // Ordered so the answer does not depend on which order the accounts + // happened to arrive in, the same way a context tie is broken. + .min { $0.id.uuidString < $1.id.uuidString } + + guard let sibling else { return .local } + return .account(sibling.id) + } + /// Marks a deletion as confirmed by the server. Until then it stays in the /// outbox and is retried, because a tombstone that never lands lets a stale /// device resurrect the account. @@ -540,9 +583,15 @@ final class AccountsManager { // publish that choice with a newer timestamp; adopting it would leave // every device pointing at an account that is never coming back. The // choice's own timestamp is kept, so a later real selection still wins, - // and every device that holds the tombstone repairs it identically. + // and the repair is never published — a device that repairs to something + // else does not overwrite anybody. if let accountID = incoming.context.accountID, deletedAccountIDs.contains(accountID) { - incoming = StoredDataContext(context: .local, changedAt: incoming.changedAt) + // Repaired the same way a local deletion is: onto a signed-in login + // on the same server when there is one, local otherwise. + incoming = StoredDataContext( + context: repairTarget(onInstance: deletedAccountInstanceIDs[accountID]), + changedAt: incoming.changedAt + ) LoggingService.shared.info( "Ignoring a synced context that names a deleted account", @@ -902,6 +951,14 @@ final class AccountsManager { deletedInstanceIDs = Set(ids.compactMap(UUID.init(uuidString:))) } + if let pairs = defaults.dictionary(forKey: key("deletedAccountInstances")) as? [String: String] { + deletedAccountInstanceIDs = pairs.reduce(into: [:]) { result, pair in + guard let accountID = UUID(uuidString: pair.key), + let instanceID = UUID(uuidString: pair.value) else { return } + result[accountID] = instanceID + } + } + pendingContextUpload = defaults.bool(forKey: key("pendingContextUpload")) purgeUnresolvedNamespace() @@ -911,7 +968,7 @@ final class AccountsManager { private static let storedKeyNames = [ "accounts", "context", "hasContext", "migrated", "pendingUploads", "pendingDeletions", "pendingMarkerUploads", "deletedAccounts", - "deletedInstances", "pendingContextUpload" + "deletedInstances", "deletedAccountInstances", "pendingContextUpload" ] /// Deletes whatever was written while the identity was still unresolved. @@ -962,6 +1019,12 @@ final class AccountsManager { defaults.set(pendingMarkerUploads.map(\.uuidString), forKey: key("pendingMarkerUploads")) defaults.set(deletedAccountIDs.map(\.uuidString), forKey: key("deletedAccounts")) defaults.set(deletedInstanceIDs.map(\.uuidString), forKey: key("deletedInstances")) + defaults.set( + deletedAccountInstanceIDs.reduce(into: [String: String]()) { result, pair in + result[pair.key.uuidString] = pair.value.uuidString + }, + forKey: key("deletedAccountInstances") + ) defaults.set(pendingContextUpload, forKey: key("pendingContextUpload")) } @@ -985,6 +1048,7 @@ final class AccountsManager { pendingMarkerUploads = [] deletedAccountIDs = [] deletedInstanceIDs = [] + deletedAccountInstanceIDs = [:] pendingContextUpload = false load() diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 2f05d1c0..7dca3408 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -509,12 +509,13 @@ struct AccountsManagerTests { @MainActor struct DataContextValidatorTests { private func makeValidator( - instances: InstancesManager + instances: InstancesManager, + defaults: UserDefaults? = nil ) -> (AccountsManager, DataContextValidator) { let accounts = AccountsManager( identityScope: CredentialIdentityScope(fixedScope: "identity-A"), instancesManager: instances, - defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + defaults: defaults ?? UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! ) let validator = DataContextValidator( accountsManager: accounts, @@ -551,6 +552,196 @@ struct DataContextValidatorTests { instances.remove(instance) } + + @Test("Only an account that is genuinely gone moves the context") + func correctionRunsOnlyOnAbsence() { + let instances = InstancesManager() + var instance = Instance(type: .invidious, url: URL(string: "https://inv-correct.example.com")!) + instances.add(instance) + defer { instances.remove(instance) } + + let defaults = UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + let (accounts, validator) = makeValidator(instances: instances, defaults: defaults) + let account = accounts.addAccount(instanceID: instance.id) + accounts.setStatus(.signedIn, for: account.id) + accounts.setContext(.account(account.id)) + + // Unusable, but the account is still there — the user gets it back by + // switching the instance on again, so nothing may be corrected. + instance.isEnabled = false + instances.update(instance) + validator.correctIfContextRemoved() + #expect(accounts.currentContext == .account(account.id)) + + instance.isEnabled = true + instances.update(instance) + accounts.setStatus(.credentialMissing, for: account.id) + validator.correctIfContextRemoved() + #expect(accounts.currentContext == .account(account.id)) + + // An account the store has never heard of is the one case that counts. + accounts.setContext(.account(UUID())) + validator.correctIfContextRemoved() + #expect(accounts.currentContext == .local) + + // Persisted, not merely held in memory: the correction has to survive + // the relaunch that would otherwise re-read the dead selection. + let (reloaded, _) = makeValidator(instances: instances, defaults: defaults) + #expect(reloaded.currentContext == .local) + } + + @Test("Only accounts the app can actually resolve are offered") + func availableContextsAreFiltered() { + let instances = InstancesManager() + let enabled = Instance(type: .invidious, url: URL(string: "https://inv-a.example.com")!) + let disabled = Instance(type: .invidious, url: URL(string: "https://inv-b.example.com")!, isEnabled: false) + // No account concept on PeerTube, so a context there could never be + // served from. + let unsupported = Instance(type: .peertube, url: URL(string: "https://pt.example.com")!) + for instance in [enabled, disabled, unsupported] { instances.add(instance) } + defer { for instance in [enabled, disabled, unsupported] { instances.remove(instance) } } + + let (accounts, validator) = makeValidator(instances: instances) + let usable = accounts.addAccount(instanceID: enabled.id) + _ = accounts.addAccount(instanceID: disabled.id) + _ = accounts.addAccount(instanceID: unsupported.id) + // An account whose instance has not synced yet has no known + // capabilities, which is not the same as having none. + _ = accounts.addAccount(instanceID: UUID()) + + #expect(validator.availableContexts == [.local, .account(usable.id)]) + } +} + +@Suite("Data context repair", .serialized) +@MainActor +struct DataContextRepairTests { + private func makeInstance(_ host: String) -> Instance { + Instance(type: .invidious, url: URL(string: "https://\(host)-\(UUID().uuidString).example.com")!) + } + + private func makeManager(instances: InstancesManager) -> AccountsManager { + AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: "identity-A"), + instancesManager: instances, + defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + ) + } + + @Test("Deleting the selected account falls back to a signed-in sibling") + func deletionPrefersSignedInSibling() { + 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.removeAccount(selected.id) + + #expect(manager.currentContext == .account(sibling.id)) + // A repair, not a choice: it keeps the replaced selection's timestamp + // and is never published. + #expect(manager.storedContext.changedAt == chosenAt) + #expect(!manager.pendingContextUpload) + } + + @Test("Without a signed-in sibling the repair lands on local") + func deletionWithoutSiblingFallsBackToLocal() { + 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) + // Present but not usable here — a context pointing at it would fall + // straight back to local data anyway. + let sibling = manager.addAccount(instanceID: instance.id) + manager.setStatus(.credentialMissing, for: sibling.id) + manager.setContext(.account(selected.id)) + + manager.removeAccount(selected.id) + + #expect(manager.currentContext == .local) + #expect(!manager.pendingContextUpload) + } + + @Test("A signed-in account on another server is never the repair") + func repairNeverCrossesInstances() { + let instances = InstancesManager() + let first = makeInstance("inv-first") + let second = makeInstance("inv-second") + instances.add(first) + instances.add(second) + defer { + instances.remove(first) + instances.remove(second) + } + + let manager = makeManager(instances: instances) + let selected = manager.addAccount(instanceID: first.id) + let elsewhere = manager.addAccount(instanceID: second.id) + manager.setStatus(.signedIn, for: elsewhere.id) + manager.setContext(.account(selected.id)) + + manager.removeAccount(selected.id) + + #expect(manager.currentContext == .local) + } + + @Test("A synced context naming a deleted account repairs to its sibling") + func syncedTombstoneRepairPrefersSibling() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let manager = makeManager(instances: instances) + let deleted = manager.addAccount(instanceID: instance.id) + let sibling = manager.addAccount(instanceID: instance.id) + manager.setStatus(.signedIn, for: sibling.id) + + manager.setContext(.local) + manager.removeAccount(deleted.id) + + // Another device selected the account before it heard about the + // deletion, and stamped that choice later than anything here. + let incoming = StoredDataContext( + context: .account(deleted.id), + changedAt: Date().addingTimeInterval(60) + ) + manager.applySyncedContext(incoming) + + #expect(manager.currentContext == .account(sibling.id)) + } + + @Test("A synced context for a deleted account with no sibling lands on local") + func syncedTombstoneRepairFallsBackToLocal() { + let instances = InstancesManager() + let instance = makeInstance("inv") + instances.add(instance) + defer { instances.remove(instance) } + + let manager = makeManager(instances: instances) + let deleted = manager.addAccount(instanceID: instance.id) + + manager.setContext(.local) + manager.removeAccount(deleted.id) + + let incoming = StoredDataContext( + context: .account(deleted.id), + changedAt: Date().addingTimeInterval(60) + ) + manager.applySyncedContext(incoming) + + #expect(manager.currentContext == .local) + } } @Suite("Account capabilities")