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")