mirror of
https://github.com/yattee/yattee.git
synced 2026-10-23 13:12:53 +00:00
Stop automatic context repairs from outranking real choices
Correcting away from a deleted account was stamped with the current time and published. A device hearing about the deletion late would therefore sink an explicit choice another device made in between, and push its own automatic fallback over it. The repair now keeps the timestamp of the selection it replaces and is never uploaded: every device performs the same repair when it sees the deletion, so they converge without anyone writing anything. The tombstone is queued before the removal can persist. Deleting the selected account also corrects the context, and that wrote storage first — a crash in the gap left the account gone here and alive everywhere else, with nothing owed to the server. An identity reload replaces the context too, so it announces one. The legacy selection and the feed cache, which is not keyed by account, would otherwise keep serving the previous Apple account's data; the cache is now cleared unconditionally on an identity change, and the legacy selection falls back to local when the new namespace's context cannot be mapped yet. The legacy validator no longer corrects a selection whose account still exists. It predates account-scoped credentials and reads "not signed in" as "not available", which is now routinely temporary — an unresolved identity scope, or a credential iCloud Keychain has yet to deliver — and correcting on that desynchronized it from the data context, which is preserved. Background refresh routes on the effective context, so a disabled instance no longer receives background requests.
This commit is contained in:
@@ -207,8 +207,11 @@ final class AccountsManager {
|
||||
func removeAccount(_ accountID: UUID) {
|
||||
guard account(id: accountID) != nil else { return }
|
||||
|
||||
forgetAccount(accountID)
|
||||
// Queued before anything else can persist: a crash in between would
|
||||
// otherwise leave the account deleted here with no tombstone owed to
|
||||
// the server, and every other device would keep — or recreate — it.
|
||||
pendingDeletions.insert(accountID)
|
||||
forgetAccount(accountID)
|
||||
|
||||
save()
|
||||
|
||||
@@ -232,10 +235,24 @@ final class AccountsManager {
|
||||
// A deletion the app can point at is one of the few things allowed to
|
||||
// correct the context.
|
||||
if storedContext.context == .account(accountID) {
|
||||
setContext(.local)
|
||||
correctContextAfterDeletion()
|
||||
}
|
||||
}
|
||||
|
||||
/// Repairs a context left pointing at an account that is gone.
|
||||
///
|
||||
/// Deliberately not a new choice: it keeps the timestamp of the selection
|
||||
/// it replaces and is never published. Stamping it "now" would let an
|
||||
/// automatic fallback outrank a deliberate choice another device made
|
||||
/// between the deletion and this device hearing about it — and publishing
|
||||
/// it would push that fallback over their choice. Every device that sees
|
||||
/// the deletion performs the same repair, so they converge without anyone
|
||||
/// writing anything.
|
||||
private func correctContextAfterDeletion() {
|
||||
storedContext = StoredDataContext(context: .local, changedAt: storedContext.changedAt)
|
||||
NotificationCenter.default.post(name: .dataContextDidChange, object: nil)
|
||||
}
|
||||
|
||||
/// Marks a deletion as confirmed by the server. Until then it stays in the
|
||||
/// outbox and is retried, because a tombstone that never lands lets a stale
|
||||
/// device resurrect the account.
|
||||
@@ -366,8 +383,8 @@ final class AccountsManager {
|
||||
@discardableResult
|
||||
func applyRemoteAccountDeletionEvent(_ accountID: UUID) -> Account? {
|
||||
let removed = account(id: accountID)
|
||||
forgetAccount(accountID)
|
||||
pendingDeletions.insert(accountID)
|
||||
forgetAccount(accountID)
|
||||
save()
|
||||
|
||||
NotificationCenter.default.post(name: .accountsDidChange, object: nil)
|
||||
@@ -649,6 +666,10 @@ final class AccountsManager {
|
||||
reconcileLegacyAccounts()
|
||||
|
||||
NotificationCenter.default.post(name: .accountsDidChange, object: nil)
|
||||
// The context belongs to the namespace too, so this reload replaced it.
|
||||
// Anything keyed to the selection — the legacy setting, the feed cache —
|
||||
// would otherwise keep serving the previous Apple account's data.
|
||||
NotificationCenter.default.post(name: .dataContextDidChange, object: nil)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -68,7 +68,7 @@ final class AppEnvironment {
|
||||
private(set) var activeControlsCenterSettings: CenterSectionSettings = .default
|
||||
|
||||
@ObservationIgnored private var controlsSettingsObservers: [NSObjectProtocol] = []
|
||||
@ObservationIgnored private var contextMirrorObserver: NSObjectProtocol?
|
||||
@ObservationIgnored private var contextMirrorObservers: [NSObjectProtocol] = []
|
||||
|
||||
// MARK: - Shared Instance
|
||||
|
||||
@@ -385,12 +385,17 @@ final class AppEnvironment {
|
||||
// setting several views still read has to follow it, and the feed cache
|
||||
// is global rather than account-keyed, so it would otherwise keep
|
||||
// serving the previous account's videos.
|
||||
contextMirrorObserver = NotificationCenter.default.addObserver(
|
||||
forName: .dataContextDidChange,
|
||||
object: nil,
|
||||
queue: .main
|
||||
) { [weak self] _ in
|
||||
MainActor.assumeIsolated { self?.mirrorContextToLegacySelection() }
|
||||
//
|
||||
// Accounts arriving is a second trigger, not a redundant one: a context
|
||||
// naming an account that has not synced yet cannot be mapped onto the
|
||||
// legacy shape, and this is what restores the mapping once it can be
|
||||
// made rather than leaving that moment permanent.
|
||||
for name: Notification.Name in [.dataContextDidChange, .accountsDidChange] {
|
||||
contextMirrorObservers.append(
|
||||
NotificationCenter.default.addObserver(forName: name, object: nil, queue: .main) { [weak self] _ in
|
||||
MainActor.assumeIsolated { self?.mirrorContextToLegacySelection() }
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
// A deletion that arrives from another device takes the credential with
|
||||
@@ -416,7 +421,15 @@ final class AppEnvironment {
|
||||
// relying on observer registration order to land after it would be
|
||||
// silently fragile.
|
||||
Task { @MainActor [weak self] in
|
||||
self?.refreshAccountStatuses()
|
||||
guard let self else { return }
|
||||
self.refreshAccountStatuses()
|
||||
|
||||
// The feed cache is not keyed by account, so whatever it holds
|
||||
// belongs to the Apple account that just went away. Cleared
|
||||
// unconditionally: unlike a context change, there is no state
|
||||
// here worth keeping across identities.
|
||||
SubscriptionFeedCache.shared.handleAccountChange()
|
||||
self.mirrorContextToLegacySelection(resetWhenUnresolvable: true)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -525,7 +538,12 @@ final class AppEnvironment {
|
||||
/// selection synced from another device, or the selected account being
|
||||
/// deleted there. Phase 3's switcher removes the last readers of the legacy
|
||||
/// setting and this mirror with them.
|
||||
private func mirrorContextToLegacySelection() {
|
||||
/// - Parameter resetWhenUnresolvable: whether a context that cannot be
|
||||
/// mapped should fall back to `.local` instead of leaving the previous
|
||||
/// value in place. True only when the Apple account changed: keeping the
|
||||
/// previous identity's selection is worse than losing a mapping, and the
|
||||
/// correct value returns as soon as the new namespace's accounts arrive.
|
||||
private func mirrorContextToLegacySelection(resetWhenUnresolvable: Bool = false) {
|
||||
let legacy: SubscriptionAccount
|
||||
|
||||
switch accountsManager.currentContext {
|
||||
@@ -538,7 +556,12 @@ final class AppEnvironment {
|
||||
// context is preserved, and the validator reports it unavailable
|
||||
// rather than pointing the UI at something else.
|
||||
guard let account = accountsManager.account(id: id),
|
||||
let instance = accountsManager.instance(for: account) else { return }
|
||||
let instance = accountsManager.instance(for: account) else {
|
||||
if resetWhenUnresolvable, settingsManager.subscriptionAccount != .local {
|
||||
settingsManager.subscriptionAccount = .local
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
switch instance.type {
|
||||
case .invidious: legacy = .invidious(instance.id)
|
||||
|
||||
@@ -41,10 +41,12 @@ final class BackgroundFeedRefresher {
|
||||
return
|
||||
}
|
||||
|
||||
// Route on the selected data context. An account context resolves its
|
||||
// own instance, so a second login on the same instance refreshes the
|
||||
// right user's feed.
|
||||
switch appEnvironment.accountsManager.currentContext {
|
||||
// Route on the *effective* context. An account context resolves its own
|
||||
// instance, so a second login on the same instance refreshes the right
|
||||
// user's feed — but an account whose instance is switched off or whose
|
||||
// credential has not arrived falls back to local rather than sending
|
||||
// background requests to a server the user is not currently using.
|
||||
switch appEnvironment.dataContextValidator.effectiveContext {
|
||||
case .local:
|
||||
await performLocalAccountRefresh()
|
||||
case let .account(id):
|
||||
|
||||
@@ -127,6 +127,24 @@ final class SubscriptionAccountValidator {
|
||||
return
|
||||
}
|
||||
|
||||
// A selection whose account still exists is never corrected here. This
|
||||
// validator predates account-scoped credentials and reads "not signed
|
||||
// in" as "not available", which is now routinely temporary — an
|
||||
// unresolved identity scope, or a credential iCloud Keychain has yet to
|
||||
// deliver. Correcting on that would silently discard the user's choice
|
||||
// and desynchronize this setting from the data context, which is
|
||||
// preserved. Only a deletion removes an account, and that path corrects
|
||||
// the context and this setting together.
|
||||
if let instanceID = settingsManager.subscriptionAccount.instanceID,
|
||||
let accountsManager,
|
||||
!accountsManager.accounts(forInstance: instanceID).isEmpty {
|
||||
LoggingService.shared.debug(
|
||||
"Leaving the subscription account alone — its login has not been removed",
|
||||
category: .general
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
let previousAccount = settingsManager.subscriptionAccount
|
||||
|
||||
// Find first available account to switch to
|
||||
|
||||
@@ -186,6 +186,50 @@ struct AccountsManagerTests {
|
||||
#expect(manager.context(forLegacyInstanceID: instanceID) == .account(second.id))
|
||||
}
|
||||
|
||||
@Test("Correcting away from a deleted account never outranks a real choice")
|
||||
func deletionCorrectionIsNotAChoice() {
|
||||
let manager = makeManager()
|
||||
let account = manager.addAccount(instanceID: UUID())
|
||||
|
||||
manager.setContext(.account(account.id))
|
||||
let chosenAt = manager.storedContext.changedAt
|
||||
manager.confirmContextUpload(changedAt: chosenAt)
|
||||
|
||||
manager.removeAccount(account.id)
|
||||
#expect(manager.currentContext == .local)
|
||||
|
||||
// Another device picked something else between the deletion and this
|
||||
// device hearing about it. Timestamping the fallback "now" would sink
|
||||
// that choice — and publishing it would push the fallback over it.
|
||||
#expect(manager.storedContext.changedAt == chosenAt)
|
||||
#expect(!manager.pendingContextUpload)
|
||||
|
||||
let elsewhere = StoredDataContext(
|
||||
context: .account(UUID()),
|
||||
changedAt: chosenAt.addingTimeInterval(1)
|
||||
)
|
||||
#expect(!manager.applySyncedContext(elsewhere))
|
||||
#expect(manager.currentContext == elsewhere.context)
|
||||
}
|
||||
|
||||
@Test("A deletion is queued before anything else can persist")
|
||||
func deletionOutboxIsQueuedFirst() {
|
||||
let defaults = makeDefaults()
|
||||
let manager = makeManager(defaults: defaults)
|
||||
let account = manager.addAccount(instanceID: UUID())
|
||||
manager.setContext(.account(account.id))
|
||||
|
||||
manager.removeAccount(account.id)
|
||||
|
||||
// Deleting the *selected* account also corrects the context, and every
|
||||
// step of that writes storage. A crash before the tombstone was owed
|
||||
// would leave the account gone here and alive everywhere else.
|
||||
let relaunched = makeManager(defaults: defaults)
|
||||
#expect(relaunched.pendingDeletions.contains(account.id))
|
||||
#expect(relaunched.isDeleted(account.id))
|
||||
#expect(relaunched.currentContext == .local)
|
||||
}
|
||||
|
||||
@Test("A migrated context loses to any explicit choice")
|
||||
func migratedContextLosesToExplicitChoice() {
|
||||
let manager = makeManager()
|
||||
|
||||
Reference in New Issue
Block a user