From 4fc26212895cad5443999eec9de2bcd88f913b7c Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 13:28:02 -0400 Subject: [PATCH 1/3] fix: add defensive cycle guard to prerequisite evaluation Adds an ancestor-set (current-path) cycle guard to the recursive prerequisite walk in _variationInternal, bringing the shared JS client-side SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have detected and gracefully handled cyclic prerequisite graphs for years. The LaunchDarkly service validates prerequisite graphs on mutation and rejects any change that would produce a cycle, so under normal operation the SDK does not see a cyclic graph. This is defensive code for exceptional cases -- for example, delivery of updates out of order or a persisted state loaded from disk that predates a subsequent correction. The Set tracking ancestor keys is allocated lazily: variation calls on prereq-less flags (the common case) allocate zero collections. Once created, the set is shared for the rest of the walk via add-before-recurse / delete-after-recurse, guarded by try/finally so a recursive descent that throws cannot leave a stale ancestor entry visible to a sibling branch. When a cycle is detected the requested flag's cached value and reason are returned unchanged; only the recursive prerequisite event walk is affected. Also declares the client-prereq-cycle-detection capability on the browser SDK's contract-test service so the matching sdk-test-harness contract tests activate for @launchdarkly/js-client-sdk. Downstream wrappers (React, React Native, Electron, node-client, Vue) will declare the capability as part of separate follow-up PRs. --- .../entity/src/TestHarnessWebSocket.ts | 1 + .../__tests__/LDClientImpl.events.test.ts | 104 ++++++++++++++++++ .../shared/sdk-client/src/LDClientImpl.ts | 26 ++++- 3 files changed, 128 insertions(+), 3 deletions(-) diff --git a/packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts b/packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts index 6f82d0c5ea..07b7af0799 100644 --- a/packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts +++ b/packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts @@ -42,6 +42,7 @@ export default class TestHarnessWebSocket { 'anonymous-redaction', 'strongly-typed', 'client-prereq-events', + 'client-prereq-cycle-detection', 'client-per-context-summaries', 'track-hooks', ]; diff --git a/packages/shared/sdk-client/__tests__/LDClientImpl.events.test.ts b/packages/shared/sdk-client/__tests__/LDClientImpl.events.test.ts index e85379d917..4d47e06aa6 100644 --- a/packages/shared/sdk-client/__tests__/LDClientImpl.events.test.ts +++ b/packages/shared/sdk-client/__tests__/LDClientImpl.events.test.ts @@ -240,4 +240,108 @@ describe('sdk-client object', () => { }), ); }); + + describe('prerequisite cycles', () => { + // Cycle-detection tests exercise the ancestor-set cycle guard added to _variationInternal. + // Prior to that guard, any of these flag configurations would cause unbounded recursion during + // a variation() call. Each test constructs a cyclic prereq graph, drives it through identify(), + // evaluates one flag on the cycle, and asserts (a) the returned value equals the cached value + // and (b) the emitted feature events match exactly one recording per cycle-safe descent. + // + // The outer `beforeEach` captures a JSON snapshot of `defaultPutResponse` inside the stream mock, + // so mutations here would not otherwise propagate to the SDK's flag store. `installCycleFlags` + // both mutates `defaultPutResponse` and rebuilds the mock so the SDK loads the updated set on + // the next identify(). + const makeFlag = ( + prerequisites?: string[], + ): { + value: boolean; + variation: number; + version: number; + reason: { kind: string }; + trackEvents: boolean; + prerequisites?: string[]; + } => ({ + value: true, + variation: 0, + version: 1, + reason: { kind: 'FALLTHROUGH' }, + trackEvents: true, + ...(prerequisites ? { prerequisites } : {}), + }); + const installCycleFlags = (flags: Flags) => { + Object.assign(defaultPutResponse, flags); + mockPlatform.requests.createEventSource.mockImplementation( + (streamUri: string = '', options: any = {}) => { + mockEventSource = new MockEventSource(streamUri, options); + mockEventSource.simulateEvents('put', [{ data: JSON.stringify(defaultPutResponse) }]); + return mockEventSource; + }, + ); + }; + const featureEventKeysAfterIdentify = () => + mockedSendEvent.mock.calls + .map((call) => call[0]) + .filter((event) => event.kind === 'feature') + .map((event) => event.key); + + it('skips a self-loop prerequisite and returns the cached value', async () => { + installCycleFlags({ flagA: makeFlag(['flagA']) } as unknown as Flags); + await ldc.identify({ kind: 'user', key: 'bob' }); + expect(ldc.variation('flagA', false)).toBe(true); + // Only flagA emits a feature event; the self-prereq is cycle-skipped. + expect(featureEventKeysAfterIdentify()).toEqual(['flagA']); + }); + + it('handles a two-cycle evaluating A', async () => { + installCycleFlags({ + flagA: makeFlag(['flagB']), + flagB: makeFlag(['flagA']), + } as unknown as Flags); + await ldc.identify({ kind: 'user', key: 'bob' }); + expect(ldc.variation('flagA', false)).toBe(true); + // A -> B -> [A skipped]. Events (deepest-first): B (as prereq of A), then A. + expect(featureEventKeysAfterIdentify()).toEqual(['flagB', 'flagA']); + }); + + it('handles a two-cycle evaluating B', async () => { + installCycleFlags({ + flagA: makeFlag(['flagB']), + flagB: makeFlag(['flagA']), + } as unknown as Flags); + await ldc.identify({ kind: 'user', key: 'bob' }); + expect(ldc.variation('flagB', false)).toBe(true); + // Symmetric: same graph, entry from B. Events: A (as prereq of B), then B. + expect(featureEventKeysAfterIdentify()).toEqual(['flagA', 'flagB']); + }); + + it('handles a three-cycle', async () => { + installCycleFlags({ + flagA: makeFlag(['flagB']), + flagB: makeFlag(['flagC']), + flagC: makeFlag(['flagA']), + } as unknown as Flags); + await ldc.identify({ kind: 'user', key: 'bob' }); + expect(ldc.variation('flagA', false)).toBe(true); + // A -> B -> C -> [A skipped]. Events emitted deepest-first: C, B, A. + expect(featureEventKeysAfterIdentify()).toEqual(['flagC', 'flagB', 'flagA']); + }); + + it('emits the shared descendant once per path in a non-cyclic diamond', async () => { + // Diamond: A -> [B, C], B -> [D], C -> [D]. Not a cycle. Ancestor-set (current-path) semantics + // must let D be reached on each of the two independent paths -- so D emits twice. A naive + // "visited across the whole walk" implementation would drop the second D event; this case + // guards against that regression. + installCycleFlags({ + flagA: makeFlag(['flagB', 'flagC']), + flagB: makeFlag(['flagD']), + flagC: makeFlag(['flagD']), + flagD: makeFlag(), + } as unknown as Flags); + await ldc.identify({ kind: 'user', key: 'bob' }); + expect(ldc.variation('flagA', false)).toBe(true); + // Events (deepest-first per path): D (via B), B, D (via C), C, A. D appears twice. + expect(featureEventKeysAfterIdentify()).toEqual(['flagD', 'flagB', 'flagD', 'flagC', 'flagA']); + }); + }); }); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 8594fa26b2..ce15ba3f5c 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -618,6 +618,7 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { defaultValue: any, eventFactory: EventFactory, typeChecker?: (value: any) => [boolean, string], + visited?: Set, ): LDEvaluationDetail { // 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 @@ -679,9 +680,28 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { successDetail.value = defaultValue; } - prerequisites?.forEach((prereqKey) => { - this._variationInternal(prereqKey, undefined, this._eventFactoryDefault); - }); + if (prerequisites && prerequisites.length > 0) { + // Recurse on prerequisites to emulate prereq evaluations occurring with desirable side effects + // such as events for prereqs. + // + // `visited` tracks the chain of prerequisite dependencies from the top-level evaluation to + // (but not including) the current flag. It is allocated lazily: variation calls on + // prereq-less flags (the common case) allocate zero collections. + const ancestors = visited ?? new Set(); + ancestors.add(flagKey); + try { + for (const prereqKey of prerequisites) { + if (ancestors.has(prereqKey)) { + // Cyclic edge: skip descent, continue with remaining prerequisites at this level. + // The requested flag's value and reason are unaffected. + continue; + } + this._variationInternal(prereqKey, undefined, this._eventFactoryDefault, undefined, ancestors); + } + } finally { + ancestors.delete(flagKey); + } + } if (hasContext) { this._eventProcessor?.sendEvent( eventFactory.evalEventClient( From a4821439d65919e1bbf40dace97b68ce93115957 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 13:41:57 -0400 Subject: [PATCH 2/3] chore: bump sdk-client size limit to 39300 for cycle-guard bytes --- .github/workflows/sdk-client.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sdk-client.yml b/.github/workflows/sdk-client.yml index c968a697f6..4b366d43cd 100644 --- a/.github/workflows/sdk-client.yml +++ b/.github/workflows/sdk-client.yml @@ -32,4 +32,4 @@ jobs: target_file: 'packages/shared/sdk-client/dist/esm/index.mjs' package_name: '@launchdarkly/js-client-sdk-common' pr_number: ${{ github.event.number }} - size_limit: 39000 + size_limit: 39300 From 324587e51ab6ea33fa1bfb599f3a597809b758b7 Mon Sep 17 00:00:00 2001 From: Todd Anderson <127344469+tanderson-ld@users.noreply.github.com> Date: Tue, 21 Jul 2026 14:36:05 -0400 Subject: [PATCH 3/3] Update packages/shared/sdk-client/src/LDClientImpl.ts Co-authored-by: joker23 <2494686+joker23@users.noreply.github.com> --- packages/shared/sdk-client/src/LDClientImpl.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index ce15ba3f5c..4ab7109f10 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -685,8 +685,7 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { // such as events for prereqs. // // `visited` tracks the chain of prerequisite dependencies from the top-level evaluation to - // (but not including) the current flag. It is allocated lazily: variation calls on - // prereq-less flags (the common case) allocate zero collections. + // (but not including) the current flag. const ancestors = visited ?? new Set(); ancestors.add(flagKey); try {