From e3a56f582211ba5a05f0389f3528722fa991fd2f Mon Sep 17 00:00:00 2001 From: Thomas Myrden Date: Thu, 5 Mar 2026 12:42:06 -0400 Subject: [PATCH 1/2] @W-21415593: Fixing retry race condition --- .../project.pbxproj | 4 + .../Classes/RestAPI/SFRestAPI.m | 12 +- .../SFRestAPIDataTaskRaceTests.m | 314 ++++++++++++++++++ 3 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFRestAPIDataTaskRaceTests.m diff --git a/libs/SalesforceSDKCore/SalesforceSDKCore.xcodeproj/project.pbxproj b/libs/SalesforceSDKCore/SalesforceSDKCore.xcodeproj/project.pbxproj index 828d7c7f6f..bba012f897 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCore.xcodeproj/project.pbxproj +++ b/libs/SalesforceSDKCore/SalesforceSDKCore.xcodeproj/project.pbxproj @@ -183,6 +183,7 @@ A3C7475C29F7047400D72B7F /* ScreenLockManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = A3C7475B29F7047400D72B7F /* ScreenLockManager.swift */; }; A3C7475F29F7087B00D72B7F /* BiometricAuthenticationManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = A3C7475E29F7087B00D72B7F /* BiometricAuthenticationManager.swift */; }; A3C7476129F709EB00D72B7F /* BiometricAuthenticationManagerInternal.swift in Sources */ = {isa = PBXBuildFile; fileRef = A3C7476029F709EB00D72B7F /* BiometricAuthenticationManagerInternal.swift */; }; + AA9154482F59E3C900A0A41C /* SFRestAPIDataTaskRaceTests.m in Sources */ = {isa = PBXBuildFile; fileRef = AA9154472F59E3C900A0A41C /* SFRestAPIDataTaskRaceTests.m */; }; B70135621F87F63900995171 /* SFSDKAuthErrorManager.h in Headers */ = {isa = PBXBuildFile; fileRef = B70135601F87F63900995171 /* SFSDKAuthErrorManager.h */; settings = {ATTRIBUTES = (Private, ); }; }; B70135641F87F63900995171 /* SFSDKAuthErrorManager.m in Sources */ = {isa = PBXBuildFile; fileRef = B70135611F87F63900995171 /* SFSDKAuthErrorManager.m */; }; B711290D1F8A780800436CFB /* SFSDKAlertView.m in Sources */ = {isa = PBXBuildFile; fileRef = B71129071F8A780700436CFB /* SFSDKAlertView.m */; }; @@ -822,6 +823,7 @@ A3C7475B29F7047400D72B7F /* ScreenLockManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ScreenLockManager.swift; sourceTree = ""; }; A3C7475E29F7087B00D72B7F /* BiometricAuthenticationManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BiometricAuthenticationManager.swift; sourceTree = ""; }; A3C7476029F709EB00D72B7F /* BiometricAuthenticationManagerInternal.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BiometricAuthenticationManagerInternal.swift; sourceTree = ""; }; + AA9154472F59E3C900A0A41C /* SFRestAPIDataTaskRaceTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = SFRestAPIDataTaskRaceTests.m; path = SalesforceSDKCoreTests/SFRestAPIDataTaskRaceTests.m; sourceTree = SOURCE_ROOT; }; B70135601F87F63900995171 /* SFSDKAuthErrorManager.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SFSDKAuthErrorManager.h; sourceTree = ""; }; B70135611F87F63900995171 /* SFSDKAuthErrorManager.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SFSDKAuthErrorManager.m; sourceTree = ""; }; B71129071F8A780700436CFB /* SFSDKAlertView.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SFSDKAlertView.m; sourceTree = ""; }; @@ -1056,6 +1058,7 @@ 4F7EB3F61BFFC84700768720 /* SalesforceSDKCoreTests */ = { isa = PBXGroup; children = ( + AA9154472F59E3C900A0A41C /* SFRestAPIDataTaskRaceTests.m */, 23F200AB2E551C890091C5F5 /* ActionTypeTests.swift */, 4FAUTHFLOW112345678901ABC /* AuthFlowTypesViewTests.swift */, 696E91452AD0A14A00205884 /* BiometricAuthenticationManagerTests.swift */, @@ -2241,6 +2244,7 @@ 4F06AF8A1C49A18E00F70798 /* SalesforceOAuthUnitTests.m in Sources */, 23A4C7462D0CAFA000DF55EB /* ScreenLockManagerTests.swift in Sources */, 23EED88A2E2ACD3300646B10 /* SFOAuthCoordinatorTests.swift in Sources */, + AA9154482F59E3C900A0A41C /* SFRestAPIDataTaskRaceTests.m in Sources */, B7A901BE228E4DFB0036D749 /* SFSDKLogoutBlocker.m in Sources */, 693E623B24A29B6B0017B222 /* SFSDKKeyValueEncryptedFileStoreTests.m in Sources */, 8214D955205316BA0007349E /* SFSDKSalesforceAnalyticsManagerTests.m in Sources */, diff --git a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m index 030eec53ef..2a3044cb52 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m +++ b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m @@ -327,7 +327,12 @@ - (void)enqueueRequest:(SFRestRequest *)request shouldRetry:(BOOL)shouldRetry { __block NSURLSessionDataTask *dataTask = [network sendRequest:finalRequest dataResponseBlock:^(NSData *data, NSURLResponse *response, NSError *error) { __strong typeof(weakSelf) strongSelf = weakSelf; [SFNetwork removeSharedInstanceForIdentifier:instanceIdentifier]; - + + // Guard: ignore callbacks from stale dataTasks superseded by retry. + if (dataTask != request.sessionDataTask) { + return; + } + // Network error. if (error) { [SFSDKCoreLogger d:[strongSelf class] format:@"REST request failed with error: Error Code: %ld, Description: %@, URL: %@", (long) error.code, error.localizedDescription, finalRequest.URL]; @@ -488,6 +493,9 @@ - (void)flushPendingRequestQueue:(NSError *)error rawResponse:(NSURLResponse *)r @synchronized (self) { NSSet *pendingRequests = [self.activeRequests asSet]; for (SFRestRequest *request in pendingRequests) { + NSURLSessionDataTask *oldTask = request.sessionDataTask; + request.sessionDataTask = nil; + [oldTask cancel]; if (request.failureBlock) { request.failureBlock(nil, error, rawResponse); } @@ -500,10 +508,12 @@ - (void)resendActiveRequestsRequiringAuthentication { @synchronized (self) { NSSet *pendingRequests = [self.activeRequests asSet]; for (SFRestRequest *request in pendingRequests) { + NSURLSessionDataTask *oldTask = request.sessionDataTask; [self send:request failureBlock:request.failureBlock successBlock:request.successBlock shouldRetry:NO]; + [oldTask cancel]; } self.pendingRequestsBeingProcessed = NO; } diff --git a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFRestAPIDataTaskRaceTests.m b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFRestAPIDataTaskRaceTests.m new file mode 100644 index 0000000000..893b3edf71 --- /dev/null +++ b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFRestAPIDataTaskRaceTests.m @@ -0,0 +1,314 @@ +/* + Copyright (c) 2026-present, salesforce.com, inc. All rights reserved. + + Redistribution and use of this software in source and binary forms, with or without modification, + are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright notice, this list of conditions + and the following disclaimer. + * Redistributions in binary form must reproduce the above copyright notice, this list of + conditions and the following disclaimer in the documentation and/or other materials provided + with the distribution. + * Neither the name of salesforce.com, inc. nor the names of its contributors may be used to + endorse or promote products derived from this software without specific prior written + permission of salesforce.com, inc. + + THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR + IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, + WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY + WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#import +#import "SFRestAPI+Internal.h" +#import "SFRestRequest+Internal.h" +#import "SFRestAPI+Blocks.h" +#import "SFNetwork.h" + +#pragma mark - Expose private methods for testing + +@interface SFRestAPI (DataTaskRaceTesting) + +- (id)initWithUser:(SFUserAccount *)user; + +- (void)send:(SFRestRequest *)request +failureBlock:(SFRestRequestFailBlock)failureBlock +successBlock:(SFRestResponseBlock)successBlock + shouldRetry:(BOOL)shouldRetry; + +- (void)resendActiveRequestsRequiringAuthentication; +- (void)flushPendingRequestQueue:(NSError *)error rawResponse:(NSURLResponse *)rawResponse; + +@end + +#pragma mark - DeferredURLProtocol + +/** + * NSURLProtocol subclass that intercepts all requests and holds them until + * the test explicitly delivers a response. This lets us control exactly + * when each NSURLSessionDataTask's completion handler fires. + */ +@interface DeferredURLProtocol : NSURLProtocol ++ (void)reset; ++ (NSUInteger)pendingCount; ++ (void)deliverResponseAtIndex:(NSUInteger)index statusCode:(NSInteger)statusCode; +@end + +static NSMutableArray *sPendingProtocols; + +@implementation DeferredURLProtocol + ++ (void)initialize { + if (self == [DeferredURLProtocol class]) { + sPendingProtocols = [NSMutableArray new]; + } +} + ++ (void)reset { + @synchronized (sPendingProtocols) { + [sPendingProtocols removeAllObjects]; + } +} + ++ (NSUInteger)pendingCount { + @synchronized (sPendingProtocols) { + return sPendingProtocols.count; + } +} + ++ (BOOL)canInitWithRequest:(NSURLRequest *)request { + return YES; +} + ++ (NSURLRequest *)canonicalRequestForRequest:(NSURLRequest *)request { + return request; +} + +- (void)startLoading { + @synchronized (sPendingProtocols) { + [sPendingProtocols addObject:self]; + } +} + +- (void)stopLoading { + // Intentionally empty; responses are delivered manually. +} + ++ (void)deliverResponseAtIndex:(NSUInteger)index statusCode:(NSInteger)statusCode { + DeferredURLProtocol *proto; + @synchronized (sPendingProtocols) { + proto = sPendingProtocols[index]; + } + NSHTTPURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:proto.request.URL + statusCode:statusCode + HTTPVersion:@"HTTP/1.1" + headerFields:nil]; + NSData *body = [@"{\"ok\":true}" dataUsingEncoding:NSUTF8StringEncoding]; + [proto.client URLProtocol:proto didReceiveResponse:response cacheStoragePolicy:NSURLCacheStorageNotAllowed]; + [proto.client URLProtocol:proto didLoadData:body]; + [proto.client URLProtocolDidFinishLoading:proto]; +} + +@end + +#pragma mark - Tests + +@interface SFRestAPIDataTaskRaceTests : XCTestCase +@property (nonatomic, strong) SFRestAPI *api; +@end + +@implementation SFRestAPIDataTaskRaceTests + +- (void)setUp { + [super setUp]; + [DeferredURLProtocol reset]; + + NSURLSessionConfiguration *config = [NSURLSessionConfiguration ephemeralSessionConfiguration]; + config.protocolClasses = @[[DeferredURLProtocol class]]; + [SFNetwork setSessionConfiguration:config identifier:kSFNetworkEphemeralInstanceIdentifier]; + + self.api = [[SFRestAPI alloc] initWithUser:nil]; +} + +- (void)tearDown { + [self.api cancelAllRequests]; + [self.api cleanup]; + [DeferredURLProtocol reset]; + [SFNetwork removeSharedEphemeralInstance]; + [super tearDown]; +} + +/** + * Creates a request that bypasses auth checks by using an absolute URL. + * Unique per-call so DeferredURLProtocol can distinguish them if needed. + */ +- (SFRestRequest *)makeRequest { + static int counter = 0; + NSString *url = [NSString stringWithFormat:@"https://test.example.com/api/%d", ++counter]; + SFRestRequest *request = [SFRestRequest requestWithMethod:SFRestMethodGET path:url queryParams:nil]; + request.requiresAuthentication = NO; + return request; +} + +/** + * Helper: spin the run loop until `condition` returns YES, up to `timeout` seconds. + */ +- (BOOL)waitForCondition:(BOOL (^)(void))condition timeout:(NSTimeInterval)timeout { + NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:timeout]; + while (!condition() && [deadline timeIntervalSinceNow] > 0) { + [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate dateWithTimeIntervalSinceNow:0.01]]; + } + return condition(); +} + +#pragma mark - Test: resendActiveRequestsRequiringAuthentication race + +/** + * Reproduces the crash scenario: + * 1. Request is sent, creating dataTask #1 (in-flight). + * 2. Token refresh completes; resendActiveRequestsRequiringAuthentication + * re-sends the same request, creating dataTask #2. + * 3. dataTask #1 completes (200 OK) -> successBlock should NOT be called + * (stale task, guard drops it). + * 4. dataTask #2 completes (200 OK) -> successBlock IS called (current task). + * + * Without the stale-task guard, successBlock fires twice (crash). + * With the guard, successBlock fires exactly once. + */ +- (void)testResendActiveRequestsDoesNotDoubleInvokeBlocks { + __block int successCount = 0; + __block int failureCount = 0; + XCTestExpectation *expectation = [self expectationWithDescription:@"block called"]; + + SFRestRequest *request = [self makeRequest]; + + // Step 1: Send the request. This creates dataTask #1. + [self.api send:request + failureBlock:^(id response, NSError *e, NSURLResponse *rawResponse) { + failureCount++; + } successBlock:^(id response, NSURLResponse *rawResponse) { + successCount++; + [expectation fulfill]; + } shouldRetry:NO]; + + // Wait for dataTask #1 to be registered with DeferredURLProtocol. + XCTAssertTrue([self waitForCondition:^{ return @([DeferredURLProtocol pendingCount] >= 1).boolValue; } timeout:2], + @"dataTask #1 should be pending"); + + // Step 2: Simulate what happens after token refresh succeeds: + // resendActiveRequestsRequiringAuthentication re-sends all active requests. + // This creates dataTask #2 for the same request. + [self.api resendActiveRequestsRequiringAuthentication]; + + // Wait for dataTask #2 to be registered. + XCTAssertTrue([self waitForCondition:^{ return @([DeferredURLProtocol pendingCount] >= 2).boolValue; } timeout:2], + @"dataTask #2 should be pending"); + + // Step 3: dataTask #1 (index 0) completes with 200 OK. + // With the fix: guard detects dataTask #1 != request.sessionDataTask, drops it. + // Without fix: successBlock fires (first resume). + [DeferredURLProtocol deliverResponseAtIndex:0 statusCode:200]; + + // Step 4: dataTask #2 (index 1) completes with 200 OK. + // successBlock fires (this is the current task). + // Without fix: successBlock fires again (second resume -> crash). + [DeferredURLProtocol deliverResponseAtIndex:1 statusCode:200]; + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + XCTAssertEqual(successCount, 1, @"successBlock must be called exactly once, was called %d times", successCount); + XCTAssertEqual(failureCount, 0, @"failureBlock must not be called"); +} + +#pragma mark - Test: flushPendingRequestQueue race + +/** + * Reproduces the flush variant: + * 1. Request is sent, creating dataTask #1 (in-flight). + * 2. Token refresh FAILS; flushPendingRequestQueue calls failureBlock + * for all active requests AND cancels their dataTasks. + * 3. dataTask #1's cancel callback fires -> guard drops it (stale task). + * + * Without the fix: failureBlock fires twice (once from flush, once from cancel callback). + * With the fix: failureBlock fires exactly once. + */ +- (void)testFlushPendingRequestQueueDoesNotDoubleInvokeBlocks { + __block int successCount = 0; + __block int failureCount = 0; + XCTestExpectation *expectation = [self expectationWithDescription:@"failure block called"]; + + SFRestRequest *request = [self makeRequest]; + + // Step 1: Send the request. This creates dataTask #1. + [self.api send:request + failureBlock:^(id response, NSError *e, NSURLResponse *rawResponse) { + failureCount++; + [expectation fulfill]; + } successBlock:^(id response, NSURLResponse *rawResponse) { + successCount++; + } shouldRetry:NO]; + + // Wait for dataTask #1 to be registered. + XCTAssertTrue([self waitForCondition:^{ return @([DeferredURLProtocol pendingCount] >= 1).boolValue; } timeout:2], + @"dataTask #1 should be pending"); + + // Step 2: Simulate token refresh failure. flushPendingRequestQueue calls + // failureBlock for all active requests. With the fix, it also cancels + // and invalidates the old sessionDataTask first. + NSError *refreshError = [NSError errorWithDomain:@"TestDomain" code:401 userInfo:nil]; + NSHTTPURLResponse *rawResponse = [[NSHTTPURLResponse alloc] initWithURL:[NSURL URLWithString:@"https://test.example.com"] + statusCode:401 + HTTPVersion:@"HTTP/1.1" + headerFields:nil]; + [self.api flushPendingRequestQueue:refreshError rawResponse:rawResponse]; + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + // Give any stale cancel callbacks time to fire. + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.5]]; + + XCTAssertEqual(failureCount, 1, @"failureBlock must be called exactly once, was called %d times", failureCount); + XCTAssertEqual(successCount, 0, @"successBlock must not be called"); +} + +#pragma mark - Test: cancelAllRequests still works + +/** + * Ensures that legitimate cancellation via cancelAllRequests still delivers + * the NSURLErrorCancelled error to the failureBlock (the stale-task guard + * must NOT interfere because cancelAllRequests doesn't re-send). + */ +- (void)testCancelAllRequestsStillDeliversCancellationError { + __block int failureCount = 0; + __block NSError *receivedError = nil; + XCTestExpectation *expectation = [self expectationWithDescription:@"cancel delivered"]; + + SFRestRequest *request = [self makeRequest]; + + [self.api send:request + failureBlock:^(id response, NSError *e, NSURLResponse *rawResponse) { + failureCount++; + receivedError = e; + [expectation fulfill]; + } successBlock:^(id response, NSURLResponse *rawResponse) { + XCTFail(@"successBlock should not be called on cancellation"); + } shouldRetry:NO]; + + // Wait for the dataTask to be in-flight. + XCTAssertTrue([self waitForCondition:^{ return @([DeferredURLProtocol pendingCount] >= 1).boolValue; } timeout:2], + @"dataTask should be pending"); + + // Cancel all requests. This cancels the task but does NOT re-send, + // so sessionDataTask is unchanged. The guard should pass. + [self.api cancelAllRequests]; + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + XCTAssertEqual(failureCount, 1, @"failureBlock must be called exactly once"); + XCTAssertEqual(receivedError.code, NSURLErrorCancelled, @"Error should be NSURLErrorCancelled"); +} + +@end From a295bc7ad7c3011b994c4c79cdf06b1626c95508 Mon Sep 17 00:00:00 2001 From: Amisha Goyal Date: Thu, 5 Mar 2026 16:34:15 -0400 Subject: [PATCH 2/2] Add logs to dientify stale task callbacks --- .../SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m index 2a3044cb52..54a025e4cc 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m +++ b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/RestAPI/SFRestAPI.m @@ -330,6 +330,7 @@ - (void)enqueueRequest:(SFRestRequest *)request shouldRetry:(BOOL)shouldRetry { // Guard: ignore callbacks from stale dataTasks superseded by retry. if (dataTask != request.sessionDataTask) { + [SFSDKCoreLogger d:[strongSelf class] format:@"Ignoring callback from stale task for request: %@", request.path]; return; }