From 30213331beb11fea1024322de35f4beacaa94c3d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:06:30 +0000 Subject: [PATCH] fix: Return a malformed flag result instead of throwing for a definition the evaluator cannot read A flag or segment with a missing or mistyped field, such as a flag without variations or a clause without values, threw a TypeError while it was evaluated. variation() rejected, and allFlagsState() never resolved because the iteration helper ran Promise.all(...).then(cb) with no rejection path. The evaluator now runs each evaluation inside a guard and routes exceptions from the asynchronous continuations (store lookups, big segment membership, iteration steps deferred to a resolved promise) to the same handler. The handler builds a MALFORMED_FLAG error through EvalResult.forError, like the evaluator's other error results, and delivers it through the normal callback exactly once. The message names the flag and includes the underlying error. An exception raised after delivery came from the caller and is rethrown as before. The iteration helpers take an onError callback and deliver a throw from a check or completion callback to it instead of letting it escape or hang. They also report a collection that is not an array, which for a rule with clauses set to an object previously matched every context. getVariation checks that variations is an array, since a string was indexed character by character. allFlagsState keeps its existing handling of error results, reporting the flag through onError and continuing with the other flags. If assembling the state fails for another reason it reports the failure and settles with an invalid state instead of leaving the promise pending. Data from LaunchDarkly is well formed. This protects evaluations fed by a custom store, the file data source, test data, or a future override source. --- .../__tests__/LDClient.allFlags.test.ts | 124 ++++++++ .../__tests__/LDClient.evaluation.test.ts | 21 ++ .../evaluation/Evaluator.malformed.test.ts | 273 +++++++++++++++++ .../__tests__/evaluation/collection.test.ts | 139 +++++++++ .../shared/sdk-server/src/LDClientImpl.ts | 17 ++ .../sdk-server/src/evaluation/Evaluator.ts | 288 +++++++++++------- .../sdk-server/src/evaluation/collection.ts | 115 ++++--- .../sdk-server/src/evaluation/variations.ts | 5 + 8 files changed, 841 insertions(+), 141 deletions(-) create mode 100644 packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts create mode 100644 packages/shared/sdk-server/__tests__/evaluation/collection.test.ts diff --git a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts index 830f25100b..a9fdc91acf 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts @@ -1,5 +1,8 @@ import { LDClientImpl } from '../src'; import TestData from '../src/integrations/test_data/TestData'; +import AsyncStoreFacade from '../src/store/AsyncStoreFacade'; +import InMemoryFeatureStore from '../src/store/InMemoryFeatureStore'; +import VersionedDataKinds from '../src/store/VersionedDataKinds'; import { createBasicPlatform } from './createBasicPlatform'; import TestLogger, { LogLevel } from './Logger'; import makeCallbacks from './makeCallbacks'; @@ -369,3 +372,124 @@ describe('given an offline client', () => { }); }); }); + +describe('given an LDClient with a flag the evaluator cannot read', () => { + let client: LDClientImpl; + let td: TestData; + let onError: jest.Mock; + + beforeEach(async () => { + td = new TestData(); + onError = jest.fn(); + client = new LDClientImpl( + 'sdk-key-all-flags-malformed', + createBasicPlatform(), + { + updateProcessor: td.getFactory(), + sendEvents: false, + }, + { ...makeCallbacks(true), onError }, + ); + + await client.waitForInitialization({ timeout: 10 }); + // LaunchDarkly never sends a flag without variations, but a custom store or a data file + // can. A healthy flag sits next to it to show the rest of the state is still built. + await td.usePreconfiguredFlag({ + key: 'malformed', + version: 1, + on: true, + fallthrough: { variation: 0 }, + }); + await td.usePreconfiguredFlag({ + key: 'healthy', + version: 1, + on: false, + offVariation: 0, + variations: ['a'], + }); + }); + + afterEach(() => { + client.close(); + }); + + it('resolves allFlagsState, reports the flag through the error callback, and keeps the rest', async () => { + const state = await client.allFlagsState(defaultUser, { withReasons: true }); + + expect(state.valid).toBe(true); + expect(state.getFlagValue('healthy')).toBe('a'); + expect(state.getFlagValue('malformed')).toBeNull(); + expect(state.getFlagReason('malformed')).toEqual({ + kind: 'ERROR', + errorKind: 'MALFORMED_FLAG', + }); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError.mock.calls[0][0].message).toMatch( + /^Error for feature flag "malformed" while evaluating all flags: /, + ); + }); +}); + +describe('given an LDClient whose store holds an item that fails after evaluation', () => { + let client: LDClientImpl; + let onError: jest.Mock; + + beforeEach(async () => { + const store = new InMemoryFeatureStore(); + const td = new TestData(); + onError = jest.fn(); + client = new LDClientImpl( + 'sdk-key-all-flags-unreadable', + createBasicPlatform(), + { + updateProcessor: td.getFactory(), + sendEvents: false, + featureStore: store, + }, + { ...makeCallbacks(true), onError }, + ); + await client.waitForInitialization({ timeout: 10 }); + + // The evaluator turns a definition it cannot read into an error result, which allFlagsState + // handles like any other error. This item evaluates cleanly and then throws when its event + // settings are read while the state is assembled, which stands in for any unexpected + // failure outside of evaluation. Before the error path existed, the promise never settled. + const unreadable = new Proxy( + { key: 'unreadable', version: 1, on: false, offVariation: 0, variations: ['a'] }, + { + get(target, property) { + if (property === 'trackEvents') { + throw new Error('unreadable item'); + } + return target[property as keyof typeof target]; + }, + }, + ); + // The store keeps the object handed to init, while upsert copies it and would drop the trap. + await new AsyncStoreFacade(store).init({ + [VersionedDataKinds.Features.namespace]: { unreadable }, + [VersionedDataKinds.Segments.namespace]: {}, + }); + }); + + afterEach(() => { + client.close(); + }); + + it('reports the failure through the error callback and resolves with an invalid state', async () => { + const state = await client.allFlagsState(defaultUser); + + expect(state.valid).toBe(false); + expect(state.allValues()).toEqual({}); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError.mock.calls[0][0].message).toBe('unreadable item'); + }); + + it('delivers the invalid state to a callback', (done) => { + client.allFlagsState(defaultUser, {}, (err, state) => { + expect(err).toBeNull(); + expect(state.valid).toBe(false); + done(); + }); + }); +}); diff --git a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts index 1711068410..fa0ff07746 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts @@ -34,6 +34,27 @@ describe('given an LDClient with test data', () => { client.close(); }); + it('returns the default value with a malformed flag reason for a flag it cannot read', async () => { + // A target without values throws while the flag is read. LaunchDarkly never sends one, + // but a custom store or a data file can. + await td.usePreconfiguredFlag({ + key: 'malformed', + version: 1, + on: true, + targets: [{ variation: 0 }], + fallthrough: { variation: 1 }, + variations: ['a', 'b'], + }); + + const detail = await client.variationDetail('malformed', defaultUser, 'default'); + expect(detail).toEqual({ + value: 'default', + variationIndex: null, + reason: { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }, + }); + expect(await client.variation('malformed', defaultUser, 'default')).toBe('default'); + }); + it('evaluates a flag which has a fallthrough and a rule', async () => { const testId = 'abcd'.repeat(8); const flagKey = 'testFlag'; diff --git a/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts b/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts new file mode 100644 index 0000000000..0d4d458674 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts @@ -0,0 +1,273 @@ +import { AttributeReference, Context } from '@launchdarkly/js-sdk-common'; + +import { BigSegmentStoreMembership } from '../../src/api/interfaces'; +import { Clause } from '../../src/evaluation/data/Clause'; +import { Flag } from '../../src/evaluation/data/Flag'; +import { Segment } from '../../src/evaluation/data/Segment'; +import EvalResult from '../../src/evaluation/EvalResult'; +import Evaluator from '../../src/evaluation/Evaluator'; +import { Queries } from '../../src/evaluation/Queries'; +import EventFactory from '../../src/events/EventFactory'; +import { createBasicPlatform } from '../createBasicPlatform'; + +// LaunchDarkly never sends these shapes, but a custom store, a data file, or an override can +// hand the evaluator anything. Each test awaits the evaluation directly: if the evaluator threw, +// the promise would reject, and if it never delivered a result, the test would time out. + +const userContext = Context.fromLDContext({ key: 'user-key' }); + +// The definitions are deliberately malformed, so they are built as plain objects and cast +// rather than typed as Flag or Segment. +function makeFlag(overrides: Record): Flag { + return { + key: 'malformed-flag', + version: 1, + on: true, + fallthrough: { variation: 1 }, + variations: ['zero', 'one'], + ...overrides, + } as unknown as Flag; +} + +function makeClause(overrides: Record): Clause { + return { + attribute: 'key', + attributeReference: new AttributeReference('key'), + op: 'in', + values: ['user-key'], + contextKind: 'user', + ...overrides, + } as unknown as Clause; +} + +function makeSegment(overrides: Record): Segment { + return { key: 'segment', version: 1, ...overrides } as unknown as Segment; +} + +function makeFlagMatchingSegment(segmentKey: string): Flag { + return makeFlag({ + rules: [ + { + id: 'rule', + variation: 0, + clauses: [makeClause({ op: 'segmentMatch', values: [segmentKey] })], + }, + ], + }); +} + +class TestQueries implements Queries { + constructor( + private readonly _data: { + flags?: Flag[]; + segments?: Segment[]; + membership?: BigSegmentStoreMembership; + }, + private readonly _callsBackAsync: boolean = false, + ) {} + + getFlag(key: string, cb: (flag: Flag | undefined) => void): void { + this._deliver(() => cb(this._data.flags?.find((flag) => flag.key === key))); + } + + getSegment(key: string, cb: (segment: Segment | undefined) => void): void { + this._deliver(() => cb(this._data.segments?.find((segment) => segment.key === key))); + } + + getBigSegmentsMembership(): Promise<[BigSegmentStoreMembership | null, string] | undefined> { + const result: [BigSegmentStoreMembership | null, string] = [ + this._data.membership ?? null, + 'HEALTHY', + ]; + return Promise.resolve(result); + } + + // A store backed by a database calls back after its own promise resolves, which takes the + // evaluator off the stack of the call that started the evaluation. + private _deliver(fn: () => void): void { + if (this._callsBackAsync) { + Promise.resolve().then(fn); + } else { + fn(); + } + } +} + +function expectMalformed(result: EvalResult) { + expect(result.isError).toBe(true); + expect(result.detail).toMatchObject({ + value: null, + variationIndex: null, + reason: { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }, + }); +} + +describe('given an evaluator and flag definitions it cannot read', () => { + let evaluator: Evaluator; + + beforeEach(() => { + evaluator = new Evaluator(createBasicPlatform(), new TestQueries({})); + }); + + it('returns a malformed flag result for a flag with no variations', async () => { + const result = await evaluator.evaluate(makeFlag({ variations: undefined }), userContext); + expectMalformed(result); + expect(result.message).toBe('Flag variations are not an array'); + }); + + it('returns a malformed flag result when variations are not an array', async () => { + // A string has a length and can be indexed, so without an explicit check one of its + // characters would be served as the flag value. + const result = await evaluator.evaluate(makeFlag({ variations: 'abc' }), userContext); + expectMalformed(result); + expect(result.message).toBe('Flag variations are not an array'); + }); + + it('returns a malformed flag result for a rule whose clauses are not an array', async () => { + // A rule with no clauses at all is treated as not matching. Clauses that are present but + // not a list cannot be iterated, and an object would otherwise match every context. + const flag = makeFlag({ rules: [{ id: 'rule', variation: 0, clauses: {} }] }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + expect(result.message).toContain('Expected an array but received object'); + }); + + it('returns a malformed flag result for a clause with no values', async () => { + const flag = makeFlag({ + rules: [{ id: 'rule', variation: 0, clauses: [makeClause({ values: undefined })] }], + }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a target with no values', async () => { + const flag = makeFlag({ targets: [{ variation: 0 }] }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a context target with no values', async () => { + // A context target is only read for a context of its kind. + const flag = makeFlag({ contextTargets: [{ contextKind: 'org', variation: 0 }] }); + const orgContext = Context.fromLDContext({ kind: 'org', key: 'org-key' }); + const result = await evaluator.evaluate(flag, orgContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('delivers the result once and propagates an exception thrown by the receiving callback', () => { + // The guards are for exceptions raised while reading the definition. One raised by the + // caller after the result was delivered is not a flag problem and must stay visible, and + // it must not cause the result to be delivered a second time on its way out. + const flag = makeFlag({ + rules: [{ id: 'rule', variation: 0, clauses: [makeClause({ values: undefined })] }], + }); + const cb = jest.fn(() => { + throw new Error('from the caller'); + }); + expect(() => { + evaluator.evaluateCb(flag, userContext, cb); + }).toThrow('from the caller'); + expect(cb).toHaveBeenCalledTimes(1); + }); +}); + +describe.each([ + ['synchronously', false], + ['asynchronously', true], +])('given a store that calls back %s', (_description, callsBackAsync) => { + it('returns a malformed flag result for a segment whose included list is not an array', async () => { + const segment = makeSegment({ included: {} }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment] }, callsBackAsync), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a segment rule clause with no values', async () => { + const segment = makeSegment({ rules: [{ clauses: [makeClause({ values: undefined })] }] }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment] }, callsBackAsync), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for the parent of a malformed prerequisite', async () => { + const prerequisite = makeFlag({ key: 'prerequisite', targets: [{ variation: 0 }] }); + const parent = makeFlag({ + key: 'parent', + prerequisites: [{ key: 'prerequisite', variation: 1 }], + }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ flags: [prerequisite] }, callsBackAsync), + ); + const result = await evaluator.evaluate(parent, userContext, new EventFactory(true)); + expectMalformed(result); + expect(result.message).toContain('Flag "parent" has a malformed definition'); + // The prerequisite never produced a result, so there is no evaluation to record for it + // and no event to send. Recording it without an event would misreport what ran. + expect(result.prerequisites).toBeUndefined(); + expect(result.events).toBeUndefined(); + }); +}); + +describe('given an unbounded segment whose rules cannot be read', () => { + // Big segment membership is looked up through a promise, so the rest of the evaluation + // runs inside promise continuations rather than on the caller's stack. + const segment = makeSegment({ + unbounded: true, + generation: 1, + rules: [{ clauses: [makeClause({ values: undefined })] }], + }); + let evaluator: Evaluator; + + beforeEach(() => { + evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment], membership: {} }), + ); + }); + + it('returns a malformed flag result instead of an unhandled rejection', async () => { + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + // The error result carries the state gathered before the failure, like other errors do. + expect(result.detail.reason.bigSegmentsStatus).toBe('HEALTHY'); + }); +}); + +describe('given a big segment lookup that rejects', () => { + // The big segments manager turns store failures into a status, so a rejection here would be + // a bug. Before the guard it was an unhandled rejection and the evaluation never finished. + const segment = makeSegment({ unbounded: true, generation: 1 }); + + class RejectingQueries extends TestQueries { + override getBigSegmentsMembership(): Promise< + [BigSegmentStoreMembership | null, string] | undefined + > { + return Promise.reject(new Error('lookup failed')); + } + } + + it('returns a malformed flag result carrying the rejection message', async () => { + const evaluator = new Evaluator( + createBasicPlatform(), + new RejectingQueries({ segments: [segment] }), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + expect(result.message).toContain('lookup failed'); + }); +}); diff --git a/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts b/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts new file mode 100644 index 0000000000..14b8b29fc7 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts @@ -0,0 +1,139 @@ +import { allAsync, allSeriesAsync, firstSeriesAsync } from '../../src/evaluation/collection'; + +// The series helpers recurse synchronously for small collections and defer to a resolved +// promise once a collection has more than 50 items, so a throw after that point happens off the +// caller's stack. Both sizes are exercised. Each run settles on whichever callback fires first, +// so a result delivered where an error was expected fails the assertion rather than hanging. If +// a helper swallowed an exception and called neither callback, the test would time out. +const smallCollection = [0, 1, 2]; +const largeCollection = Array.from({ length: 60 }, (_, index) => index); + +type SeriesHelper = typeof allSeriesAsync; +type SeriesCheck = Parameters[1]; +type Outcome = { result: boolean | null | undefined } | { error: unknown }; + +function runSeries( + helper: SeriesHelper, + collection: unknown, + check: SeriesCheck, +): Promise { + return new Promise((resolve) => { + helper( + collection as number[], + check, + (result) => resolve({ result }), + (error) => resolve({ error }), + ); + }); +} + +function runAll(collection: number[], check: Parameters[1]): Promise { + return new Promise((resolve) => { + allAsync( + collection, + check, + (result) => resolve({ result }), + (error) => resolve({ error }), + ); + }); +} + +describe.each<[string, SeriesHelper, boolean]>([ + // The third column is the value a check must produce for iteration to move to the next item. + ['allSeriesAsync', allSeriesAsync, true], + ['firstSeriesAsync', firstSeriesAsync, false], +])('%s', (_name, helper, continueValue) => { + it('calls cb with the aggregate result when no check throws', async () => { + const outcome = await runSeries(helper, largeCollection, (_item, _index, itemCb) => + itemCb(continueValue), + ); + expect(outcome).toEqual({ result: continueValue }); + }); + + it.each<[string, number[], number]>([ + ['a small', smallCollection, 1], + ['a large', largeCollection, 55], + ])( + 'reports an exception from a check in %s collection through onError', + async (_description, collection, throwAt) => { + const outcome = await runSeries(helper, collection, (item, _index, itemCb) => { + if (item === throwAt) { + throw new Error(`boom at ${item}`); + } + itemCb(continueValue); + }); + expect(outcome).toEqual({ error: new Error(`boom at ${throwAt}`) }); + }, + ); + + it('reports an exception from the completion callback after an asynchronous check', async () => { + // Once a check calls back from a promise, the rest of the iteration runs in that + // continuation, where the try/catch around the original call is no longer on the stack. + // The first item ends the iteration early, so the completion callback runs there directly. + const error = await new Promise((resolve) => { + helper( + smallCollection, + (_item, _index, itemCb) => { + Promise.resolve().then(() => itemCb(!continueValue)); + }, + () => { + throw new Error('from cb after async callback'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb after async callback')); + }); + + it('reports an exception from the completion callback through onError', async () => { + const error = await new Promise((resolve) => { + helper( + smallCollection, + (_item, _index, itemCb) => itemCb(continueValue), + () => { + throw new Error('from cb'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb')); + }); + + it('reports a collection that is not an array through onError without running a check', async () => { + const check = jest.fn((_item, _index, itemCb) => itemCb(continueValue)); + const outcome = await runSeries(helper, {}, check); + expect(outcome).toEqual({ error: new TypeError('Expected an array but received object') }); + expect(check).not.toHaveBeenCalled(); + }); +}); + +describe('allAsync', () => { + it('calls cb with true when every check passes', async () => { + const outcome = await runAll(largeCollection, (_item, itemCb) => itemCb(true)); + expect(outcome).toEqual({ result: true }); + }); + + it('reports an exception from a check through onError', async () => { + const outcome = await runAll(smallCollection, (item, itemCb) => { + if (item === 1) { + throw new Error('boom at 1'); + } + itemCb(true); + }); + expect(outcome).toEqual({ error: new Error('boom at 1') }); + }); + + it('reports an exception from the completion callback through onError', async () => { + const error = await new Promise((resolve) => { + allAsync( + smallCollection, + (_item, itemCb) => itemCb(true), + () => { + throw new Error('from cb'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb')); + }); +}); diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index 38329c97b7..1e82398952 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -1152,6 +1152,7 @@ export default class LDClientImpl implements LDClient { const clientOnly = !!options?.clientSideOnly; const detailsOnlyIfTracked = !!options?.detailsOnlyForTrackedFlags; + let delivered = false; allAsync( Object.values(allFlags), (storeItem, iterCb) => { @@ -1183,10 +1184,26 @@ export default class LDClientImpl implements LDClient { }); }, () => { + delivered = true; const res = builder.build(); callback?.(null, res); resolve(res); }, + (err) => { + if (delivered) { + // The state has already been handed to the caller, so this exception came from + // the caller's callback. Let it propagate as it did before. + throw err; + } + delivered = true; + // The evaluator reports a malformed flag as an error result, which is handled + // above, so this is an unexpected failure while assembling the state. Report it + // and return an invalid state rather than leaving the promise pending. + this._onError(err instanceof Error ? err : new Error(String(err))); + const res = new FlagsStateBuilder(false, false).build(); + callback?.(null, res); + resolve(res); + }, ); }); if (!this.initialized()) { diff --git a/packages/shared/sdk-server/src/evaluation/Evaluator.ts b/packages/shared/sdk-server/src/evaluation/Evaluator.ts index b3acc25369..097f0e5fdb 100644 --- a/packages/shared/sdk-server/src/evaluation/Evaluator.ts +++ b/packages/shared/sdk-server/src/evaluation/Evaluator.ts @@ -70,6 +70,19 @@ interface EvalState { bigSegmentsStatus?: BigSegmentStoreStatusString; bigSegmentsMembership?: Record; + + /** + * True once the top-level result has been handed to the caller. Nothing is delivered twice. + */ + delivered: boolean; + + /** + * Receives an exception raised while reading the flag, or a prerequisite or segment it + * references, so that it can be delivered as an error result instead of escaping. The state + * is shared with prerequisite evaluations, so an exception in a prerequisite fails the + * top-level flag. + */ + onError: (err: unknown) => void; } interface Match { @@ -92,6 +105,44 @@ function makeError(result: EvalResult): MatchError { return { error: true, isMatch: false, result }; } +/** + * Run part of an evaluation that may execute outside of the try/catch in evaluateCb, such as + * the continuation of a store lookup, routing any exception to the state's error handler. + */ +function guarded(state: EvalState, fn: () => void): void { + try { + fn(); + } catch (err) { + state.onError(err); + } +} + +/** + * Deliver the top-level result to the caller exactly once, decorated with the state + * accumulated during the evaluation. + */ +function deliverResult(state: EvalState, res: EvalResult, cb: (res: EvalResult) => void): void { + if (state.delivered) { + return; + } + state.delivered = true; + if (state.bigSegmentsStatus) { + res.detail.reason = { + ...res.detail.reason, + bigSegmentsStatus: state.bigSegmentsStatus, + }; + } + if (state.prerequisites) { + res.prerequisites = state.prerequisites; + } + res.events = state.events; + cb(res); +} + +function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + /** * MatchOrError effectively creates a discriminated union for the segment * matching process. Allowing encoding a true/false match result, or an @@ -124,28 +175,40 @@ export default class Evaluator { cb: (res: EvalResult) => void, eventFactory?: EventFactory, ) { - const state: EvalState = {}; - this._evaluateInternal( - flag, - context, - state, - [], - (res) => { - if (state.bigSegmentsStatus) { - res.detail.reason = { - ...res.detail.reason, - bigSegmentsStatus: state.bigSegmentsStatus, - }; - } - if (state.prerequisites) { - res.prerequisites = state.prerequisites; + const state: EvalState = { + delivered: false, + onError: (err) => { + if (state.delivered) { + // The result has already been handed to the caller, so this exception came from the + // caller's callback or from work that ran after delivery, not from reading the flag. + // Let it propagate as it did before. + throw err; } - res.events = state.events; - cb(res); + deliverResult( + state, + EvalResult.forError( + ErrorKinds.MalformedFlag, + `Flag "${flag.key}" has a malformed definition, or references a malformed` + + ` prerequisite or segment: ${errorMessage(err)}`, + ), + cb, + ); }, - true, - eventFactory, - ); + }; + // Reading a malformed definition throws, and most of an evaluation runs synchronously, so + // this guard turns those exceptions into an error result. Steps that resume after a store + // callback or a promise are guarded where they resume. + guarded(state, () => { + this._evaluateInternal( + flag, + context, + state, + [], + (res) => deliverResult(state, res, cb), + true, + eventFactory, + ); + }); } /** @@ -250,50 +313,55 @@ export default class Evaluator { return; } const updatedVisitedFlags = [...visitedFlags, prereq.key]; - this._queries.getFlag(prereq.key, (prereqFlag) => { - if (!prereqFlag) { - prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); - iterCb(false); - return; - } + this._queries.getFlag(prereq.key, (prereqFlag) => + // The store may call back asynchronously, after the try/catch around the top-level + // evaluation has left the stack, so the continuation is guarded here. + guarded(state, () => { + if (!prereqFlag) { + prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); + iterCb(false); + return; + } - this._evaluateInternal( - prereqFlag, - context, - state, - updatedVisitedFlags, - (res) => { - state.events ??= []; - if (topLevel) { - state.prerequisites ??= []; - - state.prerequisites.push(prereqFlag.key); - } - if (eventFactory) { - state.events.push( - eventFactory.evalEventServer(prereqFlag, context, res.detail, null, flag), - ); - } + this._evaluateInternal( + prereqFlag, + context, + state, + updatedVisitedFlags, + (res) => { + state.events ??= []; + if (topLevel) { + state.prerequisites ??= []; + + state.prerequisites.push(prereqFlag.key); + } + if (eventFactory) { + state.events.push( + eventFactory.evalEventServer(prereqFlag, context, res.detail, null, flag), + ); + } - if (res.isError) { - prereqResult = res; - return iterCb(false); - } + if (res.isError) { + prereqResult = res; + return iterCb(false); + } - if (res.isOff || res.detail.variationIndex !== prereq.variation) { - prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); - return iterCb(false); - } - return iterCb(true); - }, - false, // topLevel false evaluating the prerequisite. - eventFactory, - ); - }); + if (res.isOff || res.detail.variationIndex !== prereq.variation) { + prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); + return iterCb(false); + } + return iterCb(true); + }, + false, // topLevel false evaluating the prerequisite. + eventFactory, + ); + }), + ); }, () => { cb(prereqResult); }, + state.onError, ); } @@ -323,6 +391,7 @@ export default class Evaluator { }); }, () => cb(ruleResult), + state.onError, ); } @@ -338,30 +407,33 @@ export default class Evaluator { firstSeriesAsync( clause.values, (value, _index, iterCb) => { - this._queries.getSegment(value, (segment) => { - if (segment) { - if (segmentsVisited.includes(segment.key)) { - errorResult = EvalResult.forError( - ErrorKinds.MalformedFlag, - `Segment rule referencing segment ${segment.key} caused a circular reference. ` + - 'This is probably a temporary condition due to an incomplete update', - ); - // There was an error, so stop checking further segments. - iterCb(true); - return; - } - - const newVisited = [...segmentsVisited, segment?.key]; - this.segmentMatchContext(segment, context, state, newVisited, (res) => { - if (res.error) { - errorResult = res.result; + this._queries.getSegment(value, (segment) => + // The store may call back asynchronously; see _checkPrerequisites. + guarded(state, () => { + if (segment) { + if (segmentsVisited.includes(segment.key)) { + errorResult = EvalResult.forError( + ErrorKinds.MalformedFlag, + `Segment rule referencing segment ${segment.key} caused a circular reference. ` + + 'This is probably a temporary condition due to an incomplete update', + ); + // There was an error, so stop checking further segments. + iterCb(true); + return; } - iterCb(res.error || res.isMatch); - }); - } else { - iterCb(false); - } - }); + + const newVisited = [...segmentsVisited, segment?.key]; + this.segmentMatchContext(segment, context, state, newVisited, (res) => { + if (res.error) { + errorResult = res.result; + } + iterCb(res.error || res.isMatch); + }); + } else { + iterCb(false); + } + }), + ); }, (match) => { if (errorResult) { @@ -370,6 +442,7 @@ export default class Evaluator { return cb(makeMatch(maybeNegate(clause, match))); }, + state.onError, ); return; } @@ -429,6 +502,7 @@ export default class Evaluator { } return cb(undefined); }, + state.onError, ); } @@ -556,6 +630,7 @@ export default class Evaluator { return cb(makeMatch(false)); }, + state.onError, ); } @@ -590,6 +665,7 @@ export default class Evaluator { return cb(makeMatch(matched)); }, + state.onError, ); } @@ -637,32 +713,40 @@ export default class Evaluator { segment, context, state, - ).then(cb); + ) + .then(cb) + .catch(state.onError); return; } - this._queries.getBigSegmentsMembership(keyForBigSegment).then((result) => { - state.bigSegmentsMembership = state.bigSegmentsMembership || {}; - if (result) { - const [membership, status] = result; - state.bigSegmentsMembership[keyForBigSegment] = membership; - state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( - state.bigSegmentsStatus, - status as BigSegmentStoreStatusString, - ); - } else { - state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( - state.bigSegmentsStatus, - 'NOT_CONFIGURED', + this._queries + .getBigSegmentsMembership(keyForBigSegment) + .then((result) => { + state.bigSegmentsMembership = state.bigSegmentsMembership || {}; + if (result) { + const [membership, status] = result; + state.bigSegmentsMembership[keyForBigSegment] = membership; + state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( + state.bigSegmentsStatus, + status as BigSegmentStoreStatusString, + ); + } else { + state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( + state.bigSegmentsStatus, + 'NOT_CONFIGURED', + ); + } + return this.bigSegmentMatchContext( + state.bigSegmentsMembership[keyForBigSegment], + segment, + context, + state, ); - } - this.bigSegmentMatchContext( - state.bigSegmentsMembership[keyForBigSegment], - segment, - context, - state, - ).then(cb); - }); + }) + .then(cb) + // The rest of the evaluation runs inside these continuations, so an exception there would + // otherwise be an unhandled rejection and the result would never be delivered. + .catch(state.onError); } bigSegmentMatchContext( diff --git a/packages/shared/sdk-server/src/evaluation/collection.ts b/packages/shared/sdk-server/src/evaluation/collection.ts index 709acde953..cc20b3f699 100644 --- a/packages/shared/sdk-server/src/evaluation/collection.ts +++ b/packages/shared/sdk-server/src/evaluation/collection.ts @@ -18,6 +18,13 @@ export function firstResult( return res; } +/** + * Receives an exception thrown by a check or completion callback during asynchronous + * iteration. Iteration stops and the completion callback is not called, so the caller + * can report the failure instead of waiting for a result that would never arrive. + */ +export type IterationErrorHandler = (err: unknown) => void; + const ITERATION_RECURSION_LIMIT = 50; function seriesAsync( @@ -26,35 +33,51 @@ function seriesAsync( all: boolean, index: number, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - if (!collection) { - cb(false); - return; - } - if (index < collection?.length) { + try { + if (!collection) { + cb(false); + return; + } + if (!Array.isArray(collection)) { + // A string would be indexed character by character and an object would be treated as + // an empty collection, so a value that is not an array is reported instead of iterated. + throw new TypeError(`Expected an array but received ${typeof collection}`); + } + if (index >= collection.length) { + cb(all); + return; + } check(collection[index], index, (res) => { - if (all) { - if (!res) { - cb(false); + // The check may call back asynchronously, after the try/catch around this function has + // left the stack, so the continuation is guarded on its own. + try { + if (all) { + if (!res) { + cb(false); + return; + } + } else if (res) { + cb(true); return; } - } else if (res) { - cb(true); - return; - } - if (collection.length > ITERATION_RECURSION_LIMIT) { - // When we hit the recursion limit we defer execution - // by using a resolved promise. This is similar to using setImmediate - // but more portable. - Promise.resolve().then(() => { - seriesAsync(collection, check, all, index + 1, cb); - }); - } else { - seriesAsync(collection, check, all, index + 1, cb); + if (collection.length > ITERATION_RECURSION_LIMIT) { + // When we hit the recursion limit we defer execution + // by using a resolved promise. This is similar to using setImmediate + // but more portable. + Promise.resolve().then(() => { + seriesAsync(collection, check, all, index + 1, cb, onError); + }); + } else { + seriesAsync(collection, check, all, index + 1, cb, onError); + } + } catch (err) { + onError(err); } }); - } else { - cb(all); + } catch (err) { + onError(err); } } @@ -63,13 +86,15 @@ function seriesAsync( * @param collection The collection to iterate. * @param check The check to perform for each item in the container. * @param cb Called with true if all items pass the check. + * @param onError Called instead of cb if a check or cb throws. */ export function allSeriesAsync( collection: T[] | undefined, check: (val: T, index: number, cb: (res: boolean) => void) => void, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - seriesAsync(collection, check, true, 0, cb); + seriesAsync(collection, check, true, 0, cb, onError); } /** @@ -78,13 +103,15 @@ export function allSeriesAsync( * @param check The check to perform for each item in the container. * @param cb called with true on the first item that passes the check. False * means no items passed the check. + * @param onError Called instead of cb if a check or cb throws. */ export function firstSeriesAsync( collection: T[] | undefined, check: (val: T, index: number, cb: (res: boolean) => void) => void, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - seriesAsync(collection, check, false, 0, cb); + seriesAsync(collection, check, false, 0, cb, onError); } /** @@ -95,25 +122,35 @@ export function firstSeriesAsync( * @param cb Callback executed when all items have been checked. The callback * will be called with true if each item resulted in true, otherwise it will * be called with false. + * @param onError Called instead of cb if a check or cb throws. */ export function allAsync( collection: T[] | undefined, check: (val: T, cb: (res: boolean) => void) => void, cb: (res: boolean | null | undefined) => void, + onError: IterationErrorHandler, ): void { - if (!collection) { - cb(false); - return; - } + try { + if (!collection) { + cb(false); + return; + } - Promise.all( - collection?.map( - (item) => - new Promise((resolve) => { - check(item, resolve); - }), - ), - ).then((results) => { - cb(results.every((success) => success)); - }); + Promise.all( + collection.map( + (item) => + new Promise((resolve) => { + check(item, resolve); + }), + ), + ) + .then((results) => { + cb(results.every((success) => success)); + }) + // A throw inside a check rejects its promise, and a throw inside cb rejects the chain. + // Without this handler either one would be an unhandled rejection and cb would never run. + .catch(onError); + } catch (err) { + onError(err); + } } diff --git a/packages/shared/sdk-server/src/evaluation/variations.ts b/packages/shared/sdk-server/src/evaluation/variations.ts index 9095162308..842a3ef200 100644 --- a/packages/shared/sdk-server/src/evaluation/variations.ts +++ b/packages/shared/sdk-server/src/evaluation/variations.ts @@ -23,6 +23,11 @@ const KEY_ATTR_REF = new AttributeReference('key'); * @internal */ export function getVariation(flag: Flag, index: number, reason: LDEvaluationReason): EvalResult { + if (!Array.isArray(flag.variations)) { + // A flag from a custom store or a data file may have no variations, or a string in their + // place, which would otherwise be read as an array of characters. + return EvalResult.forError(ErrorKinds.MalformedFlag, 'Flag variations are not an array'); + } if (TypeValidators.Number.is(index) && index >= 0 && index < flag.variations.length) { return EvalResult.forSuccess(flag.variations[index], reason, index); }