From 7dbe5747be3e0eb8e440568848e14b43e500f6fd Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 14:08:50 -0400 Subject: [PATCH] fix: add defensive cycle guard to prerequisite evaluation 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. --- pkgs/sdk/client/contract-tests/TestService.cs | 1 + pkgs/sdk/client/src/LdClient.cs | 31 ++++- .../LdClientEventTests.cs | 127 ++++++++++++++++++ 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/pkgs/sdk/client/contract-tests/TestService.cs b/pkgs/sdk/client/contract-tests/TestService.cs index ca28960b..c8f6b4fa 100644 --- a/pkgs/sdk/client/contract-tests/TestService.cs +++ b/pkgs/sdk/client/contract-tests/TestService.cs @@ -42,6 +42,7 @@ public class Webapp "inline-context-all", "anonymous-redaction", "client-prereq-events", + "client-prereq-cycle-detection", "client-per-context-summaries" }; diff --git a/pkgs/sdk/client/src/LdClient.cs b/pkgs/sdk/client/src/LdClient.cs index 834ffd8c..b88178df 100644 --- a/pkgs/sdk/client/src/LdClient.cs +++ b/pkgs/sdk/client/src/LdClient.cs @@ -755,7 +755,7 @@ EvaluationDetail VariationInternal(string featureKey, LdValue defaultJson, } EvaluationDetail EvaluateInternal(string featureKey, LdValue defaultJson, LdValue.Converter converter, - bool checkType, EventFactory eventFactory) + bool checkType, EventFactory eventFactory, HashSet visited = null) { T defaultValue = converter.ToType(defaultJson); @@ -791,12 +791,33 @@ EvaluationDetail errorResult(EvaluationErrorKind kind) => } // Prerequisites are evaluated directly via EvaluateInternal to avoid triggering hooks. - if (flag.Prerequisites != null) + // + // `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 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. + if (flag.Prerequisites != null && flag.Prerequisites.Count > 0) { - foreach (var prerequisiteKey in flag.Prerequisites) + var ancestors = visited ?? new HashSet(); + ancestors.Add(featureKey); + try + { + foreach (var prerequisiteKey in flag.Prerequisites) + { + if (ancestors.Contains(prerequisiteKey)) + { + // Cyclic edge: skip descent, continue with remaining prerequisites at this level. + // The requested flag's value and reason (below) are unaffected. + continue; + } + EvaluateInternal(prerequisiteKey, LdValue.Null, LdValue.Convert.Json, false, + _eventFactoryWithReasons, ancestors); + } + } + finally { - EvaluateInternal(prerequisiteKey, LdValue.Null, LdValue.Convert.Json, false, - _eventFactoryWithReasons); + ancestors.Remove(featureKey); } } diff --git a/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientEventTests.cs b/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientEventTests.cs index d8be77d9..3698d5f1 100644 --- a/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientEventTests.cs +++ b/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientEventTests.cs @@ -373,6 +373,133 @@ public void VariationSendsFeatureEventForPrerequisites() } } + // Cycle-detection tests exercise the ancestor-set cycle guard added to EvaluateInternal. + // Each test constructs a cyclic prereq graph, evaluates one flag on the cycle, and asserts + // (a) the requested flag returns its cached value unchanged and (b) the recorded evaluation + // events match exactly one entry per cycle-safe descent. + + [Fact] + public void VariationSkipsSelfLoopPrerequisiteAndReturnsCachedValue() + { + var flagA = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagA").Build(); + _testData.Update(_testData.Flag("flagA").PreconfiguredFlag(flagA)); + + using (LdClient client = MakeClient(user)) + { + Assert.True(client.BoolVariation("flagA")); + // Only flagA emits a feature event; the self-prereq is cycle-skipped. + Assert.Collection(eventProcessor.Events, + e => CheckIdentifyEvent(e, user), + e => CheckEvaluationEvent(e, "flagA") + ); + } + } + + [Fact] + public void VariationHandlesTwoCycleEvaluatingA() + { + var flagA = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagB").Build(); + var flagB = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagA").Build(); + _testData.Update(_testData.Flag("flagA").PreconfiguredFlag(flagA)); + _testData.Update(_testData.Flag("flagB").PreconfiguredFlag(flagB)); + + using (LdClient client = MakeClient(user)) + { + Assert.True(client.BoolVariation("flagA")); + // A -> B -> [A skipped]. Events deepest-first: B (as prereq of A), then A. + Assert.Collection(eventProcessor.Events, + e => CheckIdentifyEvent(e, user), + e => CheckEvaluationEvent(e, "flagB"), + e => CheckEvaluationEvent(e, "flagA") + ); + } + } + + [Fact] + public void VariationHandlesTwoCycleEvaluatingB() + { + var flagA = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagB").Build(); + var flagB = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagA").Build(); + _testData.Update(_testData.Flag("flagA").PreconfiguredFlag(flagA)); + _testData.Update(_testData.Flag("flagB").PreconfiguredFlag(flagB)); + + using (LdClient client = MakeClient(user)) + { + Assert.True(client.BoolVariation("flagB")); + // Symmetric: same graph, entry from B. Events: A (as prereq of B), then B. + Assert.Collection(eventProcessor.Events, + e => CheckIdentifyEvent(e, user), + e => CheckEvaluationEvent(e, "flagA"), + e => CheckEvaluationEvent(e, "flagB") + ); + } + } + + [Fact] + public void VariationHandlesThreeCycle() + { + var flagA = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagB").Build(); + var flagB = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagC").Build(); + var flagC = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagA").Build(); + _testData.Update(_testData.Flag("flagA").PreconfiguredFlag(flagA)); + _testData.Update(_testData.Flag("flagB").PreconfiguredFlag(flagB)); + _testData.Update(_testData.Flag("flagC").PreconfiguredFlag(flagC)); + + using (LdClient client = MakeClient(user)) + { + Assert.True(client.BoolVariation("flagA")); + // A -> B -> C -> [A skipped]. Events emitted deepest-first: C, B, A. + Assert.Collection(eventProcessor.Events, + e => CheckIdentifyEvent(e, user), + e => CheckEvaluationEvent(e, "flagC"), + e => CheckEvaluationEvent(e, "flagB"), + e => CheckEvaluationEvent(e, "flagA") + ); + } + } + + [Fact] + public void VariationEmitsSharedDescendantOncePerPathInDiamond() + { + // Diamond: A -> [B, C], B -> [D], C -> [D]. Not a cycle. Ancestor-set (current-path) + // semantics let D be reached on each of the two independent paths, so D emits twice. + // A naive "visited across the whole walk" implementation would drop the second event. + var flagA = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagB", "flagC").Build(); + var flagB = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagD").Build(); + var flagC = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Prerequisites("flagD").Build(); + var flagD = new FeatureFlagBuilder().Value(LdValue.Of(true)).Variation(1).Version(1) + .TrackEvents(false).TrackReason(false).Build(); + _testData.Update(_testData.Flag("flagA").PreconfiguredFlag(flagA)); + _testData.Update(_testData.Flag("flagB").PreconfiguredFlag(flagB)); + _testData.Update(_testData.Flag("flagC").PreconfiguredFlag(flagC)); + _testData.Update(_testData.Flag("flagD").PreconfiguredFlag(flagD)); + + using (LdClient client = MakeClient(user)) + { + Assert.True(client.BoolVariation("flagA")); + // Events (deepest-first per path): D (via B), B, D (via C), C, A. D appears twice. + Assert.Collection(eventProcessor.Events, + e => CheckIdentifyEvent(e, user), + e => CheckEvaluationEvent(e, "flagD"), + e => CheckEvaluationEvent(e, "flagB"), + e => CheckEvaluationEvent(e, "flagD"), + e => CheckEvaluationEvent(e, "flagC"), + e => CheckEvaluationEvent(e, "flagA") + ); + } + } + private void CheckIdentifyEvent(object e, Context c) { IdentifyEvent ie = Assert.IsType(e);