From 50a0218c13a2d154e0a2c790621c71593a997bbd Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 14:17:47 -0400 Subject: [PATCH 1/7] fix: add defensive cycle guard to prerequisite evaluation 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. --- .../client-contract-tests/src/main.cpp | 1 + libs/client-sdk/src/client_impl.cpp | 43 ++++++++++++++----- libs/client-sdk/src/client_impl.hpp | 5 ++- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/contract-tests/client-contract-tests/src/main.cpp b/contract-tests/client-contract-tests/src/main.cpp index 7b5a878ae..ae7c42446 100644 --- a/contract-tests/client-contract-tests/src/main.cpp +++ b/contract-tests/client-contract-tests/src/main.cpp @@ -47,6 +47,7 @@ int main(int argc, char* argv[]) { srv.add_capability("tls:skip-verify-peer"); srv.add_capability("tls:custom-ca"); srv.add_capability("client-prereq-events"); + srv.add_capability("client-prereq-cycle-detection"); srv.add_capability("wrapper"); // Proxies are supported only with CURL networking. #ifdef LD_CURL_NETWORKING diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index 374f92e57..089518814 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -247,7 +247,8 @@ template EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, Value default_value, bool check_type, - bool detailed) { + bool detailed, + std::unordered_set* visited) { auto desc = flag_manager_.Store().Get(key); events::FeatureEventParams event = { @@ -308,19 +309,41 @@ EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, // prerequisites (recursively), which is necessary to ensure LaunchDarkly // analytics functions properly. // - // 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 - // evaluating prerequisites. + // `visited` tracks the chain of prerequisite dependencies from the + // top-level evaluation to (but not including) the current flag. It is + // allocated lazily: variation calls on prereq-less flags allocate no + // set. Once created it is shared for the rest of the walk via + // insert-before-recurse / erase-after-recurse (guarded by a scope-exit + // erase so an exception below cannot leave a stale ancestor entry + // visible to a sibling branch). A prerequisite already in `visited` + // closes a cycle; descent is skipped and the loop continues with the + // remaining prerequisites at the current level. // // NOTE: if "hooks" functionality is implemented into this SDK, take care // that evaluating prerequisites does not trigger hooks. This may require - // refactoring the code below to not use JsonVariation. - if (auto const prereqs = flag.Prerequisites()) { - for (auto const& prereq : *prereqs) { - JsonVariation(prereq, Value::Null()); + // refactoring the code below. + if (auto const prereqs = flag.Prerequisites(); + prereqs && !prereqs->empty()) { + std::unordered_set local_visited; + auto* ancestors = visited ? visited : &local_visited; + ancestors->insert(key); + try { + for (auto const& prereq : *prereqs) { + if (ancestors->count(prereq) != 0) { + // Cyclic edge: skip descent, continue with remaining + // prerequisites at this level. The requested flag's value + // and reason (below) are unaffected. + continue; + } + (void)VariationInternal(prereq, Value::Null(), + /*check_type=*/false, + /*detailed=*/false, ancestors); + } + } catch (...) { + ancestors->erase(key); + throw; } + ancestors->erase(key); } if (check_type && default_value.Type() != Value::Type::kNull && diff --git a/libs/client-sdk/src/client_impl.hpp b/libs/client-sdk/src/client_impl.hpp index 78142e347..ac03a4ae3 100644 --- a/libs/client-sdk/src/client_impl.hpp +++ b/libs/client-sdk/src/client_impl.hpp @@ -23,7 +23,9 @@ #include #include #include +#include #include +#include namespace launchdarkly::client_side { class ClientImpl : public IClient { @@ -93,7 +95,8 @@ class ClientImpl : public IClient { [[nodiscard]] EvaluationDetail VariationInternal(FlagKey const& key, Value default_value, bool check_type, - bool detailed); + bool detailed, + std::unordered_set* visited = nullptr); void TrackInternal(std::string event_name, std::optional data, std::optional metric_value); From 31dc04086793cf9ed52fdea808e8da8661911471 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Wed, 22 Jul 2026 11:08:00 -0400 Subject: [PATCH 2/7] docs: trim verbose visited-set comment --- libs/client-sdk/src/client_impl.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index 089518814..c7c522a95 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -309,15 +309,8 @@ EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, // prerequisites (recursively), which is necessary to ensure LaunchDarkly // analytics functions properly. // - // `visited` tracks the chain of prerequisite dependencies from the - // top-level evaluation to (but not including) the current flag. It is - // allocated lazily: variation calls on prereq-less flags allocate no - // set. Once created it is shared for the rest of the walk via - // insert-before-recurse / erase-after-recurse (guarded by a scope-exit - // erase so an exception below cannot leave a stale ancestor entry - // visible to a sibling branch). A prerequisite already in `visited` - // closes a cycle; descent is skipped and the loop continues with the - // remaining prerequisites at the current level. + // `visited` tracks the current path so a cyclic prerequisite graph + // terminates instead of recursing without bound. // // NOTE: if "hooks" functionality is implemented into this SDK, take care // that evaluating prerequisites does not trigger hooks. This may require From 5fffbec0fbf610acfbb99ee5567360e689f27f7b Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Wed, 22 Jul 2026 11:49:49 -0400 Subject: [PATCH 3/7] style: split if-init clause and trim visited comment --- libs/client-sdk/src/client_impl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index c7c522a95..9f61deb50 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -315,8 +315,8 @@ EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, // NOTE: if "hooks" functionality is implemented into this SDK, take care // that evaluating prerequisites does not trigger hooks. This may require // refactoring the code below. - if (auto const prereqs = flag.Prerequisites(); - prereqs && !prereqs->empty()) { + auto const prereqs = flag.Prerequisites(); + if (prereqs && !prereqs->empty()) { std::unordered_set local_visited; auto* ancestors = visited ? visited : &local_visited; ancestors->insert(key); From f5e3f168cb16b0432299552cdb37d83aa545cf49 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Wed, 22 Jul 2026 11:49:49 -0400 Subject: [PATCH 4/7] ci: suppress unrelated anonymous-context server-sdk tests 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. --- .github/workflows/server.yml | 4 ++++ contract-tests/server-contract-tests/test-suppressions.txt | 2 ++ 2 files changed, 6 insertions(+) create mode 100644 contract-tests/server-contract-tests/test-suppressions.txt diff --git a/.github/workflows/server.yml b/.github/workflows/server.yml index 2b5623f04..4a9e8c637 100644 --- a/.github/workflows/server.yml +++ b/.github/workflows/server.yml @@ -20,6 +20,7 @@ jobs: # Port the test service (implemented in this repo) should bind to. TEST_SERVICE_PORT: 8123 TEST_SERVICE_BINARY: ./build/contract-tests/server-contract-tests/server-tests + SUPPRESSION_FILE: contract-tests/server-contract-tests/test-suppressions.txt steps: # https://github.com/actions/checkout/releases/tag/v4.3.0 - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 @@ -35,6 +36,7 @@ jobs: # Inform the test harness of test service's port. test_service_port: ${{ env.TEST_SERVICE_PORT }} token: ${{ secrets.GITHUB_TOKEN }} + extra_params: -skip-from=${{ env.SUPPRESSION_FILE }} contract-tests-fdv2: runs-on: ubuntu-22.04 @@ -66,6 +68,7 @@ jobs: # Port the test service (implemented in this repo) should bind to. TEST_SERVICE_PORT: 8123 TEST_SERVICE_BINARY: ./build/contract-tests/server-contract-tests/server-tests + SUPPRESSION_FILE: contract-tests/server-contract-tests/test-suppressions.txt steps: # https://github.com/actions/checkout/releases/tag/v4.3.0 - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 @@ -82,6 +85,7 @@ jobs: # Inform the test harness of test service's port. test_service_port: ${{ env.TEST_SERVICE_PORT }} token: ${{ secrets.GITHUB_TOKEN }} + extra_params: -skip-from=${{ env.SUPPRESSION_FILE }} contract-tests-fdv2-curl: runs-on: ubuntu-22.04 diff --git a/contract-tests/server-contract-tests/test-suppressions.txt b/contract-tests/server-contract-tests/test-suppressions.txt new file mode 100644 index 000000000..4f38aeb1b --- /dev/null +++ b/contract-tests/server-contract-tests/test-suppressions.txt @@ -0,0 +1,2 @@ +events/custom events/single-kind anonymous context redacts all attributes +events/custom events/multi-kind with anonymous context redacts attributes appropriately From 4bc1fe814f9966d785c0c42d2f496716e88f56bc Mon Sep 17 00:00:00 2001 From: Matthew Keeler Date: Thu, 23 Jul 2026 13:32:42 -0400 Subject: [PATCH 5/7] remove suppressions --- contract-tests/server-contract-tests/test-suppressions.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contract-tests/server-contract-tests/test-suppressions.txt b/contract-tests/server-contract-tests/test-suppressions.txt index 4f38aeb1b..8b1378917 100644 --- a/contract-tests/server-contract-tests/test-suppressions.txt +++ b/contract-tests/server-contract-tests/test-suppressions.txt @@ -1,2 +1 @@ -events/custom events/single-kind anonymous context redacts all attributes -events/custom events/multi-kind with anonymous context redacts attributes appropriately + From 540629afea3f48c6cf56c070ff9df3fe227296b1 Mon Sep 17 00:00:00 2001 From: Matthew Keeler Date: Thu, 23 Jul 2026 13:34:37 -0400 Subject: [PATCH 6/7] Revert "ci: suppress unrelated anonymous-context server-sdk tests" This reverts commit f5e3f168cb16b0432299552cdb37d83aa545cf49. --- .github/workflows/server.yml | 4 ---- contract-tests/server-contract-tests/test-suppressions.txt | 1 - 2 files changed, 5 deletions(-) delete mode 100644 contract-tests/server-contract-tests/test-suppressions.txt diff --git a/.github/workflows/server.yml b/.github/workflows/server.yml index 4a9e8c637..2b5623f04 100644 --- a/.github/workflows/server.yml +++ b/.github/workflows/server.yml @@ -20,7 +20,6 @@ jobs: # Port the test service (implemented in this repo) should bind to. TEST_SERVICE_PORT: 8123 TEST_SERVICE_BINARY: ./build/contract-tests/server-contract-tests/server-tests - SUPPRESSION_FILE: contract-tests/server-contract-tests/test-suppressions.txt steps: # https://github.com/actions/checkout/releases/tag/v4.3.0 - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 @@ -36,7 +35,6 @@ jobs: # Inform the test harness of test service's port. test_service_port: ${{ env.TEST_SERVICE_PORT }} token: ${{ secrets.GITHUB_TOKEN }} - extra_params: -skip-from=${{ env.SUPPRESSION_FILE }} contract-tests-fdv2: runs-on: ubuntu-22.04 @@ -68,7 +66,6 @@ jobs: # Port the test service (implemented in this repo) should bind to. TEST_SERVICE_PORT: 8123 TEST_SERVICE_BINARY: ./build/contract-tests/server-contract-tests/server-tests - SUPPRESSION_FILE: contract-tests/server-contract-tests/test-suppressions.txt steps: # https://github.com/actions/checkout/releases/tag/v4.3.0 - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 @@ -85,7 +82,6 @@ jobs: # Inform the test harness of test service's port. test_service_port: ${{ env.TEST_SERVICE_PORT }} token: ${{ secrets.GITHUB_TOKEN }} - extra_params: -skip-from=${{ env.SUPPRESSION_FILE }} contract-tests-fdv2-curl: runs-on: ubuntu-22.04 diff --git a/contract-tests/server-contract-tests/test-suppressions.txt b/contract-tests/server-contract-tests/test-suppressions.txt deleted file mode 100644 index 8b1378917..000000000 --- a/contract-tests/server-contract-tests/test-suppressions.txt +++ /dev/null @@ -1 +0,0 @@ - From b9e27d5d151075339e6f31b50d038d2e5d9ffaed Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Fri, 24 Jul 2026 16:49:23 -0400 Subject: [PATCH 7/7] refactor: log cycle detection, restore Value::Null() context, drop try/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. --- libs/client-sdk/src/client_impl.cpp | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index 9f61deb50..82a09db35 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -309,6 +309,9 @@ EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, // prerequisites (recursively), which is necessary to ensure LaunchDarkly // analytics functions properly. // + // We're passing Value::Null() to match a server-side SDK's behavior when + // evaluating prerequisites. + // // `visited` tracks the current path so a cyclic prerequisite graph // terminates instead of recursing without bound. // @@ -320,21 +323,19 @@ EvaluationDetail ClientImpl::VariationInternal(FlagKey const& key, std::unordered_set local_visited; auto* ancestors = visited ? visited : &local_visited; ancestors->insert(key); - try { - for (auto const& prereq : *prereqs) { - if (ancestors->count(prereq) != 0) { - // Cyclic edge: skip descent, continue with remaining - // prerequisites at this level. The requested flag's value - // and reason (below) are unaffected. - continue; - } - (void)VariationInternal(prereq, Value::Null(), - /*check_type=*/false, - /*detailed=*/false, ancestors); + for (auto const& prereq : *prereqs) { + if (ancestors->count(prereq) != 0) { + // Cyclic edge: skip descent, continue with remaining + // prerequisites at this level. The requested flag's value + // and reason (below) are unaffected. + LD_LOG(logger_, LogLevel::kWarn) + << "prerequisite cycle detected: skipping edge from '" + << key << "' to '" << prereq << "'"; + continue; } - } catch (...) { - ancestors->erase(key); - throw; + (void)VariationInternal(prereq, Value::Null(), + /*check_type=*/false, + /*detailed=*/false, ancestors); } ancestors->erase(key); }