From fc6bde1afd27ab4bd88e96f65b9dc9c699667be1 Mon Sep 17 00:00:00 2001 From: Matt Hamann Date: Sat, 27 Dec 2025 00:42:45 -0500 Subject: [PATCH 1/4] fix(state): stale pointer access could cause app crashes --- .../Models/Context/ReSwiftObserver.swift | 131 ++++++++---------- Sources/Rownd/Rownd.swift | 6 +- Sources/Rownd/framework/InstantUsers.swift | 1 + Sources/Rownd/framework/RowndEvent.swift | 1 + Tests/RowndTests/ObservableStateTests.swift | 1 + .../RowndTests/SubscriberMutationTests.swift | 1 + 6 files changed, 68 insertions(+), 73 deletions(-) diff --git a/Sources/Rownd/Models/Context/ReSwiftObserver.swift b/Sources/Rownd/Models/Context/ReSwiftObserver.swift index ae86824..193138d 100644 --- a/Sources/Rownd/Models/Context/ReSwiftObserver.swift +++ b/Sources/Rownd/Models/Context/ReSwiftObserver.swift @@ -10,17 +10,11 @@ import Foundation import ReSwift import SwiftUI -// MARK: - Main Thread Dispatch Helper - -/// Helper to centralize main-thread dispatch with weak self handling. -/// Reduces duplication and ensures consistent patterns across observable state types. -private func dispatchOnMain(_ instance: T, execute work: @escaping (T) -> Void) { - DispatchQueue.main.async { [weak instance] in - guard let instance = instance else { return } - work(instance) - } -} +// MARK: - ObservableState +/// Observable wrapper for ReSwift state slices that publishes changes to SwiftUI. +/// Uses @MainActor to ensure all @Published property access is thread-safe. +@MainActor public class ObservableState: ObservableObject, StoreSubscriber, ObservableSubscription { @@ -42,42 +36,39 @@ public class ObservableState: ObservableObject, StoreSubscriber, Ob public func subscribe() { guard !isSubscribed else { return } - // Capture selector directly to avoid retaining self in the transform closure let selector = self.selector - dispatchOnMain(self) { instance in - guard !instance.isSubscribed else { return } - Context.currentContext.store.subscribe( - instance, transform: { $0.select(selector) }) - instance.isSubscribed = true - } + Context.currentContext.store.subscribe(self, transform: { $0.select(selector) }) + isSubscribed = true } func unsubscribe() { guard isSubscribed else { return } - dispatchOnMain(self) { instance in - guard instance.isSubscribed else { return } - Context.currentContext.store.unsubscribe(instance) - instance.isSubscribed = false - } + Context.currentContext.store.unsubscribe(self) + isSubscribed = false } deinit { - unsubscribe() + // Note: deinit is nonisolated even for @MainActor classes. + // ReSwift's SubscriptionBox holds a weak reference to subscribers, + // so cleanup happens automatically when this object is deallocated. } - public func newState(state: T) { - // All @Published property access must happen on main thread - dispatchOnMain(self) { instance in - guard instance.current != state else { return } - let old = instance.current - if let animation = instance.animation { + /// Called by ReSwift when state changes. This method is nonisolated because + /// ReSwift may call it from any thread. We dispatch to MainActor to safely + /// access @Published properties. + nonisolated public func newState(state: T) { + Task { @MainActor [weak self] in + guard let self = self else { return } + guard self.current != state else { return } + let old = self.current + if let animation = self.animation { withAnimation(animation) { - instance.current = state + self.current = state } } else { - instance.current = state + self.current = state } - instance.objectDidChange.send(DidChangeSubject(old: old, new: instance.current)) + self.objectDidChange.send(DidChangeSubject(old: old, new: self.current)) } } @@ -105,26 +96,26 @@ public class ObservableThrottledState: ObservableState { .store(in: &cancellables) } - override public func newState(state: T) { - // All @Published property access must happen on main thread to avoid crashes - // in swift_retain when accessing Combine's Published wrapper from background threads - dispatchOnMain(self) { instance in - guard instance.current != state else { return } - let old = instance.current - if let animation = instance.animation { + nonisolated override public func newState(state: T) { + Task { @MainActor [weak self] in + guard let self = self else { return } + guard self.current != state else { return } + let old = self.current + if let animation = self.animation { withAnimation(animation) { - instance.objectThrottled.send(state) + self.objectThrottled.send(state) } } else { - instance.objectThrottled.send(state) + self.objectThrottled.send(state) } - instance.objectDidChange.send(DidChangeSubject(old: old, new: instance.current)) + self.objectDidChange.send(DidChangeSubject(old: old, new: self.current)) } } private let objectThrottled = PassthroughSubject() } +@MainActor public class ObservableDerivedState: ObservableObject, StoreSubscriber, ObservableSubscription { @@ -151,42 +142,37 @@ public class ObservableDerivedState: Obse func subscribe() { guard !isSubscribed else { return } - // Capture selector directly to avoid retaining self in the transform closure let selector = self.selector - dispatchOnMain(self) { instance in - guard !instance.isSubscribed else { return } - Context.currentContext.store.subscribe( - instance, transform: { $0.select(selector) }) - instance.isSubscribed = true - } + Context.currentContext.store.subscribe(self, transform: { $0.select(selector) }) + isSubscribed = true } func unsubscribe() { guard isSubscribed else { return } - dispatchOnMain(self) { instance in - guard instance.isSubscribed else { return } - Context.currentContext.store.unsubscribe(instance) - instance.isSubscribed = false - } + Context.currentContext.store.unsubscribe(self) + isSubscribed = false } deinit { - unsubscribe() + // Note: deinit is nonisolated even for @MainActor classes. + // ReSwift's SubscriptionBox holds a weak reference to subscribers, + // so cleanup happens automatically when this object is deallocated. } - public func newState(state original: Original) { - dispatchOnMain(self) { instance in - let old = instance.current - instance.objectWillChange.send(ChangeSubject(old: old, new: instance.current)) + nonisolated public func newState(state original: Original) { + Task { @MainActor [weak self] in + guard let self = self else { return } + let old = self.current + self.objectWillChange.send(ChangeSubject(old: old, new: self.current)) - if let animation = instance.animation { + if let animation = self.animation { withAnimation(animation) { - instance.current = instance.transform(original) + self.current = self.transform(original) } } else { - instance.current = instance.transform(original) + self.current = self.transform(original) } - instance.objectDidChange.send(ChangeSubject(old: old, new: instance.current)) + self.objectDidChange.send(ChangeSubject(old: old, new: self.current)) } } @@ -220,17 +206,18 @@ public class ObservableDerivedThrottledState( select selector: @escaping (RowndState) -> (T), animation: SwiftUI.Animation? = nil ) -> ObservableState { ObservableState(select: selector, animation: animation) } + @MainActor public func subscribe( select selector: @escaping (RowndState) -> (Original), transform: @escaping (Original) -> Derived, animation: SwiftUI.Animation? = nil @@ -252,6 +241,7 @@ extension Store where State == RowndState { ObservableDerivedState(select: selector, transform: transform, animation: animation) } + @MainActor public func subscribeThrottled( select selector: @escaping (RowndState) -> (T), throttleInMs: Int = 350, animation: SwiftUI.Animation? = nil @@ -259,6 +249,7 @@ extension Store where State == RowndState { ObservableThrottledState(select: selector, animation: animation, throttleInMs: throttleInMs) } + @MainActor public func subscribeThrottled( select selector: @escaping (RowndState) -> (Original), transform: @escaping (Original) -> Derived, throttleInMs: Int = 350, diff --git a/Sources/Rownd/Rownd.swift b/Sources/Rownd/Rownd.swift index 719bef2..4196afe 100644 --- a/Sources/Rownd/Rownd.swift +++ b/Sources/Rownd/Rownd.swift @@ -95,10 +95,10 @@ public class Rownd: NSObject { store.dispatch(UserData.fetch()) store.dispatch(PasskeyData.fetchPasskeyRegistration()) } - } - InstantUsers(context: Context.currentContext) - .tmpForceInstantUserConversionIfRequested() + InstantUsers(context: Context.currentContext) + .tmpForceInstantUserConversionIfRequested() + } return state } diff --git a/Sources/Rownd/framework/InstantUsers.swift b/Sources/Rownd/framework/InstantUsers.swift index f2c0fd8..63ca434 100644 --- a/Sources/Rownd/framework/InstantUsers.swift +++ b/Sources/Rownd/framework/InstantUsers.swift @@ -7,6 +7,7 @@ import Combine +@MainActor class InstantUsers { private let context: Context private var cancellables = Set() diff --git a/Sources/Rownd/framework/RowndEvent.swift b/Sources/Rownd/framework/RowndEvent.swift index b1c83ba..11c231d 100644 --- a/Sources/Rownd/framework/RowndEvent.swift +++ b/Sources/Rownd/framework/RowndEvent.swift @@ -30,6 +30,7 @@ public protocol RowndEventHandlerDelegate: AnyObject { func handleRowndEvent(_ event: RowndEvent) } +@MainActor class RowndEventEmitter { static private var cancellables = Set() static func emit(_ event: RowndEvent) { diff --git a/Tests/RowndTests/ObservableStateTests.swift b/Tests/RowndTests/ObservableStateTests.swift index 8b8466b..121a02d 100644 --- a/Tests/RowndTests/ObservableStateTests.swift +++ b/Tests/RowndTests/ObservableStateTests.swift @@ -14,6 +14,7 @@ import Testing @testable import Rownd +@MainActor struct ObservableStateTests { /// Tests that ObservableState can handle newState being called from background threads. diff --git a/Tests/RowndTests/SubscriberMutationTests.swift b/Tests/RowndTests/SubscriberMutationTests.swift index 186c479..ccff0c9 100644 --- a/Tests/RowndTests/SubscriberMutationTests.swift +++ b/Tests/RowndTests/SubscriberMutationTests.swift @@ -5,6 +5,7 @@ import Testing @testable import Rownd +@MainActor struct SubscriberMutationTests { @Test func rapidClockSyncAndObserverChurnDoesNotCrash() async throws { From 2262fd3013f7375794b2b10d6a86e5c2c5446799 Mon Sep 17 00:00:00 2001 From: Matt Hamann Date: Mon, 29 Dec 2025 00:41:25 -0500 Subject: [PATCH 2/4] fix(state): potential race conditions --- .../Models/Context/ReSwiftObserver.swift | 143 ++++++++++++------ 1 file changed, 97 insertions(+), 46 deletions(-) diff --git a/Sources/Rownd/Models/Context/ReSwiftObserver.swift b/Sources/Rownd/Models/Context/ReSwiftObserver.swift index 193138d..bd64df6 100644 --- a/Sources/Rownd/Models/Context/ReSwiftObserver.swift +++ b/Sources/Rownd/Models/Context/ReSwiftObserver.swift @@ -10,10 +10,47 @@ import Foundation import ReSwift import SwiftUI +// MARK: - Main Actor Dispatch Helper + +/// Dispatches work to the MainActor from a nonisolated context. +/// +/// This helper serializes state updates through a Combine subject to maintain FIFO ordering, +/// then processes them on the MainActor. This prevents the ordering issues that can occur +/// with unstructured Task spawning under high-frequency state changes. +/// +/// - Parameters: +/// - instance: The object to operate on (captured weakly) +/// - state: The state value to process +/// - work: The MainActor-isolated work to perform +private func dispatchToMainActor( + _ instance: T, + state: S, + work: @escaping @MainActor (T, S) -> Void +) { + // Use DispatchQueue.main.async for FIFO ordering, then Task for @MainActor isolation + DispatchQueue.main.async { [weak instance] in + guard let instance = instance else { return } + Task { @MainActor in + work(instance, state) + } + } +} + // MARK: - ObservableState /// Observable wrapper for ReSwift state slices that publishes changes to SwiftUI. /// Uses @MainActor to ensure all @Published property access is thread-safe. +/// +/// ## Thread Safety +/// ReSwift may call `newState(state:)` from any thread. This class uses @MainActor +/// isolation to ensure all @Published property access occurs on the main thread, +/// preventing crashes in swift_retain when accessing Combine's Published wrapper. +/// +/// ## State Update Ordering +/// State updates are dispatched through DispatchQueue.main.async to maintain FIFO ordering, +/// then processed on the MainActor. While this preserves ordering of dispatch calls, +/// the actual property updates occur asynchronously. For most SwiftUI use cases this is +/// acceptable since SwiftUI will render the final state. @MainActor public class ObservableState: ObservableObject, StoreSubscriber, ObservableSubscription { @@ -54,22 +91,27 @@ public class ObservableState: ObservableObject, StoreSubscriber, Ob } /// Called by ReSwift when state changes. This method is nonisolated because - /// ReSwift may call it from any thread. We dispatch to MainActor to safely - /// access @Published properties. + /// ReSwift may call it from any thread. Updates are dispatched to MainActor + /// via DispatchQueue.main to maintain FIFO ordering. nonisolated public func newState(state: T) { - Task { @MainActor [weak self] in - guard let self = self else { return } - guard self.current != state else { return } - let old = self.current - if let animation = self.animation { - withAnimation(animation) { - self.current = state - } - } else { - self.current = state + dispatchToMainActor(self, state: state) { instance, newState in + instance.applyStateUpdate(newState) + } + } + + /// Applies the state update on MainActor. Separated from newState to keep + /// the dispatch logic clean and enable subclass overrides. + fileprivate func applyStateUpdate(_ state: T) { + guard current != state else { return } + let old = current + if let animation = animation { + withAnimation(animation) { + current = state } - self.objectDidChange.send(DidChangeSubject(old: old, new: self.current)) + } else { + current = state } + objectDidChange.send(DidChangeSubject(old: old, new: current)) } public let objectDidChange = PassthroughSubject, Never>() @@ -97,19 +139,22 @@ public class ObservableThrottledState: ObservableState { } nonisolated override public func newState(state: T) { - Task { @MainActor [weak self] in - guard let self = self else { return } - guard self.current != state else { return } - let old = self.current - if let animation = self.animation { - withAnimation(animation) { - self.objectThrottled.send(state) - } - } else { - self.objectThrottled.send(state) + dispatchToMainActor(self, state: state) { instance, newState in + instance.applyThrottledStateUpdate(newState) + } + } + + fileprivate func applyThrottledStateUpdate(_ state: T) { + guard current != state else { return } + let old = current + if let animation = animation { + withAnimation(animation) { + objectThrottled.send(state) } - self.objectDidChange.send(DidChangeSubject(old: old, new: self.current)) + } else { + objectThrottled.send(state) } + objectDidChange.send(DidChangeSubject(old: old, new: current)) } private let objectThrottled = PassthroughSubject() @@ -160,20 +205,23 @@ public class ObservableDerivedState: Obse } nonisolated public func newState(state original: Original) { - Task { @MainActor [weak self] in - guard let self = self else { return } - let old = self.current - self.objectWillChange.send(ChangeSubject(old: old, new: self.current)) - - if let animation = self.animation { - withAnimation(animation) { - self.current = self.transform(original) - } - } else { - self.current = self.transform(original) + dispatchToMainActor(self, state: original) { instance, newState in + instance.applyStateUpdate(newState) + } + } + + fileprivate func applyStateUpdate(_ original: Original) { + let old = current + objectWillChange.send(ChangeSubject(old: old, new: current)) + + if let animation = animation { + withAnimation(animation) { + current = transform(original) } - self.objectDidChange.send(ChangeSubject(old: old, new: self.current)) + } else { + current = transform(original) } + objectDidChange.send(ChangeSubject(old: old, new: current)) } public let objectWillChange = PassthroughSubject, Never>() @@ -207,18 +255,21 @@ public class ObservableDerivedThrottledState() From c1d69baea014065858659918221289d1f42acca4 Mon Sep 17 00:00:00 2001 From: Matt Hamann Date: Wed, 18 Feb 2026 23:59:29 -0500 Subject: [PATCH 3/4] fix(events): check isAccessTokenValid synchronously before subscribing When signInCompleted is emitted and the access token is already valid, fire the event immediately rather than creating a Combine subscription. This prevents a race condition where the subscription misses the already-settled value and the event is permanently lost. Extracted notifyListeners helper to reduce duplication. --- Sources/Rownd/framework/RowndEvent.swift | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/Sources/Rownd/framework/RowndEvent.swift b/Sources/Rownd/framework/RowndEvent.swift index 11c231d..87be59b 100644 --- a/Sources/Rownd/framework/RowndEvent.swift +++ b/Sources/Rownd/framework/RowndEvent.swift @@ -35,19 +35,31 @@ class RowndEventEmitter { static private var cancellables = Set() static func emit(_ event: RowndEvent) { if event.event == .signInCompleted { + // Check if the access token is already valid — if so, fire immediately + // to avoid a race where the Combine subscription misses a value that's + // already settled before the sink is attached. + let authState = Context.currentContext.store.state.auth + if authState.isAccessTokenValid { + Self.notifyListeners(event) + return + } + + // Token not yet valid — subscribe and wait for it let subscription = Context.currentContext.store.subscribe { $0.auth.isAccessTokenValid } subscription.$current.sink { isAccessTokenValid in if isAccessTokenValid { subscription.unsubscribe() - Context.currentContext.eventListeners.forEach { listener in - listener.handleRowndEvent(event) - } + Self.notifyListeners(event) } }.store(in: &Self.cancellables) } else { - Context.currentContext.eventListeners.forEach { listener in - listener.handleRowndEvent(event) - } + Self.notifyListeners(event) + } + } + + private static func notifyListeners(_ event: RowndEvent) { + Context.currentContext.eventListeners.forEach { listener in + listener.handleRowndEvent(event) } } } From c48dcbadb750f1ad98753019b3299382d3143355 Mon Sep 17 00:00:00 2001 From: Matt Hamann Date: Thu, 19 Feb 2026 00:15:43 -0500 Subject: [PATCH 4/4] fix(state): correct dispatchToMainActor docs and throttled objectDidChange timing - Updated dispatchToMainActor docstring to accurately describe the DispatchQueue.main.async + @MainActor Task pattern (was incorrectly referencing a Combine subject). - Moved objectDidChange notifications in throttled state variants to fire after current is actually updated in the throttle sink, rather than prematurely in the applyThrottledStateUpdate method where current still holds the old value. --- .../Models/Context/ReSwiftObserver.swift | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/Sources/Rownd/Models/Context/ReSwiftObserver.swift b/Sources/Rownd/Models/Context/ReSwiftObserver.swift index bd64df6..f34fa9a 100644 --- a/Sources/Rownd/Models/Context/ReSwiftObserver.swift +++ b/Sources/Rownd/Models/Context/ReSwiftObserver.swift @@ -14,9 +14,10 @@ import SwiftUI /// Dispatches work to the MainActor from a nonisolated context. /// -/// This helper serializes state updates through a Combine subject to maintain FIFO ordering, -/// then processes them on the MainActor. This prevents the ordering issues that can occur -/// with unstructured Task spawning under high-frequency state changes. +/// Uses `DispatchQueue.main.async` to preserve FIFO ordering of state updates, +/// then hops into a `@MainActor` Task for proper isolation. This prevents the +/// ordering issues that can occur with unstructured Task spawning under +/// high-frequency state changes. /// /// - Parameters: /// - instance: The object to operate on (captured weakly) @@ -134,7 +135,12 @@ public class ObservableThrottledState: ObservableState { objectThrottled .throttle(for: .milliseconds(throttleInMs), scheduler: DispatchQueue.main, latest: true) - .sink { [weak self] in self?.current = $0 } + .sink { [weak self] in + guard let self = self else { return } + let old = self.current + self.current = $0 + self.objectDidChange.send(DidChangeSubject(old: old, new: self.current)) + } .store(in: &cancellables) } @@ -146,7 +152,6 @@ public class ObservableThrottledState: ObservableState { fileprivate func applyThrottledStateUpdate(_ state: T) { guard current != state else { return } - let old = current if let animation = animation { withAnimation(animation) { objectThrottled.send(state) @@ -154,7 +159,6 @@ public class ObservableThrottledState: ObservableState { } else { objectThrottled.send(state) } - objectDidChange.send(DidChangeSubject(old: old, new: current)) } private let objectThrottled = PassthroughSubject() @@ -249,7 +253,10 @@ public class ObservableDerivedThrottledState()