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);