fix: [SDK-4874] single-flight UpdateSubscription to stop in-flight PATCH races (#1689)

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: nan-li

iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift

@@ -36,9 +36,15 @@ public class MockOneSignalClient: NSObject, IOneSignalClient {
     public var lastHTTPRequest: OneSignalRequest?
     public var networkRequestCount = 0
     public var executedRequests: [OneSignalRequest] = []
+    /// Requests that have entered `execute` (including those still held / delayed).
+    public private(set) var startedRequests: [OneSignalRequest] = []
     public var executeInstantaneously = false
     /// Set to true to make it unnecessary to setup mock responses for every request possible
     public var fireSuccessForAllRequests = false
+    /// When true, `execute` records the request but does not complete until `releaseHeldResponses()`.
+    public var holdResponses = false
+
+    private var heldExecutions: [(request: OneSignalRequest, onSuccess: OSResultSuccessBlock, onFailure: OSClientFailureBlock)] = []
 
     var remoteParamsResponse: [String: Any]?
     var shouldUseProvisionalAuthorization = false // new in iOS 12 (aka Direct to History)
@@ -84,6 +90,9 @@ public class MockOneSignalClient: NSObject, IOneSignalClient {
         lastHTTPRequest = nil
         networkRequestCount = 0
         executedRequests.removeAll()
+        startedRequests.removeAll()
+        heldExecutions.removeAll()
+        holdResponses = false
         executeInstantaneously = true
         remoteParamsResponse = nil
         shouldUseProvisionalAuthorization = false
@@ -93,6 +102,19 @@ public class MockOneSignalClient: NSObject, IOneSignalClient {
     public func execute(_ request: OneSignalRequest, onSuccess successBlock: @escaping OSResultSuccessBlock, onFailure failureBlock: @escaping OSClientFailureBlock) {
         print("🧪 MockOneSignalClient execute called")
 
+        // Check hold + enqueue under one lock so releaseHeldResponses can't miss a callback mid-hold.
+        let shouldHold = lock.withLock { () -> Bool in
+            startedRequests.append(request)
+            guard holdResponses else {
+                return false
+            }
+            heldExecutions.append((request, successBlock, failureBlock))
+            return true
+        }
+        if shouldHold {
+            return
+        }
+
         if executeInstantaneously {
             finishExecutingRequest(request, onSuccess: successBlock, onFailure: failureBlock)
         } else {
@@ -102,6 +124,19 @@ public class MockOneSignalClient: NSObject, IOneSignalClient {
         }
     }
 
+    /// Completes every request currently held by `holdResponses`, and stops holding further executes.
+    public func releaseHeldResponses() {
+        let held: [(request: OneSignalRequest, onSuccess: OSResultSuccessBlock, onFailure: OSClientFailureBlock)] = lock.withLock {
+            holdResponses = false
+            let copy = heldExecutions
+            heldExecutions.removeAll()
+            return copy
+        }
+        for item in held {
+            finishExecutingRequest(item.request, onSuccess: item.onSuccess, onFailure: item.onFailure)
+        }
+    }
+
     /// Helper method to stringify the name of a request for identification and comparison
     private func stringify(_ request: OneSignalRequest) -> String {
         var stringified = request.description

iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift

@@ -397,6 +397,12 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor {
         guard !request.sentToClient else {
             return
         }
+        // Single-flight: don't send a second UpdateSubscription for the same model while one is in flight.
+        // A later coalesced request stays queued and is drained when the in-flight one finishes.
+        let modelId = request.subscriptionModel.modelId
+        guard !updateRequestQueue.contains(where: { $0 !== request && $0.sentToClient && $0.subscriptionModel.modelId == modelId }) else {
+            return
+        }
         guard request.prepareForExecution(newRecordsState: newRecordsState) else {
             return
         }
@@ -413,13 +419,9 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor {
             self.dispatchQueue.async {
                 self.updateRequestQueue.removeAll(where: { $0 == request})
                 OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue)
-                if inBackground {
-                    OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
-                }
-            }
 
-            if let onesignalId = OneSignalUserManagerImpl.sharedInstance.onesignalId {
-                if let rywToken = response?["ryw_token"] as? String
+                if let onesignalId = OneSignalUserManagerImpl.sharedInstance.onesignalId {
+                    if let rywToken = response?["ryw_token"] as? String
                     {
                         let rywDelay = response?["ryw_delay"] as? NSNumber
                         OSConsistencyManager.shared.setRywTokenAndDelay(
@@ -431,6 +433,12 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor {
                         // handle a potential regression where ryw_token is no longer returned by API
                         OSConsistencyManager.shared.resolveConditionsWithID(id: OSIamFetchReadyCondition.CONDITIONID)
                     }
+                }
+
+                self.executeNextPendingUpdateSubscription(for: modelId, inBackground: inBackground)
+                if inBackground {
+                    OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
+                }
             }
         } onFailure: { error in
             OneSignalLog.onesignalLog(.LL_ERROR, message: "OSSubscriptionOperationExecutor update subscription request failed with error: \(error.debugDescription)")
@@ -440,11 +448,27 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor {
                     // Fail, no retry, remove from cache and queue
                     self.updateRequestQueue.removeAll(where: { $0 == request})
                     OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue)
+                    self.executeNextPendingUpdateSubscription(for: modelId, inBackground: inBackground)
+                } else {
+                    // Make the request eligible for the next flush
+                    request.sentToClient = false
                 }
                 if inBackground {
                     OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
                 }
             }
         }
     }
+
+}
+
+extension OSSubscriptionOperationExecutor {
+    /// Sends the oldest unsent UpdateSubscription for `modelId`, if any. Caller must be on `dispatchQueue`.
+    private func executeNextPendingUpdateSubscription(for modelId: String, inBackground: Bool) {
+        let pending = updateRequestQueue.filter { !$0.sentToClient && $0.subscriptionModel.modelId == modelId }
+        guard let next = pending.min(by: { $0.timestamp < $1.timestamp }) else {
+            return
+        }
+        executeUpdateSubscriptionRequest(next, inBackground: inBackground)
+    }
 }

iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/SubscriptionUpdateRaceTests.swift

@@ -133,6 +133,118 @@ final class SubscriptionUpdateRaceTests: XCTestCase {
         XCTAssertEqual(updateRequests.count, 1, "Unsent updates for the same subscription should be coalesced")
     }
 
+    /**
+     An UpdateSubscription already on the wire must not race a follow-up PATCH.
+     The follow-up stays queued until the in-flight request completes, then sends live state.
+     */
+    func testInFlightUpdateBlocksFollowUpUntilCompleteThenSendsLiveState() throws {
+        let client = MockOneSignalClient()
+        client.holdResponses = true
+        client.fireSuccessForAllRequests = true
+        OneSignalCoreImpl.setSharedClient(client)
+
+        let executor = OSSubscriptionOperationExecutor(newRecordsState: OSNewRecordsState())
+        let model = makePushSubscriptionModel(notificationTypes: promptedNeverAnswered, subscriptionId: subscriptionId)
+        let identityModelId = UUID().uuidString
+
+        executor.enqueueDelta(OSDelta(
+            name: OS_UPDATE_SUBSCRIPTION_DELTA,
+            identityModelId: identityModelId,
+            model: model,
+            property: "notificationTypes",
+            value: promptedNeverAnswered
+        ))
+        executor.processDeltaQueue(inBackground: false)
+        OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.2)
+
+        XCTAssertEqual(client.startedRequests.count, 1, "First UpdateSubscription should be in flight")
+        let firstPayload = try XCTUnwrap(
+            (client.startedRequests[0] as? OSRequestUpdateSubscription)?.parameters?["subscription"] as? [String: Any]
+        )
+        XCTAssertEqual(firstPayload["notification_types"] as? Int, promptedNeverAnswered)
+        XCTAssertEqual(firstPayload["enabled"] as? Bool, false)
+
+        // Permission granted while first PATCH is still in flight.
+        model.notificationTypes = subscribedNotificationTypes
+        XCTAssertTrue(model.enabled)
+
+        executor.enqueueDelta(OSDelta(
+            name: OS_UPDATE_SUBSCRIPTION_DELTA,
+            identityModelId: identityModelId,
+            model: model,
+            property: "notificationTypes",
+            value: subscribedNotificationTypes
+        ))
+        executor.processDeltaQueue(inBackground: false)
+        OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.2)
+
+        XCTAssertEqual(client.startedRequests.count, 1, "Follow-up must wait for in-flight UpdateSubscription")
+
+        client.releaseHeldResponses()
+        OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5)
+
+        XCTAssertEqual(client.startedRequests.count, 2, "Pending follow-up should send after in-flight completes")
+        let secondPayload = try XCTUnwrap(
+            (client.startedRequests[1] as? OSRequestUpdateSubscription)?.parameters?["subscription"] as? [String: Any]
+        )
+        XCTAssertEqual(secondPayload["notification_types"] as? Int, subscribedNotificationTypes)
+        XCTAssertEqual(secondPayload["enabled"] as? Bool, true)
+    }
+
+    /**
+     A retryable failure (e.g. 500/timeout) must not leave the single-flight gate locked.
+     The failed request becomes resendable and later updates for the model still go out.
+     */
+    func testRetryableFailureDoesNotBlockSubsequentUpdates() throws {
+        let client = MockOneSignalClient()
+        client.fireSuccessForAllRequests = true
+        OneSignalCoreImpl.setSharedClient(client)
+
+        let executor = OSSubscriptionOperationExecutor(newRecordsState: OSNewRecordsState())
+        let model = makePushSubscriptionModel(notificationTypes: promptedNeverAnswered, subscriptionId: subscriptionId)
+        let identityModelId = UUID().uuidString
+
+        // Fail the first update with a retryable error (mock responses are keyed by request description).
+        let requestKey = "OSRequestUpdateSubscription with model: \(model.modelId)"
+        client.setMockFailureResponseForRequest(
+            request: requestKey,
+            error: OneSignalClientError(code: 500, message: "retryable", responseHeaders: nil, response: nil, underlyingError: nil)
+        )
+
+        executor.enqueueDelta(OSDelta(
+            name: OS_UPDATE_SUBSCRIPTION_DELTA,
+            identityModelId: identityModelId,
+            model: model,
+            property: "notificationTypes",
+            value: promptedNeverAnswered
+        ))
+        executor.processDeltaQueue(inBackground: false)
+        OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5)
+
+        XCTAssertEqual(client.executedRequests.count, 1, "First update should have been attempted and failed retryably")
+
+        // Server recovers; user accepts permission.
+        client.setMockResponseForRequest(request: requestKey, response: [:])
+        model.notificationTypes = subscribedNotificationTypes
+        XCTAssertTrue(model.enabled)
+
+        executor.enqueueDelta(OSDelta(
+            name: OS_UPDATE_SUBSCRIPTION_DELTA,
+            identityModelId: identityModelId,
+            model: model,
+            property: "notificationTypes",
+            value: subscribedNotificationTypes
+        ))
+        executor.processDeltaQueue(inBackground: false)
+        OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5)
+
+        let updateRequests = client.executedRequests.compactMap { $0 as? OSRequestUpdateSubscription }
+        XCTAssertEqual(updateRequests.count, 2, "Follow-up update must still send after a retryable failure")
+        let lastPayload = try XCTUnwrap(updateRequests.last?.parameters?["subscription"] as? [String: Any])
+        XCTAssertEqual(lastPayload["notification_types"] as? Int, subscribedNotificationTypes)
+        XCTAssertEqual(lastPayload["enabled"] as? Bool, true)
+    }
+
     // MARK: - Helpers
 
     private func makePushSubscriptionModel(notificationTypes: Int, subscriptionId: String?) -> OSSubscriptionModel {