Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
37 changes: 37 additions & 0 deletions packages/sdk/browser/__tests__/BrowserClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,43 @@ describe('given a mock platform for a BrowserClient', () => {
});
});

it('can evaluate flags with bootstrap data before identify completes', async () => {
const client = makeClient(
'client-side-id',
AutoEnvAttributes.Disabled,
{
streaming: false,
logger,
diagnosticOptOut: true,
},
platform,
);

const identifyPromise = client.identify(
{ kind: 'user', key: 'bob' },
{
bootstrap: goodBootstrapDataWithReasons,
},
);

const flagValue = client.jsonVariationDetail('json', undefined);
expect(flagValue).toEqual({
reason: {
kind: 'OFF',
},
value: ['a', 'b', 'c', 'd'],
variationIndex: 1,
});

expect(client.getContext()).toBeUndefined();

// Wait for identify to complete
await identifyPromise;

// Verify that active context is now set
expect(client.getContext()).toEqual({ kind: 'user', key: 'bob' });
});

it('can shed intermediate identify calls', async () => {
const client = makeClient(
'client-side-id',
Expand Down
19 changes: 19 additions & 0 deletions packages/sdk/browser/src/BrowserClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,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 @@ -44,6 +45,10 @@ class BrowserClientImpl extends LDClientImpl {
private _initResolve?: (result: LDWaitForInitializationResult) => void;
private _initializeResult?: LDWaitForInitializationResult;

// NOTE: keeps track of when we tried an initial identification. We should consolidate this
// with the waitForInitialization logic in the future.
private _identifyAttempted: boolean = false;

constructor(
clientSideId: string,
autoEnvAttributes: AutoEnvAttributes,
Expand Down Expand Up @@ -222,6 +227,20 @@ class BrowserClientImpl extends LDClientImpl {
if (identifyOptions?.sheddable === undefined) {
identifyOptionsWithUpdatedDefaults.sheddable = true;
}

if (!this._identifyAttempted && identifyOptionsWithUpdatedDefaults.bootstrap) {
this._identifyAttempted = true;
const bootstrapData = readFlagsFromBootstrap(
this.logger,
identifyOptionsWithUpdatedDefaults.bootstrap,
);
try {
this.presetFlags(bootstrapData);
} catch {
this.logger.error('Failed to bootstrap data');
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Bootstrap preset flag logic executes on wrong identify call

The _identifyAttempted flag is only set to true inside the conditional block that requires bootstrap data to be present. This means if the first identify call has no bootstrap but a subsequent call does, presetFlags will execute incorrectly. At that point, presetFlags would overwrite the existing flags with bootstrap data for the new context, but the active context is still from the previous completed identify. This creates a window where flag evaluations return data for the wrong context. The flag should be set unconditionally at the start of identifyResult to properly track whether any initial identification has occurred.

Fix in Cursor Fix in Web


const res = await super.identifyResult(context, identifyOptionsWithUpdatedDefaults);
if (res.status === 'completed') {
this._initializeResult = { status: 'complete' };
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/sdk-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"build": "npx tsc --noEmit && rollup -c rollup.config.js && npm run make-package-jsons",
"clean": "rimraf dist",
"lint": "npx eslint . --ext .ts",
"lint:fix": "yarn run lint -- --fix",
"lint:fix": "npx eslint . --ext .ts --fix",
"prettier": "prettier --write 'src/*.@(js|ts|tsx|json)'",
"check": "yarn && yarn prettier && yarn lint && tsc && yarn test"
},
Expand Down
145 changes: 85 additions & 60 deletions packages/shared/sdk-client/src/LDClientImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import { LDIdentifyOptions } from './api/LDIdentifyOptions';
import { createAsyncTaskQueue } from './async/AsyncTaskQueue';
import { Configuration, ConfigurationImpl, LDClientInternalOptions } from './configuration';
import { addAutoEnv } from './context/addAutoEnv';
import {
ActiveContextTracker,
createActiveContextTracker,
} from './context/createActiveContextTracker';
import { ensureKey } from './context/ensureKey';
import { DataManager, DataManagerFactory } from './DataManager';
import createDiagnosticsManager from './diagnostics/createDiagnosticsManager';
Expand All @@ -43,6 +47,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 All @@ -55,12 +60,12 @@ const DEFAULT_IDENTIFY_TIMEOUT_SECONDS = 5;

export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
private readonly _config: Configuration;
private _uncheckedContext?: LDContext;
private _checkedContext?: Context;
private readonly _diagnosticsManager?: internal.DiagnosticsManager;
private _eventProcessor?: internal.EventProcessor;
readonly logger: LDLogger;

private _activeContextTracker: ActiveContextTracker = createActiveContextTracker();

private readonly _highTimeoutThreshold: number = 15;

private _eventFactoryDefault = new EventFactory(false);
Expand Down Expand Up @@ -200,27 +205,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
// code. We are returned the unchecked context so that if a consumer identifies with an invalid context
// and then calls getContext, they get back the same context they provided, without any assertion about
// validity.
return this._uncheckedContext ? clone<LDContext>(this._uncheckedContext) : undefined;
return this._activeContextTracker.hasContext()
? clone<LDContext>(this._activeContextTracker.getPristineContext())
: undefined;
}

protected getInternalContext(): Context | undefined {
return this._checkedContext;
return this._activeContextTracker.getContext();
}

private _createIdentifyPromise(): {
identifyPromise: Promise<void>;
identifyResolve: () => void;
identifyReject: (err: Error) => void;
} {
let res: any;
let rej: any;

const basePromise = new Promise<void>((resolve, reject) => {
res = resolve;
rej = reject;
});

return { identifyPromise: basePromise, identifyResolve: res, identifyReject: rej };
/**
* Preset flags are used to set the flags before the client is initialized. This is useful for
* when client has precached flags that are ready to evaluate without full initialization.
* @param newFlags - The flags to preset.
*/
protected presetFlags(newFlags: { [key: string]: ItemDescriptor }) {
this._flagManager.presetFlags(newFlags);
}

/**
Expand Down Expand Up @@ -307,15 +307,14 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
this.emitter.emit('error', context, error);
return Promise.reject(error);
}
this._uncheckedContext = context;
this._checkedContext = checkedContext;
this._activeContextTracker.set(context, checkedContext);

this._eventProcessor?.sendEvent(
this._eventFactoryDefault.identifyEvent(this._checkedContext),
this._eventFactoryDefault.identifyEvent(checkedContext),
);
const { identifyPromise, identifyResolve, identifyReject } =
this._createIdentifyPromise();
this.logger.debug(`Identifying ${JSON.stringify(this._checkedContext)}`);
this._activeContextTracker.newIdentificationPromise();
this.logger.debug(`Identifying ${JSON.stringify(checkedContext)}`);

await this.dataManager.identify(
identifyResolve,
Expand Down Expand Up @@ -370,7 +369,7 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
}

track(key: string, data?: any, metricValue?: number): void {
if (!this._checkedContext || !this._checkedContext.valid) {
if (!this._activeContextTracker.hasValidContext()) {
this.logger.warn(ClientMessages.MissingContextKeyNoEvent);
return;
}
Expand All @@ -382,14 +381,19 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {

this._eventProcessor?.sendEvent(
this._config.trackEventModifier(
this._eventFactoryDefault.customEvent(key, this._checkedContext!, data, metricValue),
this._eventFactoryDefault.customEvent(
key,
this._activeContextTracker.getContext()!,
data,
metricValue,
),
),
);

this._hookRunner.afterTrack({
key,
// The context is pre-checked above, so we know it can be unwrapped.
context: this._uncheckedContext!,
context: this._activeContextTracker.getPristineContext()!,
data,
metricValue,
});
Expand All @@ -401,23 +405,34 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
eventFactory: EventFactory,
typeChecker?: (value: any) => [boolean, string],
): LDEvaluationDetail {
if (!this._uncheckedContext) {
this.logger.debug(ClientMessages.MissingContextKeyNoEvent);
return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue);
// We are letting evaulations happen without a context. The main case for this
// is when cached data is loaded, but the client is not fully initialized. In this
// case, we will write out a warning for each evaluation attempt.

// NOTE: we will be changing this behavior soon once we have a tracker on the
// client initialization state.
const hasContext = this._activeContextTracker.hasContext();
if (!hasContext) {
this.logger?.warn(
'Flag evaluation called before client is fully initialized, data from this evaulation could be stale.',
);
}

const evalContext = Context.fromLDContext(this._uncheckedContext);
const evalContext = this._activeContextTracker.getContext()!;
const foundItem = this._flagManager.get(flagKey);

if (foundItem === undefined || foundItem.flag.deleted) {
const defVal = defaultValue ?? null;
const error = new LDClientError(
`Unknown feature flag "${flagKey}"; returning default value ${defVal}.`,
);
this.emitter.emit('error', this._uncheckedContext, error);
this._eventProcessor?.sendEvent(
this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext),
);

this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error);
Copy link

Choose a reason for hiding this comment

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

Bug: Error events emitted with undefined context during pre-initialization evaluation

When evaluating flags before identification completes (using preset bootstrap data), error events for "flag not found" or "wrong type" are emitted via emitter.emit('error', this._activeContextTracker.getPristineContext(), error) where getPristineContext() returns undefined. Previously, _variationInternal returned early when there was no context, so these error events were never emitted in the pre-initialization state. Error listeners that expect a defined context as the first argument may now receive undefined, potentially causing issues in downstream handlers.

Additional Locations (1)

Fix in Cursor Fix in Web

if (hasContext) {
this._eventProcessor?.sendEvent(
this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext),
);
}
return createErrorEvaluationDetail(ErrorKinds.FlagNotFound, defaultValue);
}

Expand All @@ -426,20 +441,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
if (typeChecker) {
const [matched, type] = typeChecker(value);
if (!matched) {
this._eventProcessor?.sendEvent(
eventFactory.evalEventClient(
flagKey,
defaultValue, // track default value on type errors
defaultValue,
foundItem.flag,
evalContext,
reason,
),
);
if (hasContext) {
this._eventProcessor?.sendEvent(
eventFactory.evalEventClient(
flagKey,
defaultValue, // track default value on type errors
defaultValue,
foundItem.flag,
evalContext,
reason,
),
);
}
const error = new LDClientError(
`Wrong type "${type}" for feature flag "${flagKey}"; returning default value`,
);
this.emitter.emit('error', this._uncheckedContext, error);
this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error);
return createErrorEvaluationDetail(ErrorKinds.WrongType, defaultValue);
}
}
Expand All @@ -453,31 +470,36 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
prerequisites?.forEach((prereqKey) => {
this._variationInternal(prereqKey, undefined, this._eventFactoryDefault);
});
this._eventProcessor?.sendEvent(
eventFactory.evalEventClient(
flagKey,
value,
defaultValue,
foundItem.flag,
evalContext,
reason,
),
);
if (hasContext) {
this._eventProcessor?.sendEvent(
eventFactory.evalEventClient(
flagKey,
value,
defaultValue,
foundItem.flag,
evalContext,
reason,
),
);
}
return successDetail;
}

variation(flagKey: string, defaultValue?: LDFlagValue): LDFlagValue {
const { value } = this._hookRunner.withEvaluation(
flagKey,
this._uncheckedContext,
this._activeContextTracker.getPristineContext(),
defaultValue,
() => this._variationInternal(flagKey, defaultValue, this._eventFactoryDefault),
);
return value;
}
variationDetail(flagKey: string, defaultValue?: LDFlagValue): LDEvaluationDetail {
return this._hookRunner.withEvaluation(flagKey, this._uncheckedContext, defaultValue, () =>
this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons),
return this._hookRunner.withEvaluation(
flagKey,
this._activeContextTracker.getPristineContext(),
defaultValue,
() => this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons),
);
}

Expand All @@ -487,8 +509,11 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult {
eventFactory: EventFactory,
typeChecker: (value: unknown) => [boolean, string],
): LDEvaluationDetailTyped<T> {
return this._hookRunner.withEvaluation(key, this._uncheckedContext, defaultValue, () =>
this._variationInternal(key, defaultValue, eventFactory, typeChecker),
return this._hookRunner.withEvaluation(
key,
this._activeContextTracker.getPristineContext(),
defaultValue,
() => this._variationInternal(key, defaultValue, eventFactory, typeChecker),
);
}

Expand Down
Loading