Skip to content

fix: add defensive cycle guard to prerequisite evaluation#317

Merged
tanderson-ld merged 3 commits into
mainfrom
ta/SDK-2708-cycle-detection
Jul 23, 2026
Merged

fix: add defensive cycle guard to prerequisite evaluation#317
tanderson-ld merged 3 commits into
mainfrom
ta/SDK-2708-cycle-detection

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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. 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, 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.


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.

Reviewed by Cursor Bugbot for commit 81fbdce. Bugbot is set up for automated code reviews on this repo. Configure here.

Adds an ancestor-set (current-path) cycle guard to the recursive
prerequisite walk in EvaluateInternal, bringing the .NET client SDK's
behavior into line with the LaunchDarkly server SDK evaluators, which
have detected and gracefully handled cyclic prerequisite graphs for
years.

The HashSet tracking ancestor keys is allocated lazily: variation
calls on prereq-less flags allocate zero collections. Once created,
the set is shared for the rest of the walk via
add-before-recurse / remove-after-recurse in a 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.
@tanderson-ld
tanderson-ld marked this pull request as ready for review July 22, 2026 14:30
@tanderson-ld
tanderson-ld requested a review from a team as a code owner July 22, 2026 14:30
@tanderson-ld
tanderson-ld merged commit babb87a into main Jul 23, 2026
18 of 19 checks passed
@tanderson-ld
tanderson-ld deleted the ta/SDK-2708-cycle-detection branch July 23, 2026 13:31
tanderson-ld pushed a commit that referenced this pull request Jul 23, 2026
🤖 I have created a release *beep* *boop*
---


##
[5.9.2](LaunchDarkly.ClientSdk-v5.9.1...LaunchDarkly.ClientSdk-v5.9.2)
(2026-07-23)


### Bug Fixes

* add defensive cycle guard to prerequisite evaluation
([#317](#317))
([babb87a](babb87a))

---
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**
> Mechanical version and documentation updates only; no runtime code in
the diff.
> 
> **Overview**
> Release Please bumps **LaunchDarkly.ClientSdk** from **5.9.1** to
**5.9.2** across the manifest, `LaunchDarkly.ClientSdk.csproj`,
`CHANGELOG.md`, and `PROVENANCE.md`.
> 
> The **5.9.2** changelog entry documents a bug fix (already on main via
[#317](#317)): a
**defensive cycle guard** during **prerequisite flag evaluation** so
cyclic prerequisite graphs cannot recurse indefinitely. This PR does not
change evaluation code—only release metadata for publishing that fix.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
e2ee74c. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants