fix: add defensive cycle guard to prerequisite evaluation#1816
fix: add defensive cycle guard to prerequisite evaluation#1816tanderson-ld wants to merge 4 commits into
Conversation
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.
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
| 'anonymous-redaction', | ||
| 'strongly-typed', | ||
| 'client-prereq-events', | ||
| 'client-prereq-cycle-detection', |
There was a problem hiding this comment.
could we try to enable this capability on react sdk as well (since it is just a wrapper)
There was a problem hiding this comment.
Yes, planning to in the story for React
Co-authored-by: joker23 <2494686+joker23@users.noreply.github.com>
Summary
Adds defensive cycle detection to the recursive prerequisite walk in
LDClientImpl._variationInternal, bringing the JS client-side SDKs' behavior into line with the LaunchDarkly server SDK evaluators.The fix
_variationInternalnow threads a lazily-allocatedSet<string>through the recursion carrying the flag keys on the current evaluation path. Before descending into a prerequisite, the walker checks whether that key is already on the path; if so, it skips that edge and continues with remaining prerequisites at the same level.Set<string>is created only when the walker descends into a prerequisite for the first time in a call, then reused for the rest of the walk.add/try/finally delete: no per-descent copy.ancestors.add(flagKey)before recursing,ancestors.delete(flagKey)in thefinally— the invariant "the set contains exactly the current path" holds even under early exits.A -> [B, C], B -> [D], C -> [D]) is not a cycle; each path should emit its own prerequisite event. A global visited-set would silently drop the second event; the ancestor-set pattern correctly emits D twice. There is a unit test guarding this.The optional
visited?: Set<string>parameter is trailing, so the four existing entry points into_variationInternal(variation,variationDetail, and the two typed-eval callsites) work unchanged — none of the publicvariation/variationDetail/boolVariation/intVariation/stringVariation/doubleVariation/jsonVariationmethods need to change.Caller-visible behavior on a cycle
The requested flag returns its cached value and reason unchanged. A client-side prerequisite cycle is not surfaced as
MALFORMED_FLAGand does not fall back to the caller-provided default value. This differs from server-side behavior — the client already holds an authoritative pre-evaluated result from the server; only the ancillary event walk is affected by the cycle.Files changed
packages/shared/sdk-client/src/LDClientImpl.ts— cycle guard in_variationInternal. Existing callers unchanged.packages/shared/sdk-client/__tests__/LDClientImpl.events.test.ts— 5 new Jest tests grouped under a nesteddescribe('prerequisite cycles', ...): self-loop, two-cycle (evaluating each end), three-cycle, and a non-cyclic diamond that asserts the shared descendant emits an event for each path (the ancestor-set regression fence).packages/sdk/browser/contract-tests/entity/src/TestHarnessWebSocket.ts— declaresclient-prereq-cycle-detectionso the matching contract tests in sdk-test-harness v2.38.0+ activate for the browser SDK. Downstream wrapper packages (React, React Native, Electron, node-client, Vue) will declare the capability in follow-up PRs against SDK-2711 through SDK-2715.Verification
yarn workspace @launchdarkly/js-client-sdk-common run test→ 834 tests, 0 failures. The 5 new cycle-detection tests are visible under theprerequisite cyclesgroup and passing.events/prerequisite events handle cyclesandevents/summary events/prerequisites/handles cycles) fire and pass — 15 subtests total between them.Changelog
Note
Medium Risk
Touches the core variation/prerequisite event path used on every flag evaluation, but behavior is a defensive guard with explicit tests and no public API changes.
Overview
Adds ancestor-set cycle detection to prerequisite recursion in
LDClientImpl._variationInternal, matching server SDK behavior and preventing unbounded recursion when flag graphs loop. Cyclic edges are skipped during the prereq walk; the evaluated flag still returns its cached value and reason unchanged.Prereq traversal now uses a lazily allocated
Seton the current path (add/finallydelete) so diamond graphs still emit prerequisite feature events once per independent path, not once globally.Adds five unit tests under
prerequisite cycles(self-loop, 2- and 3-cycles, diamond regression), advertisesclient-prereq-cycle-detectionin the browser contract harness, and bumps the shared client bundle size limit from 39,000 to 39,300 bytes.Reviewed by Cursor Bugbot for commit 324587e. Bugbot is set up for automated code reviews on this repo. Configure here.