Skip to content

fix: bootstrapped client can do evaluation when not ready #1024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
joker23 wants to merge 2 commits into from
Closed

fix: bootstrapped client can do evaluation when not ready #1024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
becc1fd
fix: bootstrapped client can do evaluation when not ready
joker23 Dec 3, 2025
15e59c7
Merge branch 'main' into skz/sdk-1376/bootstrap-evaluations
joker23 Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/sdk/browser/src/BrowserClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
Platform,
} from '@launchdarkly/js-client-sdk-common';

import { readFlagsFromBootstrap } from './bootstrap';
import { getHref } from './BrowserApi';
import BrowserDataManager from './BrowserDataManager';
import { BrowserIdentifyOptions as LDIdentifyOptions } from './BrowserIdentifyOptions';
Expand All @@ -32,6 +33,7 @@ import BrowserPlatform from './platform/BrowserPlatform';
class BrowserClientImpl extends LDClientImpl {
private readonly _goalManager?: GoalManager;
private readonly _plugins?: LDPlugin[];
private _bootstrapAttempted = false;

constructor(
clientSideId: string,
Expand Down Expand Up @@ -197,6 +199,12 @@ class BrowserClientImpl extends LDClientImpl {
);
}

/**
* @ignore
* NOTE: this identify is not doing anything as the `makeClient` function maps the
* identify function to identifyResults. We will need to consolidate this function
* in the js-client-sdk-common package.
*/
override async identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void> {
return super.identify(context, identifyOptions);
}
Expand All @@ -211,6 +219,21 @@ class BrowserClientImpl extends LDClientImpl {
if (identifyOptions?.sheddable === undefined) {
identifyOptionsWithUpdatedDefaults.sheddable = true;
}

if (!this._bootstrapAttempted && identifyOptionsWithUpdatedDefaults.bootstrap) {
Copy link
Member

@kinyoklion kinyoklion Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to gate on only the very first identify if bootstrap is there.

try {
const bootstrapData = readFlagsFromBootstrap(
this.logger,
identifyOptionsWithUpdatedDefaults.bootstrap,
);
this.setBootstrap(context, bootstrapData);
Copy link

@cursor cursor bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Bootstrap uses unprocessed context causing potential key mismatch

The setBootstrap call receives the raw context parameter before super.identifyResult processes it through ensureKey and addAutoEnv. When AutoEnvAttributes are enabled, addAutoEnv converts single-kind contexts to multi-kind by adding ld_application and ld_device kinds, changing the canonical key. This means the _activeContextKey set during early bootstrap differs from the final processed context's canonical key. While this gets corrected when BrowserDataManager._finishIdentifyFromBootstrap is later called with the processed context, flag evaluations during the brief window between these calls may behave unexpectedly.

Fix in Cursor Fix in Web

} catch (e) {
this.logger.error('failed to bootstrap data');
} finally {
this._bootstrapAttempted = true;
}
}

const res = await super.identifyResult(context, identifyOptionsWithUpdatedDefaults);
this._goalManager?.startTracking();
return res;
Expand Down
14 changes: 14 additions & 0 deletions packages/shared/sdk-client/src/LDClientImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import createEventProcessor from './events/createEventProcessor';
import EventFactory from './events/EventFactory';
import DefaultFlagManager, { FlagManager } from './flag-manager/FlagManager';
import { FlagChangeType } from './flag-manager/FlagUpdater';
import { ItemDescriptor } from './flag-manager/ItemDescriptor';
import HookRunner from './HookRunner';
import { getInspectorHook } from './inspection/getInspectorHook';
import InspectorManager from './inspection/InspectorManager';
Expand Down Expand Up @@ -360,6 +361,19 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
return Promise.race([callSitePromise, timeoutPromise]);
}

/**
* Exposes the bootstrap functionality to the derived classes. This function is mainly used to help set
* flag values for bootstrapped clients before the initialization is complete.
*
* @param pristineContext The LDContext object to be identified.
* @param newFlags The new flags to set.
*/
protected setBootstrap(pristineContext: LDContext, newFlags: { [key: string]: ItemDescriptor }) {
this._uncheckedContext = pristineContext;
this._checkedContext = Context.fromLDContext(this._uncheckedContext);
Copy link
Member

@kinyoklion kinyoklion Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this will still get ultimately set by the normal code path, which would then handle any normal context processing? Do we need to be setting context information at all? I think in the previous SDK bootstrap would fail to send events before init complete, so this may be better for some cases.

Copy link
Member

@kinyoklion kinyoklion Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, if nothing else, we need to make sure that the bootstrap data actually gets set even if the context has anonymous contexts without keys.

Copy link
Contributor Author

@joker23 joker23 Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea so it looks like the bootstrapping will fail when we use anonymous key... will look into this

this._flagManager.setBootstrap(this._checkedContext, newFlags);
}
Copy link

@cursor cursor bot Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Bootstrap fails silently for contexts without valid keys

The setBootstrap method doesn't validate that the context is valid before calling _flagManager.setBootstrap. When Context.fromLDContext creates an invalid context (e.g., for anonymous contexts without keys like { kind: 'user', anonymous: true }), accessing canonicalKey on this invalid context throws a TypeError because _context is undefined. This error is caught in BrowserClient.identifyResult and logged as "failed to bootstrap data" without explaining the root cause. The bootstrap data is silently lost, and users won't understand why their bootstrap configuration isn't working for anonymous contexts.

Additional Locations (1)

Fix in Cursor Fix in Web


on(eventName: EventName, listener: Function): void {
this.emitter.on(eventName, listener);
}
Expand Down