diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index f815ecce..5484b8b0 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -606,6 +606,41 @@ final class AccountsManager { accountStatuses[accountID] ?? (identityScope.isResolved ? .credentialMissing : .checking) } + /// Records what a server call implies about the account that made it. + /// + /// Deliberately conservative about rejection: a raw 401 marks the + /// credential only when no reverse-proxy basic auth is configured, because + /// otherwise the 401 may well have come from the proxy. The two-stage probe + /// that disambiguates the rest is Phase 2 work; until then an ambiguous + /// failure leaves the status alone rather than wrongly telling the user to + /// sign in again. + /// + /// Shared by the subscription providers and the feed cache: a credential + /// that expires during a feed-only refresh has to mark the account too, or + /// nothing ever reports it and the fallback never engages. + func recordServerOutcome(_ error: Error?, for account: Account) { + guard let error else { + setStatus(.signedIn, for: account.id) + return + } + + // A server that cannot be reached says nothing about the credential. + // Recorded all the same: the account has to stop reporting itself as + // usable, or reads never fall back and the user is shown failures + // instead of local data. + switch error { + case APIError.timeout, APIError.noConnection: + setStatus(.unreachable, for: account.id) + case APIError.cancelled: + return + case APIError.unauthorized: + guard instance(for: account)?.usesBasicAuth == false else { return } + setStatus(.credentialRejected, for: account.id) + default: + return + } + } + // MARK: - Legacy migration /// Turns each pre-revamp single-login instance into an account. diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index b5ccf067..ded2929d 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -208,6 +208,18 @@ final class AccountSyncCoordinator { // MARK: - Fetching + /// Notes which identity the fetch now starting belongs to. + /// + /// The engine's fetch spans several delegate events, and the Apple account + /// can move between them. Staging under whatever is current when the + /// records happen to arrive would file one identity's accounts under + /// another's. + func fetchDidBegin() { + fetchIdentity = (namespace, identityGeneration) + } + + private var fetchIdentity: (namespace: String?, generation: Int)? + /// Stages the account-category part of one fetch event. /// /// Called before the delegate returns, so the staged state is never behind @@ -218,13 +230,17 @@ final class AccountSyncCoordinator { guard !accountRecords.isEmpty || !accountDeletions.isEmpty else { return } - guard let namespace else { + // The identity the *fetch* began under, not whatever is current now. + let owner = fetchIdentity ?? (namespace, identityGeneration) + + guard let namespace = owner.namespace, owner.generation == identityGeneration else { // Staging under a guessed identity is the one thing worse than // losing the records: they would be applied to, and uploaded from, - // an account they never belonged to. + // an account they never belonged to. A bootstrap recovers them for + // whichever identity they turn out to belong to. store.hasPendingIdentityRecovery = true LoggingService.shared.logCloudKit( - "Dropping \(accountRecords.count + accountDeletions.count) account records fetched before the identity resolved" + "Dropping \(accountRecords.count + accountDeletions.count) account records fetched under an identity that has since moved" ) return } diff --git a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift index 7d7210d5..5d342b39 100644 --- a/Yattee/Services/CloudKit/CloudKitSyncEngine.swift +++ b/Yattee/Services/CloudKit/CloudKitSyncEngine.swift @@ -1716,6 +1716,9 @@ extension CloudKitSyncEngine: CKSyncEngineDelegate { case .willFetchChanges: LoggingService.shared.logCloudKit("CKSyncEngine will fetch changes (auto-triggered)") isReceivingChanges = true + // Binds everything this fetch delivers to the identity it + // started under; the Apple account can move mid-fetch. + accountSync?.fetchDidBegin() case .didFetchChanges: LoggingService.shared.logCloudKit("CKSyncEngine finished fetching changes") diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index 18167f21..1a206638 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -237,6 +237,19 @@ final class SubscriptionFeedCache { saveToDisk() } + /// Reports what a feed request implies about the account it used. + /// + /// The feed talks to the same servers as the subscription providers but + /// does not go through one, so without this a credential that expires + /// during a feed-only refresh would keep looking signed in and the local + /// fallback would never engage. + private func recordOutcome(_ error: Error?, backend: AppEnvironment.FeedBackend, using appEnvironment: AppEnvironment) { + let type: InstanceType = backend == .invidious ? .invidious : .piped + guard let account = appEnvironment.account(forSelectedContext: type) else { return } + + appEnvironment.accountsManager.recordServerOutcome(error, for: account) + } + // MARK: - Account Change Handling /// Clears cache when subscription account changes. @@ -412,6 +425,7 @@ final class SubscriptionFeedCache { guard isCurrentAccountsResult(generation) else { return } LoggingService.shared.info("Piped feed: Received \(feedVideos.count) videos", category: .general) + recordOutcome(nil, backend: .piped, using: appEnvironment) update(videos: feedVideos, generation: generation) appEnvironment.dataManager.updateLastVideoPublishedDates(from: feedVideos) @@ -419,6 +433,7 @@ final class SubscriptionFeedCache { feedLoadState = .ready } catch { LoggingService.shared.error("Failed to fetch Piped feed: \(error.localizedDescription)", category: .general) + recordOutcome(error, backend: .piped, using: appEnvironment) guard isCurrentAccountsResult(generation) else { return } feedLoadState = .error(.networkError(error.localizedDescription)) } @@ -474,6 +489,7 @@ final class SubscriptionFeedCache { "Invidious feed: Received \(feedVideos.count) videos (page \(currentPage))", category: .general ) + recordOutcome(nil, backend: .invidious, using: appEnvironment) // For first page, replace all videos. For subsequent pages, this shouldn't be called. update(videos: feedVideos, generation: generation) @@ -492,6 +508,7 @@ final class SubscriptionFeedCache { "Failed to fetch Invidious feed: \(error.localizedDescription)", category: .general ) + recordOutcome(error, backend: .invidious, using: appEnvironment) guard isCurrentAccountsResult(generation) else { return } feedLoadState = .error(.networkError(error.localizedDescription)) } diff --git a/Yattee/Services/SubscriptionService.swift b/Yattee/Services/SubscriptionService.swift index 1f2fa3a8..25bf6d5c 100644 --- a/Yattee/Services/SubscriptionService.swift +++ b/Yattee/Services/SubscriptionService.swift @@ -103,6 +103,18 @@ final class SubscriptionService { ) { [weak self] _ in MainActor.assumeIsolated { self?.discardAllProviders() } } + + // Switching context deliberately keeps the providers — each is one + // account's cache and stays valid — but a fetch already running belongs + // to the account that was selected when it started, and finishing after + // the switch would put its channels on screen for another one. + NotificationCenter.default.addObserver( + forName: .dataContextDidChange, + object: nil, + queue: .main + ) { [weak self] _ in + MainActor.assumeIsolated { self?.providerGeneration += 1 } + } } // MARK: - Current Provider @@ -174,7 +186,7 @@ final class SubscriptionService { /// 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. - private var providerGeneration = 0 + fileprivate var providerGeneration = 0 /// The provider for **writes**. /// diff --git a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift index a35ac358..b7deff17 100644 --- a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift @@ -24,39 +24,11 @@ final class InvidiousSubscriptionProvider: SubscriptionProvider { private let credentialsManager: InvidiousCredentialsManager private let accountsManager: AccountsManager - /// Records the outcome of a server call against the account. - /// - /// Deliberately conservative: a raw 401 marks the credential rejected only - /// when no reverse-proxy basic auth is configured, because otherwise the - /// 401 may well have come from the proxy rather than the account. The - /// two-stage probe that disambiguates the remaining case is Phase 2 work; - /// until then an ambiguous failure leaves the status untouched rather than - /// wrongly telling the user to sign in again. + /// Records the outcome of a server call against the account. The + /// classification lives in one place so the feed cache, which talks to the + /// same servers without going through a provider, reaches the same verdict. private func recordOutcome(_ error: Error?) { - guard let error else { - accountsManager.setStatus(.signedIn, for: account.id) - return - } - - // A server that cannot be reached says nothing about the credential. - // Recorded all the same: the account has to stop reporting itself as - // usable, or reads never fall back and the user is shown failures - // instead of local data. - switch error { - case APIError.timeout, APIError.noConnection: - accountsManager.setStatus(.unreachable, for: account.id) - return - case APIError.cancelled: - return - case APIError.unauthorized: - break - default: - return - } - - guard accountsManager.instance(for: account)?.usesBasicAuth == false else { return } - - accountsManager.setStatus(.credentialRejected, for: account.id) + accountsManager.recordServerOutcome(error, for: account) } private let settingsManager: SettingsManager diff --git a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift index ee46a7c9..3dcfe252 100644 --- a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift @@ -24,39 +24,11 @@ final class PipedSubscriptionProvider: SubscriptionProvider { private let credentialsManager: PipedCredentialsManager private let accountsManager: AccountsManager - /// Records the outcome of a server call against the account. - /// - /// Deliberately conservative: a raw 401 marks the credential rejected only - /// when no reverse-proxy basic auth is configured, because otherwise the - /// 401 may well have come from the proxy rather than the account. The - /// two-stage probe that disambiguates the remaining case is Phase 2 work; - /// until then an ambiguous failure leaves the status untouched rather than - /// wrongly telling the user to sign in again. + /// Records the outcome of a server call against the account. The + /// classification lives in one place so the feed cache, which talks to the + /// same servers without going through a provider, reaches the same verdict. private func recordOutcome(_ error: Error?) { - guard let error else { - accountsManager.setStatus(.signedIn, for: account.id) - return - } - - // A server that cannot be reached says nothing about the credential. - // Recorded all the same: the account has to stop reporting itself as - // usable, or reads never fall back and the user is shown failures - // instead of local data. - switch error { - case APIError.timeout, APIError.noConnection: - accountsManager.setStatus(.unreachable, for: account.id) - return - case APIError.cancelled: - return - case APIError.unauthorized: - break - default: - return - } - - guard accountsManager.instance(for: account)?.usesBasicAuth == false else { return } - - accountsManager.setStatus(.credentialRejected, for: account.id) + accountsManager.recordServerOutcome(error, for: account) } private let settingsManager: SettingsManager diff --git a/Yattee/Views/Subscriptions/SubscriptionsView.swift b/Yattee/Views/Subscriptions/SubscriptionsView.swift index 50ac60e7..c6e1dc72 100644 --- a/Yattee/Views/Subscriptions/SubscriptionsView.swift +++ b/Yattee/Views/Subscriptions/SubscriptionsView.swift @@ -22,6 +22,9 @@ struct SubscriptionsView: View { @State private var feedCache = SubscriptionFeedCache.shared @State private var subscriptions: [Subscription] = [] @State private var subscriptionsLoaded = false + /// The effective context the visible subscriptions were loaded for, so a + /// status change that does not move it does not force a reload. + @State private var loadedContext: DataContext? @State private var selectedChannelID: String? = nil @State private var errorMessage: String? @State private var watchEntriesMap: [String: WatchEntry] = [:] @@ -507,15 +510,17 @@ struct SubscriptionsView: View { // switching between them would change nothing here and this // view would keep showing the first account's subscriptions // while writes went to the second. + // + // Status changes count too, through the *effective* context: + // a credential arriving or being rejected moves reads + // between the account and local without the stored context + // moving at all. .onReceive(NotificationCenter.default.publisher(for: .dataContextDidChange)) { _ in - feedCache.handleAccountChange() - subscriptions = [] - subscriptionsLoaded = false - Task { - guard let appEnvironment else { return } - await loadSubscriptionsAsync() - await feedCache.refresh(using: appEnvironment) - } + reloadForContextChange() + } + .onReceive(NotificationCenter.default.publisher(for: .accountStatusesDidChange)) { _ in + guard appEnvironment?.dataContextValidator.effectiveContext != loadedContext else { return } + reloadForContextChange() } .task(id: instanceConfigurationID) { LoggingService.shared.debug("SubscriptionsView task triggered, instanceConfigurationID: \(instanceConfigurationID)", category: .general) @@ -1333,9 +1338,28 @@ struct SubscriptionsView: View { /// Loads subscriptions asynchronously from the current provider. /// For Invidious, this fetches from the API and creates temporary Subscription objects for UI. + /// Drops what is on screen and reloads it for whatever context is current. + private func reloadForContextChange() { + loadedContext = appEnvironment?.dataContextValidator.effectiveContext + feedCache.handleAccountChange() + subscriptions = [] + subscriptionsLoaded = false + + Task { + guard let appEnvironment else { return } + await loadSubscriptionsAsync() + await feedCache.refresh(using: appEnvironment) + } + } + private func loadSubscriptionsAsync() async { guard let subscriptionService, let appEnvironment else { return } + // Recorded on every load, not only on a context change: the first load + // of a session has to establish what is on screen, or the first status + // change would look like a move and reload for nothing. + loadedContext = appEnvironment.dataContextValidator.effectiveContext + // For local account, just load from DataManager (fast) if appEnvironment.accountsManager.currentContext.isLocal { subscriptions = dataManager?.subscriptions() ?? []