diff --git a/__tests__/race-condition.test.ts b/__tests__/race-condition.test.ts new file mode 100644 index 0000000..1e48d93 --- /dev/null +++ b/__tests__/race-condition.test.ts @@ -0,0 +1,157 @@ +import { SdkEvaluationApi } from '@amplitude/experiment-core'; +import AsyncStorage from '@react-native-async-storage/async-storage'; + +import { ExperimentClient } from '../src/experimentClient'; +import { HttpClient, SimpleResponse } from '../src/types/transport'; +import { ExperimentUser } from '../src/types/user'; + +const delay = (ms: number) => new Promise((res) => setTimeout(res, ms)); + +const API_KEY = 'client-test-race-condition'; + +const testUser: ExperimentUser = { user_id: 'test_user' }; + +beforeEach(async () => { + await AsyncStorage.clear(); +}); + +/** + * Mock HTTP client that allows us to control the timing of responses + */ +class DelayedHttpClient implements HttpClient { + private readonly delayMs: number; + private readonly response: SimpleResponse; + + constructor(delayMs: number, response: SimpleResponse) { + this.delayMs = delayMs; + this.response = response; + } + + async request(): Promise { + await delay(this.delayMs); + return this.response; + } +} + +/** + * Test that demonstrates the race condition bug: + * When two fetches are called in quick succession, if the second completes before + * the first, the older response can overwrite the newer one. + */ +test('ExperimentClient.fetch, race condition bug - older response overwrites newer', async () => { + // Create two different HTTP clients with different delays and responses + // The first request will take 200ms and return 'old-value' + const slowClient = new DelayedHttpClient(200, { + status: 200, + body: JSON.stringify({ + 'test-flag': { + key: 'old', + value: 'old-value', + }, + }), + }); + + // The second request will take 50ms and return 'new-value' + const fastClient = new DelayedHttpClient(50, { + status: 200, + body: JSON.stringify({ + 'test-flag': { + key: 'new', + value: 'new-value', + }, + }), + }); + + // Create client with the slow HTTP client for the first request + const client = new ExperimentClient(API_KEY, { + httpClient: slowClient, + }); + + // Start the first fetch (will take 200ms) + const firstFetch = client.fetch(testUser); + + // Wait a tiny bit to ensure first fetch has started + await delay(10); + + // Swap to the fast client for the second request + // Note: In real code, this would be a second fetch call that happens to complete faster + // due to network conditions, server load, etc. + (client as any).evaluationApi = new SdkEvaluationApi( + API_KEY, + 'https://api.lab.amplitude.com', + { request: fastClient.request.bind(fastClient) }, + ); + + // Start the second fetch (will take 50ms and complete first) + const secondFetch = client.fetch(testUser); + + // Wait for both to complete + await Promise.all([firstFetch, secondFetch]); + + // The bug: the result should be 'new-value' from the second (most recent) fetch, + // but instead it's 'old-value' from the first (older) fetch that completed last + const variant = client.variant('test-flag'); + + // This test will FAIL with the bug present, showing the issue + // Expected: 'new-value' (from the second, more recent fetch) + // Actual: 'old-value' (from the first, older fetch that completed last) + expect(variant.value).toBe('new-value'); +}); + +/** + * Simpler version: Multiple rapid fetches should always use the last one initiated + */ +test('ExperimentClient.fetch, multiple rapid fetches - last initiated wins', async () => { + class CountingHttpClient implements HttpClient { + private readonly id: number; + private readonly delayMs: number; + + constructor(id: number, delayMs: number) { + this.id = id; + this.delayMs = delayMs; + } + + async request(): Promise { + await delay(this.delayMs); + return { + status: 200, + body: JSON.stringify({ + 'test-flag': { + key: `response-${this.id}`, + value: `value-${this.id}`, + }, + }), + }; + } + } + + // First request: 100ms delay + const client1 = new ExperimentClient(API_KEY, { + httpClient: new CountingHttpClient(1, 100), + }); + + // Start first fetch + const fetch1 = client1.fetch(testUser); + + await delay(10); + + // Second request: 50ms delay (will complete first) + (client1 as any).evaluationApi = new SdkEvaluationApi( + API_KEY, + 'https://api.lab.amplitude.com', + { + request: new CountingHttpClient(2, 50).request.bind( + new CountingHttpClient(2, 50), + ), + }, + ); + + const fetch2 = client1.fetch(testUser); + + // Wait for both + await Promise.all([fetch1, fetch2]); + + // Should have value from fetch 2 (the most recent one initiated) + const variant = client1.variant('test-flag'); + expect(variant.value).toBe('value-2'); +}); diff --git a/src/experimentClient.ts b/src/experimentClient.ts index 5323456..c3d63f0 100644 --- a/src/experimentClient.ts +++ b/src/experimentClient.ts @@ -81,6 +81,8 @@ export class ExperimentClient implements Client { private isRunning = false; private readonly flagsAndVariantsLoadedPromise: Promise[] | undefined; private readonly initialFlags: EvaluationFlag[] | undefined; + private fetchSequenceNumber = 0; + private storedFetchSequenceNumber = 0; /** * Creates a new ExperimentClient instance. @@ -646,9 +648,12 @@ export class ExperimentClient implements Client { this.stopRetries(); } + // Assign a sequence number to this fetch to handle race conditions + const sequenceNumber = ++this.fetchSequenceNumber; + try { const variants = await this.doFetch(user, timeoutMillis, options); - await this.storeVariants(variants, options); + await this.storeVariants(variants, sequenceNumber, options); return variants; } catch (e) { if (retry && this.shouldRetryFetch(e)) { @@ -691,8 +696,17 @@ export class ExperimentClient implements Client { private async storeVariants( variants: Variants, + sequenceNumber: number, options?: FetchOptions, ): Promise { + // Only store if this response is from a more recent fetch than what's already stored + if (sequenceNumber <= this.storedFetchSequenceNumber) { + this.debug( + `[Experiment] Ignoring stale fetch response (sequence ${sequenceNumber} <= ${this.storedFetchSequenceNumber})`, + ); + return; + } + let failedFlagKeys = options && options.flagKeys ? options.flagKeys : []; if (failedFlagKeys.length === 0) { this.variants.clear(); @@ -706,6 +720,10 @@ export class ExperimentClient implements Client { this.variants.remove(key); } await this.variants.store(); + + // Update the stored sequence number after successfully storing + this.storedFetchSequenceNumber = sequenceNumber; + this.debug('[Experiment] Stored variants: ', variants); }