From dbca1d476fddf518c4aa907e6a092176e65973fc Mon Sep 17 00:00:00 2001 From: variablefate Date: Tue, 5 May 2026 17:47:24 -0700 Subject: [PATCH 1/5] feat(sdk): rethrow publishProfileAndMark + ProfileBackupCoordinator.publishAndMark Onboarding watchdog (ADR-0016) inferred publish failure from a sync-store dirty flag because both `publishProfileAndMark` and `publishAndMark` swallowed errors at the SDK boundary. Even an instant relay rejection left the user staring for the full 60s window before the banner appeared. Promote the two helpers to `async throws`. AppState's onboarding path catches and runs the same connectivity-gated check the watchdog uses, firing the banner immediately when online (and deferring to the watchdog's offline-park branch when not). The watchdog stays as the hang-case safety net. `ProfileBackupCoordinator.publishAndMark` preserves its republish-loop coalescing semantic via a last-iteration error: a coalesced republish that succeeds rescues an earlier failed iteration; only a terminal failure throws. Non-onboarding callers (`SyncCoordinator.flushPendingSyncPublishes`, `SyncDomainStrategy.publishLocal`, settings/locations UI taps) wrap with `try?` to preserve their best-effort contract. ADR-0017 documents the change. Closes #97 item 2. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../RoadFlare/ProfileBackupCoordinator.swift | 21 +++- .../RoadFlare/RoadflareDomainService.swift | 17 ++- .../ProfileBackupCoordinatorTests.swift | 26 +++-- .../RoadflareDomainServiceTests.swift | 23 +++- .../Views/Settings/PaymentMethodsScreen.swift | 2 +- .../Views/Settings/SavedLocationsView.swift | 8 +- .../Views/Settings/SettingsTab.swift | 2 +- .../RoadFlareCore/ViewModels/AppState.swift | 61 +++++++---- .../ViewModels/SyncCoordinator.swift | 12 ++- .../OnboardingPublishWatchdogTests.swift | 58 ++++++++++ .../0017-publish-and-mark-error-surface.md | 100 ++++++++++++++++++ 11 files changed, 278 insertions(+), 52 deletions(-) create mode 100644 decisions/0017-publish-and-mark-error-surface.md diff --git a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift index 580196f..3e49df3 100644 --- a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift +++ b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift @@ -112,10 +112,18 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { /// longer matches, and we exit WITHOUT touching shared state (a new /// publish session may own it) and WITHOUT calling `markPublished` /// (identity has changed). + /// - Error semantic (ADR-0017): rethrows the *terminal* iteration's error + /// if and only if no successful publish landed during the call window. + /// A coalesced republish that succeeds rescues an earlier failed + /// iteration — the call returns without throw. Coalesced calls (the + /// ones that hit the `shouldQueue` short-circuit) never throw; they + /// have no awaitable publish of their own to fail. A session + /// invalidated by `clearAll()` returns without throw — the caller's + /// identity has been replaced and the error is meaningless. public func publishAndMark( settings: UserSettingsRepository, savedLocations: SavedLocationsRepository - ) async { + ) async throws { var entryGeneration: UInt64 = 0 let shouldQueue: Bool = lock.withLock { if isPublishing { @@ -129,6 +137,7 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { } guard !shouldQueue else { return } + var lastIterationError: (any Error)? while true { let content = buildContent(settings: settings, savedLocations: savedLocations) do { @@ -138,8 +147,10 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { syncStoreRef?.markPublished(.profileBackup, at: event.createdAt) RidestrLogger.info("[ProfileBackupCoordinator] Published profile backup") } + lastIterationError = nil } catch { RidestrLogger.info("[ProfileBackupCoordinator] Failed to publish profile backup: \(error.localizedDescription)") + lastIterationError = error } // Atomic exit: either continue with a fresh iteration (consuming @@ -154,7 +165,13 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { isPublishing = false return false } - if !shouldContinue { return } + if !shouldContinue { + if let error = lastIterationError, + lock.withLock({ generation == entryGeneration }) { + throw error + } + return + } } } diff --git a/RidestrSDK/Sources/RidestrSDK/RoadFlare/RoadflareDomainService.swift b/RidestrSDK/Sources/RidestrSDK/RoadFlare/RoadflareDomainService.swift index a8d565d..b35fae2 100644 --- a/RidestrSDK/Sources/RidestrSDK/RoadFlare/RoadflareDomainService.swift +++ b/RidestrSDK/Sources/RidestrSDK/RoadFlare/RoadflareDomainService.swift @@ -181,23 +181,22 @@ public final class RoadflareDomainService: @unchecked Sendable { // MARK: - Publish-and-Mark Convenience Helpers // // Each helper: reads from its repo → builds content → publishes → marks - // syncStore published on success. Throws swallowed by logging. + // syncStore published on success. `publishProfileAndMark` rethrows so the + // onboarding eager-error path (ADR-0017) can surface the banner without + // waiting for the watchdog timeout; the followed-drivers and ride-history + // helpers stay best-effort because no UI surface observes them directly. public func publishProfileAndMark( from settings: UserSettingsRepository, syncStore: RoadflareSyncStateStore - ) async { + ) async throws { let profile = UserProfileContent( name: settings.profileName, displayName: settings.profileName ) - do { - let event = try await publishProfile(profile) - syncStore.markPublished(.profile, at: event.createdAt) - RidestrLogger.info("[RoadflareDomainService] Published profile") - } catch { - RidestrLogger.info("[RoadflareDomainService] Failed to publish profile: \(error.localizedDescription)") - } + let event = try await publishProfile(profile) + syncStore.markPublished(.profile, at: event.createdAt) + RidestrLogger.info("[RoadflareDomainService] Published profile") } public func publishFollowedDriversListAndMark( diff --git a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ProfileBackupCoordinatorTests.swift b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ProfileBackupCoordinatorTests.swift index f79719b..b665de2 100644 --- a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ProfileBackupCoordinatorTests.swift +++ b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ProfileBackupCoordinatorTests.swift @@ -86,7 +86,7 @@ struct ProfileBackupCoordinatorTests { let kit = try await makeKit() kit.settings.setRoadflarePaymentMethods(["zelle"]) - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) + try await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) #expect(kit.syncStore.metadata(for: .profileBackup).lastSuccessfulPublishAt > 0) #expect(kit.relay.publishedEvents.count == 1) @@ -99,7 +99,7 @@ struct ProfileBackupCoordinatorTests { // Fire two concurrent calls — second should coalesce into a republish. async let first: Void = kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) async let second: Void = kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) - _ = await (first, second) + _ = try await (first, second) // First call publishes once, second sets republishRequested → first loops and publishes again. // Since the relay is fake and completes immediately, the second call's guard may or may not @@ -112,8 +112,13 @@ struct ProfileBackupCoordinatorTests { kit.relay.shouldFailPublish = true kit.settings.setRoadflarePaymentMethods(["zelle"]) - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) - + // ADR-0017: terminal-iteration failure rethrows so the onboarding + // eager-error path can fire the banner without waiting for the watchdog. + await #expect(throws: (any Error).self) { + try await kit.coordinator.publishAndMark( + settings: kit.settings, savedLocations: kit.savedLocations + ) + } #expect(kit.syncStore.metadata(for: .profileBackup).lastSuccessfulPublishAt == 0) } @@ -136,7 +141,10 @@ struct ProfileBackupCoordinatorTests { kit.relay.publishDelay = .milliseconds(100) let publishTask = Task { - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) + // Session crossed by clearAll: ADR-0017 contract says we return + // without throwing — the caller's identity has been replaced and + // the error is meaningless. `try await` accepts both. + try await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) } // Let publish start its await @@ -147,9 +155,9 @@ struct ProfileBackupCoordinatorTests { // New publish session starts clean kit.settings.setRoadflarePaymentMethods(["venmo"]) - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) + try await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) - await publishTask.value + try await publishTask.value // The new session's publish reached the relay. #expect(kit.syncStore.metadata(for: .profileBackup).lastSuccessfulPublishAt > 0) @@ -167,12 +175,12 @@ struct ProfileBackupCoordinatorTests { let kit = try await makeKit() kit.settings.setRoadflarePaymentMethods(["zelle"]) - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) + try await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) let firstTimestamp = kit.syncStore.metadata(for: .profileBackup).lastSuccessfulPublishAt try await Task.sleep(for: .milliseconds(10)) kit.settings.setRoadflarePaymentMethods(["venmo"]) - await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) + try await kit.coordinator.publishAndMark(settings: kit.settings, savedLocations: kit.savedLocations) let secondTimestamp = kit.syncStore.metadata(for: .profileBackup).lastSuccessfulPublishAt #expect(secondTimestamp >= firstTimestamp) diff --git a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/RoadflareDomainServiceTests.swift b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/RoadflareDomainServiceTests.swift index d2cb85e..8c24373 100644 --- a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/RoadflareDomainServiceTests.swift +++ b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/RoadflareDomainServiceTests.swift @@ -340,12 +340,33 @@ struct RoadflareDomainServiceTests { let settings = UserSettingsRepository(persistence: InMemoryUserSettingsPersistence()) _ = settings.setProfileName("Alice") - await service.publishProfileAndMark(from: settings, syncStore: syncStore) + try await service.publishProfileAndMark(from: settings, syncStore: syncStore) #expect(syncStore.metadata(for: .profile).lastSuccessfulPublishAt > 0) #expect(relay.publishedEvents.count == 1) } + @Test func publishProfileAndMarkRethrowsRelayFailure() async throws { + // ADR-0017: SDK helper rethrows so the onboarding eager-error path + // can fire the failure banner without waiting for the watchdog. + let keypair = try NostrKeypair.generate() + let relay = FakeRelayManager() + try await relay.connect(to: [URL(string: "wss://fake")!]) + relay.shouldFailPublish = true + let service = RoadflareDomainService(relayManager: relay, keypair: keypair) + let syncStore = RoadflareSyncStateStore( + defaults: UserDefaults(suiteName: "test_\(UUID().uuidString)")!, + namespace: UUID().uuidString + ) + let settings = UserSettingsRepository(persistence: InMemoryUserSettingsPersistence()) + _ = settings.setProfileName("Alice") + + await #expect(throws: (any Error).self) { + try await service.publishProfileAndMark(from: settings, syncStore: syncStore) + } + #expect(syncStore.metadata(for: .profile).lastSuccessfulPublishAt == 0) + } + @Test func publishFollowedDriversListAndMarkMarksSyncStore() async throws { let keypair = try NostrKeypair.generate() let relay = FakeRelayManager() diff --git a/RoadFlare/RoadFlare/Views/Settings/PaymentMethodsScreen.swift b/RoadFlare/RoadFlare/Views/Settings/PaymentMethodsScreen.swift index 9490ccf..150d03d 100644 --- a/RoadFlare/RoadFlare/Views/Settings/PaymentMethodsScreen.swift +++ b/RoadFlare/RoadFlare/Views/Settings/PaymentMethodsScreen.swift @@ -15,7 +15,7 @@ struct PaymentMethodsScreen: View { PaymentMethodPicker(settings: appState.settings) .onChange(of: appState.settings.roadflarePaymentMethods) { oldValue, newValue in guard oldValue != newValue, appState.authState == .ready else { return } - Task { await appState.publishProfileBackup() } + Task { try? await appState.publishProfileBackup() } } } .padding(.horizontal, 16) diff --git a/RoadFlare/RoadFlare/Views/Settings/SavedLocationsView.swift b/RoadFlare/RoadFlare/Views/Settings/SavedLocationsView.swift index c0ab220..e2691dd 100644 --- a/RoadFlare/RoadFlare/Views/Settings/SavedLocationsView.swift +++ b/RoadFlare/RoadFlare/Views/Settings/SavedLocationsView.swift @@ -238,7 +238,7 @@ struct AddFavoriteSheet: View { timestampMs: Int(Date.now.timeIntervalSince1970 * 1000) ) appState.saveLocation(loc) - await appState.publishProfileBackup() + try? await appState.publishProfileBackup() } } catch { // Non-fatal @@ -302,7 +302,7 @@ struct EditLocationSheet: View { if !location.isFavorite { Button { appState.pinLocation(id: location.id, nickname: nickname.isEmpty ? location.displayName : nickname) - Task { await appState.publishProfileBackup() } + Task { try? await appState.publishProfileBackup() } dismiss() } label: { Label("Save as Favorite", systemImage: "star.fill") @@ -314,7 +314,7 @@ struct EditLocationSheet: View { if !nickname.isEmpty { appState.pinLocation(id: location.id, nickname: nickname) } - Task { await appState.publishProfileBackup() } + Task { try? await appState.publishProfileBackup() } dismiss() } label: { Text("Save") @@ -326,7 +326,7 @@ struct EditLocationSheet: View { Button(role: .destructive) { appState.removeLocation(id: location.id) - Task { await appState.publishProfileBackup() } + Task { try? await appState.publishProfileBackup() } dismiss() } label: { Text(location.isFavorite ? "Remove Favorite" : "Remove") diff --git a/RoadFlare/RoadFlare/Views/Settings/SettingsTab.swift b/RoadFlare/RoadFlare/Views/Settings/SettingsTab.swift index 71b5f90..1595463 100644 --- a/RoadFlare/RoadFlare/Views/Settings/SettingsTab.swift +++ b/RoadFlare/RoadFlare/Views/Settings/SettingsTab.swift @@ -387,7 +387,7 @@ struct EditProfileSheet: View { saveState = .saving appState.settings.setProfileName(trimmed) Task { - await appState.saveAndPublishSettings() + try? await appState.saveAndPublishSettings() saveState = .saved try? await Task.sleep(for: .milliseconds(600)) dismiss() diff --git a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift index b169f89..0f06db1 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift @@ -310,17 +310,36 @@ public final class AppState { // round-trip completes regardless — `publishProfileAndMark` doesn't // observe cooperative cancellation. guard !Task.isCancelled else { return } - #if DEBUG - if let hook = onboardingPublishHookForTesting { - await hook(domain) - return - } - #endif - switch domain { - case .profile: - await publishProfile() - case .settingsBackup: - await saveAndPublishSettings() + do { + #if DEBUG + if let hook = onboardingPublishHookForTesting { + try await hook(domain) + return + } + #endif + switch domain { + case .profile: + try await publishProfile() + case .settingsBackup: + try await saveAndPublishSettings() + } + } catch { + // Eager-error surface (ADR-0017): an SDK throw is a faster signal + // than the dirty-flag watchdog. If the relay is reachable, fire + // the banner immediately and cancel the watchdog (its +60s + // `.failed(domain:)` write would be a redundant idempotent set). + // If offline, do nothing — the watchdog's offline-park loop is + // the right place to wait for connectivity to come back. + guard !Task.isCancelled else { return } + AppLogger.auth.warning( + "Onboarding publish (\(String(describing: domain))) failed: \(error.localizedDescription)" + ) + let online = await isOnboardingPublishOnline() + guard !Task.isCancelled else { return } + if online { + onboardingPublishStatus = .failed(domain: domain) + onboardingPublishWatchdogTask?.cancel() + } } } @@ -409,21 +428,21 @@ public final class AppState { // MARK: - Forwarding to SDK (through SyncCoordinator) - func publishProfile() async { + func publishProfile() async throws { guard let service = roadflareDomainService, let syncStore = syncCoordinator?.roadflareSyncStore else { return } - await service.publishProfileAndMark(from: settings, syncStore: syncStore) + try await service.publishProfileAndMark(from: settings, syncStore: syncStore) } - public func publishProfileBackup() async { - await syncCoordinator?.profileBackupCoordinator?.publishAndMark( + public func publishProfileBackup() async throws { + try await syncCoordinator?.profileBackupCoordinator?.publishAndMark( settings: settings, savedLocations: savedLocations ) } - public func saveAndPublishSettings() async { - await publishProfile() - await publishProfileBackup() + public func saveAndPublishSettings() async throws { + try await publishProfile() + try await publishProfileBackup() } func buildProfileBackupContent() -> ProfileBackupContent { @@ -545,7 +564,7 @@ public final class AppState { /// Test-only overrides for the onboarding-publish failure surface. See /// `setOnboardingPublishHooksForTesting(...)` for usage. - var onboardingPublishHookForTesting: ((OnboardingPublishDomain) async -> Void)? + var onboardingPublishHookForTesting: ((OnboardingPublishDomain) async throws -> Void)? var onboardingPublishConnectivityHookForTesting: (() async -> Bool)? var onboardingPublishIsDirtyHookForTesting: ((OnboardingPublishDomain) -> Bool)? var onboardingPublishTimeoutOverrideForTesting: TimeInterval? @@ -1303,7 +1322,7 @@ extension AppState { /// Clear all saved locations and publish the profile backup. public func clearAllLocations() async { savedLocations.clearAll() - await publishProfileBackup() + try? await publishProfileBackup() } } @@ -1373,7 +1392,7 @@ extension AppState { /// than minutes. Pass `nil` for any parameter to keep the production /// behavior; pass a value to override. func setOnboardingPublishHooksForTesting( - publish: ((OnboardingPublishDomain) async -> Void)? = nil, + publish: ((OnboardingPublishDomain) async throws -> Void)? = nil, connectivity: (() async -> Bool)? = nil, isDirty: ((OnboardingPublishDomain) -> Bool)? = nil, timeout: TimeInterval? = nil, diff --git a/RoadFlare/RoadFlareCore/ViewModels/SyncCoordinator.swift b/RoadFlare/RoadFlareCore/ViewModels/SyncCoordinator.swift index 645fe1c..9404bcc 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/SyncCoordinator.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/SyncCoordinator.swift @@ -143,7 +143,11 @@ final class SyncCoordinator { }, undecodableWarning: "Latest profile metadata is not decodable; preserving local profile state", publishLocal: { @Sendable in - await service.publishProfileAndMark(from: settings, syncStore: syncStore) + // Best-effort: failure here keeps the dirty flag set so the + // next reconnect-flush retries. The onboarding eager-error + // path (ADR-0017) lives in `runOnboardingPublishImpl`, not + // in startup-sync `publishLocal` closures. + try? await service.publishProfileAndMark(from: settings, syncStore: syncStore) } ) @@ -191,7 +195,7 @@ final class SyncCoordinator { }, undecodableWarning: "Latest profile backup is not decodable; preserving local backup state", publishLocal: { @Sendable in - await backupCoordinator.publishAndMark(settings: settings, savedLocations: savedLocations) + try? await backupCoordinator.publishAndMark(settings: settings, savedLocations: savedLocations) }, onSnapshotSeen: { @Sendable value in backupCoordinator.preserveSettingsTemplate(value.settings) @@ -250,13 +254,13 @@ final class SyncCoordinator { let backupCoordinator = profileBackupCoordinator else { return } if syncStore.metadata(for: .profile).isDirty { - await service.publishProfileAndMark(from: settings, syncStore: syncStore) + try? await service.publishProfileAndMark(from: settings, syncStore: syncStore) } if syncStore.metadata(for: .followedDrivers).isDirty { await rideCoordinator?.publishFollowedDriversList() } if syncStore.metadata(for: .profileBackup).isDirty { - await backupCoordinator.publishAndMark(settings: settings, savedLocations: savedLocations) + try? await backupCoordinator.publishAndMark(settings: settings, savedLocations: savedLocations) } if syncStore.metadata(for: .rideHistory).isDirty { // Empty history is valid — may be a delete-all await service.publishRideHistoryAndMark(from: rideHistory, syncStore: syncStore) diff --git a/RoadFlare/RoadFlareTests/AppState/OnboardingPublishWatchdogTests.swift b/RoadFlare/RoadFlareTests/AppState/OnboardingPublishWatchdogTests.swift index daac0b0..5dd9a46 100644 --- a/RoadFlare/RoadFlareTests/AppState/OnboardingPublishWatchdogTests.swift +++ b/RoadFlare/RoadFlareTests/AppState/OnboardingPublishWatchdogTests.swift @@ -250,4 +250,62 @@ struct OnboardingPublishWatchdogTests { Issue.record("Expected .failed(settingsBackup), got \(appState.onboardingPublishStatus)") } } + + /// ADR-0017: when the publish hook throws AND the user is online, the + /// banner fires eagerly without waiting for the watchdog timeout. With a + /// 60s `timeout` configured (production value) and a sub-second wait, a + /// surface in <500ms can only have come from the eager-error path — + /// proves the watchdog isn't the one firing. + @Test func eagerErrorSurfacesBannerBeforeWatchdogTimeoutWhenOnline() async { + let appState = AppState() + struct PublishFailure: Error {} + appState.setOnboardingPublishHooksForTesting( + publish: { _ in throw PublishFailure() }, + connectivity: { true }, + isDirty: { _ in true }, + timeout: 60.0, // production value; far longer than the test window + rearmInterval: 60.0 + ) + + let started = Date.now + await appState.completeProfileSetup(name: "Alice") + let surfaced = await waitFor(timeout: 1.0) { + appState.onboardingPublishStatus == .failed(domain: .profile) + } + let elapsed = Date.now.timeIntervalSince(started) + + #expect(surfaced) + #expect(elapsed < 0.5, "Eager path expected to fire well under the 60s watchdog timeout, got \(elapsed)s") + } + + /// ADR-0017: when the publish throws while OFFLINE, the eager path stays + /// quiet and lets the watchdog's offline-park branch handle it. The + /// banner only fires once connectivity returns. + @Test func eagerErrorStaysQuietWhenOffline() async { + let appState = AppState() + struct PublishFailure: Error {} + var online = false + appState.setOnboardingPublishHooksForTesting( + publish: { _ in throw PublishFailure() }, + connectivity: { online }, + isDirty: { _ in true }, + timeout: 0.05, + rearmInterval: 0.05 + ) + + await appState.completeProfileSetup(name: "Alice") + + // Offline: eager path defers to the watchdog's parking loop. Wait + // long enough for several rearm cycles to confirm the banner stays + // idle until connectivity returns. + try? await Task.sleep(nanoseconds: 300_000_000) + #expect(appState.onboardingPublishStatus == .idle) + + // Bring user online; next watchdog rearm tick surfaces failure. + online = true + let surfaced = await waitFor(timeout: 1.0) { + appState.onboardingPublishStatus == .failed(domain: .profile) + } + #expect(surfaced) + } } diff --git a/decisions/0017-publish-and-mark-error-surface.md b/decisions/0017-publish-and-mark-error-surface.md new file mode 100644 index 0000000..91baa10 --- /dev/null +++ b/decisions/0017-publish-and-mark-error-surface.md @@ -0,0 +1,100 @@ +# ADR-0017: SDK Publish-and-Mark Error Surface + +**Status:** Active +**Created:** 2026-05-05 +**Tags:** sync, ux, public-api, sdk + +## Context + +[ADR-0016](0016-onboarding-publish-failure-surface.md) shipped a 60-second watchdog that fires the onboarding publish-failure banner after the dirty flag in `RoadflareSyncStateStore` has stayed set past the timeout window. The watchdog observes the dirty flag as a *proxy* for failure — the SDK's `RoadflareDomainService.publishProfileAndMark` and `ProfileBackupCoordinator.publishAndMark` both swallow `try`-thrown errors at the SDK boundary (log only, return `Void`). The watchdog is the only signal the app has that something went wrong. + +This works, but it has two costs: + +1. **Latency.** A relay that returns an explicit error in 200ms still leaves the user staring at a "successful" Continue tap for the full 60-second window before the banner appears. The dirty-flag proxy can't distinguish "publish failed instantly" from "publish is in flight" from "publish hung." + +2. **No diagnostic precision.** The banner copy is generic ("Your profile hasn't been backed up yet") because the watchdog has no `Error` to look at — at the moment the banner is decided, the original failure cause has been logged and discarded. Even if we wanted to differentiate "relay rejected your event" from "we couldn't reach the relay," the information is gone. + +Issue #97 item 2 captured this as the next architectural follow-up. The precedent — [ADR-0013](0013-stale-key-refresh-flow.md)'s `LocationSyncCoordinator.requestKeyRefresh` migration from "best-effort, swallows errors" to `throws` — already exists. + +## Decision + +Promote `RoadflareDomainService.publishProfileAndMark` and `ProfileBackupCoordinator.publishAndMark` from `async` to `async throws`. App-side wrappers (`AppState.publishProfile`, `publishProfileBackup`, `saveAndPublishSettings`) propagate. `runOnboardingPublishImpl` catches at the AppState boundary and surfaces the banner eagerly when online. + +### Scope + +Two SDK helpers change signature: + +- `RoadflareDomainService.publishProfileAndMark(from:syncStore:)` — `async` → `async throws` +- `ProfileBackupCoordinator.publishAndMark(settings:savedLocations:)` — `async` → `async throws` + +Two SDK helpers stay `async` (out of scope): + +- `RoadflareDomainService.publishFollowedDriversListAndMark` — not on the onboarding watchdog path. The watchdog observes `.profile` and `.profileBackup` only; followed-drivers failures are caught by the next reconnect-flush. Migrating this would be cosmetic. +- `RoadflareDomainService.publishRideHistoryAndMark` — same reason. Plus `RideHistorySyncCoordinator.publishAndMark` already marks the domain dirty on failure inside its own internal `Task`, which is the contract its callers rely on. Throwing out of it would change a fire-and-forget API into one its synchronous call sites can't observe. + +Limiting scope to the two helpers the onboarding watchdog actually consumes keeps the change focused. Future need (e.g. surfacing followed-drivers backup failures with a different UI) can revisit. + +### Eager-error path in `runOnboardingPublishImpl` + +`runOnboardingPublishImpl` wraps the publish in `do/catch`. On catch: + +1. Re-check `Task.isCancelled` (a retry that lands during the await must not clobber the new state). +2. Log the underlying error so the diagnostic context survives. +3. Run the same `isOnboardingPublishOnline()` connectivity check the watchdog uses. +4. If online: set `onboardingPublishStatus = .failed(domain:)` and cancel the watchdog (it would otherwise fire at +60s with the same value — idempotent but noisy). +5. If offline: do nothing. The watchdog's offline-park loop is the right place to wait for connectivity to come back; no point duplicating it inside the publish Task. + +The watchdog Task itself is preserved unchanged. It now serves two narrower purposes: (a) safety net for the case where the SDK call hangs without returning at all (no `throw`, no completion), and (b) the offline-park loop the eager path defers to. + +### Test seam migration + +`setOnboardingPublishHooksForTesting`'s `publish` parameter changes from `((OnboardingPublishDomain) async -> Void)?` to `((OnboardingPublishDomain) async throws -> Void)?`. Existing tests' non-throwing closures stay valid (Swift accepts a non-throwing closure where a throwing closure is expected). One new test exercises the eager-error path by throwing from the hook. + +## Rationale + +- **`throws` over `Outcome` enum.** ADR-0013's `KeyRefreshOutcome` is an enum because the SwiftUI button handler renders three distinct states (success toast, rate-limit toast, publish-failed toast). Onboarding has only two: success (banner stays idle) and failure (banner fires). A binary outcome is what `throws` is for, and it preserves the underlying error type for logging without forcing every caller to invent its own error-classification. + +- **Two helpers, not all four.** Migrating `publishFollowedDriversListAndMark` and `publishRideHistoryAndMark` simultaneously would touch ~30 lines of test fixtures and three additional call sites for no observable user benefit — neither feeds the onboarding watchdog. The `RideHistorySyncCoordinator.publishAndMark` case is an actual mismatch: its callers don't `await` it (it spawns its own internal `Task`), so making the inner publish throw would change a fire-and-forget contract. + +- **Eager path defers to the watchdog when offline.** The first cut of this design had the eager-error path call `checkOnboardingPublishOutcome` directly, which would have made the publish Task itself become the offline-park loop (recursive `Task.sleep` until connectivity returns). That works but conflates publish-Task lifetime with watchdog-Task lifetime in a way that's hard to reason about (why is the publish Task still running 30 seconds after the publish errored?). Letting the watchdog own the offline-park branch keeps each Task's purpose distinct. + +- **Online-eager-fire cancels the watchdog.** When the eager path fires the banner, the watchdog's +60s `.failed(domain:)` write would be a redundant idempotent set — same value. Cancelling the watchdog avoids a stray `Task.sleep(60)` continuation hanging around for no purpose. (Cancellation is cheap; the continuation wakes early with `CancellationError`, the catch returns, the Task deallocates.) + +- **`ProfileBackupCoordinator.publishAndMark` republish-loop semantics preserved.** The coordinator has a republish-on-dirty loop that coalesces concurrent `publishAndMark` calls. A naive `try await` inside the loop would throw on the first iteration's error and abandon the queued republish. Instead, the loop tracks the most recent iteration's error, returns success (no throw) if any iteration succeeded during the call window, and throws the *final* iteration's error if no successful publish landed. This preserves the existing coalescing semantic (a fast follow-up call can still rescue an initial failure) while still surfacing terminal failures. + +- **Why the connectivity gate at all on the eager path.** A `try` failure can be a network error ("can't reach relay") just as easily as a relay-rejection error. Without the connectivity gate, a user who toggled airplane mode mid-onboarding would get an instant banner saying "your profile hasn't been backed up" — which is true, but the cause is their own network choice, not a server-side problem. Same reasoning as ADR-0016's connectivity gate; it applies identically to the eager-error path. + +## Alternatives Considered + +- **Outcome enum (`PublishAndMarkOutcome { case published(eventId), failed(Error) }`).** Rejected as ADR-0013 precedent doesn't apply: that enum existed to gate three SwiftUI render branches; here we have two, which is exactly the case `throws` was designed for. Adding an enum would also add a public type to the SDK API surface for no observable benefit. + +- **Add a parallel throwing variant alongside the existing `Void` one (`publishProfileAndMarkThrowing`).** Rejected because the `Void` variant has no remaining call site that *actively benefits* from error suppression — every existing caller either (a) catches with `try?` (no behavior change), or (b) wants the error (the new onboarding path). Keeping both variants would just add API surface that callers have to choose between, with no clear "use this when..." guidance. + +- **Migrate all four `publishXAndMark` helpers in one PR.** Rejected as out of scope for #97 item 2; the followed-drivers and ride-history paths have different consumer expectations. Future need can revisit (e.g. if a new feature wants to surface ride-history backup failures). + +- **Retire the watchdog entirely now that the eager path exists.** Rejected because the watchdog covers a case the eager path can't: a `try await publishProfile()` that hangs indefinitely without throwing. Rare, but observed enough on flaky relays that the safety net is worth keeping. The +60s window for the hang case is the same as it was before the eager path existed — no regression. + +- **Differentiate banner copy by error type ("relay rejected" vs "couldn't reach relay").** Rejected for now. The user's action in both cases is the same (tap Retry); differentiated copy would mostly just communicate "we tried and something specific happened" without giving the user new agency. The error is still logged, which is enough for diagnostic purposes. A future UX iteration could revisit. + +## Consequences + +- **SDK API change.** Two `public` methods become `throws`. Existing call sites in `SyncCoordinator.flushPendingSyncPublishes` and `SyncCoordinator.performStartupSync` (via `SyncDomainStrategy.publishLocal` closures) wrap with `try?` to preserve current best-effort behavior. The previous SDK-level log line (`"[RoadflareDomainService] Failed to publish profile: ..."`) moves up to the call site that cares — `runOnboardingPublishImpl` logs in its catch block; `try?` discard sites lose the log because the dirty flag is enough signal there (the next reconnect-flush will retry). + +- **Eager banner.** A relay-rejected onboarding publish surfaces the banner in the time it takes to round-trip the rejection (~hundreds of ms) instead of waiting +60s. Watchdog still fires for the hang case. + +- **Test seam signature change.** `setOnboardingPublishHooksForTesting`'s `publish` parameter is now `((OnboardingPublishDomain) async throws -> Void)?`. Existing non-throwing test closures keep working without modification. + +- **No persistence change.** Dirty flags continue to be the source of truth for "did this domain reach a relay." The `throws` change only adds a faster *signal* for the failure case; it doesn't change what the SDK persists. + +- **One new failure-mode test (`OnboardingPublishWatchdogTests`).** Verifies that an SDK throw fires the banner without waiting for the watchdog timeout. + +## Affected Files + +- `RidestrSDK/Sources/RidestrSDK/RoadFlare/RoadflareDomainService.swift` — `publishProfileAndMark` becomes `throws`; SDK-level catch+log removed. +- `RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift` — `publishAndMark` becomes `throws`; republish loop tracks last-iteration error and rethrows on terminal failure. +- `RidestrSDK/Tests/RidestrSDKTests/RoadFlare/RoadflareDomainServiceTests.swift` — `try await` at the test call site; new throw-on-relay-failure assertion. +- `RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ProfileBackupCoordinatorTests.swift` — `try await` at test call sites; failure-path test asserts the throw. +- `RoadFlare/RoadFlareCore/ViewModels/AppState.swift` — `publishProfile`, `publishProfileBackup`, `saveAndPublishSettings` propagate; `runOnboardingPublishImpl` catches and runs the connectivity-gated eager surface; test seam hook signature now `throws`; doc-comment on `onboardingPublishTask` updated. +- `RoadFlare/RoadFlareCore/ViewModels/SyncCoordinator.swift` — `flushPendingSyncPublishes` and the four `SyncDomainStrategy.publishLocal` closures wrap the now-throwing helpers with `try?`. +- `RoadFlare/RoadFlareTests/AppState/OnboardingPublishWatchdogTests.swift` — new test for the eager-error path. +- `decisions/0017-publish-and-mark-error-surface.md` — this file. From 5954585b339131af82c27bf4c63a0155f2e0ea46 Mon Sep 17 00:00:00 2001 From: variablefate Date: Tue, 5 May 2026 18:03:35 -0700 Subject: [PATCH 2/5] fix(onboarding): saveAndPublishSettings always attempts both publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-PR-99 the two `await` calls swallowed errors internally so both ran unconditionally. After the throws migration, `try await publishProfile()` short-circuited the chain — `publishProfileBackup()` never ran when the first call failed, leaving the Kind 30177 backup unattempted until a manual Retry or the next reconnect-flush. Capture the first error, run both publishes, throw if either failed. The eager-error catch in `runOnboardingPublishImpl` still surfaces the banner when at least one fails. Addresses code review feedback on PR #99. Co-Authored-By: Claude Opus 4.7 (1M context) --- RoadFlare/RoadFlareCore/ViewModels/AppState.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift index 0f06db1..335b0ed 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift @@ -441,8 +441,14 @@ public final class AppState { } public func saveAndPublishSettings() async throws { - try await publishProfile() - try await publishProfileBackup() + // Always attempt both publishes — they target independent Nostr kinds + // (Kind 0 profile vs Kind 30177 backup) and a transient failure of one + // shouldn't suppress the other. Capture the first error and rethrow so + // the onboarding eager-error path (ADR-0017) still fires the banner. + var firstError: (any Error)? + do { try await publishProfile() } catch { firstError = error } + do { try await publishProfileBackup() } catch { firstError = firstError ?? error } + if let firstError { throw firstError } } func buildProfileBackupContent() -> ProfileBackupContent { From d21e159acd9bf571b10d91a5c8db07eee4a1d7b3 Mon Sep 17 00:00:00 2001 From: variablefate Date: Tue, 5 May 2026 18:46:35 -0700 Subject: [PATCH 3/5] chore(appstate): align publishProfileBackup to publishProfile's guard pattern Both methods silently no-op when the sync coordinator isn't configured, but `publishProfile` made it explicit via `guard let ... else { return }` while `publishProfileBackup` relied on optional-chaining a throwing call, which silently returns nil-coalesced Void without throwing. Same end state, but the explicit guard reads better and matches the sibling. Code review follow-up on PR #99. Co-Authored-By: Claude Opus 4.7 (1M context) --- RoadFlare/RoadFlareCore/ViewModels/AppState.swift | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift index 335b0ed..22fd3e4 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift @@ -435,9 +435,8 @@ public final class AppState { } public func publishProfileBackup() async throws { - try await syncCoordinator?.profileBackupCoordinator?.publishAndMark( - settings: settings, savedLocations: savedLocations - ) + guard let coordinator = syncCoordinator?.profileBackupCoordinator else { return } + try await coordinator.publishAndMark(settings: settings, savedLocations: savedLocations) } public func saveAndPublishSettings() async throws { From 173b01c4b724f55c5a928f798bafd9847b63a209 Mon Sep 17 00:00:00 2001 From: variablefate Date: Wed, 6 May 2026 20:27:43 -0700 Subject: [PATCH 4/5] chore: address sub-80 code-review polish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ProfileBackupCoordinator: capture `generationStillValid` inside the atomic-exit lock so the post-loop throw decision uses state from the same critical section, restoring the "atomic single-lock exits" contract that .claude/CLAUDE.md mandates for publish state machines. - AppState: update `startOnboardingPublish` doc to acknowledge that the publish is no longer "unsupervised" — the eager-error catch block (ADR-0017) is a second failure-surface path that runs concurrently with the watchdog. - AppState: update `onboardingPublishTask` doc to reflect that cancellation matters at three places now (entry-bail + two checks bracketing the connectivity await in the catch block), not just pre-scheduling. Code-review follow-up. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../RoadFlare/ProfileBackupCoordinator.swift | 10 +++-- .../RoadFlareCore/ViewModels/AppState.swift | 40 ++++++++++++------- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift index 3e49df3..c0958b5 100644 --- a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift +++ b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ProfileBackupCoordinator.swift @@ -155,9 +155,14 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { // Atomic exit: either continue with a fresh iteration (consuming // the republish request) OR release isPublishing. If generation - // changed, bail without touching shared state. + // changed, bail without touching shared state. Captures + // `generationStillValid` inside the same lock region so the + // post-loop throw decision doesn't need a second lock acquisition + // (per .claude/CLAUDE.md "atomic single-lock exits"). + var generationStillValid = false let shouldContinue: Bool = lock.withLock { guard generation == entryGeneration else { return false } + generationStillValid = true if republishRequested { republishRequested = false return true @@ -166,8 +171,7 @@ public final class ProfileBackupCoordinator: @unchecked Sendable { return false } if !shouldContinue { - if let error = lastIterationError, - lock.withLock({ generation == entryGeneration }) { + if let error = lastIterationError, generationStillValid { throw error } return diff --git a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift index 22fd3e4..4dc9d29 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift @@ -278,12 +278,18 @@ public final class AppState { } /// Spawn the publish + watchdog for an onboarding domain. Cancels any - /// in-flight watchdog and clears prior failure state, so a fast user - /// chaining ProfileSetup → PaymentSetup doesn't surface a stale - /// `.failed` from the first publish while the second is still in - /// flight. The publish itself is unsupervised (matches pre-watchdog - /// optimistic-transition contract from ADR-0014); the watchdog is the - /// only signal we need to cancel-and-restart. + /// in-flight publish and watchdog and clears prior failure state, so a + /// fast user chaining ProfileSetup → PaymentSetup doesn't surface a + /// stale `.failed` from the first publish while the second is still in + /// flight. + /// + /// Two failure-surface paths run concurrently per ADR-0017: the publish + /// Task's catch block (eager-error — fires the banner immediately when + /// the SDK throws and the relay is reachable, then cancels the + /// watchdog) and the watchdog Task (safety net at the timeout for the + /// hang case). Either is enough to surface `.failed`; both are tracked + /// here so retry / chain / identity-replacement can cancel them + /// atomically. private func startOnboardingPublish(domain: OnboardingPublishDomain) { onboardingPublishWatchdogTask?.cancel() onboardingPublishTask?.cancel() @@ -582,14 +588,20 @@ public final class AppState { private var onboardingPublishWatchdogTask: Task? /// In-flight publish Task spawned alongside the watchdog. Tracked so a - /// retry / chained Continue can mark it cancelled before the publish - /// switch runs (`runOnboardingPublishImpl` early-bails on - /// `Task.isCancelled`). Note: the underlying SDK call - /// (`publishProfileAndMark`) doesn't check cancellation itself, so a - /// publish whose `await publishProfile()` has already started completes - /// regardless. Cancellation only avoids the duplicate when the cancel - /// lands before the spawned Task is scheduled — which is the common - /// case for back-to-back Continue taps and rapid retries. + /// retry / chained Continue / identity-replacement can mark it + /// cancelled. Cooperative cancellation matters at three places in + /// `runOnboardingPublishImpl`: (a) the entry-point `guard !Task.isCancelled` + /// before the publish switch (catches the cancel-before-scheduled case for + /// back-to-back Continue taps); and (b)+(c) the two `guard + /// !Task.isCancelled` checks bracketing the connectivity await in the + /// catch block (introduced by the eager-error path in ADR-0017 — they + /// suppress a stale `.failed` write if the user chained or retried + /// during the relay's `await isOnboardingPublishOnline()` resolution). + /// Note: the underlying SDK call (`publishProfileAndMark`) doesn't + /// check cancellation itself, so a publish whose `await publishProfile()` + /// has already started completes regardless — we can avoid the + /// duplicate publish only when the cancel lands before the SDK switch + /// runs. private var onboardingPublishTask: Task? /// Returns `true` when `driver` is a valid ping target. From 60f84759ef4d6b8a679a945eb5c4dde187b474d1 Mon Sep 17 00:00:00 2001 From: variablefate Date: Thu, 7 May 2026 15:10:32 -0700 Subject: [PATCH 5/5] chore: third-pass code-review polish (doc + test coverage) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update startOnboardingPublish doc to acknowledge the watchdog's TWO roles (hang-case safety net + offline-park loop). Without this, a reader of the comment alone wouldn't understand why the catch block intentionally does nothing in the offline branch. - Add fine-grained DEBUG test seams (publishProfileSDKHookForTesting, publishProfileBackupSDKHookForTesting) so unit tests can drive the saveAndPublishSettings "always run both" invariant. The existing onboardingPublishHookForTesting short-circuits at a coarser granularity and can't exercise this path. - Add SaveAndPublishSettingsTests covering: success path, profile-fail → backup-still-runs (the key invariant from 5954585), backup-fail with profile success, and first-error-wins when both fail. A regression to `try await publishProfile(); try await publishProfileBackup()` (short-circuit) would now fail CI. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../RoadFlareCore/ViewModels/AppState.swift | 36 ++++++- .../SaveAndPublishSettingsTests.swift | 98 +++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 RoadFlare/RoadFlareTests/AppState/SaveAndPublishSettingsTests.swift diff --git a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift index 4dc9d29..9679f4d 100644 --- a/RoadFlare/RoadFlareCore/ViewModels/AppState.swift +++ b/RoadFlare/RoadFlareCore/ViewModels/AppState.swift @@ -286,10 +286,15 @@ public final class AppState { /// Two failure-surface paths run concurrently per ADR-0017: the publish /// Task's catch block (eager-error — fires the banner immediately when /// the SDK throws and the relay is reachable, then cancels the - /// watchdog) and the watchdog Task (safety net at the timeout for the - /// hang case). Either is enough to surface `.failed`; both are tracked - /// here so retry / chain / identity-replacement can cancel them - /// atomically. + /// watchdog) and the watchdog Task. The watchdog now serves two + /// purposes: (a) safety net for the case where the SDK call hangs + /// without throwing or returning, and (b) the offline-park loop the + /// eager path defers to when the relay isn't reachable — the catch + /// block intentionally does nothing in the offline branch because + /// the watchdog's parking + rearm-poll is the right place to wait + /// for connectivity to come back. Either path is enough to surface + /// `.failed`; both are tracked here so retry / chain / + /// identity-replacement can cancel them atomically. private func startOnboardingPublish(domain: OnboardingPublishDomain) { onboardingPublishWatchdogTask?.cancel() onboardingPublishTask?.cancel() @@ -435,12 +440,24 @@ public final class AppState { // MARK: - Forwarding to SDK (through SyncCoordinator) func publishProfile() async throws { + #if DEBUG + if let hook = publishProfileSDKHookForTesting { + try await hook() + return + } + #endif guard let service = roadflareDomainService, let syncStore = syncCoordinator?.roadflareSyncStore else { return } try await service.publishProfileAndMark(from: settings, syncStore: syncStore) } public func publishProfileBackup() async throws { + #if DEBUG + if let hook = publishProfileBackupSDKHookForTesting { + try await hook() + return + } + #endif guard let coordinator = syncCoordinator?.profileBackupCoordinator else { return } try await coordinator.publishAndMark(settings: settings, savedLocations: savedLocations) } @@ -578,6 +595,17 @@ public final class AppState { var onboardingPublishHookForTesting: ((OnboardingPublishDomain) async throws -> Void)? var onboardingPublishConnectivityHookForTesting: (() async -> Bool)? var onboardingPublishIsDirtyHookForTesting: ((OnboardingPublishDomain) -> Bool)? + + /// Test-only overrides for the per-publish SDK calls inside + /// `publishProfile()` and `publishProfileBackup()`. Lets unit tests + /// drive `saveAndPublishSettings`'s "always run both" invariant + /// without standing up a full RoadflareDomainService + + /// ProfileBackupCoordinator. The `onboardingPublishHookForTesting` + /// short-circuits at a coarser granularity (replaces the entire + /// `runOnboardingPublishImpl` body), so it can't exercise the + /// `saveAndPublishSettings` switch case. + var publishProfileSDKHookForTesting: (() async throws -> Void)? + var publishProfileBackupSDKHookForTesting: (() async throws -> Void)? var onboardingPublishTimeoutOverrideForTesting: TimeInterval? var onboardingPublishRearmOverrideForTesting: TimeInterval? #endif diff --git a/RoadFlare/RoadFlareTests/AppState/SaveAndPublishSettingsTests.swift b/RoadFlare/RoadFlareTests/AppState/SaveAndPublishSettingsTests.swift new file mode 100644 index 0000000..5e3f312 --- /dev/null +++ b/RoadFlare/RoadFlareTests/AppState/SaveAndPublishSettingsTests.swift @@ -0,0 +1,98 @@ +import Testing +import Foundation +@testable import RoadFlareCore + +// Direct unit tests for `AppState.saveAndPublishSettings` "always run both" +// invariant introduced in PR #99. The eager-error path tests in +// `OnboardingPublishWatchdogTests` use the coarser +// `onboardingPublishHookForTesting`, which short-circuits before +// `saveAndPublishSettings` runs. Without these tests a regression to +// `try await publishProfile(); try await publishProfileBackup()` (the +// original short-circuit version) would silently pass CI while leaving +// the Kind 30177 backup unattempted whenever the Kind 0 publish fails. + +@Suite("AppState saveAndPublishSettings") +@MainActor +struct SaveAndPublishSettingsTests { + + private struct ProfileFailure: Error {} + private struct BackupFailure: Error {} + + @Test func bothPublishesRunWhenProfileSucceeds() async throws { + let appState = AppState() + var profileCalls = 0 + var backupCalls = 0 + appState.publishProfileSDKHookForTesting = { profileCalls += 1 } + appState.publishProfileBackupSDKHookForTesting = { backupCalls += 1 } + + try await appState.saveAndPublishSettings() + + #expect(profileCalls == 1) + #expect(backupCalls == 1) + } + + @Test func backupRunsEvenWhenProfileThrows() async { + let appState = AppState() + var profileCalls = 0 + var backupCalls = 0 + appState.publishProfileSDKHookForTesting = { + profileCalls += 1 + throw ProfileFailure() + } + appState.publishProfileBackupSDKHookForTesting = { backupCalls += 1 } + + do { + try await appState.saveAndPublishSettings() + Issue.record("Expected saveAndPublishSettings to throw the profile error") + } catch is ProfileFailure { + // expected + } catch { + Issue.record("Expected ProfileFailure, got \(error)") + } + + #expect(profileCalls == 1) + // The key invariant: backup must run even though profile threw. + // This guards against a regression to the original short-circuit + // chain `try await publishProfile(); try await publishProfileBackup()`. + #expect(backupCalls == 1) + } + + @Test func profileRunsEvenWhenBackupWillThrow() async { + let appState = AppState() + var profileCalls = 0 + var backupCalls = 0 + appState.publishProfileSDKHookForTesting = { profileCalls += 1 } + appState.publishProfileBackupSDKHookForTesting = { + backupCalls += 1 + throw BackupFailure() + } + + do { + try await appState.saveAndPublishSettings() + Issue.record("Expected saveAndPublishSettings to throw the backup error") + } catch is BackupFailure { + // expected + } catch { + Issue.record("Expected BackupFailure, got \(error)") + } + + #expect(profileCalls == 1) + #expect(backupCalls == 1) + } + + @Test func firstErrorWins_whenBothFail() async { + let appState = AppState() + appState.publishProfileSDKHookForTesting = { throw ProfileFailure() } + appState.publishProfileBackupSDKHookForTesting = { throw BackupFailure() } + + do { + try await appState.saveAndPublishSettings() + Issue.record("Expected saveAndPublishSettings to throw") + } catch is ProfileFailure { + // expected — profile failed first; saveAndPublishSettings rethrows + // the first error per ADR-0017. + } catch { + Issue.record("Expected ProfileFailure (first error), got \(error)") + } + } +}