diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 182ebc1d..a6c032a0 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -224,6 +224,47 @@ final class AccountsManager { NotificationCenter.default.post(name: .accountsDidChange, object: nil) } + /// Undoes an account that was created moments ago and could not be + /// completed — a sign-in whose credential the Keychain refused to store. + /// + /// Not the same as deleting it. Creating an account also marks its instance + /// migrated and queues the permanent marker that stops every other device + /// from synthesizing an account for that instance's pre-revamp login. A + /// deletion is meant to leave that marker standing; a creation that never + /// happened must not, or a legacy login that is still perfectly valid would + /// be suppressed everywhere on the strength of an account nobody has. + func rollbackAccountCreation(_ accountID: UUID) { + guard let account = account(id: accountID) else { return } + + // Confirmed by the server already? Then it is a real account elsewhere, + // and undoing it means a real deletion, tombstone and all. + guard pendingUploads.contains(accountID) else { + removeAccount(accountID) + return + } + + accounts.removeAll { $0.id == accountID } + accountStatuses[accountID] = nil + pendingUploads.remove(accountID) + + // Retracted only while the marker is still ours to retract: never + // confirmed by the server, and nothing else on the instance relying on + // it. A tombstone for the migrated account keeps synthesis suppressed + // on its own, so nothing is resurrected by this. + if pendingMarkerUploads.contains(account.instanceID), + accounts(forInstance: account.instanceID).isEmpty { + pendingMarkerUploads.remove(account.instanceID) + migratedInstanceIDs.remove(account.instanceID) + } + + if storedContext.context == .account(accountID) { + correctContextAfterDeletion() + } + + save() + NotificationCenter.default.post(name: .accountsDidChange, object: nil) + } + /// Drops an account and everything keyed to it, and records the permanent /// local tombstone. /// diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 685eb326..b8155f6a 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -463,13 +463,14 @@ final class AppEnvironment { // the API layer records. It still runs the mirror, because a credential // arriving or being rejected moves the effective context. for name: Notification.Name in [ - .dataContextDidChange, .accountsDidChange, .instancesDidChange, .accountStatusesDidChange + .dataContextDidChange, .accountsDidChange, .instancesDidChange, + .accountStatusesDidChange, .connectivityDidReturn ] { contextMirrorObservers.append( NotificationCenter.default.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in MainActor.assumeIsolated { guard let self else { return } - if name == .accountsDidChange || name == .instancesDidChange { + if name != .dataContextDidChange, name != .accountStatusesDidChange { self.refreshAccountStatuses() } self.mirrorContextToLegacySelection() @@ -678,7 +679,11 @@ final class AppEnvironment { let defaults = UserDefaults.standard guard let owner = credentialIdentityScope.current else { - guard atLaunch, defaults.string(forKey: Self.cacheOwnerKey) != nil else { return } + // No recorded owner is not the same as "nothing to protect": the + // first launch after upgrading finds caches written before anyone + // recorded whose they are, which is exactly the case that must not + // be adopted. + guard atLaunch else { return } clearAccountScopedCaches() defaults.removeObject(forKey: Self.cacheOwnerKey) @@ -745,9 +750,13 @@ final class AppEnvironment { /// instead of the pre-revamp instance-keyed Keychain lookup, which a new /// login no longer writes. func account(forInstance instance: Instance) -> Account? { + // The selected account is preferred only when it can actually answer. + // Returning a credential-less one would report the whole instance as + // signed out while a sibling login sits right there, usable. if case let .account(id) = accountsManager.currentContext, let account = accountsManager.account(id: id), - account.instanceID == instance.id { + account.instanceID == instance.id, + accountCredentialsManager(for: account)?.isLoggedIn(for: account) == true { return account } diff --git a/Yattee/Services/HomeInstanceCache.swift b/Yattee/Services/HomeInstanceCache.swift index 3e62f1dd..a07e6cfc 100644 --- a/Yattee/Services/HomeInstanceCache.swift +++ b/Yattee/Services/HomeInstanceCache.swift @@ -28,6 +28,12 @@ final class HomeInstanceCache { /// entries that load carry the same purge. private var pendingFeedPurge = false + /// Which login the cached feeds belong to. Bumped whenever a feed is + /// dropped because the account behind it changed, so a request that was + /// already awaiting the server cannot write the previous user's videos back + /// in — clearing a cache does not cancel what is in flight. + private var accountGeneration = 0 + private init() {} // MARK: - Cache Key Management @@ -100,7 +106,8 @@ final class HomeInstanceCache { } let key = cacheKey(instanceID: instanceID, contentType: contentType) - + let generation = accountGeneration + LoggingService.shared.debug( "HomeInstanceCache.refresh: Fetching \(contentType.rawValue) from \(instance.displayName)", category: .general @@ -138,6 +145,14 @@ final class HomeInstanceCache { videos = try await appEnvironment.contentService.feed(for: instance, credential: credential) } + guard generation == accountGeneration else { + LoggingService.shared.info( + "Discarding a \(contentType.rawValue) result that belongs to the previous login", + category: .general + ) + return + } + // Update cache cache[key] = videos lastUpdated[key] = Date() @@ -170,6 +185,8 @@ final class HomeInstanceCache { /// Clears cached content for a specific instance and content type. func clear(instanceID: UUID, contentType: InstanceContentType) { + accountGeneration += 1 + let key = cacheKey(instanceID: instanceID, contentType: contentType) cache.removeValue(forKey: key) lastUpdated.removeValue(forKey: key) @@ -193,6 +210,8 @@ final class HomeInstanceCache { /// otherwise be served each other's. Popular and trending are public and /// stay. func clearAllFeeds() { + accountGeneration += 1 + // A purge can be asked for before the disk cache has been read — the // identity check runs at startup, the load is async. Dropping only what // is in memory would then be undone by the load, so the requirement is diff --git a/Yattee/Services/Navigation/ConnectivityMonitor.swift b/Yattee/Services/Navigation/ConnectivityMonitor.swift index d96174e4..12a5d248 100644 --- a/Yattee/Services/Navigation/ConnectivityMonitor.swift +++ b/Yattee/Services/Navigation/ConnectivityMonitor.swift @@ -42,7 +42,17 @@ final class ConnectivityMonitor: @unchecked Sendable { func startMonitoring() { monitor.pathUpdateHandler = { [weak self] path in Task { @MainActor [weak self] in + let wasOffline = self?.isOnline == false self?.isOnline = path.status == .satisfied + + // Anything that gave up while there was no network needs to + // hear that there is one again — an account marked unreachable + // stops reads from going out at all, so nothing else would ever + // discover it. + if wasOffline, path.status == .satisfied { + NotificationCenter.default.post(name: .connectivityDidReturn, object: nil) + } + self?.isCellular = path.usesInterfaceType(.cellular) self?.isExpensive = path.isExpensive self?.isConstrained = path.isConstrained @@ -56,3 +66,8 @@ final class ConnectivityMonitor: @unchecked Sendable { monitor.cancel() } } + +extension Notification.Name { + /// Posted when the device goes from offline to online. + static let connectivityDidReturn = Notification.Name("stream.yattee.connectivityDidReturn") +} diff --git a/Yattee/Services/SubscriptionService.swift b/Yattee/Services/SubscriptionService.swift index 93b9d641..1f2fa3a8 100644 --- a/Yattee/Services/SubscriptionService.swift +++ b/Yattee/Services/SubscriptionService.swift @@ -101,7 +101,7 @@ final class SubscriptionService { object: nil, queue: .main ) { [weak self] _ in - MainActor.assumeIsolated { self?.accountProviders.removeAll() } + MainActor.assumeIsolated { self?.discardAllProviders() } } } @@ -160,6 +160,17 @@ final class SubscriptionService { providerGeneration += 1 } + /// Drops every cached provider, for a change of Apple account. + /// + /// Bumping the generation is the half that matters: emptying the dictionary + /// only affects future lookups, while a fetch already awaiting a response + /// still holds its provider and would otherwise deliver the previous + /// identity's subscriptions. + func discardAllProviders() { + accountProviders.removeAll() + providerGeneration += 1 + } + /// Which generation of logins the cached providers belong to. Bumped /// whenever one is discarded, so a fetch that outlived its provider can be /// told from a current one. diff --git a/Yattee/Views/Settings/EditSourceView.swift b/Yattee/Views/Settings/EditSourceView.swift index 1118f3f2..bbec3d62 100644 --- a/Yattee/Views/Settings/EditSourceView.swift +++ b/Yattee/Views/Settings/EditSourceView.swift @@ -418,7 +418,11 @@ private struct EditRemoteServerContent: View { // behind it. guard credentials.credential(for: account) != nil else { if target.account == nil { - appEnvironment.accountsManager.removeAccount(account.id) + // Rolled back, not deleted: creating it also queued the marker + // that stops other devices synthesizing an account for this + // instance's pre-revamp login, and a sign-in that never + // completed must not leave that behind. + appEnvironment.accountsManager.rollbackAccountCreation(account.id) } appEnvironment.toastManager.showError( String(localized: "accounts.signIn.storeFailed") diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 10682f4e..6c0c6a01 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -186,6 +186,40 @@ struct AccountsManagerTests { #expect(manager.context(forLegacyInstanceID: instanceID) == .account(second.id)) } + @Test("A sign-in that could not be stored leaves no migration marker") + func rolledBackLoginRetractsItsMarker() { + let manager = makeManager() + let instanceID = UUID() + + let account = manager.addAccount(instanceID: instanceID) + #expect(manager.migratedInstanceIDs.contains(instanceID)) + + manager.rollbackAccountCreation(account.id) + + // The marker would have told every other device that this instance's + // pre-revamp login is already represented — on the strength of an + // account that does not exist anywhere. + #expect(!manager.migratedInstanceIDs.contains(instanceID)) + #expect(!manager.pendingMarkerUploads.contains(instanceID)) + #expect(manager.account(id: account.id) == nil) + // And no tombstone: nothing was ever published to retract. + #expect(!manager.isDeleted(account.id)) + #expect(!manager.pendingDeletions.contains(account.id)) + } + + @Test("Rolling back a confirmed account is a real deletion") + func rolledBackConfirmedAccountIsDeleted() { + let manager = makeManager() + let account = manager.addAccount(instanceID: UUID()) + manager.confirmUpload(account.id) + + manager.rollbackAccountCreation(account.id) + + // It exists on the server, so other devices have to be told. + #expect(manager.isDeleted(account.id)) + #expect(manager.pendingDeletions.contains(account.id)) + } + @Test("An unresolved identity is not a missing credential") func unknownStatusIsNotSignedOut() { let defaults = makeDefaults()