From 44231058e76ba20dabbb40575441a6d923d54d49 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Tue, 21 Jul 2026 14:05:13 -0400 Subject: [PATCH 1/2] 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 Flutter client SDK's behavior into line with the LaunchDarkly server SDK evaluators, which have detected and gracefully handled cyclic prerequisite graphs for years. 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 add-before-recurse / remove-after-recurse, guarded by 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. --- .../bin/contract_test_service.dart | 1 + .../lib/src/ld_common_client.dart | 32 +++++++-- .../test/ld_dart_client_test.dart | 71 +++++++++++++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/apps/flutter_client_contract_test_service/bin/contract_test_service.dart b/apps/flutter_client_contract_test_service/bin/contract_test_service.dart index ee99bfea..fc925a62 100644 --- a/apps/flutter_client_contract_test_service/bin/contract_test_service.dart +++ b/apps/flutter_client_contract_test_service/bin/contract_test_service.dart @@ -27,6 +27,7 @@ class TestApiImpl extends SdkTestApi { 'anonymous-redaction', 'client-per-context-summaries', 'client-prereq-events', + 'client-prereq-cycle-detection', 'auto-env-attributes', 'client-event-source-http-errors', 'fdv1-fallback', diff --git a/packages/common_client/lib/src/ld_common_client.dart b/packages/common_client/lib/src/ld_common_client.dart index 3124a664..bd8c496c 100644 --- a/packages/common_client/lib/src/ld_common_client.dart +++ b/packages/common_client/lib/src/ld_common_client.dart @@ -772,15 +772,39 @@ final class LDCommonClient { LDEvaluationDetail _variationInternal( String flagKey, LDValue defaultValue, - {required bool isDetailed, LDValueType? type}) { + {required bool isDetailed, LDValueType? type, Set? visited}) { final evalResult = _flagManager.get(flagKey); LDEvaluationDetail detail; if (evalResult != null && evalResult.flag != null) { - evalResult.flag?.prerequisites?.forEach((prereq) { - _variationInternal(prereq, LDValue.ofNull(), isDetailed: isDetailed); - }); + final prerequisites = evalResult.flag!.prerequisites; + if (prerequisites != null && prerequisites.isNotEmpty) { + // 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. 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 finally block. + final ancestors = visited ?? {}; + ancestors.add(flagKey); + try { + for (final prereq in prerequisites) { + if (ancestors.contains(prereq)) { + // Cyclic edge: skip descent, continue with remaining + // prerequisites at this level. The requested flag's value and + // reason are unaffected. + continue; + } + _variationInternal(prereq, LDValue.ofNull(), + isDetailed: isDetailed, visited: ancestors); + } + } finally { + ancestors.remove(flagKey); + } + } if (type == null || type == evalResult.flag!.detail.value.type) { detail = evalResult.flag!.detail; diff --git a/packages/common_client/test/ld_dart_client_test.dart b/packages/common_client/test/ld_dart_client_test.dart index 4dcfe828..9e573e5c 100644 --- a/packages/common_client/test/ld_dart_client_test.dart +++ b/packages/common_client/test/ld_dart_client_test.dart @@ -393,5 +393,76 @@ void main() { expect(mockEventProcessor.evalEvents[0].flagKey, 'flagAB'); expect(mockEventProcessor.evalEvents[1].flagKey, 'flagA'); }); + + // Cycle-detection tests exercise the ancestor-set cycle guard added to + // _variationInternal. Each test constructs a cyclic prereq graph via the + // storage fixture, 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. + Future runCycleCase(String storageJson, String evalKey, + List expectedEventKeys) async { + final contextPersistenceKey = + sha256.convert(utf8.encode('bob')).toString(); + mockPersistence.storage[sdkKeyPersistence] = { + contextPersistenceKey: storageJson, + }; + await client.start(); + final res = client.stringVariation(evalKey, 'default'); + expect(res, 'cached'); + expect(mockEventProcessor.evalEvents.map((e) => e.flagKey).toList(), + expectedEventKeys); + } + + test('skips a self-loop prerequisite and returns the cached value', + () async { + // flagA's only prerequisite is itself; the cycle guard skips descent. + await runCycleCase( + '{"flagA":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagA"]}}', + 'flagA', + ['flagA'], + ); + }); + + test('handles a two-cycle evaluating A', () async { + // A -> B -> [A skipped]. Events deepest-first: B (as prereq of A), then A. + await runCycleCase( + '{"flagA":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagB"]},"flagB":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagA"]}}', + 'flagA', + ['flagB', 'flagA'], + ); + }); + + test('handles a two-cycle evaluating B', () async { + // Symmetric: same graph, entry from B. Events: A (as prereq of B), then B. + await runCycleCase( + '{"flagA":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagB"]},"flagB":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagA"]}}', + 'flagB', + ['flagA', 'flagB'], + ); + }); + + test('handles a three-cycle', () async { + // A -> B -> C -> [A skipped]. Events emitted deepest-first: C, B, A. + await runCycleCase( + '{"flagA":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagB"]},"flagB":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagC"]},"flagC":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagA"]}}', + 'flagA', + ['flagC', 'flagB', 'flagA'], + ); + }); + + test( + 'emits the shared descendant once per path in a non-cyclic diamond', + () async { + // 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; this case guards against + // that regression. + await runCycleCase( + '{"flagA":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagB","flagC"]},"flagB":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagD"]},"flagC":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"},"prerequisites":["flagD"]},"flagD":{"version":1,"value":"cached","variation":0,"reason":{"kind":"OFF"}}}', + 'flagA', + ['flagD', 'flagB', 'flagD', 'flagC', 'flagA'], + ); + }); }); } From 7ea0310d8f4ea3091eda97d0fb784f6fd2897d73 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Wed, 22 Jul 2026 09:33:52 -0400 Subject: [PATCH 2/2] style: apply dart format to cycle-detection tests --- packages/common_client/test/ld_dart_client_test.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/common_client/test/ld_dart_client_test.dart b/packages/common_client/test/ld_dart_client_test.dart index 9e573e5c..6782735f 100644 --- a/packages/common_client/test/ld_dart_client_test.dart +++ b/packages/common_client/test/ld_dart_client_test.dart @@ -450,8 +450,7 @@ void main() { ); }); - test( - 'emits the shared descendant once per path in a non-cyclic diamond', + test('emits the shared descendant once per path in a non-cyclic diamond', () async { // 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