fix: add defensive cycle guard to prerequisite evaluation#329
Merged
Conversation
Adds an ancestor-set (current-path) cycle guard to the recursive prerequisite walk in _variationInternal, bringing the Flutter client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have detected and gracefully handled cyclic prerequisite graphs for years. 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 / remove-after-recurse, guarded by try/finally so an exception below 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 so the matching sdk-test-harness contract tests activate for this SDK.
This was referenced Jul 21, 2026
tanderson-ld
marked this pull request as ready for review
July 22, 2026 13:37
joker23
approved these changes
Jul 22, 2026
tanderson-ld
pushed a commit
that referenced
this pull request
Jul 22, 2026
🤖 I have created a release *beep* *boop* --- ## [1.14.2](launchdarkly_common_client-v1.14.1...launchdarkly_common_client-v1.14.2) (2026-07-22) ### Bug Fixes * add defensive cycle guard to prerequisite evaluation ([#329](#329)) ([167e8f0](167e8f0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Version and changelog/manifest updates only; the behavioral fix is already merged elsewhere. > > **Overview** > **Release-only PR** that publishes `launchdarkly_common_client` **1.14.2** via Release Please. > > It bumps the package version in `pubspec.yaml`, updates `.release-please-manifest.json`, and adds a **1.14.2** changelog entry. The noted fix is a **defensive cycle guard during prerequisite flag evaluation** ([#329](https://github.com/launchdarkly/flutter-client-sdk/issues/329))—no application code changes appear in this diff. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2560d02. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
tanderson-ld
added a commit
to launchdarkly/dotnet-core
that referenced
this pull request
Jul 23, 2026
## Summary Adds defensive cycle detection to the recursive prerequisite walk in `LdClient.EvaluateInternal`, bringing the .NET client SDK's behavior into line with the LaunchDarkly server SDK evaluators. Tracker: [SDK-2708](https://launchdarkly.atlassian.net/browse/SDK-2708). Parent: [SDK-2695](https://launchdarkly.atlassian.net/browse/SDK-2695). Spec change: [sdk-specs#246](launchdarkly/sdk-specs#246) (CSPE 1.2.5, 1.2.5.1, 1.2.5.2). Contract-test coverage: [sdk-test-harness#384](launchdarkly/sdk-test-harness#384) (merged in v2.38.0). Companion PRs: [android-client-sdk#377](launchdarkly/android-client-sdk#377), [ios-client-sdk#512](launchdarkly/ios-client-sdk#512), [js-core#1816](launchdarkly/js-core#1816), [flutter-client-sdk#329](launchdarkly/flutter-client-sdk#329). ## Background The LaunchDarkly service validates prerequisite graphs on every mutation and rejects any change that would produce a cycle, so under normal operation the SDK does not see a cyclic prerequisite graph. Server-side SDK evaluators nonetheless carry defensive cycle detection for exceptional cases — for example, delivery of updates out of order or a persisted state loaded from disk that predates a subsequent correction. Note that the .NET **server** SDK evaluator already carries this guard (`PrereqFlagKeyStack` in `Evaluator.cs`); this PR extends the same posture to the client SDK. ## The fix `EvaluateInternal<T>` now threads a lazily-allocated `HashSet<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. - **Lazy allocation**: variation calls on prereq-less flags (the common case) allocate zero collections. The `HashSet<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. - **Mutable `Add` / `finally Remove`**: no per-descent copy. `ancestors.Add(featureKey)` before recursing, `ancestors.Remove(featureKey)` in a `finally` — the invariant "the set contains exactly the current path" holds even under exceptions. - **Ancestor-set (current-path) semantics** — this is deliberate. A prerequisite reached via multiple non-cyclic paths (a diamond `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 trailing parameter `HashSet<string> visited = null` is added to `EvaluateInternal<T>`, so all existing callers (the public `BoolVariation` / `IntVariation` / `StringVariation` / `DoubleVariation` / `JsonVariation` and their `*Detail` counterparts) work unchanged. ## 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_FLAG` and 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 - `pkgs/sdk/client/src/LdClient.cs` — cycle guard in `EvaluateInternal<T>`. - `pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientEventTests.cs` — 5 new xUnit tests: 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). - `pkgs/sdk/client/contract-tests/TestService.cs` — declares `client-prereq-cycle-detection` so the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK. ## Verification - **Unit tests**: `dotnet test -f net8.0 --filter "Prerequisite|Cycle|Diamond"` → 6 passed, 0 failures (the original prereq event test plus 5 new cycle-detection tests). ## Changelog > Updated prerequisite evaluation event emission to match server SDK behavior for cyclic prerequisite graphs. [SDK-2708]: https://launchdarkly.atlassian.net/browse/SDK-2708?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDK-2695]: https://launchdarkly.atlassian.net/browse/SDK-2695?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches core flag evaluation and analytics event emission for prerequisite walks; behavior change is defensive but affects event ordering/counts on malformed graphs. > > **Overview** > Adds **defensive cycle detection** to the recursive prerequisite walk in `LdClient.EvaluateInternal`, aligning client SDK behavior with server evaluators and sdk-spec CSPE 1.2.5. > > Before descending into each prerequisite, the walker tracks flag keys on the **current path** in a lazily allocated `HashSet<string>` (add before recurse, remove in `finally`). If a prerequisite key is already on that path, that edge is skipped; the requested flag still returns its **cached value and reason** unchanged. > > **Ancestor-set semantics** (not a global visited set) preserve correct analytics for diamond graphs: a shared prerequisite reached via separate branches still emits one evaluation event per path. > > Contract tests declare capability `client-prereq-cycle-detection`. Five xUnit tests cover self-loops, 2- and 3-cycles, and the diamond regression case. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 81fbdce. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
tanderson-ld
added a commit
to launchdarkly/roku-client-sdk
that referenced
this pull request
Jul 23, 2026
## Summary Adds defensive cycle detection to the recursive prerequisite walk in `variationDetail` (`LaunchDarklyClient.brs`), bringing the Roku client SDK's behavior into line with the LaunchDarkly server SDK evaluators. Tracker: [SDK-2710](https://launchdarkly.atlassian.net/browse/SDK-2710). Parent: [SDK-2695](https://launchdarkly.atlassian.net/browse/SDK-2695). Spec change: [sdk-specs#246](launchdarkly/sdk-specs#246) (CSPE 1.2.5, 1.2.5.1, 1.2.5.2). Contract-test coverage: [sdk-test-harness#384](launchdarkly/sdk-test-harness#384) (merged in v2.38.0). Companion PRs: [android-client-sdk#377](launchdarkly/android-client-sdk#377), [ios-client-sdk#512](launchdarkly/ios-client-sdk#512), [js-core#1816](launchdarkly/js-core#1816), [flutter-client-sdk#329](launchdarkly/flutter-client-sdk#329), [dotnet-core#317](launchdarkly/dotnet-core#317), [cpp-sdks#582](launchdarkly/cpp-sdks#582). ## Background The LaunchDarkly service validates prerequisite graphs on every mutation and rejects any change that would produce a cycle, so under normal operation the SDK does not see a cyclic prerequisite graph. Server-side SDK evaluators nonetheless carry defensive cycle detection for exceptional cases — for example, delivery of updates out of order or a persisted state loaded from disk that predates a subsequent correction. This PR extends the same defensive posture to the Roku client SDK. ## The fix `variationDetail` now threads a lazily-allocated associative array 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. - **Lazy allocation**: variation calls on prereq-less flags (the common case) allocate no associative array. The map 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. - **Mutable add/delete around the loop**: no per-descent copy. The current flag's key is inserted before the loop and removed after, so the invariant "the map contains exactly the current path" holds across sibling descents. - **Ancestor-set (current-path) semantics** — this is deliberate. A prerequisite reached via multiple non-cyclic paths (a diamond `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 trailing parameter `launchDarklyParamVisited=invalid` is added to `variationDetail`, so existing callers (`boolVariation`, `intVariationDetail`, `stringVariationDetail`, `doubleVariationDetail`, `jsonVariationDetail`, and the untyped `variation`) work unchanged. ## 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_FLAG` and 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 - `rawsrc/LaunchDarklyClient.brs` — cycle guard in `variationDetail`. - `src/test/source/tests/Test__Client.brs` — 5 new unit tests: 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). - `src/contract-tests/components/HttpServerTask.brs` — declares `client-prereq-cycle-detection` so the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK. ## Verification - **Unit tests**: 5 new `TestCase__Client_CycleDetection_*` cases follow the existing `TestCase__Client_*` pattern (seed `client.private.store`, call `client.variation()`, flush and inspect the event queue). Local execution requires a Roku device (`ukor test`); CI provides the actual test run. - **Contract tests**: local run requires deployment to a Roku device; CI provides the run against the released harness. The new suite (`client-prereq-cycle-detection`) will activate as soon as CI picks up harness v2.38.0+. ## Changelog > Updated prerequisite evaluation event emission to match server SDK behavior for cyclic prerequisite graphs. [SDK-2710]: https://launchdarkly.atlassian.net/browse/SDK-2710?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [SDK-2695]: https://launchdarkly.atlassian.net/browse/SDK-2695?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches the prerequisite event-walk in core flag evaluation, but behavior for normal graphs is preserved and cycles only skip redundant descent; coverage is strong via new unit and contract capability tests. > > **Overview** > Adds **defensive cycle detection** when the Roku client walks flag **prerequisites** to emit evaluation events, matching other LaunchDarkly SDKs and avoiding unbounded recursion on malformed graphs. > > Core `variationDetail` logic moves into **`LaunchDarklyClientSharedPrivateFunctions`** and is merged into both the standard client and SceneGraph client. The recursive walk threads a **current-path ancestor set** (`launchDarklyParamVisited`): before descending into a prerequisite, if that flag is already on the path the edge is skipped; diamonds still emit events per path (not a global visited set). **Public variation APIs are unchanged**—they delegate to the private implementation with `m.status`. > > Contract tests advertise capability **`client-prereq-cycle-detection`**. **Five unit tests** cover self-loop, 2- and 3-cycles, and a diamond graph (cached values unchanged; feature event order asserted). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1e59528. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds defensive cycle detection to the recursive prerequisite walk in
LDCommonClient._variationInternal, bringing the Flutter client SDK's behavior into line with the LaunchDarkly server SDK evaluators.Tracker: SDK-2705. Parent: SDK-2695. Spec change: sdk-specs#246 (CSPE 1.2.5, 1.2.5.1, 1.2.5.2). Contract-test coverage: sdk-test-harness#384 (merged in v2.38.0). Companion PRs: android-client-sdk#377, ios-client-sdk#512, js-core#1816.
Background
The LaunchDarkly service validates prerequisite graphs on every mutation and rejects any change that would produce a cycle, so under normal operation the SDK does not see a cyclic prerequisite graph. Server-side SDK evaluators nonetheless carry defensive cycle detection for exceptional cases — for example, delivery of updates out of order or a persisted state loaded from disk that predates a subsequent correction. This PR extends the same defensive posture to the Flutter client SDK.
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/finally remove: no per-descent copy.ancestors.add(flagKey)before recursing,ancestors.remove(flagKey)in afinally— 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 named parameter
visitedis added last, so all existing callers (the seven typed and untypedvariation/variationDetailentry points) work unchanged.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/common_client/lib/src/ld_common_client.dart— cycle guard in_variationInternal.packages/common_client/test/ld_dart_client_test.dart— 5 new tests under the existinggiven mock flag data with prerequisitesgroup: 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).apps/flutter_client_contract_test_service/bin/contract_test_service.dart— declaresclient-prereq-cycle-detectionso the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK.Verification
dart test test/ld_dart_client_test.dart→ 31 tests, 0 failures. The 5 new cycle-detection tests all pass.Changelog
Note
Medium Risk
Changes flag evaluation’s prerequisite recursion and analytics event ordering, but behavior is narrowly scoped to abnormal cyclic graphs and is covered by new unit and contract tests.
Overview
Adds defensive cycle detection to the recursive prerequisite walk in
LDCommonClient._variationInternal, matching server SDK behavior when a cyclic prereq graph appears (e.g. stale persisted state).The walker threads a lazily allocated ancestor set (current path only): before recursing into a prerequisite it skips edges whose key is already on the path, then continues with other prerequisites. The evaluated flag still returns its cached value and reason; only the ancillary prereq event walk is affected. Diamond graphs still emit one prereq event per independent path (not treated as cycles).
Adds five unit tests (self-loop, 2- and 3-cycles, diamond regression) and advertises the
client-prereq-cycle-detectioncontract-test capability on the Flutter contract test service.Reviewed by Cursor Bugbot for commit 7ea0310. Bugbot is set up for automated code reviews on this repo. Configure here.