mirror of
https://github.com/yattee/yattee.git
synced 2026-10-23 21:22:53 +00:00
Expire the unreachable state visibly, and address the right scope
"Could not be reached" now lifts by mutating the status rather than by having the getter compare the clock. Nothing re-reads a value because time has passed, so the read fallback stayed in place and no request was ever made to discover that the server had recovered. The identity-scope notification names the scope it is about, and everything namespaced by identity observes its own. A second scope resolving would otherwise make every account list in the process reload and drop its statuses — which is also what made the new test fail. A disk load of the feed waits for any purge already asked for. The clear runs in its own task, so a load starting afterwards could still reach the file first and restore what the purge was about to delete. The comment on orphaned accounts no longer promises a removal action that does not exist yet — that screen is Phase 3 — and says what happens until then.
This commit is contained in:
@@ -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<Void, Never>] = [:]
|
||||
|
||||
/// 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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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<Void, Never>?
|
||||
|
||||
func invalidate() {
|
||||
lastUpdated = nil
|
||||
Task {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user