From cc0fd3e6f446cf7b6ed8ab41ecfff766c1b404dd Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Wed, 26 Aug 2026 23:55:50 +0200 Subject: [PATCH] Fence stale feed results and stop guessing at unknown account state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signing in again keeps the account ID and the context, so a request already in flight under the previous credential could land afterwards and write that user's private feed back into the cache and onto disk. Feed results now carry the account they were requested for and are dropped if it has moved. An account with no recorded status was reported as "sign in required". While the identity scope is unresolved nothing can read the Keychain under the right scope, so that is the absence of an answer, not one — on a tokenless or first-seen Apple account it told users with working logins to sign in again and dropped reads to local data. It is now a state of its own. Transport failures mark the account unreachable instead of leaving it "signed in", which is what the documented local read fallback keys on. Neither that nor a rejection is overwritten by the Keychain-based refresh any more: only the server can clear what the server said. Disabling sync deliberately does not rewrite Keychain items to local. That means deleting them, and deleting a synchronizable item propagates to every device — "stop syncing this login" would sign the user out elsewhere, which is not what the toggle says. The reasoning is recorded at the call site. --- Yattee/Core/AccountsManager.swift | 14 ++++++- Yattee/Core/AppEnvironment.swift | 19 ++++++---- Yattee/Localizable.xcstrings | 10 +++++ Yattee/Services/SubscriptionFeedCache.swift | 37 +++++++++++++++++++ .../Subscriptions/DataContextValidator.swift | 5 +++ .../InvidiousSubscriptionProvider.swift | 17 ++++++++- .../PipedSubscriptionProvider.swift | 17 ++++++++- .../Settings/InstanceAccountsSection.swift | 4 +- .../Views/Settings/iCloudSettingsView.swift | 14 +++++-- YatteeTests/AccountsManagerTests.swift | 18 +++++++++ 10 files changed, 140 insertions(+), 15 deletions(-) diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 10ed8555..74fbefdd 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -9,6 +9,12 @@ import Foundation /// Why an account cannot currently be used. enum AccountStatus: Hashable, Sendable { + /// Nothing is known yet — the Apple account this device's credentials are + /// keyed by has not been established, so the Keychain cannot be read under + /// the right scope. Distinct from `credentialMissing`, which is an answer: + /// this one is the absence of one, and must never be presented as "signed + /// out" or acted on. + case checking /// Signed in and usable. case signedIn /// No credential on this device. On tvOS this is permanent until the user @@ -499,8 +505,14 @@ final class AccountsManager { NotificationCenter.default.post(name: .accountsDidChange, object: nil) } + /// An account with no recorded status is *unknown*, not signed out. While + /// the identity scope is unresolved nothing can read the Keychain under the + /// right scope, and reporting a missing credential then would tell a user + /// with a perfectly good login to sign in again — and drop reads to local + /// data — for as long as iCloud stayed unreachable. func status(for accountID: UUID) -> AccountStatus { - accountStatuses[accountID] ?? .credentialMissing + if let status = accountStatuses[accountID] { return status } + return identityScope.isResolved ? .credentialMissing : .checking } // MARK: - Legacy migration diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 61845d7f..9aad5949 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -805,14 +805,17 @@ final class AppEnvironment { continue } - // A credential the server has rejected is still a credential: only - // the server can say it works again. Reporting "signed in" here - // because the Keychain item exists would erase the rejection the - // moment it was recorded — the API layer sets it, which posts the - // notification that runs this — and the user would never be asked - // to sign in again. Storing a new credential resets the status - // explicitly. - guard accountsManager.status(for: account.id) != .credentialRejected else { continue } + // A credential the server rejected — or a server that could not be + // reached — is still a credential, and only the server can say + // otherwise. Reporting "signed in" here because the Keychain item + // exists would erase those the moment they were recorded: the API + // layer sets them, and that posts the notification that runs this. + // The next successful call clears them; storing a new credential + // resets the status explicitly. + switch accountsManager.status(for: account.id) { + case .credentialRejected, .unreachable: continue + case .checking, .signedIn, .credentialMissing: break + } accountsManager.setStatus(.signedIn, for: account.id) } diff --git a/Yattee/Localizable.xcstrings b/Yattee/Localizable.xcstrings index 92bac8ec..02c91241 100644 --- a/Yattee/Localizable.xcstrings +++ b/Yattee/Localizable.xcstrings @@ -183,6 +183,16 @@ } } }, + "accounts.status.checking" : { + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "Checking\u2026" + } + } + } + }, "accounts.status.credentialMissing" : { "localizations" : { "en" : { diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index 95248efb..626b30b1 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -178,6 +178,8 @@ final class SubscriptionFeedCache { /// Updates the cache with new videos and persists to disk. func update(videos: [Video]) { + guard isCurrentAccountsResult() else { return } + let oldCount = self.videos.count let oldDate = self.lastUpdated self.videos = videos @@ -193,6 +195,8 @@ final class SubscriptionFeedCache { /// Appends videos to the existing cache (for pagination). private func appendVideos(_ newVideos: [Video]) { + guard isCurrentAccountsResult() else { return } + let existingIDs = Set(videos.map { $0.id }) let uniqueNewVideos = newVideos.filter { !existingIDs.contains($0.id) } videos.append(contentsOf: uniqueNewVideos) @@ -210,10 +214,40 @@ final class SubscriptionFeedCache { /// Call this when the user switches between local and Invidious accounts. func handleAccountChange() { LoggingService.shared.info("Subscription account changed, clearing feed cache", category: .general) + // Anything already in flight was authenticated as the previous account. + // Clearing does not cancel it, and signing in again keeps the same + // account ID and context — so without this fence the old request lands + // afterwards and writes the previous user's private feed straight back + // into the cache, and onto disk. + accountGeneration += 1 clear() feedLoadState = .idle } + /// Which account's data the running request belongs to. + /// + /// Only one refresh runs at a time (every entry point guards on + /// `isLoading`), so a single pair of counters is enough to tell "this result + /// is for the account we are showing" from "this result is a leftover". + private var accountGeneration = 0 + private var requestGeneration = 0 + + /// Marks the start of a request, capturing the account it is for. + private func beginRequest() { + requestGeneration = accountGeneration + } + + private func isCurrentAccountsResult() -> Bool { + guard requestGeneration == accountGeneration else { + LoggingService.shared.info( + "Discarding a feed result that belongs to the previous login", + category: .general + ) + return false + } + return true + } + // MARK: - Refresh /// Refreshes the feed from network. @@ -229,6 +263,8 @@ final class SubscriptionFeedCache { return } + beginRequest() + // The *effective* context, so an account still waiting on its instance // or credential falls back to local rather than reporting // notAuthenticated while the subscriptions list already shows local data. @@ -437,6 +473,7 @@ final class SubscriptionFeedCache { return } + beginRequest() isLoading = true feedLoadState = .loadingMore diff --git a/Yattee/Services/Subscriptions/DataContextValidator.swift b/Yattee/Services/Subscriptions/DataContextValidator.swift index 001f3b37..8529d993 100644 --- a/Yattee/Services/Subscriptions/DataContextValidator.swift +++ b/Yattee/Services/Subscriptions/DataContextValidator.swift @@ -9,6 +9,10 @@ import Foundation /// Why the selected context cannot be used at the moment. enum DataContextUnavailability: Hashable, Sendable { + /// Nothing is known yet: the Apple account the credentials are keyed by is + /// still being established, so the Keychain cannot be read under the right + /// scope. Never render this as "sign in required". + case checking /// The account exists but its instance hasn't arrived from iCloud yet. case instancePending /// The account's instance has been switched off. The selection is kept — @@ -105,6 +109,7 @@ final class DataContextValidator { switch accountsManager.status(for: id) { case .signedIn: return .valid + case .checking: return .temporarilyUnavailable(.checking) case .credentialMissing: return .temporarilyUnavailable(.credentialMissing) case .credentialRejected: return .temporarilyUnavailable(.credentialRejected) case .unreachable: return .temporarilyUnavailable(.unreachable) diff --git a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift index c6d25d0a..a35ac358 100644 --- a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift @@ -38,7 +38,22 @@ final class InvidiousSubscriptionProvider: SubscriptionProvider { return } - guard case APIError.unauthorized = error else { 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) diff --git a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift index 5ac93231..ee46a7c9 100644 --- a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift @@ -38,7 +38,22 @@ final class PipedSubscriptionProvider: SubscriptionProvider { return } - guard case APIError.unauthorized = error else { 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) diff --git a/Yattee/Views/Settings/InstanceAccountsSection.swift b/Yattee/Views/Settings/InstanceAccountsSection.swift index fdb2dc50..0b943225 100644 --- a/Yattee/Views/Settings/InstanceAccountsSection.swift +++ b/Yattee/Views/Settings/InstanceAccountsSection.swift @@ -153,11 +153,12 @@ struct InstanceAccountsSection: View { } private func status(for account: Account) -> AccountStatus { - appEnvironment?.accountsManager.status(for: account.id) ?? .credentialMissing + appEnvironment?.accountsManager.status(for: account.id) ?? .checking } private func statusText(for account: Account) -> String { switch status(for: account) { + case .checking: String(localized: "accounts.status.checking") case .signedIn: String(localized: "accounts.status.signedIn") case .credentialMissing: String(localized: "accounts.status.credentialMissing") case .credentialRejected: String(localized: "accounts.status.credentialRejected") @@ -171,6 +172,7 @@ struct InstanceAccountsSection: View { private func statusColor(for account: Account) -> Color { switch status(for: account) { + case .checking: .secondary case .signedIn: .secondary case .credentialMissing: .secondary case .credentialRejected: .red diff --git a/Yattee/Views/Settings/iCloudSettingsView.swift b/Yattee/Views/Settings/iCloudSettingsView.swift index 6f812964..a62c0ef3 100644 --- a/Yattee/Views/Settings/iCloudSettingsView.swift +++ b/Yattee/Views/Settings/iCloudSettingsView.swift @@ -532,11 +532,19 @@ struct iCloudSettingsView: View { resyncStoredLogins() } - /// Rewrites stored logins so they carry the sync setting that has just - /// changed. Whether a Keychain item syncs is fixed when it is written, so a - /// login stored while sync was off would stay on this device while its + /// Rewrites stored logins so they carry the sync setting that has just been + /// turned **on**. Whether a Keychain item syncs is fixed when it is written, + /// so a login stored while sync was off would stay on this device while its /// account record travelled — every other device showing an account whose /// credential never arrives. + /// + /// Deliberately not called when sync is switched off. Making a + /// synchronizable item local means deleting and rewriting it, and deleting + /// a synchronizable item propagates to every device (Apple-documented, and + /// measured for this project) — so "stop syncing this login" would sign the + /// user out on their other devices, which is not what the toggle says. + /// Turning sync off stops new writes from syncing and stops account records + /// from uploading; what is already in iCloud Keychain stays. private func resyncStoredLogins() { appEnvironment?.invidiousCredentialsManager.resyncCredentialsWithCurrentSetting() appEnvironment?.pipedCredentialsManager.resyncCredentialsWithCurrentSetting() diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index ce1f0c3e..10682f4e 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -186,6 +186,24 @@ struct AccountsManagerTests { #expect(manager.context(forLegacyInstanceID: instanceID) == .account(second.id)) } + @Test("An unresolved identity is not a missing credential") + func unknownStatusIsNotSignedOut() { + let defaults = makeDefaults() + let unresolved = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: nil), + defaults: defaults + ) + let account = unresolved.addAccount(instanceID: UUID()) + + // Nothing can read the Keychain under the right scope yet, so "no + // credential" is not an answer the app is entitled to give. + #expect(unresolved.status(for: account.id) == .checking) + + let resolved = makeManager(defaults: defaults, scope: "identity-A") + let other = resolved.addAccount(instanceID: UUID()) + #expect(resolved.status(for: other.id) == .credentialMissing) + } + @Test("A fresh device adopts the synced context") func firstSyncedContextIsAdopted() { let manager = makeManager()