From 74b8db0b120baa1869bba440d7352a7369b103ca Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Tue, 8 Sep 2026 11:26:32 +0100 Subject: [PATCH 1/3] fix(firestore): fail iOS transaction timeout without SDK retry --- packages/firestore/RNFBFirestore.podspec | 1 + .../RNFBFirestoreTransactionAttempt.h | 63 ++++++ .../RNFBFirestoreTransactionAttempt.m | 139 ++++++++++++ .../RNFBFirestoreTransactionModule.mm | 203 ++++++++---------- .../RNFBFirestoreTransactionRegistry.h | 2 +- .../RNFBFirestoreTransactionRegistry.m | 12 +- .../RNFBFirestoreTransactionAttemptTests.m | 149 +++++++++++++ .../RNFBFirestoreTransactionRegistryTests.m | 51 +++-- .../project.pbxproj | 10 + 9 files changed, 488 insertions(+), 142 deletions(-) create mode 100644 packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h create mode 100644 packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m create mode 100644 packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m diff --git a/packages/firestore/RNFBFirestore.podspec b/packages/firestore/RNFBFirestore.podspec index a1b3a643ed..4ae00f612f 100644 --- a/packages/firestore/RNFBFirestore.podspec +++ b/packages/firestore/RNFBFirestore.podspec @@ -37,6 +37,7 @@ Pod::Spec.new do |s| 'ios/RNFBFirestore/RNFBFirestoreTransactionModule.h', 'ios/RNFBFirestore/RNFBFirestoreListenerRegistry.h', 'ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.h', + 'ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h', 'ios/generated/**/*.h', ] s.exclude_files = 'ios/generated/RCTThirdPartyComponentsProvider.*', 'ios/generated/RCTAppDependencyProvider.*', 'ios/generated/RCTModuleProviders.*', 'ios/generated/RCTModulesConformingToProtocolsProvider.*', 'ios/generated/RCTUnstableModulesRequiringMainQueueSetupProvider.*', 'ios/*UnitTests/**' diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h new file mode 100644 index 0000000000..651e606176 --- /dev/null +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h @@ -0,0 +1,63 @@ +/** + * Copyright (c) 2016-present Invertase Limited & Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this library 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 + +NS_ASSUME_NONNULL_BEGIN + +/** + * Timeout NSError domain for the JS-bridge wait. Must not be FIRFirestoreErrorDomain so the iOS + * SDK treats the failure as permanent (no transaction retry). Code matches + * FIRFirestoreErrorCodeDeadlineExceeded (4) so JS maps via getCodeAndMessage:. + */ +FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionTimeoutErrorDomain; +FOUNDATION_EXPORT const NSInteger RNFBFirestoreTransactionTimeoutErrorCode; +FOUNDATION_EXPORT const int64_t RNFBFirestoreTransactionWaitTimeoutNSec; + +typedef NS_ENUM(NSInteger, RNFBFirestoreTransactionWaitResult) { + RNFBFirestoreTransactionWaitResultSignaled = 0, + RNFBFirestoreTransactionWaitResultTimeout = 1, + RNFBFirestoreTransactionWaitResultAborted = 2, + RNFBFirestoreTransactionWaitResultStale = 3, +}; + +/** + * One native transaction id's attempt state. Foundation-only so in-package XCTest can compile it + * without Firebase or TurboModules. FIRTransaction is stored as an opaque id; the module casts. + */ +@interface RNFBFirestoreTransactionAttempt : NSObject + +@property(nonatomic, readonly) BOOL aborted; +@property(nonatomic, readonly) BOOL updateBlockReturned; +@property(nonatomic, readonly, nullable) NSArray *commandBuffer; +@property(nonatomic, readonly, nullable) id nativeTransaction; + ++ (dispatch_time_t)defaultWaitTimeout; +- (NSError *)timeoutError; + +- (void)prepareForUpdateBlockWithNativeTransaction:(nullable id)nativeTransaction; +- (RNFBFirestoreTransactionWaitResult)waitUntilSignaledWithTimeout:(dispatch_time_t)timeout; +- (RNFBFirestoreTransactionWaitResult)completeWaitForSemaphore:(dispatch_semaphore_t)semaphore + timedOut:(BOOL)timedOut; + +- (BOOL)isEligibleForGet; +- (BOOL)applyCommandBuffer:(nullable NSArray *)commandBuffer; +- (void)abort; + +@end + +NS_ASSUME_NONNULL_END diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m new file mode 100644 index 0000000000..79553c8ea6 --- /dev/null +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m @@ -0,0 +1,139 @@ +/** + * Copyright (c) 2016-present Invertase Limited & Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this library 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 "RNFBFirestoreTransactionAttempt.h" + +NSString *const RNFBFirestoreTransactionTimeoutErrorDomain = + @"io.invertase.firebase.firestore.transaction"; +const NSInteger RNFBFirestoreTransactionTimeoutErrorCode = 4; +const int64_t RNFBFirestoreTransactionWaitTimeoutNSec = 15 * NSEC_PER_SEC; + +@implementation RNFBFirestoreTransactionAttempt { + dispatch_semaphore_t _semaphore; + BOOL _aborted; + BOOL _updateBlockReturned; + NSArray *_commandBuffer; + id _nativeTransaction; +} + ++ (dispatch_time_t)defaultWaitTimeout { + return dispatch_time(DISPATCH_TIME_NOW, RNFBFirestoreTransactionWaitTimeoutNSec); +} + +- (BOOL)aborted { + @synchronized(self) { + return _aborted; + } +} + +- (BOOL)updateBlockReturned { + @synchronized(self) { + return _updateBlockReturned; + } +} + +- (NSArray *)commandBuffer { + @synchronized(self) { + return _commandBuffer; + } +} + +- (id)nativeTransaction { + @synchronized(self) { + return _nativeTransaction; + } +} + +- (NSError *)timeoutError { + return [NSError errorWithDomain:RNFBFirestoreTransactionTimeoutErrorDomain + code:RNFBFirestoreTransactionTimeoutErrorCode + userInfo:@{}]; +} + +- (void)prepareForUpdateBlockWithNativeTransaction:(id)nativeTransaction { + @synchronized(self) { + _semaphore = dispatch_semaphore_create(0); + _updateBlockReturned = NO; + _commandBuffer = nil; + _nativeTransaction = nativeTransaction; + } +} + +- (RNFBFirestoreTransactionWaitResult)waitUntilSignaledWithTimeout:(dispatch_time_t)timeout { + dispatch_semaphore_t semaphore; + @synchronized(self) { + semaphore = _semaphore; + } + + if (semaphore == NULL) { + return [self completeWaitForSemaphore:dispatch_semaphore_create(0) timedOut:YES]; + } + + BOOL timedOut = dispatch_semaphore_wait(semaphore, timeout) != 0; + return [self completeWaitForSemaphore:semaphore timedOut:timedOut]; +} + +- (RNFBFirestoreTransactionWaitResult)completeWaitForSemaphore:(dispatch_semaphore_t)semaphore + timedOut:(BOOL)timedOut { + @synchronized(self) { + if (_semaphore != semaphore) { + return RNFBFirestoreTransactionWaitResultStale; + } + + _updateBlockReturned = YES; + _nativeTransaction = nil; + + if (_aborted) { + return RNFBFirestoreTransactionWaitResultAborted; + } + + if (timedOut) { + return RNFBFirestoreTransactionWaitResultTimeout; + } + + return RNFBFirestoreTransactionWaitResultSignaled; + } +} + +- (BOOL)isEligibleForGet { + @synchronized(self) { + return _semaphore != NULL && !_updateBlockReturned && !_aborted && _nativeTransaction != nil; + } +} + +- (BOOL)applyCommandBuffer:(NSArray *)commandBuffer { + @synchronized(self) { + if (_semaphore == NULL || _updateBlockReturned || _aborted) { + return NO; + } + + _commandBuffer = [commandBuffer copy]; + dispatch_semaphore_signal(_semaphore); + return YES; + } +} + +- (void)abort { + @synchronized(self) { + _aborted = YES; + if (_semaphore) { + dispatch_semaphore_signal(_semaphore); + } + } +} + +@end diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm index ddb4e3e84a..74cb9cd3c8 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm @@ -20,6 +20,7 @@ #import #import "RNFBApp/RCTConvert+FIRApp.h" +#import "RNFBFirestoreTransactionAttempt.h" #import "RNFBFirestoreTransactionModule.h" #import "RNFBFirestoreTransactionRegistry.h" #import "RNFBFirestoreTurboModules.h" @@ -29,6 +30,7 @@ @interface RNFBFirestoreTransactionModule () +- (void)rejectMissingTransaction:(RCTPromiseRejectBlock)reject; @end @implementation RNFBFirestoreTransactionModule @@ -75,18 +77,12 @@ - (void)transactionBegin:(NSString *)appName FIRFirestore *firestore = [RNFBFirestoreCommon getFirestoreForApp:firebaseApp databaseId:databaseId]; - __block BOOL aborted = false; - __block NSMutableDictionary *transactionState = [NSMutableDictionary new]; + RNFBFirestoreTransactionAttempt *attempt = [[RNFBFirestoreTransactionAttempt alloc] init]; id transactionBlock = ^id(FIRTransaction *transaction, NSError **errorPointer) { - dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); + [attempt prepareForUpdateBlockWithNativeTransaction:transaction]; - @synchronized(transactionState) { - transactionState[@"semaphore"] = semaphore; - transactionState[@"transaction"] = transaction; - } - - if (![transactions putOrSkip:transactionIdNumber value:transactionState]) { + if (![transactions putOrSkip:transactionIdNumber value:attempt]) { *errorPointer = [NSError errorWithDomain:FIRFirestoreErrorDomain code:FIRFirestoreErrorCodeAborted userInfo:@{}]; @@ -106,91 +102,85 @@ - (void)transactionBegin:(NSString *)appName }]; }); - dispatch_time_t delayTime = dispatch_time(DISPATCH_TIME_NOW, 15 * NSEC_PER_SEC); - BOOL timedOut = dispatch_semaphore_wait(semaphore, delayTime) != 0; - - @synchronized(transactionState) { - aborted = (BOOL)transactionState[@"aborted"]; + RNFBFirestoreTransactionWaitResult waitResult = + [attempt waitUntilSignaledWithTimeout:[RNFBFirestoreTransactionAttempt defaultWaitTimeout]]; - if (transactionState[@"semaphore"] != semaphore) { - return nil; - } + if (waitResult == RNFBFirestoreTransactionWaitResultStale) { + return nil; + } - if (aborted == YES) { - *errorPointer = [NSError errorWithDomain:FIRFirestoreErrorDomain - code:FIRFirestoreErrorCodeAborted - userInfo:@{}]; - return nil; - } + if (waitResult == RNFBFirestoreTransactionWaitResultAborted) { + *errorPointer = [NSError errorWithDomain:FIRFirestoreErrorDomain + code:FIRFirestoreErrorCodeAborted + userInfo:@{}]; + return nil; + } - if (timedOut == YES) { - *errorPointer = [NSError errorWithDomain:FIRFirestoreErrorDomain - code:FIRFirestoreErrorCodeDeadlineExceeded - userInfo:@{}]; - return nil; - } + if (waitResult == RNFBFirestoreTransactionWaitResultTimeout) { + *errorPointer = [attempt timeoutError]; + return nil; + } - NSArray *commandBuffer = transactionState[@"commandBuffer"]; - - for (NSDictionary *command in commandBuffer) { - NSString *type = command[@"type"]; - NSString *path = command[@"path"]; - FIRDocumentReference *documentReference = - [RNFBFirestoreCommon getDocumentForFirestore:firestore path:path]; - - if ([type isEqualToString:@"DELETE"]) { - [transaction deleteDocument:documentReference]; - } else if ([type isEqualToString:@"SET"]) { - NSDictionary *options = command[@"options"]; - NSDictionary *parsedData = [RNFBFirestoreSerialize parseNSDictionary:firestore - dictionary:command[@"data"]]; - - if (options[@"merge"]) { - [transaction setData:parsedData forDocument:documentReference merge:true]; - } else if (options[@"mergeFields"]) { - NSArray *mergeFields = options[@"mergeFields"]; - [transaction setData:parsedData forDocument:documentReference mergeFields:mergeFields]; - } else { - [transaction setData:parsedData forDocument:documentReference]; - } - } else if ([type isEqualToString:@"UPDATE"]) { - NSDictionary *parsedData = [RNFBFirestoreSerialize parseNSDictionary:firestore - dictionary:command[@"data"]]; - [transaction updateData:parsedData forDocument:documentReference]; + NSArray *commandBuffer = attempt.commandBuffer; + + for (NSDictionary *command in commandBuffer) { + NSString *type = command[@"type"]; + NSString *path = command[@"path"]; + FIRDocumentReference *documentReference = + [RNFBFirestoreCommon getDocumentForFirestore:firestore path:path]; + + if ([type isEqualToString:@"DELETE"]) { + [transaction deleteDocument:documentReference]; + } else if ([type isEqualToString:@"SET"]) { + NSDictionary *options = command[@"options"]; + NSDictionary *parsedData = [RNFBFirestoreSerialize parseNSDictionary:firestore + dictionary:command[@"data"]]; + + if (options[@"merge"]) { + [transaction setData:parsedData forDocument:documentReference merge:true]; + } else if (options[@"mergeFields"]) { + NSArray *mergeFields = options[@"mergeFields"]; + [transaction setData:parsedData forDocument:documentReference mergeFields:mergeFields]; + } else { + [transaction setData:parsedData forDocument:documentReference]; } + } else if ([type isEqualToString:@"UPDATE"]) { + NSDictionary *parsedData = [RNFBFirestoreSerialize parseNSDictionary:firestore + dictionary:command[@"data"]]; + [transaction updateData:parsedData forDocument:documentReference]; } - - return nil; } + + return nil; }; id completionBlock = ^(id result, NSError *error) { [transactions take:transactionIdNumber]; - @synchronized(transactionState) { - if (aborted == NO) { - NSMutableDictionary *eventMap = [NSMutableDictionary new]; - - if (error != nil) { - NSArray *codeAndMessage = [RNFBFirestoreCommon getCodeAndMessage:error]; - eventMap[@"type"] = @"error"; - eventMap[@"error"] = @{ - @"code" : codeAndMessage[0], - @"message" : codeAndMessage[1], - }; - } else { - eventMap[@"type"] = @"complete"; - } + if (attempt.aborted) { + return; + } - [[RNFBRCTEventEmitter shared] - sendEventWithName:RNFB_FIRESTORE_TRANSACTION_EVENT - body:@{ - @"listenerId" : transactionIdNumber, - @"appName" : [RNFBSharedUtils getAppJavaScriptName:firebaseApp.name], - @"databaseId" : databaseId, - @"body" : eventMap, - }]; - } + NSMutableDictionary *eventMap = [NSMutableDictionary new]; + + if (error != nil) { + NSArray *codeAndMessage = [RNFBFirestoreCommon getCodeAndMessage:error]; + eventMap[@"type"] = @"error"; + eventMap[@"error"] = @{ + @"code" : codeAndMessage[0], + @"message" : codeAndMessage[1], + }; + } else { + eventMap[@"type"] = @"complete"; } + + [[RNFBRCTEventEmitter shared] + sendEventWithName:RNFB_FIRESTORE_TRANSACTION_EVENT + body:@{ + @"listenerId" : transactionIdNumber, + @"appName" : [RNFBSharedUtils getAppJavaScriptName:firebaseApp.name], + @"databaseId" : databaseId, + @"body" : eventMap, + }]; }; if (maxAttempts > 0) { @@ -202,6 +192,15 @@ - (void)transactionBegin:(NSString *)appName } } +- (void)rejectMissingTransaction:(RCTPromiseRejectBlock)reject { + [RNFBSharedUtils rejectPromiseWithUserInfo:reject + userInfo:(NSMutableDictionary *)@{ + @"code" : @"internal-error", + @"message" : @"An internal error occurred whilst attempting " + @"to find a native transaction by id.", + }]; +} + - (void)transactionGetDocument:(NSString *)appName databaseId:(NSString *)databaseId transactionId:(double)transactionId @@ -210,16 +209,21 @@ - (void)transactionGetDocument:(NSString *)appName reject:(RCTPromiseRejectBlock)reject { FIRApp *firebaseApp = [RCTConvert firAppFromString:appName]; NSNumber *transactionIdNumber = @(transactionId); - NSMutableDictionary *transactionState = [transactions get:transactionIdNumber]; + RNFBFirestoreTransactionAttempt *attempt = [transactions get:transactionIdNumber]; - if (!transactionState) { - DLog(@"transactionGetDocument called for non-existent transactionId %@", transactionIdNumber); + if (![attempt isEligibleForGet]) { + [self rejectMissingTransaction:reject]; return; } - @synchronized(transactionState) { + @synchronized(attempt) { + if (![attempt isEligibleForGet]) { + [self rejectMissingTransaction:reject]; + return; + } + NSError *error = nil; - FIRTransaction *transaction = [transactionState valueForKey:@"transaction"]; + FIRTransaction *transaction = (FIRTransaction *)attempt.nativeTransaction; FIRFirestore *firestore = [RNFBFirestoreCommon getFirestoreForApp:firebaseApp databaseId:databaseId]; FIRDocumentReference *ref = [RNFBFirestoreCommon getDocumentForFirestore:firestore path:path]; @@ -248,17 +252,8 @@ - (void)transactionDispose:(NSString *)appName databaseId:(NSString *)databaseId transactionId:(double)transactionId { NSNumber *transactionIdNumber = @(transactionId); - NSMutableDictionary *transactionState = [transactions get:transactionIdNumber]; - - if (!transactionState) { - return; - } - - @synchronized(transactionState) { - dispatch_semaphore_t semaphore = transactionState[@"semaphore"]; - transactionState[@"aborted"] = @(true); - dispatch_semaphore_signal(semaphore); - } + RNFBFirestoreTransactionAttempt *attempt = [transactions get:transactionIdNumber]; + [attempt abort]; } - (void)transactionApplyBuffer:(NSString *)appName @@ -266,18 +261,8 @@ - (void)transactionApplyBuffer:(NSString *)appName transactionId:(double)transactionId commandBuffer:(NSArray *)commandBuffer { NSNumber *transactionIdNumber = @(transactionId); - NSMutableDictionary *transactionState = [transactions get:transactionIdNumber]; - - if (!transactionState) { - DLog(@"transactionApplyBuffer called for non-existent transactionId %@", transactionIdNumber); - return; - } - - @synchronized(transactionState) { - dispatch_semaphore_t semaphore = [transactionState valueForKey:@"semaphore"]; - [transactionState setValue:commandBuffer forKey:@"commandBuffer"]; - dispatch_semaphore_signal(semaphore); - } + RNFBFirestoreTransactionAttempt *attempt = [transactions get:transactionIdNumber]; + [attempt applyCommandBuffer:commandBuffer]; } @end diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.h b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.h index a4fa5bfd31..282d751300 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.h +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.h @@ -22,7 +22,7 @@ NS_ASSUME_NONNULL_BEGIN /** * Transaction state map. Unique `put`; begin uses `putOrSkip` (atomic putIfAbsentOrSame). Callers * `take` or `abortAll` then abort (flag + semaphore) outside the HandleMap lock. Stored values are - * mutable dictionaries with optional `semaphore` and `aborted` keys. + * `RNFBFirestoreTransactionAttempt` instances. `abortAll` calls `abort` on each remaining attempt. */ @interface RNFBFirestoreTransactionRegistry : NSObject diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.m b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.m index 4ab91964de..5154852814 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.m +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionRegistry.m @@ -16,6 +16,7 @@ */ #import "RNFBFirestoreTransactionRegistry.h" +#import "RNFBFirestoreTransactionAttempt.h" #if __has_include("RNFBHandleMap.h") #import "RNFBHandleMap.h" @@ -38,17 +39,10 @@ - (instancetype)init { } - (void)rnfb_abortState:(id)state { - if (![state isKindOfClass:[NSMutableDictionary class]]) { + if (![state isKindOfClass:[RNFBFirestoreTransactionAttempt class]]) { return; } - NSMutableDictionary *transactionState = (NSMutableDictionary *)state; - @synchronized(transactionState) { - transactionState[@"aborted"] = @YES; - dispatch_semaphore_t semaphore = transactionState[@"semaphore"]; - if (semaphore) { - dispatch_semaphore_signal(semaphore); - } - } + [(RNFBFirestoreTransactionAttempt *)state abort]; } - (BOOL)put:(id)key value:(id)value error:(NSError **)error { diff --git a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m new file mode 100644 index 0000000000..e690286542 --- /dev/null +++ b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m @@ -0,0 +1,149 @@ +/** + * Copyright (c) 2016-present Invertase Limited & Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this library 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 + +#import "RNFBFirestoreTransactionAttempt.h" + +@interface RNFBFirestoreTransactionAttemptTests : XCTestCase +@property(nonatomic, strong) RNFBFirestoreTransactionAttempt *attempt; +@end + +@implementation RNFBFirestoreTransactionAttemptTests + +- (void)setUp { + [super setUp]; + self.attempt = [[RNFBFirestoreTransactionAttempt alloc] init]; +} + +- (void)testTimeoutError_usesNonFirestoreDomainAndDeadlineExceededCode { + NSError *error = [self.attempt timeoutError]; + XCTAssertEqualObjects(error.domain, RNFBFirestoreTransactionTimeoutErrorDomain); + XCTAssertEqual(error.code, RNFBFirestoreTransactionTimeoutErrorCode); + XCTAssertEqual(error.code, 4); + XCTAssertFalse([error.domain isEqualToString:@"FIRFirestoreErrorDomain"]); +} + +- (void)testDefaultWaitTimeout_isFifteenSeconds { + XCTAssertEqual(RNFBFirestoreTransactionWaitTimeoutNSec, 15 * NSEC_PER_SEC); + XCTAssertNotEqual([RNFBFirestoreTransactionAttempt defaultWaitTimeout], (dispatch_time_t)0); +} + +- (void)testWait_withoutPrepare_isStale { + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultStale); +} + +- (void)testWait_withDispatchTimeNow_timesOutWithoutSleepingFifteenSeconds { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + RNFBFirestoreTransactionWaitResult result = + [self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW]; + XCTAssertEqual(result, RNFBFirestoreTransactionWaitResultTimeout); + XCTAssertTrue(self.attempt.updateBlockReturned); + XCTAssertFalse(self.attempt.isEligibleForGet); + XCTAssertNil(self.attempt.nativeTransaction); +} + +- (void)testWait_afterApplyBuffer_isSignaled { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + NSArray *buffer = @[ @{@"type" : @"DELETE"} ]; + XCTAssertTrue([self.attempt applyCommandBuffer:buffer]); + RNFBFirestoreTransactionWaitResult result = + [self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW]; + XCTAssertEqual(result, RNFBFirestoreTransactionWaitResultSignaled); + XCTAssertEqualObjects(self.attempt.commandBuffer, buffer); +} + +- (void)testWait_afterAbort_isAborted { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + [self.attempt abort]; + RNFBFirestoreTransactionWaitResult result = + [self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW]; + XCTAssertEqual(result, RNFBFirestoreTransactionWaitResultAborted); + XCTAssertTrue(self.attempt.aborted); +} + +- (void)testCompleteWait_mismatchedSemaphore_isStaleAndLeavesLiveAttempt { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + dispatch_semaphore_t leftover = dispatch_semaphore_create(0); + RNFBFirestoreTransactionWaitResult result = [self.attempt completeWaitForSemaphore:leftover + timedOut:NO]; + XCTAssertEqual(result, RNFBFirestoreTransactionWaitResultStale); + XCTAssertFalse(self.attempt.updateBlockReturned); + XCTAssertTrue(self.attempt.isEligibleForGet); +} + +- (void)testGetEligibility_requiresPreparedLiveTransaction { + XCTAssertFalse(self.attempt.isEligibleForGet); + + [self.attempt prepareForUpdateBlockWithNativeTransaction:nil]; + XCTAssertFalse(self.attempt.isEligibleForGet); + + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertTrue(self.attempt.isEligibleForGet); + + [self.attempt abort]; + XCTAssertFalse(self.attempt.isEligibleForGet); +} + +- (void)testApplyBuffer_afterUpdateBlockReturned_isNoOp { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultTimeout); + + XCTAssertFalse([self.attempt applyCommandBuffer:@[ @{@"type" : @"UPDATE"} ]]); + XCTAssertNil(self.attempt.commandBuffer); +} + +- (void)testApplyBuffer_whenAborted_isNoOp { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + [self.attempt abort]; + XCTAssertFalse([self.attempt applyCommandBuffer:@[ @{@"type" : @"SET"} ]]); +} + +- (void)testApplyBuffer_beforePrepare_isNoOp { + XCTAssertFalse([self.attempt applyCommandBuffer:@[]]); +} + +- (void)testLeftoverApplyBuffer_doesNotSignalLiveWait { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultTimeout); + XCTAssertFalse([self.attempt applyCommandBuffer:@[ @{@"type" : @"DELETE"} ]]); + + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx2"]; + RNFBFirestoreTransactionWaitResult result = + [self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW]; + XCTAssertEqual(result, RNFBFirestoreTransactionWaitResultTimeout); +} + +- (void)testAbort_withoutSemaphore_setsAborted { + [self.attempt abort]; + XCTAssertTrue(self.attempt.aborted); +} + +- (void)testPrepare_resetsReturnedFlagForRetry { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + [self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW]; + XCTAssertTrue(self.attempt.updateBlockReturned); + + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx2"]; + XCTAssertFalse(self.attempt.updateBlockReturned); + XCTAssertTrue(self.attempt.isEligibleForGet); +} + +@end diff --git a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionRegistryTests.m b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionRegistryTests.m index 03f0ab9a3b..fccc88ff79 100644 --- a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionRegistryTests.m +++ b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionRegistryTests.m @@ -17,6 +17,7 @@ #import +#import "RNFBFirestoreTransactionAttempt.h" #import "RNFBFirestoreTransactionRegistry.h" #import "RNFBHandleMap.h" @@ -31,22 +32,26 @@ - (void)setUp { self.registry = [[RNFBFirestoreTransactionRegistry alloc] init]; } +- (RNFBFirestoreTransactionAttempt *)attempt { + return [[RNFBFirestoreTransactionAttempt alloc] init]; +} + - (void)testPutGetTake_happyPath { - NSMutableDictionary *state = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *state = [self attempt]; NSError *error = nil; XCTAssertTrue([self.registry put:@1 value:state error:&error]); XCTAssertNil(error); XCTAssertEqual(state, [self.registry get:@1]); XCTAssertEqual(state, [self.registry take:@1]); XCTAssertNil([self.registry get:@1]); - XCTAssertNil(state[@"aborted"]); + XCTAssertFalse(state.aborted); } - (void)testPut_occupiedId_returnsCollision { - NSMutableDictionary *first = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *first = [self attempt]; XCTAssertTrue([self.registry put:@1 value:first error:nil]); NSError *error = nil; - XCTAssertFalse([self.registry put:@1 value:[NSMutableDictionary dictionary] error:&error]); + XCTAssertFalse([self.registry put:@1 value:[self attempt] error:&error]); XCTAssertNotNil(error); XCTAssertEqualObjects(error.domain, RNFBHandleMapErrorDomain); XCTAssertEqual(error.code, RNFBHandleMapErrorCollision); @@ -54,52 +59,52 @@ - (void)testPut_occupiedId_returnsCollision { } - (void)testPutOrSkip_uniqueId_putsValue { - NSMutableDictionary *state = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *state = [self attempt]; XCTAssertTrue([self.registry putOrSkip:@1 value:state]); XCTAssertEqual(state, [self.registry get:@1]); - XCTAssertNil(state[@"aborted"]); + XCTAssertFalse(state.aborted); } - (void)testPutOrSkip_retryGet_sameValue_shortCircuits { - NSMutableDictionary *state = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *state = [self attempt]; XCTAssertTrue([self.registry put:@1 value:state error:nil]); XCTAssertTrue([self.registry putOrSkip:@1 value:state]); XCTAssertEqual(state, [self.registry get:@1]); - XCTAssertNil(state[@"aborted"]); + XCTAssertFalse(state.aborted); } - (void)testPutOrSkip_occupiedId_skipsAndLeavesExisting { - NSMutableDictionary *first = [NSMutableDictionary dictionary]; - NSMutableDictionary *duplicate = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *first = [self attempt]; + RNFBFirestoreTransactionAttempt *duplicate = [self attempt]; XCTAssertTrue([self.registry put:@2 value:first error:nil]); XCTAssertFalse([self.registry putOrSkip:@2 value:duplicate]); XCTAssertEqual(first, [self.registry get:@2]); - XCTAssertNil(first[@"aborted"]); - XCTAssertNil(duplicate[@"aborted"]); + XCTAssertFalse(first.aborted); + XCTAssertFalse(duplicate.aborted); } - (void)testAbortAll_signalsSemaphoreAndSetsAborted { - dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); - NSMutableDictionary *state = [NSMutableDictionary dictionary]; - state[@"semaphore"] = semaphore; + RNFBFirestoreTransactionAttempt *state = [self attempt]; + [state prepareForUpdateBlockWithNativeTransaction:@"tx"]; XCTAssertTrue([self.registry put:@7 value:state error:nil]); [self.registry abortAll]; - XCTAssertEqual([state[@"aborted"] boolValue], YES); - XCTAssertEqual(dispatch_semaphore_wait(semaphore, DISPATCH_TIME_NOW), 0); + XCTAssertTrue(state.aborted); + XCTAssertEqual([state waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultAborted); XCTAssertNil([self.registry get:@7]); } -- (void)testAbortAll_dictionaryWithoutSemaphore_setsAborted { - NSMutableDictionary *state = [NSMutableDictionary dictionary]; +- (void)testAbortAll_attemptWithoutPrepare_setsAborted { + RNFBFirestoreTransactionAttempt *state = [self attempt]; XCTAssertTrue([self.registry put:@8 value:state error:nil]); [self.registry abortAll]; - XCTAssertEqual([state[@"aborted"] boolValue], YES); + XCTAssertTrue(state.aborted); XCTAssertNil([self.registry get:@8]); } -- (void)testAbortAll_nonDictionary_doesNotCrash { +- (void)testAbortAll_nonAttempt_doesNotCrash { NSObject *plain = [[NSObject alloc] init]; XCTAssertTrue([self.registry put:@9 value:plain error:nil]); [self.registry abortAll]; @@ -112,8 +117,8 @@ - (void)testAbortAll_empty_isNoOp { } - (void)testPut_afterTake_allowsReuse { - NSMutableDictionary *first = [NSMutableDictionary dictionary]; - NSMutableDictionary *second = [NSMutableDictionary dictionary]; + RNFBFirestoreTransactionAttempt *first = [self attempt]; + RNFBFirestoreTransactionAttempt *second = [self attempt]; XCTAssertTrue([self.registry put:@1 value:first error:nil]); XCTAssertEqual(first, [self.registry take:@1]); XCTAssertTrue([self.registry put:@1 value:second error:nil]); diff --git a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreUnitTests.xcodeproj/project.pbxproj b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreUnitTests.xcodeproj/project.pbxproj index 7d0ed531c4..d44dc970ba 100644 --- a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreUnitTests.xcodeproj/project.pbxproj +++ b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreUnitTests.xcodeproj/project.pbxproj @@ -12,6 +12,8 @@ D2000000000000000000000C /* RNFBFirestoreListenerRegistryTests.m in Sources */ = {isa = PBXBuildFile; fileRef = D20000000000000000000009 /* RNFBFirestoreListenerRegistryTests.m */; }; D20000000000000000000020 /* RNFBFirestoreTransactionRegistry.m in Sources */ = {isa = PBXBuildFile; fileRef = D20000000000000000000021 /* RNFBFirestoreTransactionRegistry.m */; }; D20000000000000000000022 /* RNFBFirestoreTransactionRegistryTests.m in Sources */ = {isa = PBXBuildFile; fileRef = D20000000000000000000023 /* RNFBFirestoreTransactionRegistryTests.m */; }; + D20000000000000000000032 /* RNFBFirestoreTransactionAttempt.m in Sources */ = {isa = PBXBuildFile; fileRef = D20000000000000000000031 /* RNFBFirestoreTransactionAttempt.m */; }; + D20000000000000000000034 /* RNFBFirestoreTransactionAttemptTests.m in Sources */ = {isa = PBXBuildFile; fileRef = D20000000000000000000033 /* RNFBFirestoreTransactionAttemptTests.m */; }; D20000000000000000000015 /* XCTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = D20000000000000000000014 /* XCTest.framework */; }; /* End PBXBuildFile section */ @@ -25,6 +27,9 @@ D20000000000000000000024 /* RNFBFirestoreTransactionRegistry.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = RNFBFirestoreTransactionRegistry.h; path = ../RNFBFirestore/RNFBFirestoreTransactionRegistry.h; sourceTree = SOURCE_ROOT; }; D20000000000000000000021 /* RNFBFirestoreTransactionRegistry.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = RNFBFirestoreTransactionRegistry.m; path = ../RNFBFirestore/RNFBFirestoreTransactionRegistry.m; sourceTree = SOURCE_ROOT; }; D20000000000000000000023 /* RNFBFirestoreTransactionRegistryTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = RNFBFirestoreTransactionRegistryTests.m; sourceTree = ""; }; + D20000000000000000000030 /* RNFBFirestoreTransactionAttempt.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = RNFBFirestoreTransactionAttempt.h; path = ../RNFBFirestore/RNFBFirestoreTransactionAttempt.h; sourceTree = SOURCE_ROOT; }; + D20000000000000000000031 /* RNFBFirestoreTransactionAttempt.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; name = RNFBFirestoreTransactionAttempt.m; path = ../RNFBFirestore/RNFBFirestoreTransactionAttempt.m; sourceTree = SOURCE_ROOT; }; + D20000000000000000000033 /* RNFBFirestoreTransactionAttemptTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = RNFBFirestoreTransactionAttemptTests.m; sourceTree = ""; }; D20000000000000000000014 /* XCTest.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = XCTest.framework; path = Platforms/MacOSX.platform/Developer/Library/Frameworks/XCTest.framework; sourceTree = DEVELOPER_DIR; }; /* End PBXFileReference section */ @@ -65,8 +70,11 @@ D20000000000000000000018 /* RNFBFirestoreListenerRegistry.m */, D20000000000000000000024 /* RNFBFirestoreTransactionRegistry.h */, D20000000000000000000021 /* RNFBFirestoreTransactionRegistry.m */, + D20000000000000000000030 /* RNFBFirestoreTransactionAttempt.h */, + D20000000000000000000031 /* RNFBFirestoreTransactionAttempt.m */, D20000000000000000000009 /* RNFBFirestoreListenerRegistryTests.m */, D20000000000000000000023 /* RNFBFirestoreTransactionRegistryTests.m */, + D20000000000000000000033 /* RNFBFirestoreTransactionAttemptTests.m */, ); name = Sources; sourceTree = ""; @@ -131,8 +139,10 @@ D2000000000000000000000A /* RNFBHandleMap.m in Sources */, D2000000000000000000000B /* RNFBFirestoreListenerRegistry.m in Sources */, D20000000000000000000020 /* RNFBFirestoreTransactionRegistry.m in Sources */, + D20000000000000000000032 /* RNFBFirestoreTransactionAttempt.m in Sources */, D2000000000000000000000C /* RNFBFirestoreListenerRegistryTests.m in Sources */, D20000000000000000000022 /* RNFBFirestoreTransactionRegistryTests.m in Sources */, + D20000000000000000000034 /* RNFBFirestoreTransactionAttemptTests.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; From b556a5e0018cfa1205aa3ecfccaa07544f186fc5 Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Tue, 8 Sep 2026 11:26:32 +0100 Subject: [PATCH 2/3] fix(firestore): ignore leftover JS transaction work after id is finished --- .../__tests__/runTransaction.test.ts | 164 ++++++++++++++++++ .../lib/FirestoreTransactionHandler.ts | 16 +- 2 files changed, 176 insertions(+), 4 deletions(-) diff --git a/packages/firestore/__tests__/runTransaction.test.ts b/packages/firestore/__tests__/runTransaction.test.ts index 98b59bf9bd..dbe60a4b8d 100644 --- a/packages/firestore/__tests__/runTransaction.test.ts +++ b/packages/firestore/__tests__/runTransaction.test.ts @@ -42,3 +42,167 @@ describe('runTransaction options passthrough', function () { expect(transactionBegin).not.toHaveBeenCalled(); }); }); + +describe('runTransaction dead-id guard', function () { + function nativeMocks(handler: FirestoreTransactionHandler) { + return handler._firestore.native as unknown as { + transactionApplyBuffer: jest.Mock; + transactionDispose: jest.Mock; + }; + } + + it('does not applyBuffer or reject again after the id is already finished', async function () { + const handler = createHandler(jest.fn()); + const { transactionApplyBuffer } = nativeMocks(handler); + + let releaseUpdate: (value?: unknown) => void = () => undefined; + const pending = handler._add( + () => + new Promise(resolve => { + releaseUpdate = resolve; + }), + ); + + const id = Number(Object.keys(handler._pending)[0]); + const updateWork = handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + + handler._handleError({ + listenerId: id, + body: { + type: 'error', + error: { code: 'deadline-exceeded', message: 'timeout' }, + }, + }); + + await expect(pending).rejects.toMatchObject({ + code: 'firestore/deadline-exceeded', + }); + + releaseUpdate('late'); + await updateWork; + + expect(transactionApplyBuffer).not.toHaveBeenCalled(); + }); + + it('does not applyBuffer after complete has already settled the id', async function () { + const handler = createHandler(jest.fn()); + const { transactionApplyBuffer } = nativeMocks(handler); + + let releaseUpdate: (value?: unknown) => void = () => undefined; + const pending = handler._add( + () => + new Promise(resolve => { + releaseUpdate = resolve; + }), + ); + + const id = Number(Object.keys(handler._pending)[0]); + const updateWork = handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + + handler._handleComplete({ listenerId: id, body: { type: 'complete' } }); + await expect(pending).resolves.toBeUndefined(); + + releaseUpdate('late'); + await updateWork; + + expect(transactionApplyBuffer).not.toHaveBeenCalled(); + }); + + it('still applyBuffers when a second update event arrives while the id is pending', async function () { + const handler = createHandler(jest.fn()); + const { transactionApplyBuffer } = nativeMocks(handler); + + void handler._add(async () => 'ok'); + const id = Number(Object.keys(handler._pending)[0]); + + await handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + await handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + + expect(transactionApplyBuffer).toHaveBeenCalledTimes(2); + expect(transactionApplyBuffer).toHaveBeenNthCalledWith(1, id, []); + expect(transactionApplyBuffer).toHaveBeenNthCalledWith(2, id, []); + }); + + it('rejects when updateFunction does not return a Promise', async function () { + const handler = createHandler(jest.fn()); + const pending = handler._add((() => 123) as never); + const id = Number(Object.keys(handler._pending)[0]); + + await handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + + await expect(pending).rejects.toThrow("'updateFunction' must return a Promise"); + expect(nativeMocks(handler).transactionApplyBuffer).not.toHaveBeenCalled(); + }); + + it('rejects when updateFunction throws while the id is still pending', async function () { + const handler = createHandler(jest.fn()); + const pending = handler._add(async () => { + throw new Error('user boom'); + }); + const id = Number(Object.keys(handler._pending)[0]); + + await handler._handleUpdate({ listenerId: id, body: { type: 'update' } }); + + await expect(pending).rejects.toThrow('user boom'); + expect(nativeMocks(handler).transactionApplyBuffer).not.toHaveBeenCalled(); + }); + + it('ignores update events for unknown ids', async function () { + const handler = createHandler(jest.fn()); + const { transactionApplyBuffer, transactionDispose } = nativeMocks(handler); + + await handler._handleUpdate({ listenerId: 99, body: { type: 'update' } }); + + expect(transactionApplyBuffer).not.toHaveBeenCalled(); + expect(transactionDispose).toHaveBeenCalledWith(99); + }); + + it('ignores update events without a listener id', async function () { + const handler = createHandler(jest.fn()); + await handler._handleUpdate({ body: { type: 'update' } }); + expect(nativeMocks(handler).transactionApplyBuffer).not.toHaveBeenCalled(); + }); + + it('ignores error events for unknown ids', function () { + const handler = createHandler(jest.fn()); + const { transactionDispose } = nativeMocks(handler); + + handler._handleError({ + listenerId: 99, + body: { + type: 'error', + error: { code: 'deadline-exceeded', message: 'timeout' }, + }, + }); + + expect(transactionDispose).not.toHaveBeenCalled(); + expect(handler._pending).toEqual({}); + }); + + it('ignores complete events for unknown ids', function () { + const handler = createHandler(jest.fn()); + const { transactionDispose } = nativeMocks(handler); + + handler._handleComplete({ listenerId: 99, body: { type: 'complete' } }); + + expect(transactionDispose).not.toHaveBeenCalled(); + expect(handler._pending).toEqual({}); + }); + + it('ignores error events without a listener id', function () { + const handler = createHandler(jest.fn()); + handler._handleError({ + body: { + type: 'error', + error: { code: 'deadline-exceeded', message: 'timeout' }, + }, + }); + expect(nativeMocks(handler).transactionDispose).not.toHaveBeenCalled(); + }); + + it('ignores complete events without a listener id', function () { + const handler = createHandler(jest.fn()); + handler._handleComplete({ body: { type: 'complete' } }); + expect(nativeMocks(handler).transactionDispose).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/firestore/lib/FirestoreTransactionHandler.ts b/packages/firestore/lib/FirestoreTransactionHandler.ts index bd667790bb..a9b030af2f 100644 --- a/packages/firestore/lib/FirestoreTransactionHandler.ts +++ b/packages/firestore/lib/FirestoreTransactionHandler.ts @@ -90,7 +90,7 @@ export default class FirestoreTransactionHandler { } const { meta, transaction } = this._pending[id]; - const { updateFunction, reject } = meta; + const { updateFunction } = meta; transaction._prepare(); @@ -113,14 +113,22 @@ export default class FirestoreTransactionHandler { finalError = exception; } + const pendingAfter = this._pending[id]; + if (!pendingAfter) { + return; + } + if (updateFailed || finalError) { - reject?.(finalError); + pendingAfter.meta.reject?.(finalError); return; } - transaction._pendingResult = pendingResult; + pendingAfter.transaction._pendingResult = pendingResult; - return this._firestore.native.transactionApplyBuffer(id, transaction._commandBuffer); + return this._firestore.native.transactionApplyBuffer( + id, + pendingAfter.transaction._commandBuffer, + ); } _handleError(event: TransactionEvent): void { From 389a6531f40e9e38e657f4fed8947e1e5eaf8ae1 Mon Sep 17 00:00:00 2001 From: russellwheatley Date: Tue, 8 Sep 2026 15:42:34 +0100 Subject: [PATCH 3/3] fix(firestore): reject leftover iOS transaction get as deadline-exceeded A late get after the 15s wait could beat native completion and surface as internal-error. Missing ids stay internal-error; dispose stays aborted. --- .../RNFBFirestoreTransactionAttempt.h | 16 +++++ .../RNFBFirestoreTransactionAttempt.m | 37 +++++++++++ .../RNFBFirestoreTransactionModule.mm | 18 +++-- .../RNFBFirestoreTransactionAttemptTests.m | 65 +++++++++++++++++++ 4 files changed, 130 insertions(+), 6 deletions(-) diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h index 651e606176..2f16080741 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.h @@ -28,6 +28,11 @@ FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionTimeoutErrorDomain; FOUNDATION_EXPORT const NSInteger RNFBFirestoreTransactionTimeoutErrorCode; FOUNDATION_EXPORT const int64_t RNFBFirestoreTransactionWaitTimeoutNSec; +FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionRejectCodeAborted; +FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionRejectCodeDeadlineExceeded; +FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionRejectCodeInternalError; +FOUNDATION_EXPORT NSString *const RNFBFirestoreTransactionMissingIdMessage; + typedef NS_ENUM(NSInteger, RNFBFirestoreTransactionWaitResult) { RNFBFirestoreTransactionWaitResultSignaled = 0, RNFBFirestoreTransactionWaitResultTimeout = 1, @@ -49,12 +54,23 @@ typedef NS_ENUM(NSInteger, RNFBFirestoreTransactionWaitResult) { + (dispatch_time_t)defaultWaitTimeout; - (NSError *)timeoutError; +/** + * Arms a new semaphore and native transaction, and resets `updateBlockReturned` so get is eligible + * again. Timeout must stay a non-FIRFirestoreErrorDomain error so the SDK does not retry; a retry + * would make a leftover JS get look live on the new FIRTransaction. + */ - (void)prepareForUpdateBlockWithNativeTransaction:(nullable id)nativeTransaction; - (RNFBFirestoreTransactionWaitResult)waitUntilSignaledWithTimeout:(dispatch_time_t)timeout; - (RNFBFirestoreTransactionWaitResult)completeWaitForSemaphore:(dispatch_semaphore_t)semaphore timedOut:(BOOL)timedOut; - (BOOL)isEligibleForGet; +/// JS reject payload when get is not eligible. aborted, else leftover after the update block +/// returned (`deadline-exceeded`), else `internal-error`. Missing registry id is the module. +- (NSDictionary *)ineligibleGetRejectUserInfo; +/// nil when get is still live. Otherwise the same payload as `ineligibleGetRejectUserInfo`, +/// chosen under the same lock as the eligibility check. +- (nullable NSDictionary *)rejectUserInfoIfIneligibleForGet; - (BOOL)applyCommandBuffer:(nullable NSArray *)commandBuffer; - (void)abort; diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m index 79553c8ea6..2f36a1d917 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionAttempt.m @@ -21,6 +21,11 @@ @"io.invertase.firebase.firestore.transaction"; const NSInteger RNFBFirestoreTransactionTimeoutErrorCode = 4; const int64_t RNFBFirestoreTransactionWaitTimeoutNSec = 15 * NSEC_PER_SEC; +NSString *const RNFBFirestoreTransactionRejectCodeAborted = @"aborted"; +NSString *const RNFBFirestoreTransactionRejectCodeDeadlineExceeded = @"deadline-exceeded"; +NSString *const RNFBFirestoreTransactionRejectCodeInternalError = @"internal-error"; +NSString *const RNFBFirestoreTransactionMissingIdMessage = + @"An internal error occurred whilst attempting to find a native transaction by id."; @implementation RNFBFirestoreTransactionAttempt { dispatch_semaphore_t _semaphore; @@ -115,6 +120,38 @@ - (BOOL)isEligibleForGet { } } +- (NSDictionary *)ineligibleGetRejectUserInfo { + @synchronized(self) { + if (_aborted) { + return @{ + @"code" : RNFBFirestoreTransactionRejectCodeAborted, + @"message" : @"The transaction was aborted before this get could complete.", + }; + } + + if (_updateBlockReturned) { + return @{ + @"code" : RNFBFirestoreTransactionRejectCodeDeadlineExceeded, + @"message" : @"The transaction update block returned before this get could complete.", + }; + } + + return @{ + @"code" : RNFBFirestoreTransactionRejectCodeInternalError, + @"message" : RNFBFirestoreTransactionMissingIdMessage, + }; + } +} + +- (NSDictionary *)rejectUserInfoIfIneligibleForGet { + @synchronized(self) { + if (_semaphore != NULL && !_updateBlockReturned && !_aborted && _nativeTransaction != nil) { + return nil; + } + return [self ineligibleGetRejectUserInfo]; + } +} + - (BOOL)applyCommandBuffer:(NSArray *)commandBuffer { @synchronized(self) { if (_semaphore == NULL || _updateBlockReturned || _aborted) { diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm index 74cb9cd3c8..4cdb000c7d 100644 --- a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm +++ b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.mm @@ -195,9 +195,8 @@ - (void)transactionBegin:(NSString *)appName - (void)rejectMissingTransaction:(RCTPromiseRejectBlock)reject { [RNFBSharedUtils rejectPromiseWithUserInfo:reject userInfo:(NSMutableDictionary *)@{ - @"code" : @"internal-error", - @"message" : @"An internal error occurred whilst attempting " - @"to find a native transaction by id.", + @"code" : RNFBFirestoreTransactionRejectCodeInternalError, + @"message" : RNFBFirestoreTransactionMissingIdMessage, }]; } @@ -211,14 +210,21 @@ - (void)transactionGetDocument:(NSString *)appName NSNumber *transactionIdNumber = @(transactionId); RNFBFirestoreTransactionAttempt *attempt = [transactions get:transactionIdNumber]; - if (![attempt isEligibleForGet]) { + if (attempt == nil) { [self rejectMissingTransaction:reject]; return; } + NSDictionary *ineligible = [attempt rejectUserInfoIfIneligibleForGet]; + if (ineligible != nil) { + [RNFBSharedUtils rejectPromiseWithUserInfo:reject userInfo:[ineligible mutableCopy]]; + return; + } + @synchronized(attempt) { - if (![attempt isEligibleForGet]) { - [self rejectMissingTransaction:reject]; + ineligible = [attempt rejectUserInfoIfIneligibleForGet]; + if (ineligible != nil) { + [RNFBSharedUtils rejectPromiseWithUserInfo:reject userInfo:[ineligible mutableCopy]]; return; } diff --git a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m index e690286542..53f3f7c89e 100644 --- a/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m +++ b/packages/firestore/ios/RNFBFirestoreUnitTests/RNFBFirestoreTransactionAttemptTests.m @@ -100,6 +100,71 @@ - (void)testGetEligibility_requiresPreparedLiveTransaction { XCTAssertFalse(self.attempt.isEligibleForGet); } +- (void)testIneligibleGet_neverPrepared_isInternalError { + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeInternalError); + XCTAssertEqualObjects(info[@"message"], RNFBFirestoreTransactionMissingIdMessage); +} + +- (void)testIneligibleGet_preparedWithoutTransaction_isInternalError { + [self.attempt prepareForUpdateBlockWithNativeTransaction:nil]; + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeInternalError); +} + +- (void)testIneligibleGet_afterTimeout_isDeadlineExceeded { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultTimeout); + + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeDeadlineExceeded); + XCTAssertEqualObjects(info[@"message"], + @"The transaction update block returned before this get could complete."); +} + +- (void)testIneligibleGet_afterSignaled_isDeadlineExceeded { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertTrue([self.attempt applyCommandBuffer:@[]]); + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultSignaled); + + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeDeadlineExceeded); +} + +- (void)testIneligibleGet_afterAbort_isAborted { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + [self.attempt abort]; + + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeAborted); + XCTAssertEqualObjects(info[@"message"], + @"The transaction was aborted before this get could complete."); +} + +- (void)testIneligibleGet_abortWinsOverReturnedUpdateBlock { + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + [self.attempt abort]; + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultAborted); + + NSDictionary *info = [self.attempt ineligibleGetRejectUserInfo]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeAborted); +} + +- (void)testRejectUserInfoIfIneligible_nilWhenLive { + XCTAssertNotNil([self.attempt rejectUserInfoIfIneligibleForGet]); + + [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; + XCTAssertNil([self.attempt rejectUserInfoIfIneligibleForGet]); + + XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW], + RNFBFirestoreTransactionWaitResultTimeout); + NSDictionary *info = [self.attempt rejectUserInfoIfIneligibleForGet]; + XCTAssertEqualObjects(info[@"code"], RNFBFirestoreTransactionRejectCodeDeadlineExceeded); +} + - (void)testApplyBuffer_afterUpdateBlockReturned_isNoOp { [self.attempt prepareForUpdateBlockWithNativeTransaction:@"tx"]; XCTAssertEqual([self.attempt waitUntilSignaledWithTimeout:DISPATCH_TIME_NOW],