From 38ca87d76377727acf658350e9cc4f13c6f45116 Mon Sep 17 00:00:00 2001 From: Steven Zhang Date: Thu, 4 Dec 2025 12:37:53 -0600 Subject: [PATCH 1/4] feat: add initial polling retries to BrowserDataManager This update introduces a new option for initial polling retries in the BrowserDataManager. Changes: - The `initialPollingRetries` parameter has been added to the `BrowserIdentifyOptions` interface, allowing users to specify the maximum number of retries for the initial polling request, defaulting to 3. - The `_requestPayload` method has been refactored to implement this retry logic, enhancing the robustness of the polling mechanism. --- .../sdk/browser/src/BrowserDataManager.ts | 83 +++++++++++++++---- .../sdk/browser/src/BrowserIdentifyOptions.ts | 7 ++ 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/sdk/browser/src/BrowserDataManager.ts b/packages/sdk/browser/src/BrowserDataManager.ts index c25b3d6a49..4fbdeec478 100644 --- a/packages/sdk/browser/src/BrowserDataManager.ts +++ b/packages/sdk/browser/src/BrowserDataManager.ts @@ -6,12 +6,15 @@ import { DataSourcePaths, DataSourceState, FlagManager, + httpErrorMessage, internal, LDEmitter, LDHeaders, LDIdentifyOptions, makeRequestor, Platform, + shouldRetry, + sleep, } from '@launchdarkly/js-client-sdk-common'; import { readFlagsFromBootstrap } from './bootstrap'; @@ -97,34 +100,84 @@ export default class BrowserDataManager extends BaseDataManager { this._debugLog('Identify - Flags loaded from cache. Continuing to initialize via a poll.'); } - await this._finishIdentifyFromPoll(context, identifyResolve, identifyReject); + await this._finishIdentifyFromPoll( + context, + identifyResolve, + identifyReject, + browserIdentifyOptions?.initialPollingRetries, + ); } this._updateStreamingState(); } + /** + * A helper function for the initial poll request. This is mainly here to facilitate + * the retry logic. + * + * @param context - LDContext to request payload for. + * @param maxRetries - Maximum number of retries to attempt. Defaults to 3. + * @returns Payload as a string. + */ + private async _requestPayload(context: Context, maxRetries: number = 3): Promise { + const plainContextString = JSON.stringify(Context.toLDContext(context)); + const pollingRequestor = makeRequestor( + plainContextString, + this.config.serviceEndpoints, + this.getPollingPaths(), + this.platform.requests, + this.platform.encoding!, + this.baseHeaders, + [], + this.config.withReasons, + this.config.useReport, + this._secureModeHash, + ); + + let lastError: any; + let validMaxRetries = maxRetries ?? 3; + + if (validMaxRetries < 1) { + this.logger.warn( + `initialPollingRetries is set to ${maxRetries}, which is less than 1. This is not supported and will be ignored. Defaulting to 3 retries.`, + ); + validMaxRetries = 3; + } + + for (let attempt = 0; attempt <= validMaxRetries; attempt += 1) { + try { + // eslint-disable-next-line no-await-in-loop + return await pollingRequestor.requestPayload(); + } catch (e: any) { + if (!shouldRetry(e)) { + throw e; + } + lastError = e; + this._debugLog(httpErrorMessage(e, 'initial poll request', 'will retry')); + // NOTE: current we are hardcoding the retry interval to 1 second. + // We can make this configurable in the future. + // TODO: Reviewer any thoughts on this? Probably the easiest thing is to make this configurable + // however, we can also look into using the backoff logic to calculate the delay? + if (attempt < validMaxRetries) { + // eslint-disable-next-line no-await-in-loop + await sleep(1000); + } + } + } + + throw lastError; + } + private async _finishIdentifyFromPoll( context: Context, identifyResolve: () => void, identifyReject: (err: Error) => void, + initialPollingRetries: number = 3, ) { try { this.dataSourceStatusManager.requestStateUpdate(DataSourceState.Initializing); - const plainContextString = JSON.stringify(Context.toLDContext(context)); - const pollingRequestor = makeRequestor( - plainContextString, - this.config.serviceEndpoints, - this.getPollingPaths(), - this.platform.requests, - this.platform.encoding!, - this.baseHeaders, - [], - this.config.withReasons, - this.config.useReport, - this._secureModeHash, - ); + const payload = await this._requestPayload(context, initialPollingRetries); - const payload = await pollingRequestor.requestPayload(); try { const listeners = this.createStreamListeners(context, identifyResolve); const putListener = listeners.get('put'); diff --git a/packages/sdk/browser/src/BrowserIdentifyOptions.ts b/packages/sdk/browser/src/BrowserIdentifyOptions.ts index 2241434876..7b14570ee3 100644 --- a/packages/sdk/browser/src/BrowserIdentifyOptions.ts +++ b/packages/sdk/browser/src/BrowserIdentifyOptions.ts @@ -28,4 +28,11 @@ export interface BrowserIdentifyOptions extends Omit Date: Thu, 4 Dec 2025 12:46:53 -0600 Subject: [PATCH 2/4] test: add retry logic tests for BrowserDataManager polling This commit will: - introduce new unit tests - fixed a counting issue with retries --- .../__tests__/BrowserDataManager.test.ts | 79 +++++++++++++++++++ .../sdk/browser/src/BrowserDataManager.ts | 4 +- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts index 1dfbaa18c1..e34d7dd15c 100644 --- a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts +++ b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts @@ -527,4 +527,83 @@ describe('given a BrowserDataManager with mocked dependencies', () => { '[BrowserDataManager] Identify called after data manager was closed.', ); }); + + it('retries initial polling until it succeeds', async () => { + jest.useFakeTimers(); + const context = Context.fromLDContext({ kind: 'user', key: 'test-user' }); + flagManager.loadCached.mockResolvedValue(false); + + // Mock fetch to fail twice with 500 error, then succeed + let callCount = 0; + const mockedFetch = jest.fn().mockImplementation(() => { + callCount += 1; + if (callCount <= 2) { + return mockResponse('', 500); + } + return mockResponse('{"flagA": true}', 200); + }); + + platform.requests.fetch = mockedFetch as typeof platform.requests.fetch; + + const identifyResolve = jest.fn(); + const identifyReject = jest.fn(); + + const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 3 }; + const identifyPromise = dataManager.identify( + identifyResolve, + identifyReject, + context, + identifyOptions, + ); + + // Fast-forward through the retry delays (2 retries * 1000ms each) + await jest.advanceTimersByTimeAsync(2000); + + await identifyPromise; + + expect(mockedFetch).toHaveBeenCalledTimes(3); + expect(identifyResolve).toHaveBeenCalled(); + expect(identifyReject).not.toHaveBeenCalled(); + expect(flagManager.init).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ flagA: { flag: true, version: undefined } }), + ); + + jest.useRealTimers(); + }); + + it('throws an error when initial polling reaches max retry limit', async () => { + jest.useFakeTimers(); + const context = Context.fromLDContext({ kind: 'user', key: 'test-user' }); + flagManager.loadCached.mockResolvedValue(false); + + // Mock fetch to always fail with 500 error + const mockedFetch = jest.fn().mockImplementation(() => mockResponse('', 500)); + + platform.requests.fetch = mockedFetch as typeof platform.requests.fetch; + + const identifyResolve = jest.fn(); + const identifyReject = jest.fn(); + + const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 2 }; + const identifyPromise = dataManager.identify( + identifyResolve, + identifyReject, + context, + identifyOptions, + ); + + // Fast-forward through the retry delays (2 retries * 1000ms each) + await jest.advanceTimersByTimeAsync(2000); + + await identifyPromise; + + // Should attempt initial request + 2 retries = 3 total attempts + expect(mockedFetch).toHaveBeenCalledTimes(3); + expect(identifyResolve).not.toHaveBeenCalled(); + expect(identifyReject).toHaveBeenCalled(); + expect(identifyReject).toHaveBeenCalledWith(expect.objectContaining({ status: 500 })); + + jest.useRealTimers(); + }); }); diff --git a/packages/sdk/browser/src/BrowserDataManager.ts b/packages/sdk/browser/src/BrowserDataManager.ts index 4fbdeec478..ce97311a3e 100644 --- a/packages/sdk/browser/src/BrowserDataManager.ts +++ b/packages/sdk/browser/src/BrowserDataManager.ts @@ -136,9 +136,9 @@ export default class BrowserDataManager extends BaseDataManager { let lastError: any; let validMaxRetries = maxRetries ?? 3; - if (validMaxRetries < 1) { + if (validMaxRetries < 0) { this.logger.warn( - `initialPollingRetries is set to ${maxRetries}, which is less than 1. This is not supported and will be ignored. Defaulting to 3 retries.`, + `initialPollingRetries is set to ${maxRetries}, which is less than 0. This is not supported and will be ignored. Defaulting to 3 retries.`, ); validMaxRetries = 3; } From 6786e125a61b180b4d4190da9a1b9083581873fb Mon Sep 17 00:00:00 2001 From: Steven Zhang Date: Thu, 4 Dec 2025 14:44:10 -0600 Subject: [PATCH 3/4] chore: addressing cursor review --- packages/sdk/browser/src/BrowserDataManager.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/sdk/browser/src/BrowserDataManager.ts b/packages/sdk/browser/src/BrowserDataManager.ts index ce97311a3e..a278fe865f 100644 --- a/packages/sdk/browser/src/BrowserDataManager.ts +++ b/packages/sdk/browser/src/BrowserDataManager.ts @@ -136,9 +136,9 @@ export default class BrowserDataManager extends BaseDataManager { let lastError: any; let validMaxRetries = maxRetries ?? 3; - if (validMaxRetries < 0) { + if (Number.isNaN(validMaxRetries) || validMaxRetries < 0) { this.logger.warn( - `initialPollingRetries is set to ${maxRetries}, which is less than 0. This is not supported and will be ignored. Defaulting to 3 retries.`, + `initialPollingRetries is set to an invalid value: ${maxRetries}. Defaulting to 3 retries.`, ); validMaxRetries = 3; } @@ -152,12 +152,12 @@ export default class BrowserDataManager extends BaseDataManager { throw e; } lastError = e; - this._debugLog(httpErrorMessage(e, 'initial poll request', 'will retry')); // NOTE: current we are hardcoding the retry interval to 1 second. // We can make this configurable in the future. // TODO: Reviewer any thoughts on this? Probably the easiest thing is to make this configurable // however, we can also look into using the backoff logic to calculate the delay? if (attempt < validMaxRetries) { + this._debugLog(httpErrorMessage(e, 'initial poll request', 'will retry')); // eslint-disable-next-line no-await-in-loop await sleep(1000); } From 6affd787c24e1c8dcecfebc7bfd483948deb5e8e Mon Sep 17 00:00:00 2001 From: Steven Zhang Date: Tue, 9 Dec 2025 11:53:51 -0600 Subject: [PATCH 4/4] chore: addressing PR comments --- .../__tests__/BrowserDataManager.test.ts | 24 ++++---------- .../sdk/browser/src/BrowserDataManager.ts | 31 ++++++------------- .../sdk/browser/src/BrowserIdentifyOptions.ts | 7 ----- 3 files changed, 15 insertions(+), 47 deletions(-) diff --git a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts index e34d7dd15c..493fc39e19 100644 --- a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts +++ b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts @@ -548,13 +548,7 @@ describe('given a BrowserDataManager with mocked dependencies', () => { const identifyResolve = jest.fn(); const identifyReject = jest.fn(); - const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 3 }; - const identifyPromise = dataManager.identify( - identifyResolve, - identifyReject, - context, - identifyOptions, - ); + const identifyPromise = dataManager.identify(identifyResolve, identifyReject, context); // Fast-forward through the retry delays (2 retries * 1000ms each) await jest.advanceTimersByTimeAsync(2000); @@ -585,21 +579,15 @@ describe('given a BrowserDataManager with mocked dependencies', () => { const identifyResolve = jest.fn(); const identifyReject = jest.fn(); - const identifyOptions: BrowserIdentifyOptions = { initialPollingRetries: 2 }; - const identifyPromise = dataManager.identify( - identifyResolve, - identifyReject, - context, - identifyOptions, - ); + const identifyPromise = dataManager.identify(identifyResolve, identifyReject, context); - // Fast-forward through the retry delays (2 retries * 1000ms each) - await jest.advanceTimersByTimeAsync(2000); + // Fast-forward through the retry delays (4 retries * 1000ms each) + await jest.advanceTimersByTimeAsync(4000); await identifyPromise; - // Should attempt initial request + 2 retries = 3 total attempts - expect(mockedFetch).toHaveBeenCalledTimes(3); + // Should attempt initial request + 3 retries = 4 total attempts + expect(mockedFetch).toHaveBeenCalledTimes(4); expect(identifyResolve).not.toHaveBeenCalled(); expect(identifyReject).toHaveBeenCalled(); expect(identifyReject).toHaveBeenCalledWith(expect.objectContaining({ status: 500 })); diff --git a/packages/sdk/browser/src/BrowserDataManager.ts b/packages/sdk/browser/src/BrowserDataManager.ts index a278fe865f..466ed6a8e3 100644 --- a/packages/sdk/browser/src/BrowserDataManager.ts +++ b/packages/sdk/browser/src/BrowserDataManager.ts @@ -100,12 +100,7 @@ export default class BrowserDataManager extends BaseDataManager { this._debugLog('Identify - Flags loaded from cache. Continuing to initialize via a poll.'); } - await this._finishIdentifyFromPoll( - context, - identifyResolve, - identifyReject, - browserIdentifyOptions?.initialPollingRetries, - ); + await this._finishIdentifyFromPoll(context, identifyResolve, identifyReject); } this._updateStreamingState(); } @@ -115,10 +110,9 @@ export default class BrowserDataManager extends BaseDataManager { * the retry logic. * * @param context - LDContext to request payload for. - * @param maxRetries - Maximum number of retries to attempt. Defaults to 3. * @returns Payload as a string. */ - private async _requestPayload(context: Context, maxRetries: number = 3): Promise { + private async _requestPayload(context: Context): Promise { const plainContextString = JSON.stringify(Context.toLDContext(context)); const pollingRequestor = makeRequestor( plainContextString, @@ -133,17 +127,13 @@ export default class BrowserDataManager extends BaseDataManager { this._secureModeHash, ); - let lastError: any; - let validMaxRetries = maxRetries ?? 3; + // NOTE: We are currently hardcoding in 3 retries for the initial + // poll. We can make this configurable in the future. + const maxRetries = 3; - if (Number.isNaN(validMaxRetries) || validMaxRetries < 0) { - this.logger.warn( - `initialPollingRetries is set to an invalid value: ${maxRetries}. Defaulting to 3 retries.`, - ); - validMaxRetries = 3; - } + let lastError: any; - for (let attempt = 0; attempt <= validMaxRetries; attempt += 1) { + for (let attempt = 0; attempt <= maxRetries; attempt += 1) { try { // eslint-disable-next-line no-await-in-loop return await pollingRequestor.requestPayload(); @@ -154,9 +144,7 @@ export default class BrowserDataManager extends BaseDataManager { lastError = e; // NOTE: current we are hardcoding the retry interval to 1 second. // We can make this configurable in the future. - // TODO: Reviewer any thoughts on this? Probably the easiest thing is to make this configurable - // however, we can also look into using the backoff logic to calculate the delay? - if (attempt < validMaxRetries) { + if (attempt < maxRetries) { this._debugLog(httpErrorMessage(e, 'initial poll request', 'will retry')); // eslint-disable-next-line no-await-in-loop await sleep(1000); @@ -171,12 +159,11 @@ export default class BrowserDataManager extends BaseDataManager { context: Context, identifyResolve: () => void, identifyReject: (err: Error) => void, - initialPollingRetries: number = 3, ) { try { this.dataSourceStatusManager.requestStateUpdate(DataSourceState.Initializing); - const payload = await this._requestPayload(context, initialPollingRetries); + const payload = await this._requestPayload(context); try { const listeners = this.createStreamListeners(context, identifyResolve); diff --git a/packages/sdk/browser/src/BrowserIdentifyOptions.ts b/packages/sdk/browser/src/BrowserIdentifyOptions.ts index 7b14570ee3..2241434876 100644 --- a/packages/sdk/browser/src/BrowserIdentifyOptions.ts +++ b/packages/sdk/browser/src/BrowserIdentifyOptions.ts @@ -28,11 +28,4 @@ export interface BrowserIdentifyOptions extends Omit