Skip to content

fix: add defensive cycle guard to prerequisite evaluation#512

Open
tanderson-ld wants to merge 1 commit into
v11from
ta/SDK-2704-cycle-detection
Open

fix: add defensive cycle guard to prerequisite evaluation#512
tanderson-ld wants to merge 1 commit into
v11from
ta/SDK-2704-cycle-detection

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds defensive cycle detection to the recursive prerequisite walk in LDClient.variationDetailInternal, bringing the iOS client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have handled cyclic prerequisite graphs gracefully for years.

Tracker: SDK-2704. 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 Android fix: android-client-sdk#377.

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 iOS client SDK.

The fix

variationDetailInternal now threads a lazily-allocated Set<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.

  • Lazy allocation: variation calls on prereq-less flags (the common case) allocate zero collections. The Set<String> is created only when the walker descends into a prerequisite for the first time in a call, then reused for the rest of the walk.
  • Mutable insert / defer remove: no per-descent copy. insert(flagKey) before recursing, defer { visited!.remove(flagKey) } for guaranteed cleanup — the invariant "the set contains exactly the current path" holds even under early exits.
  • Ancestor-set (current-path) semantics — this is deliberate. A prerequisite reached via multiple non-cyclic paths (a diamond 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. There is a unit test guarding this.

The public 4-argument variationDetailInternal signature is preserved as a thin wrapper that declares var visited: Set<String>? = nil and calls the new 5-argument overload — none of the 12 top-level variation methods (boolVariation, intVariation, etc.) need to change.

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_FLAG and 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

  • LaunchDarkly/LaunchDarkly/LDClientVariation.swift — cycle guard in variationDetailInternal. 4-argument public signature preserved as a wrapper; new 5-argument overload carries the inout Set<String>?.
  • LaunchDarkly/LaunchDarklyTests/LDClientSpec.swift — 5 new Quick/Nimble tests under a new context("flag store contains cyclic prerequisites"): self-loop, two-cycle (evaluating each end), three-cycle, and a non-cyclic diamond that asserts the shared descendant emits an event for each path (the ancestor-set regression fence).
  • ContractTests/Source/Controllers/SdkController.swift — declares client-prereq-cycle-detection so the matching contract tests in sdk-test-harness v2.38.0+ activate for this SDK.

Verification

  • Unit tests: swift test → 615 tests, 0 failures. The 5 new cycle-detection tests are visible in the log and passing.
  • Contract tests locally against harness v2.38.0: make contract-tests → 838 total, 15 skipped, 823 ran, all passed. Both new suites (events/prerequisite events handle cycles and events/summary events/prerequisites/handles cycles) fire and pass — 15 subtests total between them.

Changelog

Updated prerequisite evaluation event emission to match server SDK behavior for cyclic prerequisite graphs.


Note

Medium Risk
Touches core flag evaluation and prerequisite event emission; behavior change is defensive and well-tested but affects analytics paths on malformed graphs.

Overview
Adds defensive cycle detection to the recursive prerequisite walk in variationDetailInternal, aligning client behavior with server SDKs and CSPE 1.2.5.

Before descending into a prerequisite, the walker tracks flag keys on the current path in a lazily allocated Set (no extra allocation for flags without prerequisites). If a prerequisite key is already on the path, that edge is skipped; the evaluated flag still returns its cached value and reason unchanged. Ancestor-set semantics preserve correct behavior on diamond graphs (shared descendants still emit prerequisite events once per path).

Adds Quick/Nimble coverage for self-loops, 2- and 3-cycles, and the diamond regression case. Contract tests advertise capability client-prereq-cycle-detection.

Reviewed by Cursor Bugbot for commit 5dd39a6. Bugbot is set up for automated code reviews on this repo. Configure here.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant