diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 04edddea..bf2060c5 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -88,11 +88,13 @@ final class AccountsManager { init( identityScope: CredentialIdentityScope, instancesManager: InstancesManager? = nil, - defaults: UserDefaults = .standard + defaults: UserDefaults = .standard, + unreachableRetryInterval: TimeInterval = 60 ) { self.identityScope = identityScope self.instancesManager = instancesManager self.defaults = defaults + self.unreachableRetryInterval = unreachableRetryInterval load() @@ -100,7 +102,7 @@ final class AccountsManager { // so everything held in memory belongs to the previous one. NotificationCenter.default.addObserver( forName: .credentialIdentityScopeDidChange, - object: nil, + object: identityScope, queue: .main ) { [weak self] _ in MainActor.assumeIsolated { self?.reloadForCurrentIdentity() } @@ -158,9 +160,12 @@ final class AccountsManager { accounts.filter { capabilities(for: $0)?.isSelectableContext == true } } - /// Accounts whose instance is gone. Surfaced in the accounts screen with a - /// remove action rather than being deleted automatically — an instance can - /// be missing because iCloud hasn't delivered it yet. + /// Accounts whose instance is gone. + /// + /// Never deleted automatically: an instance can be missing because iCloud + /// has not delivered it yet, and absence is not a deletion. The screen that + /// lists these and offers to remove them is Phase 3 work; until it exists + /// an orphan stays out of the selectable contexts and out of the way. var orphanedAccounts: [Account] { accounts.filter { instance(for: $0) == nil } } @@ -550,11 +555,12 @@ final class AccountsManager { /// - 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()) { + func setStatus(_ status: AccountStatus, for accountID: UUID) { + unreachableExpiries[accountID]?.cancel() + unreachableExpiries[accountID] = nil + if status == .unreachable { - unreachableSince[accountID] = date - } else { - unreachableSince[accountID] = nil + scheduleUnreachableExpiry(for: accountID) } guard accountStatuses[accountID] != status else { return } @@ -562,16 +568,34 @@ final class AccountsManager { NotificationCenter.default.post(name: .accountStatusesDidChange, object: nil) } - /// When each unreachable verdict was reached. - private var unreachableSince: [UUID: Date] = [:] + /// Pending "try this account again" timers. + private var unreachableExpiries: [UUID: Task] = [:] /// 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 + /// It has to expire on its own, and visibly: the state stops reads from + /// going to the server at all, so nothing would discover that the server + /// came back — and a timeout during an outage produces no connectivity + /// change to react to either. Expiring by mutating the status, rather than + /// by having the getter compare the clock, is what makes the fallback + /// actually lift: nothing re-reads a value merely because time passed. + /// Injectable so tests do not have to wait a real minute. + private let unreachableRetryInterval: TimeInterval + + private func scheduleUnreachableExpiry(for accountID: UUID) { + unreachableExpiries[accountID] = Task { [weak self] in + try? await Task.sleep(for: .seconds(self?.unreachableRetryInterval ?? 60)) + guard !Task.isCancelled else { return } + + await MainActor.run { + guard let self, self.accountStatuses[accountID] == .unreachable else { return } + self.unreachableExpiries[accountID] = nil + // Back to being usable: the credential was never in question, + // and the next request settles it either way. + self.setStatus(.signedIn, for: accountID) + } + } + } /// An account with no recorded status is *unknown*, not signed out. While /// the identity scope is unresolved nothing can read the Keychain under the @@ -579,20 +603,7 @@ final class AccountsManager { /// 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 { - 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 + accountStatuses[accountID] ?? (identityScope.isResolved ? .credentialMissing : .checking) } // MARK: - Legacy migration diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index d1e7c8b2..cc6ec437 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -429,7 +429,7 @@ final class AppEnvironment { // otherwise a synced account shows "sign in required" until relaunch. NotificationCenter.default.addObserver( forName: .credentialIdentityScopeDidChange, - object: nil, + object: identityScope, queue: .main ) { [weak self] _ in // Deferred rather than run inline: AccountsManager observes the same diff --git a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift index 959818d7..b5ccf067 100644 --- a/Yattee/Services/CloudKit/AccountSyncCoordinator.swift +++ b/Yattee/Services/CloudKit/AccountSyncCoordinator.swift @@ -108,6 +108,7 @@ final class AccountSyncCoordinator { self.init( accountsManager: accountsManager, namespaceProvider: { identityScope.current }, + identityScope: identityScope, store: store, conflictResolver: conflictResolver, zone: zone, @@ -120,6 +121,7 @@ final class AccountSyncCoordinator { init( accountsManager: AccountsManager, namespaceProvider: @escaping @MainActor () -> String?, + identityScope: CredentialIdentityScope? = nil, store: AccountSyncStore? = nil, conflictResolver: CloudKitConflictResolver = CloudKitConflictResolver(), zone: CKRecordZone = RecordType.createZone(), @@ -134,12 +136,17 @@ final class AccountSyncCoordinator { self.isCategoryEnabled = isCategoryEnabled self.lastResolvedNamespace = namespaceProvider() - identityObserver = NotificationCenter.default.addObserver( - forName: .credentialIdentityScopeDidChange, - object: nil, - queue: .main - ) { [weak self] _ in - MainActor.assumeIsolated { self?.identityDidChange() } + // Observed on *this* scope: another one resolving says nothing about + // the identity these records are namespaced by. Tests pass no scope and + // drive `identityDidChange()` directly. + if let identityScope { + identityObserver = NotificationCenter.default.addObserver( + forName: .credentialIdentityScopeDidChange, + object: identityScope, + queue: .main + ) { [weak self] _ in + MainActor.assumeIsolated { self?.identityDidChange() } + } } // Local edits mark themselves dirty in AccountsManager; these turn that diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 6dbb02da..7fd350d3 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -109,7 +109,10 @@ final class CredentialIdentityScope { state = newState guard current != previous else { return } - NotificationCenter.default.post(name: .credentialIdentityScopeDidChange, object: nil) + // Posted with the scope as the object: everything namespaced by identity + // observes *its* scope, so an unrelated one resolving cannot make it + // throw away state that never belonged to that scope. + NotificationCenter.default.post(name: .credentialIdentityScopeDidChange, object: self) } var isResolved: Bool { current != nil } diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index baf978d4..3c845211 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -129,6 +129,11 @@ final class SubscriptionFeedCache { return } diskCacheLoaded = true + + // Ordered behind any purge already asked for, or this reads the file + // that purge is about to delete. + await diskClearTask?.value + let generation = accountGeneration if let cacheData = await FeedCache.shared.load() { @@ -165,11 +170,18 @@ final class SubscriptionFeedCache { lastUpdated = nil hasMorePages = false currentPage = 1 - Task { + diskClearTask = Task { await FeedCache.shared.clear() } } + /// The outstanding erase of the on-disk feed. + /// + /// A load starting after a purge can still reach the cache actor first and + /// bring the erased feed back — the generation it captures is already the + /// new one, so nothing downstream can tell. Loads wait for this instead. + private var diskClearTask: Task? + func invalidate() { lastUpdated = nil Task { diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 4a0c58b1..9fc907c0 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -187,8 +187,12 @@ struct AccountsManagerTests { } @Test("An unreachable server is retried rather than written off") - func unreachableStatusExpires() { - let manager = makeManager() + func unreachableStatusExpires() async throws { + let manager = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: "identity-A"), + defaults: makeDefaults(), + unreachableRetryInterval: 0.05 + ) let account = manager.addAccount(instanceID: UUID()) manager.setStatus(.unreachable, for: account.id) @@ -196,8 +200,9 @@ struct AccountsManagerTests { // 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)) + // produces no connectivity change to react to either. It has to lift + // by mutating the status: nothing re-reads a value because time passed. + try await Task.sleep(for: .seconds(0.5)) #expect(manager.status(for: account.id) == .signedIn) }