Keep an automatic repair from ever reaching the server

A repair carries the timestamp of the selection it replaced, which is what
stops it outranking a deliberate choice made elsewhere — but it also makes
every ordering comparison read it as that choice. A fetch holding the
selected account's tombstone and an older server context applied them in
that order: the tombstone repaired the selection, and the older context
then lost to it, so the "the server holds the stale copy" path marked the
repair dirty and uploaded an automatic fallback under the deleted choice's
timestamp, over an explicit choice. The stored context now remembers
whether it is a repair, and a repair that wins an ordering pushes nothing
back: the server's context stands remotely, the fallback stays local, and
the next explicit choice on any device re-converges everyone. The bit is
persisted with the context, or a relaunch would forget and publish it on
the first stale record it read.

Three more from the same trace:

- The repair prefers a signed-in login on the same server, decided off
  statuses, and it commits — once the selection no longer names the deleted
  account there is nothing left for a later status to move. Foreground
  activation refreshes statuses ahead of it, but a deletion delivered by a
  silent push arrives outside that sequence, and a credential that reached
  this device over iCloud Keychain while it was in the background announces
  nothing. The refresh is now asked for by the repair itself, which closes
  every entry point rather than each fetch trigger having to remember one.

- A device that meets a tombstone before the account it kills cannot record
  which instance that account lived on, and the account record arriving
  afterwards was discarded — with the only copy of that fact still in it.
  Every later repair of a context naming it then fell to local past a
  usable sibling. The association is kept as the record is dropped.

- Removing an orphaned account deleted its credential before recording the
  deletion, so a crash in between brought the account back on the next
  launch as one nobody can sign into, owing no deletion to any device. The
  outbox goes first, here and in the sign-out and instance cascades; the
  scoped credential key needs only the account's ID, and the store is
  resolved through the instance before anything is removed.
This commit is contained in:
Arkadiusz Fal
2026-09-05 23:06:10 +02:00
parent ca73b68ad9
commit ccf5c51460
7 changed files with 386 additions and 13 deletions

View File

