fix: add defensive cycle guard to prerequisite evaluation by tanderson-ld · Pull Request #582 · launchdarkly/cpp-sdks

tanderson-ld · 2026-07-21T18:18:22Z

Summary

Adds defensive cycle detection to the recursive prerequisite walk in ClientImpl::VariationInternal, bringing the C++ client SDK's behavior into line with the LaunchDarkly server SDK evaluators.

Tracker: SDK-2709. 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, dotnet-core#317.

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 C++ client SDK.

The fix

VariationInternal<T> now threads a lazily-allocated std::unordered_set<std::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 std::unordered_set 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 insert / catch-and-rethrow erase: no per-descent copy. ancestors->insert(key) before recursing; if a nested call throws, the exception handler restores ancestors to its pre-descent state before rethrowing. This preserves the invariant "the set contains exactly the current path" 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. The contract-test suite has a case guarding this.

The optional trailing parameter std::unordered_set<std::string>* visited = nullptr is added to VariationInternal<T>, so all existing callers (the public BoolVariation / IntVariation / StringVariation / DoubleVariation / JsonVariation and their *Detail counterparts) work unchanged. The recursive call now goes directly to VariationInternal<Value> rather than through JsonVariation, so the visited pointer can be threaded through — behavior is otherwise identical.

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

libs/client-sdk/src/client_impl.hpp — added std::unordered_set<std::string>* optional parameter to VariationInternal<T> declaration, plus the <string> / <unordered_set> includes.
libs/client-sdk/src/client_impl.cpp — cycle guard implementation in VariationInternal<T>.
contract-tests/client-contract-tests/src/main.cpp — declares client-prereq-cycle-detection capability so the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK.

Verification

Build: cmake --build build --target launchdarkly-cpp-client → clean, no warnings.
Contract tests locally against harness v2.38.1: client-tests binary + sdk-test-harness v2.38.1 → 876 total, 12 skipped, 864 ran, all passed. Both new suites (events/prerequisite events handle cycles and events/summary events/prerequisites/handles cycles) fire all 15 subtests including the deep-chain non-cyclic control, no failures.

The C++ client SDK does not have existing unit tests that exercise prerequisite walks (libs/client-sdk/tests/client_test.cpp currently covers only default-value paths with no flag data loaded). Rather than build new unit-test infrastructure for this PR, verification is provided by the contract-test suite, which now exercises the fix comprehensively.

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 prerequisites; behavior change is scoped to exceptional cyclic graphs and is covered by contract tests.

Overview
Adds defensive cycle detection to the recursive prerequisite walk in ClientImpl::VariationInternal, aligning client-side prerequisite event emission with server SDK behavior when a cyclic graph is encountered (e.g. stale persisted state).

VariationInternal now accepts an optional std::unordered_set<std::string>* tracking ancestors on the current path (not a global visited set, so diamond graphs still emit prerequisite events on each path). Before recursing into a prerequisite, if that key is already on the path the edge is skipped with a warning log; the requested flag’s cached value and reason are unchanged. Prerequisite recursion calls VariationInternal<Value> directly so the ancestor set can be threaded through.

Contract tests advertise the new client-prereq-cycle-detection capability in client-contract-tests.

