From 5dd39a6286c254d179908efe4bc8dd197a2e443b Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 09:47:12 -0400 Subject: [PATCH] fix: add defensive cycle guard to prerequisite evaluation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an ancestor-set (current-path) cycle guard to the recursive prerequisite walk in variationDetailInternal, bringing the iOS client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have detected and gracefully handled cyclic prerequisite graphs for years. The LaunchDarkly service validates prerequisite graphs on mutation and rejects any change that would produce a cycle, so under normal operation the SDK does not see a cyclic graph. This is defensive code for exceptional cases — for example, delivery of updates out of order or a persisted state loaded from disk that predates a subsequent correction. The Set tracking ancestor keys is allocated lazily: variation calls on prereq-less flags (the common case) allocate zero collections. Once created, the set is shared for the rest of the walk via insert-on-descend / remove-on-ascend, guarded by defer so a recursive descent that throws 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. --- .../Source/Controllers/SdkController.swift | 1 + .../LaunchDarkly/LDClientVariation.swift | 29 ++++++++- .../LaunchDarklyTests/LDClientSpec.swift | 63 +++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/ContractTests/Source/Controllers/SdkController.swift b/ContractTests/Source/Controllers/SdkController.swift index cbb63787..8bee1b2b 100644 --- a/ContractTests/Source/Controllers/SdkController.swift +++ b/ContractTests/Source/Controllers/SdkController.swift @@ -34,6 +34,7 @@ final class SdkController: RouteCollection { "event-gzip", "optional-event-gzip", "client-prereq-events", + "client-prereq-cycle-detection", "polling-gzip", "client-per-context-summaries" ] diff --git a/LaunchDarkly/LaunchDarkly/LDClientVariation.swift b/LaunchDarkly/LaunchDarkly/LDClientVariation.swift index 6b32d44a..d390e656 100644 --- a/LaunchDarkly/LaunchDarkly/LDClientVariation.swift +++ b/LaunchDarkly/LaunchDarkly/LDClientVariation.swift @@ -169,13 +169,36 @@ extension LDClient { } private func variationDetailInternal(_ flagKey: LDFlagKey, _ defaultValue: T, needsReason: Bool, methodName: String) -> LDEvaluationDetail where T: Decodable, T: LDValueConvertible { + var visited: Set? = nil + return variationDetailInternal(flagKey, defaultValue, needsReason: needsReason, methodName: methodName, visited: &visited) + } + + private func variationDetailInternal(_ flagKey: LDFlagKey, _ defaultValue: T, needsReason: Bool, methodName: String, visited: inout Set?) -> LDEvaluationDetail where T: Decodable, T: LDValueConvertible { return evaluateWithHooks(flagKey: flagKey, defaultValue: defaultValue, methodName: methodName) { var result: LDEvaluationDetail let featureFlag = flagStore.featureFlag(for: flagKey) if let featureFlag = featureFlag { - featureFlag.prerequisites?.forEach { prereqFlagKey in - // recurse on prerequisites to emulate prereq evaluations occurring with desirable side effects such as events for prereqs - _ = variationDetailInternal(prereqFlagKey, LDValue.null, needsReason: needsReason, methodName: methodName) + if let prerequisites = featureFlag.prerequisites, !prerequisites.isEmpty { + // Recurse on prerequisites to emulate prereq evaluations occurring with desirable side effects + // such as events for prereqs. + // + // `visited` tracks the chain of prerequisite dependencies from the top-level evaluation to + // (but not including) the current flag. The set is allocated lazily: it stays `nil` until we + // descend into the first flag whose `prerequisites` are non-empty, so a variation call on a + // leaf flag pays no heap allocation for cycle bookkeeping. + if visited == nil { + visited = Set() + } + visited!.insert(flagKey) + defer { visited!.remove(flagKey) } + for prereqFlagKey in prerequisites { + if visited!.contains(prereqFlagKey) { + // Cyclic edge: skip descent, continue with remaining prerequisites at this level. + // The requested flag's value and reason (below) are unaffected. + continue + } + _ = variationDetailInternal(prereqFlagKey, LDValue.null, needsReason: needsReason, methodName: methodName, visited: &visited) + } } if featureFlag.value == .null { diff --git a/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift b/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift index 7590ae2e..f4901cfa 100644 --- a/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift +++ b/LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift @@ -799,6 +799,69 @@ final class LDClientSpec: QuickSpec { expect(events[7].key) == "flagABD" } } + // Cycle-detection tests exercise the ancestor-set cycle guard added to + // variationDetailInternal. Prior to that guard, any of these flag configurations would + // cause unbounded recursion during a variation() call. The tests set up a cyclic + // prerequisite graph and evaluate one flag on the cycle, asserting (a) the SDK returns + // the flag's cached value unchanged and (b) the emitted evaluation events reflect + // exactly one recording per cycle-safe descent. + context("flag store contains cyclic prerequisites") { + var events = [FeatureEvent]() + beforeEach { + events = [] + testContext.eventReporterMock.recordFlagEvaluationEventsCallback = { + let args = testContext.eventReporterMock.recordFlagEvaluationEventsReceivedArguments! + events.append(FeatureEvent(key: args.flagKey, context: args.context, value: args.value, defaultValue: args.defaultValue, featureFlag: args.featureFlag, includeReason: args.includeReason, isDebug: false)) + } + } + it("skips self-loop prerequisite and returns cached value") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA])) + // Requirement 1.2.5.1: the requested flag's cached value is returned unchanged. + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // The self-prereq is cycle-skipped, so only the top-level evaluation records an event. + expect(events.map { $0.key }) == ["flagA"] + } + it("handles a two-cycle evaluating A") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // A -> B -> [A skipped]. Events emitted deepest-first: B (as prereq of A), then A. + expect(events.map { $0.key }) == ["flagB", "flagA"] + } + it("handles a two-cycle evaluating B") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB])) + expect(testContext.subject.boolVariation(forKey: "flagB", defaultValue: false)) == true + // Symmetric: same graph, entry from B. Events: A (as prereq of B), then B. + expect(events.map { $0.key }) == ["flagA", "flagB"] + } + it("handles a three-cycle") { + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagC"]) + let flagC = FeatureFlag(flagKey: "flagC", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagA"]) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB, "flagC": flagC])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // A -> B -> C -> [A skipped]. Events emitted deepest-first: C, B, A. + expect(events.map { $0.key }) == ["flagC", "flagB", "flagA"] + } + it("emits the shared descendant once per path in a non-cyclic diamond") { + // Diamond: A -> [B, C], B -> [D], C -> [D]. Not a cycle. Ancestor-set (current-path) + // semantics must let D be reached on each of the two independent paths — so D emits + // twice. A naive "visited across the whole walk" implementation would incorrectly + // count D only once; this case guards against that regression. + let flagA = FeatureFlag(flagKey: "flagA", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagB", "flagC"]) + let flagB = FeatureFlag(flagKey: "flagB", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagD"]) + let flagC = FeatureFlag(flagKey: "flagC", value: LDValue.bool(true), trackEvents: false, trackReason: false, prerequisites: ["flagD"]) + let flagD = FeatureFlag(flagKey: "flagD", value: LDValue.bool(true), trackEvents: false, trackReason: false) + testContext.flagStoreMock.replaceStore(newStoredItems: StoredItems(items: ["flagA": flagA, "flagB": flagB, "flagC": flagC, "flagD": flagD])) + expect(testContext.subject.boolVariation(forKey: "flagA", defaultValue: false)) == true + // Events (deepest-first per path): D (via B), B, D (via C), C, A. D appears twice. + expect(events.map { $0.key }) == ["flagD", "flagB", "flagD", "flagC", "flagA"] + } + } } }