diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index a6c032a0..04edddea 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -547,20 +547,52 @@ final class AccountsManager { /// change, and it cannot tell that a server rejected a credential or failed /// to answer — so sharing one notification meant every status the API layer /// recorded was overwritten by the reaction to recording it. - func setStatus(_ status: AccountStatus, for accountID: UUID) { + /// - Parameter date: when the verdict was reached. Only `unreachable` uses + /// it — to decide when to try again — and only tests pass anything but + /// the default. + func setStatus(_ status: AccountStatus, for accountID: UUID, at date: Date = Date()) { + if status == .unreachable { + unreachableSince[accountID] = date + } else { + unreachableSince[accountID] = nil + } + guard accountStatuses[accountID] != status else { return } accountStatuses[accountID] = status NotificationCenter.default.post(name: .accountStatusesDidChange, object: nil) } + /// When each unreachable verdict was reached. + private var unreachableSince: [UUID: Date] = [:] + + /// How long "could not be reached" stands before the app tries again. + /// + /// It has to expire on its own. The state stops reads from going to the + /// server at all, so nothing would ever discover that the server came back + /// — and a timeout during an outage never produces a connectivity change to + /// react to either. + private static let unreachableRetryInterval: TimeInterval = 60 + /// 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 { - if let status = accountStatuses[accountID] { return status } - return identityScope.isResolved ? .credentialMissing : .checking + guard let status = accountStatuses[accountID] else { + return identityScope.isResolved ? .credentialMissing : .checking + } + + // A server that did not answer a minute ago is worth trying again. The + // credential is untouched by any of this, so the account goes back to + // being usable and the next request settles it either way. + if status == .unreachable, + let since = unreachableSince[accountID], + Date().timeIntervalSince(since) > Self.unreachableRetryInterval { + return .signedIn + } + + return status } // MARK: - Legacy migration diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index b8155f6a..e385b5b8 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -73,6 +73,13 @@ final class AppEnvironment { /// The context the account-scoped caches currently hold data for. @ObservationIgnored private var lastCachedContext: DataContext = .local + /// The credential each rejection was recorded against, so a replacement + /// arriving over iCloud Keychain is not treated as still rejected. + @ObservationIgnored private var rejectedCredentialFingerprints: [UUID: String] = [:] + + /// Re-reads the Keychain for a while after an account turns up without one. + @ObservationIgnored private var credentialArrivalTask: Task? + // MARK: - Shared Instance /// The single, process-wide app environment. @@ -829,23 +836,84 @@ final class AppEnvironment { // A credential the server rejected is still a credential: the // Keychain cannot tell that it stopped working, so this must not - // report "signed in" over it. Only the server clears a rejection — - // on the next successful call — or storing a new credential, which - // resets the status explicitly. + // report "signed in" over it. What it *can* tell is whether the + // credential is still the same one — signing in again on another + // device replaces it under the same key — and a rejection only + // applies to the secret that earned it. // - // `unreachable` is deliberately *not* preserved. It is transient, + // `unreachable` is deliberately not preserved here. It is transient, // and it stops reads from reaching the server at all, so keeping it - // here would strand the account: nothing would ever call out to - // discover that the network came back. Recomputing it on the events - // that run this — foreground, an account or instance arriving — is - // what lets it retry. - switch accountsManager.status(for: account.id) { - case .credentialRejected: continue - case .checking, .signedIn, .credentialMissing, .unreachable: break + // would strand the account: nothing would call out to discover that + // the server came back. + let fingerprint = credentials.credential(for: account) + .map { CloudKitIdentity.shortDigest(of: Data($0.utf8)) } + + if accountsManager.status(for: account.id) == .credentialRejected { + if let rejected = rejectedCredentialFingerprints[account.id], rejected != fingerprint { + LoggingService.shared.info( + "The rejected credential has been replaced — clearing the rejection", + category: .keychain, + details: "accountID=\(account.id)" + ) + } else { + rejectedCredentialFingerprints[account.id] = fingerprint + continue + } } + rejectedCredentialFingerprints[account.id] = nil + accountsManager.setStatus(.signedIn, for: account.id) } + + waitForArrivingCredentialsIfNeeded() + } + + /// Re-reads the Keychain for a couple of minutes while any account is + /// missing its credential. + /// + /// iCloud Keychain delivers on its own schedule and announces nothing when + /// it does, so an account that syncs ahead of its credential — the ordinary + /// case on a new device — would sit at "sign in required", with reads + /// falling back to local, until something unrelated happened to recompute + /// statuses. Stops as soon as nothing is waiting, and gives up rather than + /// polling forever: on tvOS, where app Keychain items never sync at all, + /// the wait is permanent by design. + private func waitForArrivingCredentialsIfNeeded() { + let isWaiting = accountsManager.accounts.contains { accountsManager.status(for: $0.id) == .credentialMissing } + + guard isWaiting else { + credentialArrivalTask?.cancel() + credentialArrivalTask = nil + return + } + + guard credentialArrivalTask == nil else { return } + + credentialArrivalTask = Task { [weak self] in + for _ in 0 ..< 8 { + try? await Task.sleep(for: .seconds(15)) + guard !Task.isCancelled, let self else { return } + + let stillWaiting = self.accountsManager.accounts.contains { + self.accountsManager.status(for: $0.id) == .credentialMissing + } + guard stillWaiting else { break } + + for account in self.accountsManager.accounts + where self.accountsManager.status(for: account.id) == .credentialMissing { + guard let credentials = self.accountCredentialsManager(for: account), + credentials.isScopeResolved else { continue } + + credentials.refreshLoginStatus(for: account) + if credentials.isLoggedIn(for: account) { + self.accountsManager.setStatus(.signedIn, for: account.id) + } + } + } + + self?.credentialArrivalTask = nil + } } // MARK: - Credentials Management diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index 626b30b1..9fb17abc 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -129,8 +129,21 @@ final class SubscriptionFeedCache { return } diskCacheLoaded = true + let generation = accountGeneration if let cacheData = await FeedCache.shared.load() { + // The identity or the login can change while this is awaiting the + // disk. Applying what it brings back would restore the previous + // account's private feed on top of a cache that was deliberately + // emptied. + guard generation == accountGeneration else { + LoggingService.shared.info( + "Discarding a disk feed that belongs to the previous login", + category: .general + ) + return + } + videos = cacheData.videos lastUpdated = cacheData.lastUpdated hasLoadedOnce = true @@ -222,6 +235,11 @@ final class SubscriptionFeedCache { accountGeneration += 1 clear() feedLoadState = .idle + // The request that was running belongs to the previous account and its + // result is now fenced out, so it must not go on blocking the + // replacement — the new account would sit on an empty feed until + // something else asked for a refresh. + isLoading = false } /// Which account's data the running request belongs to. diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 6c0c6a01..4a0c58b1 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -186,6 +186,21 @@ struct AccountsManagerTests { #expect(manager.context(forLegacyInstanceID: instanceID) == .account(second.id)) } + @Test("An unreachable server is retried rather than written off") + func unreachableStatusExpires() { + let manager = makeManager() + let account = manager.addAccount(instanceID: UUID()) + + manager.setStatus(.unreachable, for: account.id) + #expect(manager.status(for: account.id) == .unreachable) + + // The state stops reads from going out at all, so nothing would ever + // discover that the server came back — a timeout during an outage + // produces no connectivity change to react to either. + manager.setStatus(.unreachable, for: account.id, at: Date().addingTimeInterval(-3_600)) + #expect(manager.status(for: account.id) == .signedIn) + } + @Test("A sign-in that could not be stored leaves no migration marker") func rolledBackLoginRetractsItsMarker() { let manager = makeManager()