From bba53b725e59e6c47a3ec2d89a3a2c6d17c9f5e8 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Tue, 4 Aug 2026 15:41:29 -0400 Subject: [PATCH 01/11] feat(sessions): replace promises with swift concurrency --- FirebaseSessions.podspec | 1 - .../Sources/FirebaseSessions.swift | 139 +++++++++--------- .../Sources/Public/SessionsProvider.swift | 2 +- .../Sources/SessionStartEvent.swift | 2 +- FirebaseSessions/Sources/SessionsState.swift | 54 +++++++ .../FirebaseSessionsTests+BaseBehaviors.swift | 18 +-- ...FirebaseSessionsTests+DataCollection.swift | 20 +-- .../FirebaseSessionsTests+Subscribers.swift | 16 +- .../Library/FirebaseSessionsTestsBase.swift | 12 +- .../Tests/Unit/SessionsStateTests.swift | 61 ++++++++ Package.swift | 1 - 11 files changed, 215 insertions(+), 111 deletions(-) create mode 100644 FirebaseSessions/Sources/SessionsState.swift create mode 100644 FirebaseSessions/Tests/Unit/SessionsStateTests.swift diff --git a/FirebaseSessions.podspec b/FirebaseSessions.podspec index 29bf447e6aa..47b2d6aea5c 100644 --- a/FirebaseSessions.podspec +++ b/FirebaseSessions.podspec @@ -46,7 +46,6 @@ Pod::Spec.new do |s| s.dependency 'GoogleUtilities/Environment', '~> 8.1' s.dependency 'GoogleUtilities/UserDefaults', '~> 8.1' s.dependency 'nanopb', '~> 3.30910.0' - s.dependency 'PromisesSwift', '~> 2.1' s.pod_target_xcconfig = { 'HEADER_SEARCH_PATHS' => '"${PODS_TARGET_SRCROOT}"', diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index fa3e4c05f43..d18d5b80849 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -20,11 +20,8 @@ internal import FirebaseInstallations internal import GoogleDataTransport #if swift(>=6.0) - internal import Promises #elseif swift(>=5.10) - import Promises #else - internal import Promises #endif private enum GoogleDataTransportConfig { @@ -32,7 +29,7 @@ private enum GoogleDataTransportConfig { static let sessionsTarget = GDTCORTarget.FLL } -@objc(FIRSessions) final class Sessions: NSObject, Library, SessionsProvider { +@objc(FIRSessions) final class Sessions: NSObject, Library, SessionsProvider, @unchecked Sendable { // MARK: - Private Variables /// The Firebase App ID associated with Sessions. @@ -45,13 +42,12 @@ private enum GoogleDataTransportConfig { private let appInfo: ApplicationInfoProtocol private let settings: SettingsProtocol - /// Subscribers - /// `subscribers` are used to determine the Data Collection state of the Sessions SDK. - /// If any Subscribers has Data Collection enabled, the Sessions SDK will send events - private var subscribers: [SessionsSubscriber] = [] - /// `subscriberPromises` are used to wait until all Subscribers have registered - /// themselves. Subscribers must have Data Collection state available upon registering. - private var subscriberPromises: [SessionsSubscriberName: Promise] = [:] + /// `state` holds the mutable state (subscribers array and registration) + /// ensuring mathematical safety in Swift Concurrency. + private let state: SessionsState + + /// Queue for callbacks + private let loggedEventCallbackQueue: DispatchQueue /// Notifications static let SessionIDChangedNotificationName = Notification @@ -90,7 +86,8 @@ private enum GoogleDataTransportConfig { coordinator: coordinator, initiator: initiator, appInfo: appInfo, - settings: settings) { result in + settings: settings, + loggedEventCallbackQueue: .global(qos: .background)) { result in switch result { case .success(()): Logger.logInfo("Successfully logged Session Start event") @@ -135,9 +132,7 @@ private enum GoogleDataTransportConfig { } // Initializes the SDK and begins the process of listening for lifecycle events and logging - // events. The given `logEventCallback` is invoked on a global background queue by default, - // but configurable via `loggedEventCallbackQueue` for providing a higher priority queue - // during tests to reduce flakes. + // events. The given `logEventCallback` is invoked when event logging completes. init(appID: String, sessionGenerator: SessionGenerator, coordinator: SessionCoordinatorProtocol, initiator: SessionInitiator, appInfo: ApplicationInfoProtocol, settings: SettingsProtocol, loggedEventCallbackQueue: DispatchQueue = .global(qos: .background), @@ -149,13 +144,12 @@ private enum GoogleDataTransportConfig { self.initiator = initiator self.appInfo = appInfo self.settings = settings - - super.init() + self.loggedEventCallbackQueue = loggedEventCallbackQueue let dependencies = SessionsDependencies.dependencies - for subscriberName in dependencies { - subscriberPromises[subscriberName] = Promise.pending() - } + self.state = SessionsState(expectedSubscribers: dependencies) + + super.init() Logger .logDebug( @@ -175,42 +169,56 @@ private enum GoogleDataTransportConfig { // If there are no Dependencies, then the Sessions SDK can't acknowledge // any products data collection state, so the Sessions SDK won't send events. - guard !self.subscriberPromises.isEmpty else { - loggedEventCallback(.failure(.NoDependenciesError)) + guard !self.state.expectedSubscribers.isEmpty else { + self.loggedEventCallbackQueue.async { + loggedEventCallback(.failure(.NoDependenciesError)) + } return } - // Wait until all subscriber promises have been fulfilled before + // Wait until all expected subscribers have registered before // doing any data collection. - all(self.subscriberPromises.values).then(on: loggedEventCallbackQueue) { _ in - guard self.isAnyDataCollectionEnabled else { - loggedEventCallback(.failure(.DataCollectionError)) - return - } - - Logger.logDebug("Data Collection is enabled for at least one Subscriber") - - // Fetch settings if they have expired. This must happen after the check for - // data collection because it uses the network, but it must happen before the - // check for sessionsEnabled from Settings because otherwise we would permanently - // turn off the Sessions SDK when we disabled it. - self.settings.updateSettings() - - self.addSubscriberFields(event: event) - event.setSamplingRate(samplingRate: self.settings.samplingRate) - - guard sessionInfo.shouldDispatchEvents else { - loggedEventCallback(.failure(.SessionSamplingError)) - return - } - - guard self.settings.sessionsEnabled else { - loggedEventCallback(.failure(.DisabledViaSettingsError)) - return - } - - self.coordinator.attemptLoggingSessionStart(event: event) { result in - loggedEventCallback(result) + Task { + await self.state.waitUntilAllRegistered() + let subscribers = await self.state.currentSubscribers + + self.loggedEventCallbackQueue.async { + let isAnyDataCollectionEnabled = subscribers.contains { $0.isDataCollectionEnabled } + guard isAnyDataCollectionEnabled else { + loggedEventCallback(.failure(.DataCollectionError)) + return + } + + Logger.logDebug("Data Collection is enabled for at least one Subscriber") + + // Fetch settings if they have expired. This must happen after the check for + // data collection because it uses the network, but it must happen before the + // check for sessionsEnabled from Settings because otherwise we would permanently + // turn off the Sessions SDK when we disabled it. + self.settings.updateSettings() + let samplingRate = self.settings.samplingRate + let sessionsEnabled = self.settings.sessionsEnabled + + for subscriber in subscribers { + event.set(subscriber: subscriber.sessionsSubscriberName, + isDataCollectionEnabled: subscriber.isDataCollectionEnabled, + appInfo: self.appInfo) + } + event.setSamplingRate(samplingRate: samplingRate) + + guard sessionInfo.shouldDispatchEvents else { + loggedEventCallback(.failure(.SessionSamplingError)) + return + } + + guard sessionsEnabled else { + loggedEventCallback(.failure(.DisabledViaSettingsError)) + return + } + + self.coordinator.attemptLoggingSessionStart(event: event) { result in + loggedEventCallback(result) + } } } } @@ -227,23 +235,6 @@ private enum GoogleDataTransportConfig { // MARK: - Data Collection - var isAnyDataCollectionEnabled: Bool { - for subscriber in subscribers { - if subscriber.isDataCollectionEnabled { - return true - } - } - return false - } - - func addSubscriberFields(event: SessionStartEvent) { - for subscriber in subscribers { - event.set(subscriber: subscriber.sessionsSubscriberName, - isDataCollectionEnabled: subscriber.isDataCollectionEnabled, - appInfo: appInfo) - } - } - // MARK: - SessionsProvider var currentSessionDetails: SessionDetails { @@ -265,7 +256,7 @@ private enum GoogleDataTransportConfig { } } - func register(subscriber: SessionsSubscriber) { + @objc(registerWithSubscriber:) func register(subscriber: SessionsSubscriber) { Logger .logDebug( "Registering Sessions SDK subscriber with name: \(subscriber.sessionsSubscriberName), data collection enabled: \(subscriber.isDataCollectionEnabled)" @@ -289,9 +280,11 @@ private enum GoogleDataTransportConfig { // before subscribers, so subscribers will miss the first Notification subscriber.onSessionChanged(currentSessionDetails) - // Fulfil this subscriber's promise - subscribers.append(subscriber) - subscriberPromises[subscriber.sessionsSubscriberName]?.fulfill(()) + // Register this subscriber to resume any waiting tasks + let subscriberName = subscriber.sessionsSubscriberName + Task { + await state.register(subscriber: subscriber, name: subscriberName) + } } // MARK: - Library conformance diff --git a/FirebaseSessions/Sources/Public/SessionsProvider.swift b/FirebaseSessions/Sources/Public/SessionsProvider.swift index ef73e182b31..5ca2113e469 100644 --- a/FirebaseSessions/Sources/Public/SessionsProvider.swift +++ b/FirebaseSessions/Sources/Public/SessionsProvider.swift @@ -19,5 +19,5 @@ import Foundation // interface for other 1P SDKs to talk to. @objc(FIRSessionsProvider) public protocol SessionsProvider { - @objc func register(subscriber: SessionsSubscriber) + @objc(registerWithSubscriber:) func register(subscriber: SessionsSubscriber) } diff --git a/FirebaseSessions/Sources/SessionStartEvent.swift b/FirebaseSessions/Sources/SessionStartEvent.swift index 8b3929054fc..fdc16d0884a 100644 --- a/FirebaseSessions/Sources/SessionStartEvent.swift +++ b/FirebaseSessions/Sources/SessionStartEvent.swift @@ -32,7 +32,7 @@ internal import GoogleDataTransport /// 1) Writing fields to the Session proto /// 2) Synthesizing itself for persisting to disk and logging to GoogleDataTransport /// -class SessionStartEvent: NSObject, GDTCOREventDataObject { +class SessionStartEvent: NSObject, GDTCOREventDataObject, @unchecked Sendable { var proto: firebase_appquality_sessions_SessionEvent init(sessionInfo: SessionInfo, appInfo: ApplicationInfoProtocol, diff --git a/FirebaseSessions/Sources/SessionsState.swift b/FirebaseSessions/Sources/SessionsState.swift new file mode 100644 index 00000000000..34ac763a249 --- /dev/null +++ b/FirebaseSessions/Sources/SessionsState.swift @@ -0,0 +1,54 @@ +// +// Copyright 2024-2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Foundation + +/// An internal actor that protects mutable state for the Sessions SDK. +actor SessionsState { + private var subscribers: [SessionsSubscriber] = [] + private var registeredSubscribers: Set = [] + nonisolated let expectedSubscribers: Set + private var continuations: [CheckedContinuation] = [] + + init(expectedSubscribers: Set) { + self.expectedSubscribers = expectedSubscribers + } + + func register(subscriber: SessionsSubscriber, name: SessionsSubscriberName) { + guard !registeredSubscribers.contains(name) else { return } + subscribers.append(subscriber) + registeredSubscribers.insert(name) + if registeredSubscribers.isSuperset(of: expectedSubscribers) { + for continuation in continuations { + continuation.resume() + } + continuations.removeAll() + } + } + + func waitUntilAllRegistered() async { + if expectedSubscribers.isEmpty || registeredSubscribers.isSuperset(of: expectedSubscribers) { + return + } + // Note: If cancellation is required, use withTaskCancellationHandler and a throwing continuation. + await withCheckedContinuation { continuation in + continuations.append(continuation) + } + } + + var currentSubscribers: [SessionsSubscriber] { + subscribers + } +} diff --git a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+BaseBehaviors.swift b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+BaseBehaviors.swift index 462be23ead3..31a64472046 100644 --- a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+BaseBehaviors.swift +++ b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+BaseBehaviors.swift @@ -24,8 +24,8 @@ import XCTest final class FirebaseSessionsTestsBase_BaseBehaviors: FirebaseSessionsTestsBase { // MARK: - Test Settings & Sampling - @MainActor func test_settingsDisabled_doesNotLogSessionEventButDoesFetchSettings() { - runSessionsSDK( + @MainActor func test_settingsDisabled_doesNotLogSessionEventButDoesFetchSettings() async { + await runSessionsSDK( subscriberSDKs: [ mockPerformanceSubscriber, @@ -49,8 +49,8 @@ final class FirebaseSessionsTestsBase_BaseBehaviors: FirebaseSessionsTestsBase { ) } - @MainActor func test_sessionSampled_doesNotLogSessionEventButDoesFetchSettings() { - runSessionsSDK( + @MainActor func test_sessionSampled_doesNotLogSessionEventButDoesFetchSettings() async { + await runSessionsSDK( subscriberSDKs: [ mockPerformanceSubscriber, @@ -84,15 +84,15 @@ final class FirebaseSessionsTestsBase_BaseBehaviors: FirebaseSessionsTestsBase { // This test ensures that if we go into the background for longer than // the Session Timeout, we log another event when we come to the foreground. // - // We wanted to make sure that since we've introduced promises, - // once the promise has been fulfilled, that .then'ing on the promise + // We wanted to make sure that since we've introduced Swift Concurrency, + // once all expected subscribers have been registered, awaiting on the registration // in future initiations still results in a log - @MainActor func test_multipleInitiations_logsSessionEventEachInitiation() { + @MainActor func test_multipleInitiations_logsSessionEventEachInitiation() async { var loggedCount = 0 var lastLoggedSessionID = "" let loggedTwiceExpectation = expectation(description: "Sessions SDK logged events twice") - runSessionsSDK( + await runSessionsSDK( subscriberSDKs: [ mockPerformanceSubscriber, @@ -138,7 +138,7 @@ final class FirebaseSessionsTestsBase_BaseBehaviors: FirebaseSessionsTestsBase { } ) - wait(for: [loggedTwiceExpectation], timeout: 3) + await fulfillment(of: [loggedTwiceExpectation], timeout: 3) // Make sure we logged 2 events XCTAssertEqual(loggedCount, 2) diff --git a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+DataCollection.swift b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+DataCollection.swift index 5a2c5d1da97..ae9db9d64a6 100644 --- a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+DataCollection.swift +++ b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+DataCollection.swift @@ -81,19 +81,19 @@ final class FirebaseSessionsTestsBase_DataCollection: FirebaseSessionsTestsBase // MARK: - Test Data Collection - @MainActor func test_subscriberWithDataCollectionEnabled_logsSessionEvent() { - runSessionsSDK( + @MainActor func test_subscriberWithDataCollectionEnabled_logsSessionEvent() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, ], preSessionsInit: { _ in // Nothing }, postSessionsInit: { - sessions.register(subscriber: self.mockCrashlyticsSubscriber) - // Sessions hasn't logged yet because no Subscriber SDKs have registered XCTAssertNil(self.mockCoordinator.loggedEvent) + sessions.register(subscriber: self.mockCrashlyticsSubscriber) + }, postLogEvent: { result, subscriberSDKs in // Make sure the SDK reported success, we logged an event and // Settings fetched new configs @@ -105,8 +105,8 @@ final class FirebaseSessionsTestsBase_DataCollection: FirebaseSessionsTestsBase ) } - @MainActor func test_subscribersSomeDataCollectionDisabled_logsSessionEvent() { - runSessionsSDK( + @MainActor func test_subscribersSomeDataCollectionDisabled_logsSessionEvent() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, mockPerformanceSubscriber, @@ -132,8 +132,8 @@ final class FirebaseSessionsTestsBase_DataCollection: FirebaseSessionsTestsBase ) } - @MainActor func test_subscribersAllDataCollectionDisabled_doesNotLogSessionEvent() { - runSessionsSDK( + @MainActor func test_subscribersAllDataCollectionDisabled_doesNotLogSessionEvent() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, mockPerformanceSubscriber, @@ -159,8 +159,8 @@ final class FirebaseSessionsTestsBase_DataCollection: FirebaseSessionsTestsBase ) } - @MainActor func test_defaultSamplingRate_isSetInProto() { - runSessionsSDK( + @MainActor func test_defaultSamplingRate_isSetInProto() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, diff --git a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+Subscribers.swift b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+Subscribers.swift index 139a8826e0e..c19a042ac7c 100644 --- a/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+Subscribers.swift +++ b/FirebaseSessions/Tests/Unit/FirebaseSessionsTests+Subscribers.swift @@ -37,8 +37,8 @@ final class FirebaseSessionsTestsBase_Subscribers: FirebaseSessionsTestsBase { // MARK: - Test Subscriber Callbacks - @MainActor func test_registerSubscriber_callsOnSessionChanged() { - runSessionsSDK( + @MainActor func test_registerSubscriber_callsOnSessionChanged() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, mockPerformanceSubscriber, @@ -61,8 +61,8 @@ final class FirebaseSessionsTestsBase_Subscribers: FirebaseSessionsTestsBase { // Make sure that even if the Sessions SDK is disabled, and data collection // is disabled, the Sessions SDK still generates Session IDs and provides // them to Subscribers - @MainActor func test_subscribersDataCollectionDisabled_callsOnSessionChanged() { - runSessionsSDK( + @MainActor func test_subscribersDataCollectionDisabled_callsOnSessionChanged() async { + await runSessionsSDK( subscriberSDKs: [ mockCrashlyticsSubscriber, mockPerformanceSubscriber, @@ -86,8 +86,8 @@ final class FirebaseSessionsTestsBase_Subscribers: FirebaseSessionsTestsBase { ) } - @MainActor func test_noDependencies_doesNotLogSessionEvent() { - runSessionsSDK( + @MainActor func test_noDependencies_doesNotLogSessionEvent() async { + await runSessionsSDK( subscriberSDKs: [], preSessionsInit: { _ in // Nothing @@ -102,8 +102,8 @@ final class FirebaseSessionsTestsBase_Subscribers: FirebaseSessionsTestsBase { ) } - @MainActor func test_noSubscribersWithRegistrations_doesNotCrash() { - runSessionsSDK( + @MainActor func test_noSubscribersWithRegistrations_doesNotCrash() async { + await runSessionsSDK( subscriberSDKs: [], preSessionsInit: { _ in // Nothing diff --git a/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift b/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift index 7ad39614fda..8cda22ab067 100644 --- a/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift +++ b/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift @@ -73,11 +73,11 @@ class FirebaseSessionsTestsBase: XCTestCase { /// most assertions will happen. @MainActor func runSessionsSDK(subscriberSDKs: [SessionsSubscriber], preSessionsInit: (MockSettingsProtocol) -> Void, - postSessionsInit: () -> Void, + postSessionsInit: () async -> Void, postLogEvent: @escaping @MainActor (Result, [SessionsSubscriber]) - -> Void) { + -> Void) async { // This class is static, so we need to clear global state SessionsDependencies.removeAll() @@ -110,9 +110,7 @@ class FirebaseSessionsTestsBase: XCTestCase { coordinator: mockCoordinator, initiator: initiator, appInfo: mockAppInfo, - settings: mockSettings, - // Execute the callback on a higher priority queue to avoid test flakes. - loggedEventCallbackQueue: .global(qos: .userInteractive)) { result in + settings: mockSettings) { result in DispatchQueue.main.async { // Provide the result for tests to test against postLogEvent(result, subscriberSDKs) @@ -124,11 +122,11 @@ class FirebaseSessionsTestsBase: XCTestCase { // Execute test cases after Sessions is initialized. This is a good // place register Subscriber SDKs - postSessionsInit() + await postSessionsInit() // Wait for the Sessions SDK to log the session before finishing // the test. - wait(for: [loggedEventExpectation], timeout: 3) + await fulfillment(of: [loggedEventExpectation], timeout: 3) } func assertSuccess(result: Result) { diff --git a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift new file mode 100644 index 00000000000..27b4316490d --- /dev/null +++ b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift @@ -0,0 +1,61 @@ +// +// Copyright 2024-2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import XCTest + +@testable import FirebaseSessions + +final class SessionsStateTests: XCTestCase { + func test_emptyExpectedSubscribers_returnsImmediately() async { + let state = SessionsState(expectedSubscribers: []) + await state.waitUntilAllRegistered() + XCTAssertTrue(true, "Returned immediately as expected") + } + + func test_waitUntilAllRegistered_waitsForDependencies() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics, .Performance]) + + let waitTask = Task { + await state.waitUntilAllRegistered() + } + + // Simulate one dependency registering + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + + // Ensure the task hasn't finished (it's still waiting) + // We give it a tiny delay to ensure it didn't resume early. + do { + try await Task.sleep(nanoseconds: 100_000_000) // 100ms to reduce flakiness + } catch {} + + // Now complete the registration + await state.register(subscriber: MockSubscriber(name: .Performance), name: .Performance) + + // The wait task should now complete + await waitTask.value + XCTAssertTrue(true, "Completed after all dependencies registered") + } + + func test_subsequentWaits_returnImmediately() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics]) + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + + // These should return immediately and not suspend indefinitely + await state.waitUntilAllRegistered() + await state.waitUntilAllRegistered() + + XCTAssertTrue(true, "Subsequent waits returned immediately") + } +} diff --git a/Package.swift b/Package.swift index cd3748c6f28..7c99d06c396 100644 --- a/Package.swift +++ b/Package.swift @@ -1095,7 +1095,6 @@ func packageTargets() -> [Target] { // - https://github.com/firebase/firebase-ios-sdk/issues/15276 // - https://github.com/firebase/firebase-ios-sdk/pull/15287 .product(name: "nanopb", package: "nanopb"), - .product(name: "Promises", package: "Promises"), .product(name: "GoogleDataTransport", package: "GoogleDataTransport"), .product(name: "GULEnvironment", package: "GoogleUtilities"), .product(name: "GULUserDefaults", package: "GoogleUtilities"), From 400a49c90acc2806bf3820c9ea2100e51130f4f3 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Sun, 23 Aug 2026 15:23:03 -0400 Subject: [PATCH 02/11] cleanup: style and remove unneeded #ifs --- FirebaseSessions/Sources/FirebaseSessions.swift | 9 ++------- FirebaseSessions/Sources/SessionsState.swift | 3 ++- .../Tests/Unit/SessionsStateTests.swift | 14 +++++++------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index d18d5b80849..c9e558c2aa9 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -19,11 +19,6 @@ internal import FirebaseCoreExtension internal import FirebaseInstallations internal import GoogleDataTransport -#if swift(>=6.0) -#elseif swift(>=5.10) -#else -#endif - private enum GoogleDataTransportConfig { static let sessionsLogSource = "1974" static let sessionsTarget = GDTCORTarget.FLL @@ -147,7 +142,7 @@ private enum GoogleDataTransportConfig { self.loggedEventCallbackQueue = loggedEventCallbackQueue let dependencies = SessionsDependencies.dependencies - self.state = SessionsState(expectedSubscribers: dependencies) + state = SessionsState(expectedSubscribers: dependencies) super.init() @@ -181,7 +176,7 @@ private enum GoogleDataTransportConfig { Task { await self.state.waitUntilAllRegistered() let subscribers = await self.state.currentSubscribers - + self.loggedEventCallbackQueue.async { let isAnyDataCollectionEnabled = subscribers.contains { $0.isDataCollectionEnabled } guard isAnyDataCollectionEnabled else { diff --git a/FirebaseSessions/Sources/SessionsState.swift b/FirebaseSessions/Sources/SessionsState.swift index 34ac763a249..6d8b4a411d3 100644 --- a/FirebaseSessions/Sources/SessionsState.swift +++ b/FirebaseSessions/Sources/SessionsState.swift @@ -42,7 +42,8 @@ actor SessionsState { if expectedSubscribers.isEmpty || registeredSubscribers.isSuperset(of: expectedSubscribers) { return } - // Note: If cancellation is required, use withTaskCancellationHandler and a throwing continuation. + // Note: If cancellation is required, use withTaskCancellationHandler and a throwing + // continuation. await withCheckedContinuation { continuation in continuations.append(continuation) } diff --git a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift index 27b4316490d..8609dec7450 100644 --- a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift +++ b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift @@ -26,23 +26,23 @@ final class SessionsStateTests: XCTestCase { func test_waitUntilAllRegistered_waitsForDependencies() async { let state = SessionsState(expectedSubscribers: [.Crashlytics, .Performance]) - + let waitTask = Task { await state.waitUntilAllRegistered() } - + // Simulate one dependency registering await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) - + // Ensure the task hasn't finished (it's still waiting) // We give it a tiny delay to ensure it didn't resume early. do { try await Task.sleep(nanoseconds: 100_000_000) // 100ms to reduce flakiness } catch {} - + // Now complete the registration await state.register(subscriber: MockSubscriber(name: .Performance), name: .Performance) - + // The wait task should now complete await waitTask.value XCTAssertTrue(true, "Completed after all dependencies registered") @@ -51,11 +51,11 @@ final class SessionsStateTests: XCTestCase { func test_subsequentWaits_returnImmediately() async { let state = SessionsState(expectedSubscribers: [.Crashlytics]) await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) - + // These should return immediately and not suspend indefinitely await state.waitUntilAllRegistered() await state.waitUntilAllRegistered() - + XCTAssertTrue(true, "Subsequent waits returned immediately") } } From f0a059399c5eb8ce3614989b549f55850c62f8fd Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Sun, 23 Aug 2026 15:26:06 -0400 Subject: [PATCH 03/11] cleanup: fix copyrights --- FirebaseSessions/Sources/SessionsState.swift | 3 +-- FirebaseSessions/Tests/Unit/SessionsStateTests.swift | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/FirebaseSessions/Sources/SessionsState.swift b/FirebaseSessions/Sources/SessionsState.swift index 6d8b4a411d3..e40a5c228f2 100644 --- a/FirebaseSessions/Sources/SessionsState.swift +++ b/FirebaseSessions/Sources/SessionsState.swift @@ -1,5 +1,4 @@ -// -// Copyright 2024-2026 Google LLC +// Copyright 2026 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift index 8609dec7450..85c6c0e35f6 100644 --- a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift +++ b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift @@ -1,5 +1,4 @@ -// -// Copyright 2024-2026 Google LLC +// Copyright 2026 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. From fa47e2a3804d3723f9940d59929d37be5f3296be Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Fri, 18 Sep 2026 17:21:31 -0400 Subject: [PATCH 04/11] Update Package.swift --- Package.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Package.swift b/Package.swift index e86339d23a6..711d389900a 100644 --- a/Package.swift +++ b/Package.swift @@ -1162,6 +1162,7 @@ func packageTargets() -> [Target] { .target(name: "FirebaseInAppMessaging", condition: .when(platforms: [.iOS, .tvOS])), "FirebaseInstallations", + "FirebaseMessaging", .target(name: "FirebasePerformance", condition: .when(platforms: [.iOS, .tvOS])), "FirebaseRemoteConfig", From b0d794e6690a595c452c3b5c4631624bbe8c0e96 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Sep 2026 19:10:00 -0400 Subject: [PATCH 05/11] fix(sessions): restore high-priority logged event callback queue in tests The Swift Concurrency refactor dropped the explicit `loggedEventCallbackQueue: .global(qos: .userInteractive)` argument, so the unit tests fell back to the production default of `.global(qos: .background)` while still asserting against a 3 second expectation timeout. Background QoS is aggressively deprioritized on contended machines, which made the callback arrive late and caused non-deterministic CI failures: a different test timed out on each retry, and the tests that did pass swung between 0.037s and 2.837s. Toggling only this line locally on an idle machine moves the affected suites from 0.728s to 0.027s, with individual tests 40-60x faster. Restore the argument and the comment explaining why it exists. --- .../Tests/Unit/Library/FirebaseSessionsTestsBase.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift b/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift index 8cda22ab067..dce1070cdd6 100644 --- a/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift +++ b/FirebaseSessions/Tests/Unit/Library/FirebaseSessionsTestsBase.swift @@ -110,7 +110,9 @@ class FirebaseSessionsTestsBase: XCTestCase { coordinator: mockCoordinator, initiator: initiator, appInfo: mockAppInfo, - settings: mockSettings) { result in + settings: mockSettings, + // Execute the callback on a higher priority queue to avoid test flakes. + loggedEventCallbackQueue: .global(qos: .userInteractive)) { result in DispatchQueue.main.async { // Provide the result for tests to test against postLogEvent(result, subscriberSDKs) From b3b3f920754eff10342ee2c636d0854db9143908 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Sep 2026 19:10:18 -0400 Subject: [PATCH 06/11] refactor(sessions): return subscriber snapshot from the registration gate `waitUntilAllRegistered()` now returns the registered subscribers instead of requiring a second `currentSubscribers` access, so the session start path needs one actor hop rather than two. Behavior is unchanged: the snapshot is still read after the gate opens. Also document the invariants that make the actor correct, since they are load-bearing and not obvious: - `register(...)` is deliberately non-async, so the de-duplication check, the append and the continuation resume cannot interleave. - There is no lost-wakeup race, because the `withCheckedContinuation` body runs synchronously inside the actor before the caller suspends. - The gate latches, which is what lets session starts after the first one resolve without suspending, matching the previous promise behavior. - `register(subscriber:)` is now asynchronous, so returning from it no longer implies the subscriber is visible to an in-flight session start. - `SessionStartEvent` is `@unchecked Sendable` only because it is handed off between executors and never shared; it wraps manually managed nanopb pointers, so concurrent mutation would corrupt memory. Restore the init doc comment describing `loggedEventCallbackQueue`. --- .../Sources/FirebaseSessions.swift | 15 +++++-- .../Sources/SessionStartEvent.swift | 10 +++++ FirebaseSessions/Sources/SessionsState.swift | 39 ++++++++++++++++++- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index bfa53b31e6a..16f93f65127 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -128,7 +128,9 @@ private enum GoogleDataTransportConfig { } // Initializes the SDK and begins the process of listening for lifecycle events and logging - // events. The given `logEventCallback` is invoked when event logging completes. + // events. The given `logEventCallback` is invoked on a global background queue by default, + // but configurable via `loggedEventCallbackQueue` for providing a higher priority queue + // during tests to reduce flakes. init(appID: String, sessionGenerator: SessionGenerator, coordinator: SessionCoordinatorProtocol, initiator: SessionInitiator, appInfo: ApplicationInfoProtocol, settings: SettingsProtocol, loggedEventCallbackQueue: DispatchQueue = .global(qos: .background), @@ -175,8 +177,7 @@ private enum GoogleDataTransportConfig { // Wait until all expected subscribers have registered before // doing any data collection. Task { - await self.state.waitUntilAllRegistered() - let subscribers = await self.state.currentSubscribers + let subscribers = await self.state.waitUntilAllRegistered() self.loggedEventCallbackQueue.async { let isAnyDataCollectionEnabled = subscribers.contains { $0.isDataCollectionEnabled } @@ -276,7 +277,13 @@ private enum GoogleDataTransportConfig { // before subscribers, so subscribers will miss the first Notification subscriber.onSessionChanged(currentSessionDetails) - // Register this subscriber to resume any waiting tasks + // Register this subscriber to resume any waiting tasks. + // + // Unlike the previous promise-based implementation, this hop is + // asynchronous: returning from `register(subscriber:)` does not guarantee + // the subscriber is visible to an in-flight session start. That is safe + // because a session start awaits `waitUntilAllRegistered()`, which only + // proceeds once this `Task` has run for every expected subscriber. let subscriberName = subscriber.sessionsSubscriberName Task { await state.register(subscriber: subscriber, name: subscriberName) diff --git a/FirebaseSessions/Sources/SessionStartEvent.swift b/FirebaseSessions/Sources/SessionStartEvent.swift index fdc16d0884a..1687e247ca9 100644 --- a/FirebaseSessions/Sources/SessionStartEvent.swift +++ b/FirebaseSessions/Sources/SessionStartEvent.swift @@ -32,6 +32,16 @@ internal import GoogleDataTransport /// 1) Writing fields to the Session proto /// 2) Synthesizing itself for persisting to disk and logging to GoogleDataTransport /// +/// - Note: This type is `@unchecked Sendable` rather than genuinely thread safe. +/// It wraps a mutable nanopb struct holding manually managed pointers that are +/// freed in `deinit`, so concurrent mutation would corrupt memory. The safety +/// invariant is that an event instance is only ever *handed off* between +/// executors, never shared: it is created on the initiator's thread, mutated +/// on a single callback queue, and then handed to the coordinator, which +/// serializes its own writes. Do not retain an event across those stages or +/// mutate it from more than one context. +/// TODO: Make this checked `Sendable` by making the proto writes internally +/// synchronized, or by modeling the event as a value type. class SessionStartEvent: NSObject, GDTCOREventDataObject, @unchecked Sendable { var proto: firebase_appquality_sessions_SessionEvent diff --git a/FirebaseSessions/Sources/SessionsState.swift b/FirebaseSessions/Sources/SessionsState.swift index e40a5c228f2..49ac1ee97a2 100644 --- a/FirebaseSessions/Sources/SessionsState.swift +++ b/FirebaseSessions/Sources/SessionsState.swift @@ -15,6 +15,16 @@ import Foundation /// An internal actor that protects mutable state for the Sessions SDK. +/// +/// This replaces the previous `FBLPromise`-based registration barrier. It acts +/// as a *latching gate*: once every expected subscriber has registered, the +/// gate stays open for the remaining lifetime of the process, so session +/// starts after the first one resolve without suspending. That mirrors the old +/// behavior, where the per-subscriber promises stayed fulfilled once resolved. +/// +/// If an expected subscriber never registers, waiters are never resumed and no +/// event is sent. This is also the pre-existing behavior of the unfulfilled +/// promises. actor SessionsState { private var subscribers: [SessionsSubscriber] = [] private var registeredSubscribers: Set = [] @@ -25,6 +35,17 @@ actor SessionsState { self.expectedSubscribers = expectedSubscribers } + /// Records a subscriber and opens the gate once all expected subscribers + /// have registered. + /// + /// Registration is idempotent per subscriber *name*. Subscribers that were + /// never declared as dependencies are still tracked (so they contribute to + /// the data collection check) but cannot open the gate on their own. + /// + /// This method is deliberately non-`async`: it contains no suspension + /// points, so the actor runs it to completion. The de-duplication check, the + /// append, and the continuation resume therefore cannot interleave with + /// another `register` or `waitUntilAllRegistered` call. func register(subscriber: SessionsSubscriber, name: SessionsSubscriberName) { guard !registeredSubscribers.contains(name) else { return } subscribers.append(subscriber) @@ -37,15 +58,29 @@ actor SessionsState { } } - func waitUntilAllRegistered() async { + /// Suspends until every expected subscriber has registered, then returns a + /// snapshot of the registered subscribers. + /// + /// The snapshot is returned from here rather than read through a separate + /// accessor so that the session-start path only needs a single actor hop. + /// As before, the snapshot reflects the state *after* the gate opens, so a + /// subscriber that registers late is still included. + func waitUntilAllRegistered() async -> [SessionsSubscriber] { if expectedSubscribers.isEmpty || registeredSubscribers.isSuperset(of: expectedSubscribers) { - return + return subscribers } + // No lost-wakeup race here: the closure passed to `withCheckedContinuation` + // runs synchronously in this actor's isolation domain before the caller + // suspends. A `register` call therefore cannot slip in between the check + // above and the append below, so the continuation is always either + // enqueued before the gate opens, or the fast path above already returned. + // // Note: If cancellation is required, use withTaskCancellationHandler and a throwing // continuation. await withCheckedContinuation { continuation in continuations.append(continuation) } + return subscribers } var currentSubscribers: [SessionsSubscriber] { From df354d9b1d907be2b5ccc262cf2731cc3397ef75 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Sep 2026 19:22:29 -0400 Subject: [PATCH 07/11] test(sessions): strengthen SessionsState registration gate tests The existing tests asserted `XCTAssertTrue(true)`, so they verified nothing. In particular `test_waitUntilAllRegistered_waitsForDependencies` would have passed even if the gate opened immediately, because it never asserted that the waiter was still suspended, and a regression would have hung rather than failed. Replace them with tests that assert real behavior: - the gate stays closed until every expected subscriber registers - a subscriber that was never declared as a dependency cannot open the gate on its own, but is still tracked - the gate stays open for subsequent session starts - every queued continuation is resumed, not just the first - registration is idempotent per subscriber name - concurrent registrations are serialized and de-duplicated Verified by mutation: stubbing the gate to return immediately fails two of the new tests, while all three of the previous tests passed under the same mutation. --- .../Tests/Unit/SessionsStateTests.swift | 199 ++++++++++++++++-- 1 file changed, 178 insertions(+), 21 deletions(-) diff --git a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift index 85c6c0e35f6..029a8350e94 100644 --- a/FirebaseSessions/Tests/Unit/SessionsStateTests.swift +++ b/FirebaseSessions/Tests/Unit/SessionsStateTests.swift @@ -16,45 +16,202 @@ import XCTest @testable import FirebaseSessions +/// A one-way flag used to observe whether an awaiting `Task` has resumed +/// without blocking the cooperative thread pool. +private actor Signal { + private(set) var isSet = false + + func set() { + isSet = true + } +} + final class SessionsStateTests: XCTestCase { - func test_emptyExpectedSubscribers_returnsImmediately() async { + /// Generous timeout for operations that are expected to complete. + private static let timeout: TimeInterval = 5 + + /// Gives any runnable `Task` ample opportunity to make progress. Used before + /// asserting that a waiter has *not* resumed, so that the assertion fails + /// loudly rather than passing because the waiter simply hadn't been + /// scheduled yet. + private static func drainScheduler() async { + for _ in 0 ..< 20 { + await Task.yield() + } + try? await Task.sleep(nanoseconds: 50_000_000) // 50ms + } + + // MARK: - Gate opens when there is nothing to wait for + + func test_noExpectedSubscribers_waitReturnsImmediately() async { let state = SessionsState(expectedSubscribers: []) - await state.waitUntilAllRegistered() - XCTAssertTrue(true, "Returned immediately as expected") + + let resumed = expectation(description: "waitUntilAllRegistered returned") + Task { + let subscribers = await state.waitUntilAllRegistered() + XCTAssertTrue(subscribers.isEmpty) + resumed.fulfill() + } + + await fulfillment(of: [resumed], timeout: Self.timeout) } - func test_waitUntilAllRegistered_waitsForDependencies() async { + // MARK: - Gate stays closed until every expected subscriber registers + + func test_waitDoesNotResumeUntilAllExpectedSubscribersRegister() async { let state = SessionsState(expectedSubscribers: [.Crashlytics, .Performance]) - let waitTask = Task { - await state.waitUntilAllRegistered() + let signal = Signal() + let resumed = expectation(description: "waiter resumed once all registered") + let waiter = Task { () -> [SessionsSubscriber] in + let subscribers = await state.waitUntilAllRegistered() + await signal.set() + resumed.fulfill() + return subscribers } - // Simulate one dependency registering + // Only one of the two expected subscribers has registered. await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + await Self.drainScheduler() - // Ensure the task hasn't finished (it's still waiting) - // We give it a tiny delay to ensure it didn't resume early. - do { - try await Task.sleep(nanoseconds: 100_000_000) // 100ms to reduce flakiness - } catch {} + let resumedEarly = await signal.isSet + XCTAssertFalse( + resumedEarly, + "waitUntilAllRegistered() resumed before Performance registered" + ) - // Now complete the registration + // Completing the set must open the gate. await state.register(subscriber: MockSubscriber(name: .Performance), name: .Performance) + await fulfillment(of: [resumed], timeout: Self.timeout) + + // The gate hands back a snapshot containing both subscribers. + let names = await Set(waiter.value.map(\.sessionsSubscriberName)) + XCTAssertEqual(names, [.Crashlytics, .Performance]) + } + + /// A subscriber that never declared itself as a dependency must not satisfy + /// the gate on its own, but it should still be reported as a subscriber. + /// This matches the pre-refactor promise-based behavior, where only expected + /// subscribers had a promise to fulfill but every registrant was appended to + /// the `subscribers` array. + func test_unexpectedSubscriber_doesNotOpenGateButIsStillTracked() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics]) + + let signal = Signal() + let resumed = expectation(description: "waiter resumed once Crashlytics registered") + let waiter = Task { () -> [SessionsSubscriber] in + let subscribers = await state.waitUntilAllRegistered() + await signal.set() + resumed.fulfill() + return subscribers + } + + await state.register(subscriber: MockSubscriber(name: .Performance), name: .Performance) + await Self.drainScheduler() + + let resumedEarly = await signal.isSet + XCTAssertFalse( + resumedEarly, + "An unexpected subscriber must not satisfy the registration gate" + ) + + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + await fulfillment(of: [resumed], timeout: Self.timeout) + + let names = await Set(waiter.value.map(\.sessionsSubscriberName)) + XCTAssertEqual(names, [.Crashlytics, .Performance]) + } - // The wait task should now complete - await waitTask.value - XCTAssertTrue(true, "Completed after all dependencies registered") + // MARK: - Gate stays open for subsequent session starts + + /// Each app foreground beyond the session timeout starts a new session and + /// awaits the gate again. Once satisfied, the gate must never re-close. + func test_waitAfterAllRegistered_returnsImmediatelyEveryTime() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics]) + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + + for initiation in 1 ... 3 { + let resumed = expectation(description: "wait returned for initiation \(initiation)") + Task { + let subscribers = await state.waitUntilAllRegistered() + XCTAssertEqual(subscribers.count, 1) + resumed.fulfill() + } + await fulfillment(of: [resumed], timeout: Self.timeout) + } + } + + // MARK: - Every queued continuation is resumed + + func test_multipleConcurrentWaiters_allResume() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics]) + + let waiterCount = 8 + let resumed = expectation(description: "all waiters resumed") + resumed.expectedFulfillmentCount = waiterCount + + for _ in 0 ..< waiterCount { + Task { + _ = await state.waitUntilAllRegistered() + resumed.fulfill() + } + } + + // Let the waiters queue their continuations before the gate opens. + await Self.drainScheduler() + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + + await fulfillment(of: [resumed], timeout: Self.timeout) } - func test_subsequentWaits_returnImmediately() async { + // MARK: - Registration is idempotent + + func test_duplicateRegistration_isIgnored() async { let state = SessionsState(expectedSubscribers: [.Crashlytics]) + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + await state.register(subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics) + + let subscribers = await state.currentSubscribers + XCTAssertEqual( + subscribers.count, 1, + "Registering the same subscriber name twice must not duplicate it" + ) + } + + /// `Sessions.register(subscriber:)` hops onto an unstructured `Task`, so + /// registrations can arrive concurrently and out of order. The actor must + /// serialize them without losing the wakeup or duplicating subscribers. + func test_concurrentRegistrations_areSerializedAndOpenGateExactlyOnce() async { + let state = SessionsState(expectedSubscribers: [.Crashlytics, .Performance]) + + let resumed = expectation(description: "waiter resumed") + Task { + _ = await state.waitUntilAllRegistered() + resumed.fulfill() + } + + await withTaskGroup(of: Void.self) { group in + for _ in 0 ..< 25 { + group.addTask { + await state.register( + subscriber: MockSubscriber(name: .Crashlytics), name: .Crashlytics + ) + } + group.addTask { + await state.register( + subscriber: MockSubscriber(name: .Performance), name: .Performance + ) + } + } + } - // These should return immediately and not suspend indefinitely - await state.waitUntilAllRegistered() - await state.waitUntilAllRegistered() + await fulfillment(of: [resumed], timeout: Self.timeout) - XCTAssertTrue(true, "Subsequent waits returned immediately") + let subscribers = await state.currentSubscribers + XCTAssertEqual( + subscribers.count, 2, + "Concurrent duplicate registrations must be de-duplicated" + ) } } From 83b5233d12c80af8ede7681d1b3fdf3495820f5e Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Sep 2026 22:07:34 -0400 Subject: [PATCH 08/11] docs(sessions): document the Sessions @unchecked Sendable conformance This PR adds `@unchecked Sendable` to `Sessions` so that the session start path can capture `self` in a `Task`. Record why that is safe and what blocks a checked conformance. All stored properties are `let`, so the conformance is not hiding mutable state on `Sessions` itself. The blocker is `SessionGenerator`, which has genuinely unsynchronized mutable state: writes happen on the initiator's thread while `currentSessionDetails` may be read from a subscriber's thread. That race predates this conformance and is unchanged here, so note it as a follow-up rather than widening this change. Also replace a comment introduced by the refactor claiming the actor ensures "mathematical safety in Swift Concurrency", which is meaningless, with an accurate description of what the actor isolates. --- FirebaseSessions/Sources/FirebaseSessions.swift | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index 16f93f65127..227393bdee2 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -25,6 +25,14 @@ private enum GoogleDataTransportConfig { static let sessionsTarget = GDTCORTarget.FLL } +/// - Note: The `@unchecked Sendable` conformance is required because the +/// session start path captures `self` in a `Task`. All stored properties are +/// `let`, but several of them have non-`Sendable` types, so the conformance +/// cannot be checked today. The one with genuinely unsynchronized mutable +/// state is `SessionGenerator`; its writes happen on the initiator's thread +/// while `currentSessionDetails` may be read from a subscriber's thread. +/// That predates this type's `Sendable` conformance and is unchanged here. +/// TODO: Synchronize `SessionGenerator` and make this a checked conformance. @objc(FIRSessions) final class Sessions: NSObject, Library, SessionsProvider, @unchecked Sendable { // MARK: - Private Variables @@ -38,8 +46,9 @@ private enum GoogleDataTransportConfig { private let appInfo: ApplicationInfoProtocol private let settings: SettingsProtocol - /// `state` holds the mutable state (subscribers array and registration) - /// ensuring mathematical safety in Swift Concurrency. + /// `state` holds the mutable state (the subscriber list and which + /// subscribers have registered), isolated to an actor so it can be mutated + /// and read safely from multiple concurrency domains. private let state: SessionsState /// Queue for callbacks From 300babf9b7ae47f9c8d726ea3405ed380a91500c Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Sep 2026 17:38:20 -0400 Subject: [PATCH 09/11] revert(sessions): drop the redundant ObjC selector annotation The refactor added `@objc(registerWithSubscriber:)` to `SessionsProvider.register(subscriber:)`. Verified against the compiler that this is a no-op: Swift already synthesizes `-registerWithSubscriber:` for a method with a single labeled `subscriber` parameter, which is why the existing Objective-C callers in Crashlytics and Performance already use that selector on main. Removing it keeps the public API surface byte-identical to main, so this PR no longer touches a public header. --- FirebaseSessions/Sources/FirebaseSessions.swift | 2 +- FirebaseSessions/Sources/Public/SessionsProvider.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index 227393bdee2..281f539f7bc 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -262,7 +262,7 @@ private enum GoogleDataTransportConfig { } } - @objc(registerWithSubscriber:) func register(subscriber: SessionsSubscriber) { + func register(subscriber: SessionsSubscriber) { Logger .logDebug( "Registering Sessions SDK subscriber with name: \(subscriber.sessionsSubscriberName), data collection enabled: \(subscriber.isDataCollectionEnabled)" diff --git a/FirebaseSessions/Sources/Public/SessionsProvider.swift b/FirebaseSessions/Sources/Public/SessionsProvider.swift index 5ca2113e469..ef73e182b31 100644 --- a/FirebaseSessions/Sources/Public/SessionsProvider.swift +++ b/FirebaseSessions/Sources/Public/SessionsProvider.swift @@ -19,5 +19,5 @@ import Foundation // interface for other 1P SDKs to talk to. @objc(FIRSessionsProvider) public protocol SessionsProvider { - @objc(registerWithSubscriber:) func register(subscriber: SessionsSubscriber) + @objc func register(subscriber: SessionsSubscriber) } From b75eaaa3b999fa76ca1f8bc8205272a873c50603 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Sep 2026 17:38:42 -0400 Subject: [PATCH 10/11] fix(sessions): synchronize SessionGenerator's mutable state `SessionGenerator` mutated `thisSession`, `firstSessionId` and `sessionIndex` without synchronization. Writes happen on whichever thread initiates a session while `currentSession` is read from subscriber threads via `Sessions.currentSessionDetails`, so the race was real, just pre-existing. Move the mutable fields into a private `State` struct guarded by `UnfairLock` and make `SessionGenerator` a genuinely `Sendable` final class. `collectEvents` becomes a `let` since it was never mutated. This removes the last unsynchronized mutable state behind the `Sessions` `@unchecked Sendable` conformance, so the accompanying note no longer has to document an active race; the conformance is now unchecked only because of the non-`Sendable` protocol existentials. --- .../Sources/FirebaseSessions.swift | 14 ++--- .../Sources/SessionGenerator.swift | 53 +++++++++++-------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/FirebaseSessions/Sources/FirebaseSessions.swift b/FirebaseSessions/Sources/FirebaseSessions.swift index 281f539f7bc..693b2801de7 100644 --- a/FirebaseSessions/Sources/FirebaseSessions.swift +++ b/FirebaseSessions/Sources/FirebaseSessions.swift @@ -26,13 +26,13 @@ private enum GoogleDataTransportConfig { } /// - Note: The `@unchecked Sendable` conformance is required because the -/// session start path captures `self` in a `Task`. All stored properties are -/// `let`, but several of them have non-`Sendable` types, so the conformance -/// cannot be checked today. The one with genuinely unsynchronized mutable -/// state is `SessionGenerator`; its writes happen on the initiator's thread -/// while `currentSessionDetails` may be read from a subscriber's thread. -/// That predates this type's `Sendable` conformance and is unchanged here. -/// TODO: Synchronize `SessionGenerator` and make this a checked conformance. +/// session start path captures `self` in a `Task`. Every stored property is +/// a `let`, and each one is either immutable, independently synchronized +/// (`SessionGenerator`, `SessionsState`), or already `Sendable` +/// (`DispatchQueue`). The conformance is unchecked only because the +/// injected `SessionCoordinatorProtocol`, `SettingsProtocol` and +/// `ApplicationInfoProtocol` existentials are not declared `Sendable`. +/// TODO: Mark those protocols `Sendable` and make this a checked conformance. @objc(FIRSessions) final class Sessions: NSObject, Library, SessionsProvider, @unchecked Sendable { // MARK: - Private Variables diff --git a/FirebaseSessions/Sources/SessionGenerator.swift b/FirebaseSessions/Sources/SessionGenerator.swift index a237f879710..c1dd988e1a7 100644 --- a/FirebaseSessions/Sources/SessionGenerator.swift +++ b/FirebaseSessions/Sources/SessionGenerator.swift @@ -15,9 +15,10 @@ import Foundation +private import FirebaseCoreInternal internal import FirebaseInstallations -struct SessionInfo { +struct SessionInfo: Sendable { let sessionId: String let firstSessionId: String let shouldDispatchEvents: Bool @@ -37,40 +38,50 @@ struct SessionInfo { /// 2) Persisting and reading the Session ID from the last session /// (Maybe) 3) Persisting, reading, and incrementing an increasing index /// -class SessionGenerator { - private var thisSession: SessionInfo? +/// Generation happens on whichever thread initiates a session (typically the +/// main thread, via app lifecycle notifications), while `currentSession` is +/// read by subscribers on their own threads. The mutable state is therefore +/// guarded by a lock so this type can be safely `Sendable`. +/// +final class SessionGenerator: Sendable { + /// The generator's mutable state, only reachable while holding `state`. + private struct State { + var thisSession: SessionInfo? + var firstSessionId: String = "" + /// This will be incremented to 0 on the first generation. + var sessionIndex: Int32 = -1 + } - private var firstSessionId = "" - private var sessionIndex: Int32 - private var collectEvents: Bool + private let state = UnfairLock(State()) + private let collectEvents: Bool init(collectEvents: Bool) { - // This will be incremented to 0 on the first generation - sessionIndex = -1 - self.collectEvents = collectEvents } // Generates a new Session ID. If there was already a generated Session ID // from the last session during the app's lifecycle, it will also set the last Session ID func generateNewSession() -> SessionInfo { - let newSessionId = UUID().uuidString.replacingOccurrences(of: "-", with: "").lowercased() + let collectEvents = self.collectEvents + return state.withLock { state in + let newSessionId = UUID().uuidString.replacingOccurrences(of: "-", with: "").lowercased() - // If firstSessionId is set, use it. Otherwise set it to the - // first generated Session ID - firstSessionId = firstSessionId.isEmpty ? newSessionId : firstSessionId + // If firstSessionId is set, use it. Otherwise set it to the + // first generated Session ID + state.firstSessionId = state.firstSessionId.isEmpty ? newSessionId : state.firstSessionId - sessionIndex += 1 + state.sessionIndex += 1 - let newSession = SessionInfo(sessionId: newSessionId, - firstSessionId: firstSessionId, - dispatchEvents: collectEvents, - sessionIndex: sessionIndex) - thisSession = newSession - return newSession + let newSession = SessionInfo(sessionId: newSessionId, + firstSessionId: state.firstSessionId, + dispatchEvents: collectEvents, + sessionIndex: state.sessionIndex) + state.thisSession = newSession + return newSession + } } var currentSession: SessionInfo? { - return thisSession + state.withLock { $0.thisSession } } } From 5bedd9d08c7a63dfaa373c60893525518bd92288 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Sep 2026 17:50:54 -0400 Subject: [PATCH 11/11] perf(sessions): generate the session ID outside the lock `UnfairLock` wraps `os_unfair_lock`, which should only be held for the state read-modify-write. Hoist the UUID generation and its string formatting out of the critical section; the new ID is always consumed, so nothing is wasted by computing it up front. --- FirebaseSessions/Sources/SessionGenerator.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/FirebaseSessions/Sources/SessionGenerator.swift b/FirebaseSessions/Sources/SessionGenerator.swift index c1dd988e1a7..d7c34d89ea1 100644 --- a/FirebaseSessions/Sources/SessionGenerator.swift +++ b/FirebaseSessions/Sources/SessionGenerator.swift @@ -62,10 +62,14 @@ final class SessionGenerator: Sendable { // Generates a new Session ID. If there was already a generated Session ID // from the last session during the app's lifecycle, it will also set the last Session ID func generateNewSession() -> SessionInfo { + // Generated outside the critical section: `UnfairLock` wraps + // `os_unfair_lock`, which should be held only for the state + // read-modify-write, never across allocations. The new ID is always + // consumed, so there is no wasted work. + let newSessionId = UUID().uuidString.replacingOccurrences(of: "-", with: "").lowercased() + let collectEvents = self.collectEvents return state.withLock { state in - let newSessionId = UUID().uuidString.replacingOccurrences(of: "-", with: "").lowercased() - // If firstSessionId is set, use it. Otherwise set it to the // first generated Session ID state.firstSessionId = state.firstSessionId.isEmpty ? newSessionId : state.firstSessionId