@@ -689,6 +689,78 @@ struct AccountSyncTests {
#expect(harness.accounts.storedContext.changedAt == stale.changedAt)
}
@Test("A repair made mid-batch is never uploaded over the server's older context")
func repairIsNotPublishedForAStaleServerContext() async {
let instances = InstancesManager()
let instance = Instance(type: .invidious, url: URL(string: "https://inv-\(UUID().uuidString).example.com")!)
instances.add(instance)
defer { instances.remove(instance) }
let harness = Self.makeHarness(instances: instances)
let selected = harness.accounts.addAccount(instanceID: instance.id)
let sibling = harness.accounts.addAccount(instanceID: instance.id)
harness.accounts.setStatus(.signedIn, for: sibling.id)
// Both accounts are on the server already, so nothing holds the context
// back for ordering reasons.
harness.accounts.confirmUpload(selected.id)
harness.accounts.confirmUpload(sibling.id)
harness.accounts.setContext(.account(selected.id))
let chosenAt = harness.accounts.storedContext.changedAt
harness.accounts.confirmContextUpload(changedAt: chosenAt)
// Tombstones are applied first, so the repair is in place — carrying the
// deleted choice's timestamp — by the time the older context is weighed
// against it.
let older = StoredDataContext(context: .local, changedAt: chosenAt.addingTimeInterval(-60))
harness.coordinator.stageFetched(
records: [
harness.mapper.toCKRecord(accountDeletionID: selected.id),
harness.mapper.toCKRecord(dataContext: older)
],
deletions: []
)
harness.recorder.clear()
await harness.coordinator.activate()
#expect(harness.accounts.currentContext == .account(sibling.id))
#expect(!harness.recorder.savedNames.contains("datacontext-current"))
#expect(harness.coordinator.recordToSave(named: "datacontext-current") == nil)
}
@Test("An account discarded for its tombstone still repairs the context")
func discardedAccountKeepsItsInstance() async {
let instances = InstancesManager()
let instance = Instance(type: .invidious, url: URL(string: "https://inv-\(UUID().uuidString).example.com")!)
instances.add(instance)
defer { instances.remove(instance) }
let harness = Self.makeHarness(instances: instances)
let sibling = harness.accounts.addAccount(instanceID: instance.id)
harness.accounts.setStatus(.signedIn, for: sibling.id)
// A fresh device meets all three at once: the tombstone runs first,
// while the account it kills is still unknown here, and the record that
// follows is the only thing that says which server it lived on.
let deleted = Account(instanceID: instance.id)
harness.coordinator.stageFetched(
records: [
harness.mapper.toCKRecord(accountDeletionID: deleted.id),
harness.mapper.toCKRecord(account: deleted),
harness.mapper.toCKRecord(
dataContext: StoredDataContext(context: .account(deleted.id), changedAt: Date())
)
],
deletions: []
)
await harness.coordinator.activate()
#expect(harness.accounts.deletedAccountInstanceIDs[deleted.id] == instance.id)
// Repaired onto the login the user can actually use, rather than falling
// all the way back to local because the metadata was thrown away.
#expect(harness.accounts.currentContext == .account(sibling.id))
}
@Test("Records fetched before the identity resolves are recovered by a bootstrap")
func unresolvedIdentityForcesAnotherBootstrap() async {
let defaults = Self.makeDefaults()

View File

@@ -743,11 +743,14 @@ struct DataContextRepairTests {
Instance(type: .invidious, url: URL(string: "https://\(host)-\(UUID().uuidString).example.com")!)
}
private func makeManager(instances: InstancesManager) -> AccountsManager {
private func makeManager(
instances: InstancesManager,
defaults: UserDefaults? = nil
) -> AccountsManager {
AccountsManager(
identityScope: CredentialIdentityScope(fixedScope: "identity-A"),
instancesManager: instances,
defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")!
defaults: defaults ?? UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")!
)
}
@@ -950,6 +953,113 @@ struct DataContextRepairTests {
#expect(manager.currentContext == .local)
#expect(manager.pendingContextUpload)
}
@Test("A repair is never pushed back over the server's older context")
func repairIsNeverPushedBackOverAnOlderContext() {
let instances = InstancesManager()
let instance = makeInstance("inv")
instances.add(instance)
defer { instances.remove(instance) }
let defaults = UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")!
let manager = makeManager(instances: instances, defaults: defaults)
let selected = manager.addAccount(instanceID: instance.id)
let sibling = manager.addAccount(instanceID: instance.id)
manager.setStatus(.signedIn, for: sibling.id)
manager.setContext(.account(selected.id))
let chosenAt = manager.storedContext.changedAt
manager.confirmContextUpload(changedAt: chosenAt)
#expect(!manager.pendingContextUpload)
// One fetch carrying both: the tombstone repairs the selection, keeping
// the deleted choice's timestamp, and the older context the server still
// holds arrives right behind it.
manager.applyRemoteTombstone(selected.id)
#expect(manager.currentContext == .account(sibling.id))
let older = StoredDataContext(context: .local, changedAt: chosenAt.addingTimeInterval(-60))
#expect(!manager.applySyncedContext(older))
// The repair only outranks the server's copy because it inherited that
// timestamp. Publishing it would put a fallback nobody chose over an
// explicit choice — the server's context stands, and the next real
// selection anywhere re-converges everyone.
#expect(manager.currentContext == .account(sibling.id))
#expect(manager.storedContext.changedAt == chosenAt)
#expect(!manager.pendingContextUpload)
// Still a repair after a relaunch, or the next stale record read from
// disk would publish it.
let reloaded = makeManager(instances: instances, defaults: defaults)
#expect(!reloaded.applySyncedContext(older))
#expect(!reloaded.pendingContextUpload)
// Choosing it deliberately is a different thing entirely, and publishes.
reloaded.setContext(.account(sibling.id))
#expect(reloaded.applySyncedContext(older))
#expect(reloaded.pendingContextUpload)
}
@Test("A repair recomputes statuses before it picks a sibling")
func repairRefreshesStatusesBeforeChoosing() {
let instances = InstancesManager()
let instance = makeInstance("inv")
instances.add(instance)
defer { instances.remove(instance) }
let manager = makeManager(instances: instances)
let selected = manager.addAccount(instanceID: instance.id)
let sibling = manager.addAccount(instanceID: instance.id)
// The sibling's credential arrived over iCloud Keychain while the app
// was in the background. Nothing that recomputes statuses ran, so it
// still reads as signed out.
manager.setStatus(.credentialMissing, for: sibling.id)
manager.setContext(.account(selected.id))
var refreshes = 0
manager.refreshStatusesForRepair = { [weak manager] in
refreshes += 1
manager?.setStatus(.signedIn, for: sibling.id)
}
// A tombstone delivered by a silent push, with no foreground activation
// to sequence the refresh ahead of it.
manager.applyRemoteTombstone(selected.id)
#expect(refreshes == 1)
#expect(manager.currentContext == .account(sibling.id))
}
@Test("An account dominated by a tombstone still says where it lived")
func discardedAccountKeepsItsInstanceForRepair() {
let instances = InstancesManager()
let instance = makeInstance("inv")
instances.add(instance)
defer { instances.remove(instance) }
let manager = makeManager(instances: instances)
let sibling = manager.addAccount(instanceID: instance.id)
manager.setStatus(.signedIn, for: sibling.id)
// A device that meets the tombstone before the account it kills has
// nothing to read the instance off — the map entry cannot be made then.
let deleted = Account(instanceID: instance.id)
manager.applyRemoteTombstone(deleted.id)
#expect(manager.deletedAccountInstanceIDs[deleted.id] == nil)
// The dominated record that follows is the only thing that still knows.
manager.applySyncedAccount(deleted)
#expect(manager.account(id: deleted.id) == nil)
#expect(manager.deletedAccountInstanceIDs[deleted.id] == instance.id)
// Which is what lets the context naming it land on the sibling instead
// of falling all the way back to local.
manager.applySyncedContext(
StoredDataContext(context: .account(deleted.id), changedAt: Date())
)
#expect(manager.currentContext == .account(sibling.id))
}
}
@Suite("Account capabilities")

View File

@@ -152,6 +152,35 @@ struct OrphanedAccountRemovalTests {
#expect(accountsManager.pendingDeletions.contains(account.id))
}
@Test("The deletion is durable before the secret is destroyed")
func removalRecordsTheTombstoneFirst() {
let scope = makeScope()
let instances = InstancesManager()
let instance = makeInstance(.invidious)
instances.add(instance)
let accountsManager = AccountsManager(
identityScope: scope,
instancesManager: instances,
defaults: makeDefaults()
)
let account = accountsManager.addAccount(instanceID: instance.id)
instances.remove(instance)
let store = DeletionOrderStore(accountsManager: accountsManager)
AccountRemoval.removeOrphan(
account.id,
stores: [store],
accountsManager: accountsManager,
discardData: { _ in }
)
// A crash in between would otherwise bring the account back on the next
// launch — signed out, with no deletion owed to any other device.
#expect(store.tombstoneWhenDeleted[account.id] == true)
}
@Test("The pre-revamp instance-keyed item is deliberately left alone")
func removalLeavesTheLegacyItem() {
let scope = makeScope()
@@ -186,6 +215,44 @@ struct OrphanedAccountRemovalTests {
}
}
/// A credential store that records what the account manager already knew at the
/// moment it was asked to destroy a secret.
///
/// The ordering is the whole point of the cascade and cannot be observed from
/// its outcome — both orders end in the same state unless the process dies
/// halfway.
@Observable
@MainActor
private final class DeletionOrderStore: AccountCredentialsManager {
private let accountsManager: AccountsManager
var tombstoneWhenDeleted: [UUID: Bool] = [:]
init(accountsManager: AccountsManager) {
self.accountsManager = accountsManager
}
var loggedInAccountIDs: Set<UUID> { [] }
var isScopeResolved: Bool { true }
func setCredential(_: String, for _: Account) {}
func credential(for _: Account) -> String? { nil }
func deleteCredential(for account: Account) { deleteCredential(forAccountID: account.id) }
func deleteCredential(forAccountID accountID: UUID) {
tombstoneWhenDeleted[accountID] = accountsManager.isDeleted(accountID)
&& accountsManager.pendingDeletions.contains(accountID)
}
func resyncCredentialsWithCurrentSetting() {}
func setUsername(_: String, for _: Account) {}
func username(for _: Account) -> String? { nil }
func isLoggedIn(for _: Account) -> Bool { false }
func refreshLoginStatus(for _: Account) {}
func hasAdoptableLegacyCredential(for _: Account) -> Bool { false }
@discardableResult func adoptLegacyCredential(for _: Account) -> Bool { false }
func deleteLegacyCredentialIfOwned(for _: Account) {}
}
@Suite("Instance deletion disclosure")
struct InstanceDeletionDisclosureTests {
@Test("A source with no accounts adds nothing to the question")