From 42e2682ac71125089f077f5f6722dcbb74cc80fb Mon Sep 17 00:00:00 2001 From: Ryan Nair Date: Fri, 7 Aug 2026 23:11:11 -0400 Subject: [PATCH 1/2] fix(remote-config): prevent experiment state races Protect experiment snapshots with generation-based synchronization so delayed database loads cannot overwrite newer state. Replace experiment rows transactionally and wait for queued persistence before updating A/B Testing. --- FirebaseRemoteConfig/CHANGELOG.md | 6 + .../Sources/RCNConfigDBManager.h | 8 + .../Sources/RCNConfigDBManager.m | 35 ++ .../Sources/RCNConfigExperiment.m | 372 +++++++++++++----- .../Tests/Unit/RCNConfigDBManagerTest.m | 62 ++- .../Tests/Unit/RCNConfigExperimentTest.m | 325 ++++++++++++++- 6 files changed, 683 insertions(+), 125 deletions(-) diff --git a/FirebaseRemoteConfig/CHANGELOG.md b/FirebaseRemoteConfig/CHANGELOG.md index 8a74f4fef64..339f22d4193 100644 --- a/FirebaseRemoteConfig/CHANGELOG.md +++ b/FirebaseRemoteConfig/CHANGELOG.md @@ -1,3 +1,9 @@ +# Unreleased +- [fixed] Fixed data races that could mix stale and current Remote Config + experiment state during database loading and activation. (#16303) +- [fixed] Made experiment payload replacement atomic and delayed A/B Testing + updates until queued experiment persistence finishes. + # 12.17.0 - [fixed] Fixed a memory leak in Remote Config where `activateWithCompletion:` retained the `FIRRemoteConfig` instance indefinitely (#16413). diff --git a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h index d881381186b..48038bc48fc 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h +++ b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h @@ -95,6 +95,14 @@ typedef void (^RCNDBLoadCompletion)(BOOL success, value:(NSData *)value completionHandler:(RCNDBCompletion)handler; +/// Atomically replaces all experiment records for `key`. +/// @param key The experiment data key, as defined in `RCNConfigDefines.h`. +/// @param values The serialized experiment values to persist. +/// @param handler The callback, invoked on the main queue after the replacement finishes. +- (void)replaceExperimentTableWithKey:(NSString *)key + values:(NSArray *)values + completionHandler:(RCNDBCompletion)handler; + - (void)updateMetadataWithOption:(RCNUpdateOption)option namespace:(NSString *)namespace values:(NSArray *)values diff --git a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m index c1fd403a246..0cb3ce10da7 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m +++ b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m @@ -485,6 +485,41 @@ - (void)insertExperimentTableWithKey:(NSString *)key }); } +- (void)replaceExperimentTableWithKey:(NSString *)key + values:(NSArray *)values + completionHandler:(RCNDBCompletion)handler { + NSString *keyCopy = [key copy]; + NSArray *valuesCopy = [values copy]; + dispatch_async(_databaseOperationQueue, ^{ + BOOL transactionStarted = [self executeQuery:"BEGIN IMMEDIATE TRANSACTION"]; + BOOL success = transactionStarted; + if (success) { + const char *deleteSQL = "DELETE FROM " RCNTableNameExperiment " WHERE key = ?"; + success = [self executeQuery:deleteSQL withParams:@[ keyCopy ]]; + } + + for (NSData *value in valuesCopy) { + if (!success) { + break; + } + success = [self insertExperimentTableWithKey:keyCopy value:value]; + } + + if (success) { + success = [self executeQuery:"COMMIT TRANSACTION"]; + } + if (!success && transactionStarted) { + [self executeQuery:"ROLLBACK TRANSACTION"]; + } + + if (handler) { + dispatch_async(dispatch_get_main_queue(), ^{ + handler(success, nil); + }); + } + }); +} + - (BOOL)insertExperimentTableWithKey:(NSString *)key value:(NSData *)dataValue { if ([key isEqualToString:@RCNExperimentTableKeyMetadata]) { return [self updateExperimentMetadata:dataValue]; diff --git a/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m b/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m index 019f914df79..f9b9b13c16e 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m +++ b/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m @@ -16,6 +16,8 @@ #import "FirebaseRemoteConfig/Sources/RCNConfigExperiment.h" +#import + #import "FirebaseABTesting/Sources/Private/FirebaseABTestingInternal.h" #import "FirebaseCore/Extension/FirebaseCoreInternal.h" #import "FirebaseRemoteConfig/Sources/RCNConfigDBManager.h" @@ -27,27 +29,101 @@ static NSString *const kMethodNameLatestStartTime = @"latestExperimentStartTimestampBetweenTimestamp:andPayloads:"; -@interface RCNConfigExperiment () -@property(nonatomic, strong) - NSMutableArray *experimentPayloads; ///< Experiment payloads. -@property(nonatomic, strong) - NSMutableDictionary *experimentMetadata; ///< Experiment metadata -@property(nonatomic, strong) - NSMutableArray *activeExperimentPayloads; ///< Activated experiment payloads. +@interface RCNConfigExperiment () { + // Guards the immutable state snapshots and all generation counters below. No external work is + // performed while this lock is held. + os_unfair_lock _stateLock; + NSArray *_experimentPayloads; + NSDictionary *_experimentMetadata; + NSArray *_activeExperimentPayloads; + // The aggregate generation keeps payload and metadata calculations coherent. Per-field + // generations prevent an older database load from replacing a field updated after it began. + NSUInteger _stateGeneration; + NSUInteger _experimentPayloadGeneration; + NSUInteger _experimentMetadataGeneration; + NSUInteger _activeExperimentPayloadGeneration; +} +@property(nonatomic, copy) NSArray *experimentPayloads; ///< Experiment payloads. +@property(nonatomic, copy) + NSDictionary *experimentMetadata; ///< Experiment metadata +@property(nonatomic, copy) + NSArray *activeExperimentPayloads; ///< Activated experiment payloads. @property(nonatomic, strong) RCNConfigDBManager *DBManager; ///< Database Manager. @property(nonatomic, strong) FIRExperimentController *experimentController; @property(nonatomic, strong) NSDateFormatter *experimentStartTimeDateFormatter; +/// Updates metadata from a coherent state snapshot and optionally activates its fetched payloads. +/// Retries if state changes while the experiment controller calculates the latest start time. +- (nullable NSData *)updateExperimentMetadataAndActivate:(BOOL)activate + lastStartTime:(nullable NSTimeInterval *)lastStartTime + payloads:(NSArray *_Nullable *_Nullable) + payloads; +/// Returns the payloads that contain valid JSON, logging and omitting invalid entries. +- (NSArray *)validExperimentPayloads:(NSArray *)payloads + logPrefix:(NSString *)logPrefix; +/// Serializes experiment metadata for persistence. +- (nullable NSData *)serializedExperimentMetadata: + (NSDictionary *)experimentMetadata; @end @implementation RCNConfigExperiment + +- (NSArray *)experimentPayloads { + os_unfair_lock_lock(&_stateLock); + NSArray *experimentPayloads = _experimentPayloads; + os_unfair_lock_unlock(&_stateLock); + return experimentPayloads; +} + +- (void)setExperimentPayloads:(NSArray *)experimentPayloads { + NSArray *payloadSnapshot = [experimentPayloads copy] ?: @[]; + os_unfair_lock_lock(&_stateLock); + _experimentPayloads = payloadSnapshot; + _experimentPayloadGeneration += 1; + _stateGeneration += 1; + os_unfair_lock_unlock(&_stateLock); +} + +- (NSDictionary *)experimentMetadata { + os_unfair_lock_lock(&_stateLock); + NSDictionary *experimentMetadata = _experimentMetadata; + os_unfair_lock_unlock(&_stateLock); + return experimentMetadata; +} + +- (void)setExperimentMetadata:(NSDictionary *)experimentMetadata { + NSDictionary *metadataSnapshot = [experimentMetadata copy] ?: @{}; + os_unfair_lock_lock(&_stateLock); + _experimentMetadata = metadataSnapshot; + _experimentMetadataGeneration += 1; + _stateGeneration += 1; + os_unfair_lock_unlock(&_stateLock); +} + +- (NSArray *)activeExperimentPayloads { + os_unfair_lock_lock(&_stateLock); + NSArray *activeExperimentPayloads = _activeExperimentPayloads; + os_unfair_lock_unlock(&_stateLock); + return activeExperimentPayloads; +} + +- (void)setActiveExperimentPayloads:(NSArray *)activeExperimentPayloads { + NSArray *payloadSnapshot = [activeExperimentPayloads copy] ?: @[]; + os_unfair_lock_lock(&_stateLock); + _activeExperimentPayloads = payloadSnapshot; + _activeExperimentPayloadGeneration += 1; + _stateGeneration += 1; + os_unfair_lock_unlock(&_stateLock); +} + /// Designated initializer - (instancetype)initWithDBManager:(RCNConfigDBManager *)DBManager experimentController:(FIRExperimentController *)controller { self = [super init]; if (self) { - _experimentPayloads = [[NSMutableArray alloc] init]; - _experimentMetadata = [[NSMutableDictionary alloc] init]; - _activeExperimentPayloads = [[NSMutableArray alloc] init]; + _stateLock = OS_UNFAIR_LOCK_INIT; + _experimentPayloads = @[]; + _experimentMetadata = @{}; + _activeExperimentPayloads = @[]; _experimentStartTimeDateFormatter = [[NSDateFormatter alloc] init]; [_experimentStartTimeDateFormatter setDateFormat:@"yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"]; [_experimentStartTimeDateFormatter setTimeZone:[NSTimeZone timeZoneForSecondsFromGMT:0]]; @@ -68,144 +144,224 @@ - (void)loadExperimentFromTable { if (!_DBManager) { return; } + + os_unfair_lock_lock(&_stateLock); + NSUInteger experimentPayloadGeneration = _experimentPayloadGeneration; + NSUInteger experimentMetadataGeneration = _experimentMetadataGeneration; + NSUInteger activeExperimentPayloadGeneration = _activeExperimentPayloadGeneration; + os_unfair_lock_unlock(&_stateLock); + __weak RCNConfigExperiment *weakSelf = self; - RCNDBCompletion completionHandler = ^(BOOL success, NSDictionary *result) { + [_DBManager loadExperimentWithCompletionHandler:^(BOOL success, NSDictionary *result) { RCNConfigExperiment *strongSelf = weakSelf; - if (strongSelf == nil) { + if (!strongSelf || !success) { return; } + + NSArray *experimentPayloads = nil; if (result[@RCNExperimentTableKeyPayload]) { - [strongSelf->_experimentPayloads removeAllObjects]; - for (NSData *experiment in result[@RCNExperimentTableKeyPayload]) { - NSError *error; - id experimentPayloadJSON = [NSJSONSerialization JSONObjectWithData:experiment - options:kNilOptions - error:&error]; - if (!experimentPayloadJSON || error) { - FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000031", - @"Experiment payload could not be parsed as JSON."); - } else { - [strongSelf->_experimentPayloads addObject:experiment]; - } - } + experimentPayloads = [strongSelf validExperimentPayloads:result[@RCNExperimentTableKeyPayload] + logPrefix:@"Experiment"]; } - if (result[@RCNExperimentTableKeyMetadata]) { - strongSelf->_experimentMetadata = [result[@RCNExperimentTableKeyMetadata] mutableCopy]; + NSDictionary *experimentMetadata = + [result[@RCNExperimentTableKeyMetadata] copy]; + NSArray *activeExperimentPayloads = nil; + if (result[@RCNExperimentTableKeyActivePayload]) { + activeExperimentPayloads = + [strongSelf validExperimentPayloads:result[@RCNExperimentTableKeyActivePayload] + logPrefix:@"Activated experiment"]; } - /// Load activated experiments payload and metadata. - if (result[@RCNExperimentTableKeyActivePayload]) { - [strongSelf->_activeExperimentPayloads removeAllObjects]; - for (NSData *experiment in result[@RCNExperimentTableKeyActivePayload]) { - NSError *error; - id experimentPayloadJSON = [NSJSONSerialization JSONObjectWithData:experiment - options:kNilOptions - error:&error]; - if (!experimentPayloadJSON || error) { - FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000031", - @"Activated experiment payload could not be parsed as JSON."); - } else { - [strongSelf->_activeExperimentPayloads addObject:experiment]; - } - } + os_unfair_lock_lock(&strongSelf->_stateLock); + BOOL didUpdateState = NO; + if (experimentPayloads && + strongSelf->_experimentPayloadGeneration == experimentPayloadGeneration) { + strongSelf->_experimentPayloads = experimentPayloads; + strongSelf->_experimentPayloadGeneration += 1; + didUpdateState = YES; } - }; - [_DBManager loadExperimentWithCompletionHandler:completionHandler]; + if (experimentMetadata && + strongSelf->_experimentMetadataGeneration == experimentMetadataGeneration) { + strongSelf->_experimentMetadata = experimentMetadata; + strongSelf->_experimentMetadataGeneration += 1; + didUpdateState = YES; + } + if (activeExperimentPayloads && + strongSelf->_activeExperimentPayloadGeneration == activeExperimentPayloadGeneration) { + strongSelf->_activeExperimentPayloads = activeExperimentPayloads; + strongSelf->_activeExperimentPayloadGeneration += 1; + didUpdateState = YES; + } + if (didUpdateState) { + strongSelf->_stateGeneration += 1; + } + os_unfair_lock_unlock(&strongSelf->_stateLock); + }]; } - (void)updateExperimentsWithResponse:(NSArray *> *)response { - // cache fetched experiment payloads. - [_experimentPayloads removeAllObjects]; - [_DBManager deleteExperimentTableForKey:@RCNExperimentTableKeyPayload]; - + NSMutableArray *experimentPayloads = [[NSMutableArray alloc] init]; for (NSDictionary *experiment in response) { - NSError *error; - NSData *JSONPayload = [NSJSONSerialization dataWithJSONObject:experiment + NSError *error = nil; + NSData *jsonPayload = [NSJSONSerialization dataWithJSONObject:experiment options:kNilOptions error:&error]; - if (!JSONPayload || error) { + if (!jsonPayload || error) { FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000030", @"Invalid experiment payload to be serialized."); } else { - [_experimentPayloads addObject:JSONPayload]; - [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyPayload - value:JSONPayload - completionHandler:nil]; + [experimentPayloads addObject:jsonPayload]; } } + + NSArray *payloadSnapshot = [experimentPayloads copy]; + os_unfair_lock_lock(&_stateLock); + _experimentPayloads = payloadSnapshot; + _experimentPayloadGeneration += 1; + _stateGeneration += 1; + os_unfair_lock_unlock(&_stateLock); + + [_DBManager replaceExperimentTableWithKey:@RCNExperimentTableKeyPayload + values:payloadSnapshot + completionHandler:nil]; } - (void)updateExperimentsWithHandler:(void (^)(NSError *_Nullable))handler { FIRLifecycleEvents *lifecycleEvent = [[FIRLifecycleEvents alloc] init]; + NSTimeInterval lastStartTime = 0; + NSArray *experimentPayloads = nil; + NSData *serializedExperimentMetadata = + [self updateExperimentMetadataAndActivate:YES + lastStartTime:&lastStartTime + payloads:&experimentPayloads]; - // Get the last experiment start time prior to the latest payload. - NSTimeInterval lastStartTime = - [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; - - // Update the last experiment start time with the latest payload. - [self updateExperimentStartTime]; - - /// Update activated experiments payload and metadata in DB. - [self updateActiveExperimentsInDB]; - - if (self.experimentController) { - [self.experimentController - updateExperimentsWithServiceOrigin:kServiceOrigin - events:lifecycleEvent - policy:ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest - lastStartTime:lastStartTime - payloads:_experimentPayloads - completionHandler:handler]; - } else { - if (handler) { + FIRExperimentController *experimentController = self.experimentController; + void (^updateAnalyticsExperiments)(void) = ^{ + if (experimentController) { + [experimentController + updateExperimentsWithServiceOrigin:kServiceOrigin + events:lifecycleEvent + policy: + ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest + lastStartTime:lastStartTime + payloads:experimentPayloads + completionHandler:handler]; + } else if (handler) { handler(nil); } + }; + + if (!_DBManager) { + updateAnalyticsExperiments(); + return; + } + + if (serializedExperimentMetadata) { + [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata + value:serializedExperimentMetadata + completionHandler:nil]; } + [_DBManager replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload + values:experimentPayloads + completionHandler:^(BOOL success, NSDictionary *result) { + updateAnalyticsExperiments(); + }]; } - (void)updateExperimentStartTime { - NSTimeInterval existingLastStartTime = - [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; + NSData *serializedExperimentMetadata = [self updateExperimentMetadataAndActivate:NO + lastStartTime:NULL + payloads:NULL]; + if (serializedExperimentMetadata) { + [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata + value:serializedExperimentMetadata + completionHandler:nil]; + } +} - NSTimeInterval latestStartTime = - [self latestStartTimeWithExistingLastStartTime:existingLastStartTime]; +- (nullable NSData *)updateExperimentMetadataAndActivate:(BOOL)activate + lastStartTime:(nullable NSTimeInterval *)lastStartTime + payloads:(NSArray *_Nullable *_Nullable) + payloads { + FIRExperimentController *experimentController = self.experimentController; + while (YES) { + os_unfair_lock_lock(&_stateLock); + NSUInteger stateGeneration = _stateGeneration; + NSArray *payloadSnapshot = _experimentPayloads; + NSDictionary *metadataSnapshot = _experimentMetadata; + os_unfair_lock_unlock(&_stateLock); - _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); + NSTimeInterval existingLastStartTime = + [metadataSnapshot[kExperimentMetadataKeyLastStartTime] doubleValue]; + NSTimeInterval latestStartTime = + experimentController + ? [experimentController + latestExperimentStartTimestampBetweenTimestamp:existingLastStartTime + andPayloads:payloadSnapshot] + : existingLastStartTime; + NSMutableDictionary *updatedMetadata = [metadataSnapshot mutableCopy]; + updatedMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); + NSData *serializedMetadata = [self serializedExperimentMetadata:updatedMetadata]; - if (![NSJSONSerialization isValidJSONObject:_experimentMetadata]) { - FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028", - @"Invalid fetched experiment metadata to be serialized."); - return; + os_unfair_lock_lock(&_stateLock); + if (_stateGeneration != stateGeneration) { + os_unfair_lock_unlock(&_stateLock); + continue; + } + _experimentMetadata = [updatedMetadata copy]; + _experimentMetadataGeneration += 1; + if (activate) { + _activeExperimentPayloads = payloadSnapshot; + _activeExperimentPayloadGeneration += 1; + } + _stateGeneration += 1; + os_unfair_lock_unlock(&_stateLock); + + if (lastStartTime) { + *lastStartTime = existingLastStartTime; + } + if (payloads) { + *payloads = payloadSnapshot; + } + return serializedMetadata; } - NSError *error; - NSData *serializedExperimentMetadata = - [NSJSONSerialization dataWithJSONObject:_experimentMetadata - options:NSJSONWritingPrettyPrinted - error:&error]; - [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata - value:serializedExperimentMetadata - completionHandler:nil]; } -- (void)updateActiveExperimentsInDB { - /// Put current fetched experiment payloads into activated experiment DB. - [_activeExperimentPayloads removeAllObjects]; - [_DBManager deleteExperimentTableForKey:@RCNExperimentTableKeyActivePayload]; - for (NSData *experiment in _experimentPayloads) { - [_activeExperimentPayloads addObject:experiment]; - [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyActivePayload - value:experiment - completionHandler:nil]; +- (NSArray *)validExperimentPayloads:(NSArray *)payloads + logPrefix:(NSString *)logPrefix { + NSMutableArray *validPayloads = [[NSMutableArray alloc] init]; + for (NSData *experiment in payloads) { + NSError *error = nil; + id experimentPayloadJSON = [NSJSONSerialization JSONObjectWithData:experiment + options:kNilOptions + error:&error]; + if (!experimentPayloadJSON || error) { + FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000031", + @"%@ payload could not be parsed as JSON.", logPrefix); + } else { + [validPayloads addObject:experiment]; + } } + return [validPayloads copy]; } -- (NSTimeInterval)latestStartTimeWithExistingLastStartTime:(NSTimeInterval)existingLastStartTime { - if (self.experimentController) { - return [self.experimentController - latestExperimentStartTimestampBetweenTimestamp:existingLastStartTime - andPayloads:_experimentPayloads]; - } else { - return existingLastStartTime; +- (nullable NSData *)serializedExperimentMetadata: + (NSDictionary *)experimentMetadata { + if (![NSJSONSerialization isValidJSONObject:experimentMetadata]) { + FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028", + @"Invalid fetched experiment metadata to be serialized."); + return nil; + } + + NSError *error = nil; + NSData *serializedExperimentMetadata = + [NSJSONSerialization dataWithJSONObject:experimentMetadata + options:NSJSONWritingPrettyPrinted + error:&error]; + if (!serializedExperimentMetadata || error) { + FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028", + @"Fetched experiment metadata could not be serialized."); } + return serializedExperimentMetadata; } @end diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m index 09c577346bb..00587c67a9d 100644 --- a/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m +++ b/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m @@ -32,8 +32,29 @@ - (void)removeDatabaseOnDatabaseQueueAtPath:(NSString *)path; - (void)insertExperimentTableWithKey:(NSString *)key value:(NSData *)serializedValue completionHandler:(RCNDBCompletion)handler; +- (BOOL)insertExperimentTableWithKey:(NSString *)key value:(NSData *)serializedValue; - (void)deleteExperimentTableForKey:(NSString *)key; - (void)createOrOpenDatabase; +- (BOOL)isNewDatabase; +@end + +@interface RCNFailingExperimentInsertDBManager : RCNConfigDBManager +@property(nonatomic) BOOL failExperimentInserts; +@property(nonatomic) NSUInteger successfulExperimentInsertsBeforeFailure; +@end + +@implementation RCNFailingExperimentInsertDBManager + +- (BOOL)insertExperimentTableWithKey:(NSString *)key value:(NSData *)serializedValue { + if (_failExperimentInserts) { + if (_successfulExperimentInsertsBeforeFailure == 0) { + return NO; + } + _successfulExperimentInsertsBeforeFailure -= 1; + } + return [super insertExperimentTableWithKey:key value:serializedValue]; +} + @end @interface RCNConfigDBManagerTest : XCTestCase { @@ -47,13 +68,15 @@ @implementation RCNConfigDBManagerTest - (void)setUp { [super setUp]; + // Directly initialized test managers rely on the production singleton's global setup. + (void)[RCNConfigDBManager sharedInstance]; // always remove the database at the start of testing _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; _expectionTimeout = 10.0; id classMock = OCMClassMock([RCNConfigDBManager class]); OCMStub([classMock remoteConfigPathForDatabase]).andReturn(_DBPath); - _DBManager = [[RCNConfigDBManager alloc] init]; + _DBManager = [[RCNFailingExperimentInsertDBManager alloc] init]; } - (void)tearDown { @@ -513,6 +536,43 @@ - (void)testWriteAndLoadActivatedExperiments { [self waitForExpectationsWithTimeout:_expectionTimeout handler:nil]; } +- (void)testReplaceActivatedExperimentsRollsBackAfterInsertFailure { + XCTestExpectation *expectation = + [self expectationWithDescription:@"Failed replacement preserves existing experiments"]; + NSData *oldPayload = [@"old" dataUsingEncoding:NSUTF8StringEncoding]; + NSArray *newPayloads = @[ + [@"new-1" dataUsingEncoding:NSUTF8StringEncoding], + [@"new-2" dataUsingEncoding:NSUTF8StringEncoding] + ]; + RCNFailingExperimentInsertDBManager *dbManager = + (RCNFailingExperimentInsertDBManager *)_DBManager; + + [_DBManager + replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload + values:@[ oldPayload ] + completionHandler:^(BOOL success, NSDictionary *result) { + XCTAssertTrue(success); + dbManager.successfulExperimentInsertsBeforeFailure = 1; + dbManager.failExperimentInserts = YES; + [dbManager replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload + values:newPayloads + completionHandler:^(BOOL success, NSDictionary *result) { + XCTAssertFalse(success); + [dbManager loadExperimentWithCompletionHandler:^( + BOOL success, + NSDictionary *results) { + XCTAssertTrue(success); + XCTAssertEqualObjects( + results[@RCNExperimentTableKeyActivePayload], + @[ oldPayload ]); + [expectation fulfill]; + }]; + }]; + }]; + + [self waitForExpectationsWithTimeout:_expectionTimeout handler:nil]; +} + - (void)testWriteAndLoadMetadataMultipleTimes { XCTestExpectation *updateAndLoadMetadataExpectation = [self expectationWithDescription:@"Update and load experiment metadata in database successfully"]; diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m index c1b39154b59..7b251578adf 100644 --- a/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m +++ b/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m @@ -35,14 +35,59 @@ @interface FIRExperimentController () - (instancetype)initWithAnalytics:(nullable id)analytics; @end +@interface RCNConfigDBManager (ExperimentTest) +- (void)waitForDatabaseOperationQueue; +@end + +@interface RCNControllableExperimentDBManager : RCNConfigDBManager +@property(nonatomic, copy) NSString *pendingExperimentKey; +@property(nonatomic, copy) NSArray *pendingExperimentValues; +@property(nonatomic, copy) RCNDBCompletion pendingExperimentCompletion; +@property(nonatomic) BOOL loadsPersistedExperiments; +- (void)persistPendingExperiments; +@end + +@implementation RCNControllableExperimentDBManager + +- (void)loadExperimentWithCompletionHandler:(RCNDBCompletion)handler { + if (_loadsPersistedExperiments) { + [super loadExperimentWithCompletionHandler:handler]; + } else { + handler(YES, @{ + @RCNExperimentTableKeyPayload : @[], + @RCNExperimentTableKeyMetadata : @{}, + @RCNExperimentTableKeyActivePayload : @[] + }); + } +} + +- (void)replaceExperimentTableWithKey:(NSString *)key + values:(NSArray *)values + completionHandler:(RCNDBCompletion)handler { + _pendingExperimentKey = [key copy]; + _pendingExperimentValues = [values copy]; + _pendingExperimentCompletion = [handler copy]; +} + +- (void)persistPendingExperiments { + NSString *key = _pendingExperimentKey; + NSArray *values = _pendingExperimentValues; + RCNDBCompletion completion = _pendingExperimentCompletion; + _pendingExperimentKey = nil; + _pendingExperimentValues = nil; + _pendingExperimentCompletion = nil; + [super replaceExperimentTableWithKey:key values:values completionHandler:completion]; +} + +@end + @interface RCNConfigExperiment () -@property(nonatomic, copy) NSMutableArray *experimentPayloads; -@property(nonatomic, copy) NSMutableDictionary *experimentMetadata; -@property(nonatomic, copy) NSMutableArray *activeExperimentPayloads; +@property(nonatomic, copy) NSArray *experimentPayloads; +@property(nonatomic, copy) NSDictionary *experimentMetadata; +@property(nonatomic, copy) NSArray *activeExperimentPayloads; @property(nonatomic, strong) RCNConfigDBManager *DBManager; -- (NSTimeInterval)updateExperimentStartTime; +- (void)updateExperimentStartTime; - (void)loadExperimentFromTable; -- (void)updateActiveExperimentsInDB; @end @interface RCNConfigExperimentTest : XCTestCase { @@ -60,6 +105,8 @@ @interface RCNConfigExperimentTest : XCTestCase { @implementation RCNConfigExperimentTest - (void)setUp { [super setUp]; + // Directly initialized test managers rely on the production singleton's global setup. + (void)[RCNConfigDBManager sharedInstance]; _expectationTimeout = 1.0; _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; _DBManagerMock = OCMClassMock([RCNConfigDBManager class]); @@ -85,6 +132,16 @@ - (void)setUp { value:[OCMArg any] completionHandler:nil]) .andDo(nil); + OCMStub([_DBManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completionHandler; + [invocation getArgument:&completionHandler atIndex:4]; + if (completionHandler) { + completionHandler(YES, nil); + } + }); FIRExperimentController *experimentController = [[FIRExperimentController alloc] initWithAnalytics:nil]; @@ -134,6 +191,184 @@ - (void)testUpdateExperiment { } } +- (void)testDatabaseLoadDoesNotOverwriteNewerFetchedExperiments { + __block RCNDBCompletion databaseLoadCompletion; + id dbManagerMock = OCMClassMock([RCNConfigDBManager class]); + OCMStub([dbManagerMock loadExperimentWithCompletionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completion; + [invocation getArgument:&completion atIndex:2]; + databaseLoadCompletion = [completion copy]; + }); + OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:[OCMArg any]]) + .andDo(nil); + + RCNConfigExperiment *experiment = [[RCNConfigExperiment alloc] initWithDBManager:dbManagerMock + experimentController:nil]; + NSDictionary *newPayload = @{@"experimentId" : @"new"}; + [experiment updateExperimentsWithResponse:@[ newPayload ]]; + + NSDictionary *stalePayload = @{@"experimentId" : @"stale"}; + NSData *stalePayloadData = [NSJSONSerialization dataWithJSONObject:stalePayload + options:0 + error:nil]; + NSDictionary *storedMetadata = @{@"last_experiment_start_time" : @123}; + databaseLoadCompletion(YES, @{ + @RCNExperimentTableKeyPayload : @[ stalePayloadData ], + @RCNExperimentTableKeyMetadata : storedMetadata, + @RCNExperimentTableKeyActivePayload : @[] + }); + + NSData *newPayloadData = [NSJSONSerialization dataWithJSONObject:newPayload options:0 error:nil]; + XCTAssertEqualObjects(experiment.experimentPayloads, @[ newPayloadData ]); + XCTAssertEqualObjects(experiment.experimentMetadata, storedMetadata); + [dbManagerMock stopMocking]; +} + +- (void)testDatabaseLoadDoesNotOverwriteNewerActivatedExperiments { + __block RCNDBCompletion databaseLoadCompletion; + id dbManagerMock = OCMClassMock([RCNConfigDBManager class]); + OCMStub([dbManagerMock loadExperimentWithCompletionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completion; + [invocation getArgument:&completion atIndex:2]; + databaseLoadCompletion = [completion copy]; + }); + OCMStub([dbManagerMock insertExperimentTableWithKey:[OCMArg any] + value:[OCMArg any] + completionHandler:nil]); + OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:nil]); + OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completion; + [invocation getArgument:&completion atIndex:4]; + completion(YES, nil); + }); + + RCNConfigExperiment *experiment = [[RCNConfigExperiment alloc] initWithDBManager:dbManagerMock + experimentController:nil]; + NSDictionary *newPayload = @{@"experimentId" : @"new"}; + [experiment updateExperimentsWithResponse:@[ newPayload ]]; + [experiment updateExperimentsWithHandler:nil]; + + NSDictionary *stalePayload = @{@"experimentId" : @"stale"}; + NSData *stalePayloadData = [NSJSONSerialization dataWithJSONObject:stalePayload + options:0 + error:nil]; + databaseLoadCompletion(YES, @{ + @RCNExperimentTableKeyPayload : @[ stalePayloadData ], + @RCNExperimentTableKeyMetadata : @{@"last_experiment_start_time" : @123}, + @RCNExperimentTableKeyActivePayload : @[ stalePayloadData ] + }); + + NSData *newPayloadData = [NSJSONSerialization dataWithJSONObject:newPayload options:0 error:nil]; + XCTAssertEqualObjects(experiment.experimentPayloads, @[ newPayloadData ]); + XCTAssertEqualObjects(experiment.experimentMetadata[@"last_experiment_start_time"], @0); + XCTAssertEqualObjects(experiment.activeExperimentPayloads, @[ newPayloadData ]); + [dbManagerMock stopMocking]; +} + +- (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { + __block RCNDBCompletion databaseLoadCompletion; + id dbManagerMock = OCMClassMock([RCNConfigDBManager class]); + OCMStub([dbManagerMock loadExperimentWithCompletionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completion; + [invocation getArgument:&completion atIndex:2]; + databaseLoadCompletion = [completion copy]; + }); + OCMStub([dbManagerMock insertExperimentTableWithKey:[OCMArg any] + value:[OCMArg any] + completionHandler:nil]); + OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:nil]); + OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] + values:[OCMArg any] + completionHandler:[OCMArg any]]) + .andDo(^(NSInvocation *invocation) { + __unsafe_unretained RCNDBCompletion completion; + [invocation getArgument:&completion atIndex:4]; + completion(YES, nil); + }); + + FIRExperimentController *experimentController = + [[FIRExperimentController alloc] initWithAnalytics:nil]; + id mockExperimentController = OCMPartialMock(experimentController); + dispatch_semaphore_t calculationStarted = dispatch_semaphore_create(0); + dispatch_semaphore_t continueCalculation = dispatch_semaphore_create(0); + __block NSUInteger calculationCount = 0; + __block intptr_t calculationWaitResult = 0; + OCMStub([mockExperimentController latestExperimentStartTimestampBetweenTimestamp:0 + andPayloads:[OCMArg any]]) + .ignoringNonObjectArgs() + .andDo(^(NSInvocation *invocation) { + NSTimeInterval existingLastStartTime; + [invocation getArgument:&existingLastStartTime atIndex:2]; + calculationCount += 1; + if (calculationCount == 1) { + dispatch_semaphore_signal(calculationStarted); + calculationWaitResult = dispatch_semaphore_wait( + continueCalculation, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC)); + } + NSTimeInterval result = existingLastStartTime + 1; + [invocation setReturnValue:&result]; + }); + OCMStub( + [mockExperimentController + updateExperimentsWithServiceOrigin:[OCMArg any] + events:[OCMArg any] + policy: + ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest // NOLINT + lastStartTime:0 + payloads:[OCMArg any] + completionHandler:([OCMArg invokeBlockWithArgs:[NSNull null], nil])]) + .ignoringNonObjectArgs(); + + RCNConfigExperiment *experiment = + [[RCNConfigExperiment alloc] initWithDBManager:dbManagerMock + experimentController:mockExperimentController]; + NSDictionary *newPayload = @{@"experimentId" : @"new"}; + [experiment updateExperimentsWithResponse:@[ newPayload ]]; + + XCTestExpectation *activationExpectation = + [self expectationWithDescription:@"Activation uses the loaded metadata"]; + dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ + [experiment updateExperimentsWithHandler:^(NSError *_Nullable error) { + XCTAssertNil(error); + [activationExpectation fulfill]; + }]; + }); + + XCTAssertEqual( + dispatch_semaphore_wait(calculationStarted, dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_SEC)), + 0); + NSDictionary *stalePayload = @{@"experimentId" : @"stale"}; + NSData *stalePayloadData = [NSJSONSerialization dataWithJSONObject:stalePayload + options:0 + error:nil]; + databaseLoadCompletion(YES, @{ + @RCNExperimentTableKeyPayload : @[ stalePayloadData ], + @RCNExperimentTableKeyMetadata : @{@"last_experiment_start_time" : @100}, + @RCNExperimentTableKeyActivePayload : @[ stalePayloadData ] + }); + dispatch_semaphore_signal(continueCalculation); + + [self waitForExpectationsWithTimeout:_expectationTimeout handler:nil]; + NSData *newPayloadData = [NSJSONSerialization dataWithJSONObject:newPayload options:0 error:nil]; + XCTAssertEqual(calculationWaitResult, 0, @"Activation calculation timed out"); + XCTAssertEqual(calculationCount, 2); + XCTAssertEqualObjects(experiment.experimentMetadata[@"last_experiment_start_time"], @101); + XCTAssertEqualObjects(experiment.activeExperimentPayloads, @[ newPayloadData ]); + [dbManagerMock stopMocking]; +} + - (void)testUpdateLastExperimentStartTime { [_configExperiment updateExperimentStartTime]; XCTAssertEqualObjects(_configExperiment.experimentMetadata[@"last_experiment_start_time"], @(0)); @@ -215,28 +450,86 @@ - (void)testUpdateExperiments { NSTimeInterval lastStartTime = [experiment.experimentMetadata[@"last_experiment_start_time"] doubleValue]; - OCMStub( - [mockExperimentController - updateExperimentsWithServiceOrigin:[OCMArg any] - events:[OCMArg any] - policy: - ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest // NOLINT - lastStartTime:lastStartTime - payloads:[OCMArg any] - completionHandler:[OCMArg any]]) - .andDo(nil); + OCMStub([mockExperimentController + updateExperimentsWithServiceOrigin:[OCMArg any] + events:[OCMArg any] + policy: + ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest // NOLINT + lastStartTime:lastStartTime + payloads:[OCMArg any] + completionHandler:([OCMArg invokeBlockWithArgs:[NSNull null], nil])]); NSData *payloadData = [[self class] payloadDataFromTestFile]; experiment.experimentPayloads = [@[ payloadData ] mutableCopy]; + XCTestExpectation *expectation = + [self expectationWithDescription:@"Experiments are updated after persistence"]; [experiment updateExperimentsWithHandler:^(NSError *_Nullable error) { XCTAssertNil(error); XCTAssertEqualObjects(experiment.experimentMetadata[@"last_experiment_start_time"], @(12345678)); - OCMVerify([experiment updateActiveExperimentsInDB]); XCTAssertEqualObjects(experiment.activeExperimentPayloads, @[ payloadData ]); + [expectation fulfill]; }]; + [self waitForExpectationsWithTimeout:_expectationTimeout handler:nil]; + OCMVerify([_DBManagerMock replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload + values:@[ payloadData ] + completionHandler:[OCMArg any]]); +} + +- (void)testAnalyticsUpdateWaitsForActiveExperimentPersistence { + RCNControllableExperimentDBManager *dbManager = [[RCNControllableExperimentDBManager alloc] init]; + [dbManager waitForDatabaseOperationQueue]; + FIRExperimentController *experimentController = + [[FIRExperimentController alloc] initWithAnalytics:nil]; + id mockExperimentController = OCMPartialMock(experimentController); + RCNConfigExperiment *experiment = + [[RCNConfigExperiment alloc] initWithDBManager:dbManager + experimentController:mockExperimentController]; + NSData *payloadData = [[self class] payloadDataFromTestFile]; + experiment.experimentPayloads = @[ payloadData ]; + + __block BOOL didUpdateAnalytics = NO; + OCMStub( + [mockExperimentController + updateExperimentsWithServiceOrigin:[OCMArg any] + events:[OCMArg any] + policy: + ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest // NOLINT + lastStartTime:0 + payloads:[OCMArg any] + completionHandler:[OCMArg any]]) + .ignoringNonObjectArgs() + .andDo(^(NSInvocation *invocation) { + didUpdateAnalytics = YES; + __unsafe_unretained void (^completionHandler)(NSError *_Nullable error); + [invocation getArgument:&completionHandler atIndex:7]; + void (^completionHandlerCopy)(NSError *_Nullable error) = [completionHandler copy]; + dbManager.loadsPersistedExperiments = YES; + [dbManager loadExperimentWithCompletionHandler:^(BOOL success, + NSDictionary *state) { + XCTAssertTrue(success); + XCTAssertEqualObjects(state[@RCNExperimentTableKeyActivePayload], @[ payloadData ]); + XCTAssertEqualObjects( + state[@RCNExperimentTableKeyMetadata][@"last_experiment_start_time"], @(12345678)); + completionHandlerCopy(nil); + }]; + }); + + XCTestExpectation *expectation = [self expectationWithDescription:@"Analytics update completes"]; + [experiment updateExperimentsWithHandler:^(NSError *_Nullable error) { + XCTAssertNil(error); + [expectation fulfill]; + }]; + + XCTAssertEqualObjects(dbManager.pendingExperimentKey, @RCNExperimentTableKeyActivePayload); + XCTAssertEqualObjects(dbManager.pendingExperimentValues, @[ payloadData ]); + XCTAssertFalse(didUpdateAnalytics); + [dbManager persistPendingExperiments]; + + [self waitForExpectationsWithTimeout:_expectationTimeout handler:nil]; + XCTAssertTrue(didUpdateAnalytics); } - (void)testUpdateExperimentsWithNilExperimentController { From bb58b6cdf4c882ec121dbdbc66340639ee60bc0f Mon Sep 17 00:00:00 2001 From: Ryan Nair Date: Thu, 13 Aug 2026 15:34:42 -0400 Subject: [PATCH 2/2] fix(remote-config): address experiment race review Remove redundant aggregate state tracking and make database queue initialization safe for directly initialized managers. --- FirebaseRemoteConfig/CHANGELOG.md | 2 +- .../Sources/RCNConfigDBManager.h | 3 +- .../Sources/RCNConfigDBManager.m | 16 +++++-- .../Sources/RCNConfigExperiment.m | 42 +++++++++---------- .../Tests/Unit/RCNConfigDBManagerTest.m | 3 -- .../Tests/Unit/RCNConfigExperimentTest.m | 29 +++++++------ 6 files changed, 49 insertions(+), 46 deletions(-) diff --git a/FirebaseRemoteConfig/CHANGELOG.md b/FirebaseRemoteConfig/CHANGELOG.md index 339f22d4193..ee114176bee 100644 --- a/FirebaseRemoteConfig/CHANGELOG.md +++ b/FirebaseRemoteConfig/CHANGELOG.md @@ -2,7 +2,7 @@ - [fixed] Fixed data races that could mix stale and current Remote Config experiment state during database loading and activation. (#16303) - [fixed] Made experiment payload replacement atomic and delayed A/B Testing - updates until queued experiment persistence finishes. + updates until the queued active-experiment database replacement completes. # 12.17.0 - [fixed] Fixed a memory leak in Remote Config where `activateWithCompletion:` diff --git a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h index 48038bc48fc..0b59f3237b9 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h +++ b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.h @@ -98,7 +98,8 @@ typedef void (^RCNDBLoadCompletion)(BOOL success, /// Atomically replaces all experiment records for `key`. /// @param key The experiment data key, as defined in `RCNConfigDefines.h`. /// @param values The serialized experiment values to persist. -/// @param handler The callback, invoked on the main queue after the replacement finishes. +/// @param handler The callback, invoked on the main queue after the replacement finishes. It is an +/// ordering barrier for the queued operation, not a durability guarantee. - (void)replaceExperimentTableWithKey:(NSString *)key values:(NSArray *)values completionHandler:(RCNDBCompletion)handler; diff --git a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m index 0cb3ce10da7..6a8e9a8adbe 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m +++ b/FirebaseRemoteConfig/Sources/RCNConfigDBManager.m @@ -41,6 +41,15 @@ /// Introduce a dedicated serial queue for gIsNewDatabase access. static dispatch_queue_t gIsNewDatabaseQueue; +static dispatch_queue_t RCNIsNewDatabaseQueue(void) { + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + gIsNewDatabaseQueue = dispatch_queue_create("com.google.FirebaseRemoteConfig.gIsNewDatabase", + DISPATCH_QUEUE_SERIAL); + }); + return gIsNewDatabaseQueue; +} + /// Remote Config database path for deprecated V0 version. static NSString *RemoteConfigPathForOldDatabaseV0(void) { NSArray *dirPaths = @@ -85,7 +94,7 @@ static BOOL RemoteConfigCreateFilePathIfNotExist(NSString *filePath) { } NSFileManager *fileManager = [NSFileManager defaultManager]; if (![fileManager fileExistsAtPath:filePath]) { - dispatch_sync(gIsNewDatabaseQueue, ^{ + dispatch_sync(RCNIsNewDatabaseQueue(), ^{ gIsNewDatabase = YES; }); NSError *error; @@ -124,8 +133,6 @@ + (instancetype)sharedInstance { static dispatch_once_t onceToken; static RCNConfigDBManager *sharedInstance; dispatch_once(&onceToken, ^{ - gIsNewDatabaseQueue = dispatch_queue_create("com.google.FirebaseRemoteConfig.gIsNewDatabase", - DISPATCH_QUEUE_SERIAL); sharedInstance = [[RCNConfigDBManager alloc] init]; }); return sharedInstance; @@ -139,6 +146,7 @@ + (NSString *)remoteConfigPathForDatabase { - (instancetype)init { self = [super init]; if (self) { + (void)RCNIsNewDatabaseQueue(); _databaseOperationQueue = dispatch_queue_create("com.google.GoogleConfigService.database", DISPATCH_QUEUE_SERIAL); [self createOrOpenDatabase]; @@ -1262,7 +1270,7 @@ - (BOOL)logErrorWithSQL:(const char *)SQL - (BOOL)isNewDatabase { __block BOOL isNew; - dispatch_sync(gIsNewDatabaseQueue, ^{ + dispatch_sync(RCNIsNewDatabaseQueue(), ^{ isNew = gIsNewDatabase; }); return isNew; diff --git a/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m b/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m index f9b9b13c16e..7cbb6a0d05d 100644 --- a/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m +++ b/FirebaseRemoteConfig/Sources/RCNConfigExperiment.m @@ -36,9 +36,8 @@ @interface RCNConfigExperiment () { NSArray *_experimentPayloads; NSDictionary *_experimentMetadata; NSArray *_activeExperimentPayloads; - // The aggregate generation keeps payload and metadata calculations coherent. Per-field - // generations prevent an older database load from replacing a field updated after it began. - NSUInteger _stateGeneration; + // Per-field generations prevent an older database load from replacing a field updated after it + // began. NSUInteger _experimentPayloadGeneration; NSUInteger _experimentMetadataGeneration; NSUInteger _activeExperimentPayloadGeneration; @@ -79,7 +78,6 @@ - (void)setExperimentPayloads:(NSArray *)experimentPayloads { os_unfair_lock_lock(&_stateLock); _experimentPayloads = payloadSnapshot; _experimentPayloadGeneration += 1; - _stateGeneration += 1; os_unfair_lock_unlock(&_stateLock); } @@ -95,7 +93,6 @@ - (void)setExperimentMetadata:(NSDictionary *)experimentMetadata os_unfair_lock_lock(&_stateLock); _experimentMetadata = metadataSnapshot; _experimentMetadataGeneration += 1; - _stateGeneration += 1; os_unfair_lock_unlock(&_stateLock); } @@ -111,7 +108,6 @@ - (void)setActiveExperimentPayloads:(NSArray *)activeExperimentPayload os_unfair_lock_lock(&_stateLock); _activeExperimentPayloads = payloadSnapshot; _activeExperimentPayloadGeneration += 1; - _stateGeneration += 1; os_unfair_lock_unlock(&_stateLock); } @@ -173,27 +169,20 @@ - (void)loadExperimentFromTable { } os_unfair_lock_lock(&strongSelf->_stateLock); - BOOL didUpdateState = NO; if (experimentPayloads && strongSelf->_experimentPayloadGeneration == experimentPayloadGeneration) { strongSelf->_experimentPayloads = experimentPayloads; strongSelf->_experimentPayloadGeneration += 1; - didUpdateState = YES; } if (experimentMetadata && strongSelf->_experimentMetadataGeneration == experimentMetadataGeneration) { strongSelf->_experimentMetadata = experimentMetadata; strongSelf->_experimentMetadataGeneration += 1; - didUpdateState = YES; } if (activeExperimentPayloads && strongSelf->_activeExperimentPayloadGeneration == activeExperimentPayloadGeneration) { strongSelf->_activeExperimentPayloads = activeExperimentPayloads; strongSelf->_activeExperimentPayloadGeneration += 1; - didUpdateState = YES; - } - if (didUpdateState) { - strongSelf->_stateGeneration += 1; } os_unfair_lock_unlock(&strongSelf->_stateLock); }]; @@ -218,7 +207,6 @@ - (void)updateExperimentsWithResponse:(NSArray *> * os_unfair_lock_lock(&_stateLock); _experimentPayloads = payloadSnapshot; _experimentPayloadGeneration += 1; - _stateGeneration += 1; os_unfair_lock_unlock(&_stateLock); [_DBManager replaceExperimentTableWithKey:@RCNExperimentTableKeyPayload @@ -261,11 +249,20 @@ - (void)updateExperimentsWithHandler:(void (^)(NSError *_Nullable))handler { value:serializedExperimentMetadata completionHandler:nil]; } - [_DBManager replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload - values:experimentPayloads - completionHandler:^(BOOL success, NSDictionary *result) { - updateAnalyticsExperiments(); - }]; + [_DBManager + replaceExperimentTableWithKey:@RCNExperimentTableKeyActivePayload + values:experimentPayloads + completionHandler:^(BOOL success, NSDictionary *result) { + if (!success) { + FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000040", + @"Failed to persist activated experiment payloads before " + "updating A/B Testing."); + } + // Experiment persistence remains best-effort. This completion orders + // the A/B Testing update after the queued active-payload replacement; + // it does not guarantee metadata or active-payload durability. + updateAnalyticsExperiments(); + }]; } - (void)updateExperimentStartTime { @@ -286,7 +283,8 @@ - (nullable NSData *)updateExperimentMetadataAndActivate:(BOOL)activate FIRExperimentController *experimentController = self.experimentController; while (YES) { os_unfair_lock_lock(&_stateLock); - NSUInteger stateGeneration = _stateGeneration; + NSUInteger experimentPayloadGeneration = _experimentPayloadGeneration; + NSUInteger experimentMetadataGeneration = _experimentMetadataGeneration; NSArray *payloadSnapshot = _experimentPayloads; NSDictionary *metadataSnapshot = _experimentMetadata; os_unfair_lock_unlock(&_stateLock); @@ -304,7 +302,8 @@ - (nullable NSData *)updateExperimentMetadataAndActivate:(BOOL)activate NSData *serializedMetadata = [self serializedExperimentMetadata:updatedMetadata]; os_unfair_lock_lock(&_stateLock); - if (_stateGeneration != stateGeneration) { + if (_experimentPayloadGeneration != experimentPayloadGeneration || + _experimentMetadataGeneration != experimentMetadataGeneration) { os_unfair_lock_unlock(&_stateLock); continue; } @@ -314,7 +313,6 @@ - (nullable NSData *)updateExperimentMetadataAndActivate:(BOOL)activate _activeExperimentPayloads = payloadSnapshot; _activeExperimentPayloadGeneration += 1; } - _stateGeneration += 1; os_unfair_lock_unlock(&_stateLock); if (lastStartTime) { diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m index 00587c67a9d..5e35ffff04b 100644 --- a/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m +++ b/FirebaseRemoteConfig/Tests/Unit/RCNConfigDBManagerTest.m @@ -35,7 +35,6 @@ - (void)insertExperimentTableWithKey:(NSString *)key - (BOOL)insertExperimentTableWithKey:(NSString *)key value:(NSData *)serializedValue; - (void)deleteExperimentTableForKey:(NSString *)key; - (void)createOrOpenDatabase; -- (BOOL)isNewDatabase; @end @interface RCNFailingExperimentInsertDBManager : RCNConfigDBManager @@ -68,8 +67,6 @@ @implementation RCNConfigDBManagerTest - (void)setUp { [super setUp]; - // Directly initialized test managers rely on the production singleton's global setup. - (void)[RCNConfigDBManager sharedInstance]; // always remove the database at the start of testing _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m index 7b251578adf..74baded674d 100644 --- a/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m +++ b/FirebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m @@ -105,8 +105,6 @@ @interface RCNConfigExperimentTest : XCTestCase { @implementation RCNConfigExperimentTest - (void)setUp { [super setUp]; - // Directly initialized test managers rely on the production singleton's global setup. - (void)[RCNConfigDBManager sharedInstance]; _expectationTimeout = 1.0; _DBPath = [RCNTestUtilities remoteConfigPathForTestDatabase]; _DBManagerMock = OCMClassMock([RCNConfigDBManager class]); @@ -239,16 +237,15 @@ - (void)testDatabaseLoadDoesNotOverwriteNewerActivatedExperiments { OCMStub([dbManagerMock insertExperimentTableWithKey:[OCMArg any] value:[OCMArg any] completionHandler:nil]); - OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] - values:[OCMArg any] - completionHandler:nil]); OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] values:[OCMArg any] completionHandler:[OCMArg any]]) .andDo(^(NSInvocation *invocation) { __unsafe_unretained RCNDBCompletion completion; [invocation getArgument:&completion atIndex:4]; - completion(YES, nil); + if (completion) { + completion(YES, nil); + } }); RCNConfigExperiment *experiment = [[RCNConfigExperiment alloc] initWithDBManager:dbManagerMock @@ -286,16 +283,15 @@ - (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { OCMStub([dbManagerMock insertExperimentTableWithKey:[OCMArg any] value:[OCMArg any] completionHandler:nil]); - OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] - values:[OCMArg any] - completionHandler:nil]); OCMStub([dbManagerMock replaceExperimentTableWithKey:[OCMArg any] values:[OCMArg any] completionHandler:[OCMArg any]]) .andDo(^(NSInvocation *invocation) { __unsafe_unretained RCNDBCompletion completion; [invocation getArgument:&completion atIndex:4]; - completion(YES, nil); + if (completion) { + completion(YES, nil); + } }); FIRExperimentController *experimentController = @@ -303,6 +299,7 @@ - (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { id mockExperimentController = OCMPartialMock(experimentController); dispatch_semaphore_t calculationStarted = dispatch_semaphore_create(0); dispatch_semaphore_t continueCalculation = dispatch_semaphore_create(0); + NSTimeInterval testTimeout = _expectationTimeout; __block NSUInteger calculationCount = 0; __block intptr_t calculationWaitResult = 0; OCMStub([mockExperimentController latestExperimentStartTimestampBetweenTimestamp:0 @@ -315,7 +312,8 @@ - (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { if (calculationCount == 1) { dispatch_semaphore_signal(calculationStarted); calculationWaitResult = dispatch_semaphore_wait( - continueCalculation, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC)); + continueCalculation, + dispatch_time(DISPATCH_TIME_NOW, (int64_t)(testTimeout * NSEC_PER_SEC))); } NSTimeInterval result = existingLastStartTime + 1; [invocation setReturnValue:&result]; @@ -346,9 +344,10 @@ - (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { }]; }); - XCTAssertEqual( - dispatch_semaphore_wait(calculationStarted, dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_SEC)), - 0); + XCTAssertEqual(dispatch_semaphore_wait( + calculationStarted, + dispatch_time(DISPATCH_TIME_NOW, (int64_t)(testTimeout * NSEC_PER_SEC))), + 0); NSDictionary *stalePayload = @{@"experimentId" : @"stale"}; NSData *stalePayloadData = [NSJSONSerialization dataWithJSONObject:stalePayload options:0 @@ -360,7 +359,7 @@ - (void)testActivationRetriesWhenDatabaseLoadPublishesNewerMetadata { }); dispatch_semaphore_signal(continueCalculation); - [self waitForExpectationsWithTimeout:_expectationTimeout handler:nil]; + [self waitForExpectationsWithTimeout:testTimeout handler:nil]; NSData *newPayloadData = [NSJSONSerialization dataWithJSONObject:newPayload options:0 error:nil]; XCTAssertEqual(calculationWaitResult, 0, @"Activation calculation timed out"); XCTAssertEqual(calculationCount, 2);