fix: add defensive cycle guard to prerequisite evaluation#582
Draft
tanderson-ld wants to merge 1 commit into
Draft
fix: add defensive cycle guard to prerequisite evaluation#582tanderson-ld wants to merge 1 commit into
tanderson-ld wants to merge 1 commit into
Conversation
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.
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
ClientImpl::VariationInternal, bringing the C++ client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have handled cyclic prerequisite graphs gracefully for years.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-allocatedstd::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.std::unordered_setis created only when the walker descends into a prerequisite for the first time in a call, then reused for the rest of the walk.insert/ catch-and-rethrowerase: no per-descent copy.ancestors->insert(key)before recursing; if a nested call throws, the exception handler restoresancestorsto its pre-descent state before rethrowing. This preserves the invariant "the set contains exactly the current path" even under exceptions.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 = nullptris added toVariationInternal<T>, so all existing callers (the publicBoolVariation/IntVariation/StringVariation/DoubleVariation/JsonVariationand their*Detailcounterparts) work unchanged. The recursive call now goes directly toVariationInternal<Value>rather than throughJsonVariation, 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_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
libs/client-sdk/src/client_impl.hpp— addedstd::unordered_set<std::string>*optional parameter toVariationInternal<T>declaration, plus the<string>/<unordered_set>includes.libs/client-sdk/src/client_impl.cpp— cycle guard implementation inVariationInternal<T>.contract-tests/client-contract-tests/src/main.cpp— declaresclient-prereq-cycle-detectioncapability so the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK.Verification
cmake --build build --target launchdarkly-cpp-client→ clean, no warnings.client-testsbinary +sdk-test-harness v2.38.1→ 876 total, 12 skipped, 864 ran, all passed. Both new suites (events/prerequisite events handle cyclesandevents/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.cppcurrently 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