^{Reviewed by Cursor Bugbot for commit b9e27d5. 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 VariationInternal, bringing the C++ client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have detected and gracefully handled cyclic prerequisite graphs for years. The unordered_set 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 insert-before-recurse / erase-after-recurse, guarded by a catch-and- rethrow 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.

These custom-event redaction tests fail on main; the fix belongs in the C++ server SDK and is unrelated to this PR. Wire the existing suppression pattern into the v2 harness jobs so this PR's CI can pass.

tanderson-ld · 2026-07-22T17:09:07Z

Unrelated to this PR. Needed to suppress these while @keelerm84 works through addressing these in the various affected SDKs.

Will be fixed by #583

tanderson-ld · 2026-07-22T17:17:53Z

Existing lint issues are being ignored by this PR.

This reverts commit f5e3f16.

keelerm84 · 2026-07-23T17:35:16Z

@tanderson-ld I merged in the sdk test harness fix, so I updated your PR to remove that suppression.

## 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  --- > [!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>

beekld · 2026-07-24T17:19:36Z

+                    // Cyclic edge: skip descent, continue with remaining
+                    // prerequisites at this level. The requested flag's value
+                    // and reason (below) are unaffected.
+                    continue;


Maybe we should log a warning here?

Good call — added a warn-level log in b9e27d5d. Cycles shouldn't reach the SDK under normal operation (LD validates the graph on save), so if one does fire at runtime it's worth surfacing.

beekld · 2026-07-24T17:21:57Z

-    // We're using JsonVariation because the type of the
-    // prerequisite is both unknown and irrelevant to emitting the events.
-    //
-    // We're passing Value::Null() to match a server-side SDK's behavior when


The part of this comment about Value::Null() is still valid, right? Maybe we should keep that part?

Good catch — I over-trimmed. Restored the Value::Null() sentence in b9e27d5d. Dropped only the sibling "we're using JsonVariation" sentence since we now call VariationInternal<Value> directly, so that part is stale.

beekld · 2026-07-24T17:27:30Z

+        std::unordered_set<std::string> local_visited;
+        auto* ancestors = visited ? visited : &local_visited;
+        ancestors->insert(key);
+        try {


We don't really use exceptions in this repo except to catch them when calling out to third-party or user code that might throw weird ones. Anything our SDK itself might throw (like OOM) seems like the kind of fatal thing we wouldn't handle anyway. I would suggest just not wrapping it in a try at all.

Agreed — dropped the try/catch in b9e27d5d. VariationInternal is our own code and doesn't throw, and local_visited is a stack local that stack-unwind destroys on the exceptional path, so the erase-in-catch was unnecessary.

…y/catch Addresses review feedback: - Emit a warning log when a cyclic prerequisite edge is skipped so an operator has visibility into the unusual runtime state. - Restore the sentence explaining why prereq recursion passes Value::Null() (server-side SDK parity), which was dropped in an earlier comment trim. - Remove the try/catch around the recursive prereq walk. VariationInternal is our own code and does not throw; the only realistic failure mode is OOM from unordered_set operations, which is fatal anyway. local_visited is a stack local that stack-unwind destroys, so no cleanup is needed on the exceptional path.

tanderson-ld mentioned this pull request Jul 21, 2026

fix: add defensive cycle guard to prerequisite evaluation launchdarkly/roku-client-sdk#54

Merged

tanderson-ld added 3 commits July 22, 2026 11:08

docs: trim verbose visited-set comment

31dc040

style: split if-init clause and trim visited comment

5fffbec

ci: suppress unrelated anonymous-context server-sdk tests

f5e3f16

These custom-event redaction tests fail on main; the fix belongs in the C++ server SDK and is unrelated to this PR. Wire the existing suppression pattern into the v2 harness jobs so this PR's CI can pass.

tanderson-ld marked this pull request as ready for review July 22, 2026 17:08

tanderson-ld requested a review from a team as a code owner July 22, 2026 17:08

tanderson-ld commented Jul 22, 2026

View reviewed changes

keelerm84 added 3 commits July 23, 2026 13:32

Merge branch 'main' into ta/SDK-2709-cycle-detection

f5acda5

remove suppressions

4bc1fe8

Revert "ci: suppress unrelated anonymous-context server-sdk tests"

540629a

This reverts commit f5e3f16.

beekld approved these changes Jul 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: add defensive cycle guard to prerequisite evaluation#582

fix: add defensive cycle guard to prerequisite evaluation#582
tanderson-ld wants to merge 8 commits into
mainfrom
ta/SDK-2709-cycle-detection

tanderson-ld commented Jul 21, 2026 •

edited by cursor Bot

Loading

Uh oh!

tanderson-ld Jul 22, 2026

Uh oh!

keelerm84 Jul 23, 2026

Uh oh!

tanderson-ld Jul 22, 2026 •

edited

Loading

Uh oh!

keelerm84 commented Jul 23, 2026

Uh oh!

beekld Jul 24, 2026

Uh oh!

tanderson-ld Jul 24, 2026

Uh oh!

beekld Jul 24, 2026

Uh oh!

tanderson-ld Jul 24, 2026

Uh oh!

beekld Jul 24, 2026

Uh oh!

tanderson-ld Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tanderson-ld commented Jul 21, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

The fix

Caller-visible behavior on a cycle

Files changed

Verification

Changelog

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanderson-ld Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keelerm84 commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanderson-ld commented Jul 21, 2026 •

edited by cursor Bot

Loading

tanderson-ld Jul 22, 2026 •

edited

Loading