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..6782735f 100644 --- a/packages/common_client/test/ld_dart_client_test.dart +++ b/packages/common_client/test/ld_dart_client_test.dart @@ -393,5 +393,75 @@ 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'], + ); + }); }); }