From 27b0806ad2e03e0b2369995b5422068f79518b83 Mon Sep 17 00:00:00 2001 From: Katherine Bertelsen Date: Thu, 6 Aug 2026 14:30:01 -0500 Subject: [PATCH 1/3] [PM-39767] fix: Show Sync Unsuccessful alert when a pending upgrade's sync attempt fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user with a pending Premium upgrade never got any feedback if the sync that would confirm it actually failed outright — only a generic "pending" state, indistinguishable from just still waiting. VaultListProcessor now shows a "Sync unsuccessful" alert the first time a sync attempt tied to the pending upgrade fails, with "Not now" and "Try again" (which retries via the same premiumStatusChanged() call the pending dialog's own "Sync Now" uses). Also fixes a staleness bug found while wiring this up: BillingService's reconciliation logic only cleared the failure flag when the account became Premium, so a later, unrelated sync that succeeded without granting Premium yet would leave the flag stuck at "failed" even though the most recent attempt had actually worked. --- .../en.lproj/Localizable.strings | 6 ++ .../Billing/Services/BillingService.swift | 17 +++- .../Services/BillingServiceTests.swift | 20 +++++ .../UI/Billing/Extensions/Alert+Billing.swift | 27 +++++++ .../Extensions/Alert+BillingTests.swift | 18 +++++ .../Vault/VaultList/VaultListProcessor.swift | 16 +++- .../VaultList/VaultListProcessorTests.swift | 77 +++++++++++++++++++ 7 files changed, 178 insertions(+), 3 deletions(-) diff --git a/BitwardenResources/Localizations/en.lproj/Localizable.strings b/BitwardenResources/Localizations/en.lproj/Localizable.strings index 97654f036a..0b4ddaf147 100644 --- a/BitwardenResources/Localizations/en.lproj/Localizable.strings +++ b/BitwardenResources/Localizations/en.lproj/Localizable.strings @@ -1142,6 +1142,12 @@ "OpeningCheckout" = "Opening checkout…"; "UpgradePending" = "Upgrade pending"; "YourUpgradeIsBeingProcessedDescriptionLong" = "Your upgrade is being processed. You'll remain on the free plan until it's complete. Sync now to check, or continue and it will update automatically."; +/* Generic button label for declining an action without dismissing it permanently. */ +"NotNow" = "Not now"; +/* Alert title shown when a sync attempt for a pending Premium upgrade fails. */ +"SyncUnsuccessful" = "Sync unsuccessful"; +/* Alert message shown when a sync attempt for a pending Premium upgrade fails. */ +"WeCouldntSyncYourVaultWithTheServerDescriptionLong" = "We couldn't sync your vault with the server. Your vault may not reflect your latest changes. You can try again now or it will sync automatically later."; "UpgradedToPremium" = "Upgraded to Premium"; "SubscriptionNeedsAttention" = "Your subscription needs attention"; "CheckYourPlanForDetails" = "Check your plan for details."; diff --git a/BitwardenShared/Core/Billing/Services/BillingService.swift b/BitwardenShared/Core/Billing/Services/BillingService.swift index a3ed59245d..664ee17654 100644 --- a/BitwardenShared/Core/Billing/Services/BillingService.swift +++ b/BitwardenShared/Core/Billing/Services/BillingService.swift @@ -398,11 +398,24 @@ class DefaultBillingService: BillingService { return } - guard await stateService.doesActiveAccountHavePremium() else { return } + // Reaching this point at all means a sync just completed successfully (this method + // is only invoked from a `lastSyncTimePublisher` emission, which never fires on + // failure), so the most recent attempt did not fail — clear that regardless of + // whether the account has become Premium yet, so a stale failure doesn't linger + // after a later, unrelated sync has actually succeeded. + do { + try await billingStateService.setPremiumUpgradeLastSyncAttemptFailed(false) + } catch { + errorReporter.log(error: error) + } + + guard await stateService.doesActiveAccountHavePremium() else { + await refreshPremiumUpgradePendingStateSubject() + return + } do { try await billingStateService.setPremiumUpgradePending(false) - try await billingStateService.setPremiumUpgradeLastSyncAttemptFailed(false) try await billingStateService.setUpgradedToPremiumActionCardVisible(true) } catch { errorReporter.log(error: error) diff --git a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift index 2642711938..14e24c1d42 100644 --- a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift +++ b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift @@ -608,6 +608,26 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length // MARK: start() + /// `start()` clears a stale `lastAttemptFailed` flag as soon as any sync succeeds, even + /// when the active account still isn't Premium yet — a later, unrelated sync succeeding + /// means the most recent attempt did not fail, regardless of whether Premium has been + /// granted. + @Test + func start_clearsLastAttemptFailedOnGenericSyncEvenWithoutPremium() async throws { + stateService.activeAccount = .fixture() + stateService.doesActiveAccountHavePremiumResult = false + + await subject.start() + + stateService.premiumUpgradePendingResult = true + stateService.premiumUpgradeLastSyncAttemptFailedResult = true + stateService.lastSyncTimeSubject.send(Date()) + + try await waitForAsync { stateService.premiumUpgradeLastSyncAttemptFailedResult == false } + #expect(stateService.premiumUpgradePendingResult == true) + #expect(stateService.upgradedToPremiumActionCardVisibleResult == false) + } + /// `start()` resolves a pending Premium upgrade when a generic sync completes and the /// active account has since become Premium, by any means (not just the original checkout /// attempt's own subscription). diff --git a/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift b/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift index 9f61007273..3359e56717 100644 --- a/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift +++ b/BitwardenShared/UI/Billing/Extensions/Alert+Billing.swift @@ -60,6 +60,33 @@ extension Alert { ) } + /// An alert shown when a sync attempt for a pending Premium upgrade fails. + /// + /// - Parameter tryAgainHandler: A closure called when the user taps "Try again". + /// - Returns: An `Alert` with "Not now" and "Try again" actions. + /// + static func syncUnsuccessful( + tryAgainHandler: @escaping () async -> Void, + ) -> Alert { + Alert( + title: Localizations.syncUnsuccessful, + message: Localizations.weCouldntSyncYourVaultWithTheServerDescriptionLong, + alertActions: [ + AlertAction( + title: Localizations.notNow, + style: .cancel, + ), + AlertAction( + title: Localizations.tryAgain, + style: .default, + handler: { _, _ in + await tryAgainHandler() + }, + ), + ], + ) + } + /// An alert shown when a Premium upgrade is still being processed. /// /// - Parameter syncNowHandler: A closure called when the user taps "Sync now". diff --git a/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift b/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift index 1a3f26ed7d..4e4cc9e07c 100644 --- a/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift +++ b/BitwardenShared/UI/Billing/Extensions/Alert+BillingTests.swift @@ -47,6 +47,24 @@ class AlertBillingTests: BitwardenTestCase { XCTAssertEqual(tryAgainAction.style, .default) } + /// `syncUnsuccessful(tryAgainHandler:)` builds an `Alert` with the correct title, message, and actions. + func test_syncUnsuccessful() { + let subject = Alert.syncUnsuccessful {} + + XCTAssertEqual(subject.title, Localizations.syncUnsuccessful) + XCTAssertEqual(subject.message, Localizations.weCouldntSyncYourVaultWithTheServerDescriptionLong) + XCTAssertEqual(subject.preferredStyle, .alert) + XCTAssertEqual(subject.alertActions.count, 2) + + let notNowAction = subject.alertActions[0] + XCTAssertEqual(notNowAction.title, Localizations.notNow) + XCTAssertEqual(notNowAction.style, .cancel) + + let tryAgainAction = subject.alertActions[1] + XCTAssertEqual(tryAgainAction.title, Localizations.tryAgain) + XCTAssertEqual(tryAgainAction.style, .default) + } + /// `upgradePending(syncNowHandler:)` builds an `Alert` with the correct title, message, and actions. func test_upgradePending() { let subject = Alert.upgradePending {} diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift index 1909035868..63cef37c31 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift @@ -703,11 +703,25 @@ extension VaultListProcessor { /// Streams live updates to the Premium upgrade pending state, hiding or re-evaluating the /// upsell action card immediately rather than waiting for the next screen appearance — /// needed because dismissing the "Upgrade Pending" alert returns to this same screen - /// without a fresh `.appeared`. + /// without a fresh `.appeared`. Also shows the "Sync Unsuccessful" alert the first time a + /// sync attempt for the pending upgrade fails. /// private func streamPremiumUpgradePendingState() async { + var lastAttemptFailed = false for await pendingState in services.billingService.premiumUpgradePendingStatePublisher().values { await updatePremiumUpgradeActionCardVisibility(isPending: pendingState.isPending) + + // Only show the alert on a false-to-true transition — the publisher replays its + // current value to this subscription immediately (covering a stale failure still + // persisted from a previous app session), but without tracking the transition, any + // later emission that still has `lastAttemptFailed: true` (e.g. triggered by an + // unrelated `isPending` change) would incorrectly re-show the alert. + if pendingState.lastAttemptFailed, !lastAttemptFailed { + coordinator.showAlert(.syncUnsuccessful { [weak self] in + await self?.services.billingService.premiumStatusChanged() + }) + } + lastAttemptFailed = pendingState.lastAttemptFailed } } diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift index 718234d3db..f0b847b8ed 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift @@ -1146,6 +1146,83 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ task.cancel() } + /// `perform(_:)` with `.streamPremiumUpgradePendingState` shows the "Sync unsuccessful" + /// alert the first time the publisher reports a failed sync attempt. + @MainActor + func test_perform_streamPremiumUpgradePendingState_showsSyncUnsuccessfulAlert() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: false), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + waitFor(!coordinator.alertShown.isEmpty) + + XCTAssertEqual(coordinator.alertShown.count, 1) + XCTAssertEqual(coordinator.alertShown.last?.title, Localizations.syncUnsuccessful) + } + + /// `perform(_:)` with `.streamPremiumUpgradePendingState` does not re-show the "Sync + /// unsuccessful" alert on a later, duplicate emission that still reports a failed attempt. + @MainActor + func test_perform_streamPremiumUpgradePendingState_doesNotReshowSyncUnsuccessfulAlert() { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + waitFor(!coordinator.alertShown.isEmpty) + + // A later emission with the same `lastAttemptFailed: true` (e.g. triggered by an + // unrelated `isPending` change) should not re-show the alert. + pendingStateSubject.send(PremiumUpgradePendingState(isPending: false, lastAttemptFailed: true)) + waitFor(subject.state.shouldShowPremiumUpgradeActionCard == false) + + XCTAssertEqual(coordinator.alertShown.count, 1) + } + + /// `perform(_:)` with `.streamPremiumUpgradePendingState` — tapping "Try again" on the + /// "Sync unsuccessful" alert retries via `premiumStatusChanged()`. + /// + /// Kept synchronous (not `async`) deliberately: this test runs a background `Task` + /// consuming an infinite stream while also driving another async call (`tapAction`) from + /// the test body. If the test function itself were `async`, it would run on Swift's + /// cooperative thread pool, and the blocking `waitFor` helper below would starve that same + /// pool, preventing the background `Task` from ever being scheduled — a real deadlock this + /// test hit before being fixed. Running synchronously (on the XCTest main thread, not the + /// cooperative pool) avoids that; `tapAction`'s own async call is driven via a second `Task` + /// polled with `waitFor` instead of an inline `await`. + @MainActor + func test_perform_streamPremiumUpgradePendingState_syncUnsuccessfulAlert_tryAgain() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + waitFor(!coordinator.alertShown.isEmpty) + let alert = try XCTUnwrap(coordinator.alertShown.last) + + Task { + try await alert.tapAction(title: Localizations.tryAgain) + } + waitFor(billingService.premiumStatusChangedCallsCount == 1) + } + /// `perform(_:)` with `.streamShowWebIcons` requests the value of the show /// web icons parameter from the state service. @MainActor From 9287e47c8e7c838cf97ab8184c97bfb2e23b9698 Mon Sep 17 00:00:00 2001 From: Katherine Bertelsen Date: Tue, 11 Aug 2026 13:30:27 -0500 Subject: [PATCH 2/3] Fix reconciliation false-clear on subscribe replay and strengthen dedup test reconcilePendingUpgradeIfNeeded() was reachable from the last-sync-time publisher's initial replay on subscribe (a CurrentValueSubject cached value, not evidence of a new sync), which could clear a genuine stale lastAttemptFailed flag before any real sync happened. Now compares each emission against a directly-snapshotted baseline instead of relying on positional/first-emission semantics, which also naturally ignores re-emissions caused by a different account's sync updating the shared last-sync-time store. Also rewrote the VaultListProcessor de-dup test, whose wait condition was trivially satisfied before the emission it claimed to verify was even processed. --- .../Billing/Services/BillingService.swift | 24 +++++++++---- .../Services/BillingServiceTests.swift | 36 +++++++++++++++++++ .../TestHelpers/MockStateService.swift | 9 ++++- .../VaultList/VaultListProcessorTests.swift | 15 +++++--- 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/BitwardenShared/Core/Billing/Services/BillingService.swift b/BitwardenShared/Core/Billing/Services/BillingService.swift index 664ee17654..6b6e8a0a96 100644 --- a/BitwardenShared/Core/Billing/Services/BillingService.swift +++ b/BitwardenShared/Core/Billing/Services/BillingService.swift @@ -376,7 +376,19 @@ class DefaultBillingService: BillingService { self.currentSyncSubscriber = Task { guard let publisher = try? await self.stateService.lastSyncTimePublisher() else { return } - for await _ in publisher.values { + // Snapshot the last-known sync time directly, rather than relying on + // whichever value the publisher happens to deliver first: its + // `CurrentValueSubject` backing replays the existing cached value + // immediately on subscribe (not evidence that a sync just happened), and a + // *different* account's sync can also re-emit this shared store without this + // account's own value having changed. Comparing each emission against the + // last value actually seen — rather than its position in the stream — means + // only a genuinely new sync for this account reaches + // `reconcilePendingUpgradeIfNeeded()`, regardless of subscription timing. + var lastSeenDate = try? await self.stateService.getLastSyncTime() + for await date in publisher.values { + guard date != lastSeenDate else { continue } + lastSeenDate = date await self.reconcilePendingUpgradeIfNeeded() } } @@ -398,11 +410,11 @@ class DefaultBillingService: BillingService { return } - // Reaching this point at all means a sync just completed successfully (this method - // is only invoked from a `lastSyncTimePublisher` emission, which never fires on - // failure), so the most recent attempt did not fail — clear that regardless of - // whether the account has become Premium yet, so a stale failure doesn't linger - // after a later, unrelated sync has actually succeeded. + // Reaching this point at all means a *new* sync just completed successfully — `start()` + // filters out both the initial replay and same-value re-emissions before invoking this + // method, and `lastSyncTimePublisher` never fires on failure — so the most recent + // attempt did not fail. Clear that regardless of whether the account has become Premium + // yet, so a stale failure doesn't linger after a later, unrelated sync has succeeded. do { try await billingStateService.setPremiumUpgradeLastSyncAttemptFailed(false) } catch { diff --git a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift index 14e24c1d42..86dfc96588 100644 --- a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift +++ b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift @@ -618,6 +618,12 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length stateService.doesActiveAccountHavePremiumResult = false await subject.start() + // Let the last-sync-time subscription settle before sending a "new" value below — the + // publisher's `CurrentValueSubject` backing would otherwise coalesce an immediate send + // with the not-yet-established subscription's own initial read, making it ambiguous + // whether this counts as a change. + await Task.yield() + await Task.yield() stateService.premiumUpgradePendingResult = true stateService.premiumUpgradeLastSyncAttemptFailedResult = true @@ -628,6 +634,33 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length #expect(stateService.upgradedToPremiumActionCardVisibleResult == false) } + /// `start()` does not clear a stale `lastAttemptFailed` flag just from subscribing to the + /// last-sync-time publisher — its `CurrentValueSubject` backing replays the existing cached + /// value immediately on subscribe, and that replay must not be mistaken for a new sync + /// completing. + @Test + func start_doesNotClearLastAttemptFailedOnInitialSubscriptionReplay() async throws { + stateService.activeAccount = .fixture() + stateService.doesActiveAccountHavePremiumResult = true + stateService.lastSyncTimeSubject.send(Date()) + stateService.premiumUpgradePendingResult = true + stateService.premiumUpgradeLastSyncAttemptFailedResult = true + + await subject.start() + await Task.yield() + await Task.yield() + + #expect(stateService.premiumUpgradeLastSyncAttemptFailedResult == true) + #expect(stateService.setPremiumUpgradeLastSyncAttemptFailedCallCount == 0) + + // A genuinely new sync, sent only once the initial subscription has settled, still + // clears the flag as expected. + stateService.lastSyncTimeSubject.send(Date()) + + try await waitForAsync { stateService.premiumUpgradeLastSyncAttemptFailedResult == false } + #expect(stateService.setPremiumUpgradeLastSyncAttemptFailedCallCount == 1) + } + /// `start()` resolves a pending Premium upgrade when a generic sync completes and the /// active account has since become Premium, by any means (not just the original checkout /// attempt's own subscription). @@ -637,6 +670,9 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length stateService.doesActiveAccountHavePremiumResult = false await subject.start() + // See the comment in `start_clearsLastAttemptFailedOnGenericSyncEvenWithoutPremium()`. + await Task.yield() + await Task.yield() stateService.premiumUpgradePendingResult = true stateService.premiumUpgradeLastSyncAttemptFailedResult = true diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift index e64eaf9e44..3243c3589e 100644 --- a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift +++ b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift @@ -38,6 +38,8 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS var premiumUpgradeLastSyncAttemptFailedResult: Bool = false // swiftlint:disable:this identifier_name var premiumUpgradePendingResult: Bool = false // swiftlint:disable:next identifier_name + var setPremiumUpgradeLastSyncAttemptFailedCallCount = 0 + // swiftlint:disable:next identifier_name var setPremiumUpgradeLastSyncAttemptFailedResult: Result = .success(()) var setPremiumUpgradePendingResult: Result = .success(()) var setSubscriptionAttentionCardResult: Result = .success(()) @@ -390,7 +392,11 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS func getLastSyncTime(userId: String?) async throws -> Date? { let userId = try unwrapUserId(userId) - return lastSyncTimeByUserId[userId] + // Falls back to `lastSyncTimeSubject`, mirroring `DefaultStateService`, where + // `getLastSyncTime(userId:)` and `lastSyncTimePublisher()` both read from the same + // underlying store — tests that drive `lastSyncTimeSubject` directly (rather than + // through `setLastSyncTime(_:userId:)`) still see a consistent value from either call. + return lastSyncTimeByUserId[userId] ?? lastSyncTimeSubject.value } func getLastSyncMonotonicTime(userId: String?) async throws -> TimeInterval? { @@ -663,6 +669,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS } func setPremiumUpgradeLastSyncAttemptFailed(_ failed: Bool) async throws { + setPremiumUpgradeLastSyncAttemptFailedCallCount += 1 try setPremiumUpgradeLastSyncAttemptFailedResult.get() premiumUpgradeLastSyncAttemptFailedResult = failed } diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift index f0b847b8ed..10d0724834 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift @@ -1171,8 +1171,9 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ /// unsuccessful" alert on a later, duplicate emission that still reports a failed attempt. @MainActor func test_perform_streamPremiumUpgradePendingState_doesNotReshowSyncUnsuccessfulAlert() { + billingRepository.isInAppUpgradeAvailableReturnValue = true let pendingStateSubject = CurrentValueSubject( - PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: false), ) billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() @@ -1181,12 +1182,18 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ } defer { task.cancel() } + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) waitFor(!coordinator.alertShown.isEmpty) + XCTAssertEqual(coordinator.alertShown.count, 1) - // A later emission with the same `lastAttemptFailed: true` (e.g. triggered by an - // unrelated `isPending` change) should not re-show the alert. + // A duplicate emission with the same `lastAttemptFailed: true` (e.g. triggered by an + // unrelated `isPending` change) should not re-show the alert. Combine delivers a single + // publisher's emissions in order, so sending a distinct, independently-observable + // follow-up and waiting on it proves the duplicate before it was already processed — + // without that, `waitFor` could return before the duplicate was even handled. + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) pendingStateSubject.send(PremiumUpgradePendingState(isPending: false, lastAttemptFailed: true)) - waitFor(subject.state.shouldShowPremiumUpgradeActionCard == false) + waitFor(subject.state.shouldShowPremiumUpgradeActionCard) XCTAssertEqual(coordinator.alertShown.count, 1) } From b98c74e8e0734b4039bbb8bdcbcdda4793deb118 Mon Sep 17 00:00:00 2001 From: Katherine Bertelsen Date: Tue, 11 Aug 2026 14:28:00 -0500 Subject: [PATCH 3/3] Replace flaky yield-based test sync with deterministic wait; fix silent retry failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task.yield() twice didn't reliably settle start()'s subscription before the test's next send() — DefaultBillingService isn't @MainActor, so the yields on the test's actor had no ordering relationship with the service's cooperative-pool tasks. Replaced with a wait on a new getLastSyncTimeCallCount, which fires exactly once at the point start() takes its baseline snapshot. Also fixed streamPremiumUpgradePendingState(): tapping "Try again" on the Sync Unsuccessful alert never reset the transition tracker, so a retry that also failed produced no alert and no other feedback, silently doing nothing from the user's perspective. MockStateService.setLastSyncTime(_:userId:) now also updates lastSyncTimeSubject, matching DefaultStateService's write-side symmetry and closing a latent trap where clearing the dict entry could leave a stale subject value behind. --- .../Services/BillingServiceTests.swift | 19 ++++++------ .../TestHelpers/MockStateService.swift | 11 ++++--- .../Vault/VaultList/VaultListProcessor.swift | 5 ++++ .../VaultList/VaultListProcessorTests.swift | 29 +++++++++++++++++++ 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift index 86dfc96588..de83f77fc9 100644 --- a/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift +++ b/BitwardenShared/Core/Billing/Services/BillingServiceTests.swift @@ -618,12 +618,13 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length stateService.doesActiveAccountHavePremiumResult = false await subject.start() - // Let the last-sync-time subscription settle before sending a "new" value below — the - // publisher's `CurrentValueSubject` backing would otherwise coalesce an immediate send - // with the not-yet-established subscription's own initial read, making it ambiguous - // whether this counts as a change. - await Task.yield() - await Task.yield() + // Wait for the subscription's baseline snapshot before sending a "new" value below — + // `start()` reads `getLastSyncTime()` exactly once, before subscribing to the publisher. + // Without this, the publisher's `CurrentValueSubject` backing could coalesce an + // immediate send with the not-yet-taken snapshot, making it ambiguous whether the send + // counts as a change (both nested `Task`s run on the cooperative pool, so a fixed + // number of `Task.yield()` calls from this `@MainActor` test isn't a reliable proxy). + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } stateService.premiumUpgradePendingResult = true stateService.premiumUpgradeLastSyncAttemptFailedResult = true @@ -647,8 +648,7 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length stateService.premiumUpgradeLastSyncAttemptFailedResult = true await subject.start() - await Task.yield() - await Task.yield() + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } #expect(stateService.premiumUpgradeLastSyncAttemptFailedResult == true) #expect(stateService.setPremiumUpgradeLastSyncAttemptFailedCallCount == 0) @@ -671,8 +671,7 @@ struct BillingServiceTests { // swiftlint:disable:this type_body_length await subject.start() // See the comment in `start_clearsLastAttemptFailedOnGenericSyncEvenWithoutPremium()`. - await Task.yield() - await Task.yield() + try await waitForAsync { stateService.getLastSyncTimeCallCount == 1 } stateService.premiumUpgradePendingResult = true stateService.premiumUpgradeLastSyncAttemptFailedResult = true diff --git a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift index 3243c3589e..7f32d62e2b 100644 --- a/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift +++ b/BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift @@ -95,6 +95,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS var getAccountHasBeenUnlockedInteractivelyResult: Result = .success(false) var getActiveAccountIdError: Error? var getBiometricAuthenticationEnabledResult: Result = .success(()) + var getLastSyncTimeCallCount = 0 var lastRequestToTurnOnCredentialProvider: Date? var lastSyncMonotonicTimeByUserId = [String: TimeInterval?]() var lastSyncTimeByUserId = [String: Date]() @@ -391,11 +392,12 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS } func getLastSyncTime(userId: String?) async throws -> Date? { + getLastSyncTimeCallCount += 1 let userId = try unwrapUserId(userId) - // Falls back to `lastSyncTimeSubject`, mirroring `DefaultStateService`, where - // `getLastSyncTime(userId:)` and `lastSyncTimePublisher()` both read from the same - // underlying store — tests that drive `lastSyncTimeSubject` directly (rather than - // through `setLastSyncTime(_:userId:)`) still see a consistent value from either call. + // Falls back to `lastSyncTimeSubject` for tests that drive it directly via `.send(_:)` + // instead of going through `setLastSyncTime(_:userId:)`, which keeps both stores in + // sync (mirroring `DefaultStateService`, where both calls read from the same + // underlying store). return lastSyncTimeByUserId[userId] ?? lastSyncTimeSubject.value } @@ -782,6 +784,7 @@ class MockStateService: StateService, ActiveAccountStateProvider, AutofillStateS func setLastSyncTime(_ date: Date?, userId: String?) async throws { let userId = try unwrapUserId(userId) lastSyncTimeByUserId[userId] = date + lastSyncTimeSubject.value = date } func getLastUserShouldConnectToWatch() async -> Bool { diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift index 63cef37c31..0749990112 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift @@ -718,6 +718,11 @@ extension VaultListProcessor { // unrelated `isPending` change) would incorrectly re-show the alert. if pendingState.lastAttemptFailed, !lastAttemptFailed { coordinator.showAlert(.syncUnsuccessful { [weak self] in + // Reset the tracker before retrying — if the retry's own sync also fails, + // the resulting `lastAttemptFailed: true` emission must be treated as a new + // transition rather than a duplicate, or this explicit, user-initiated + // retry would fail with no feedback at all. + lastAttemptFailed = false await self?.services.billingService.premiumStatusChanged() }) } diff --git a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift index 10d0724834..e61105423a 100644 --- a/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift +++ b/BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift @@ -1230,6 +1230,35 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ waitFor(billingService.premiumStatusChangedCallsCount == 1) } + /// `perform(_:)` with `.streamPremiumUpgradePendingState` re-shows the "Sync Unsuccessful" + /// alert if the user's own "Try again" retry also fails — the transition tracker must reset + /// before retrying, or this explicit, user-initiated retry would fail with no feedback. + @MainActor + func test_perform_streamPremiumUpgradePendingState_syncUnsuccessfulAlert_tryAgainFails() throws { + let pendingStateSubject = CurrentValueSubject( + PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true), + ) + billingService.premiumUpgradePendingStatePublisherReturnValue = pendingStateSubject.eraseToAnyPublisher() + billingService.premiumStatusChangedClosure = { + pendingStateSubject.send(PremiumUpgradePendingState(isPending: true, lastAttemptFailed: true)) + } + + let task = Task { + await subject.perform(.streamPremiumUpgradePendingState) + } + defer { task.cancel() } + + waitFor(!coordinator.alertShown.isEmpty) + let alert = try XCTUnwrap(coordinator.alertShown.last) + + Task { + try await alert.tapAction(title: Localizations.tryAgain) + } + waitFor(coordinator.alertShown.count == 2) + + XCTAssertEqual(coordinator.alertShown.last?.title, Localizations.syncUnsuccessful) + } + /// `perform(_:)` with `.streamShowWebIcons` requests the value of the show /// web icons parameter from the state service. @MainActor