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 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..4ab7109f10 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,27 @@ 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. + 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(