From f1e41ab118323a34e8ad7029871fb5d5f75cea2b Mon Sep 17 00:00:00 2001 From: Pavel Date: Thu, 17 Sep 2026 10:07:57 +0300 Subject: [PATCH 1/2] [FeatureFlags] Replay the config handler on registration and bump the OpenFeature package The Remote Configuration subscription is live from module construction, so a payload can land before application code builds a provider and registers a handler. The handler only fired on a change, so a caller waiting on it waited forever. Invoke it at registration when configuration is already held. Bump Datadog.FeatureFlags.OpenFeature to 2.3.1 so the initialization fix can ship: 2.3.0 is already published and immutable. --- .../Datadog.FeatureFlags.OpenFeature.csproj | 2 +- .../FeatureFlags/FeatureFlagsModule.cs | 18 +++++++++ .../FeatureFlags/FeatureFlagsModuleTests.cs | 38 +++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj b/tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj index 5b8a220efe54..e85bf67ac6ce 100644 --- a/tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj +++ b/tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj @@ -17,7 +17,7 @@ - 2.3.0 + 2.3.1 net462;netstandard2.0;net8.0;net9.0 diff --git a/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs b/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs index 851915079563..df31acf0e0e2 100644 --- a/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs +++ b/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs @@ -278,6 +278,24 @@ internal async Task InitializeAsync(CancellationToken cancellationToken) internal void RegisterOnNewConfigEventHandler(Action? onNewConfig) { _onNewConfigEventHandler = onNewConfig; + + // Configuration can already be held here: the Remote Configuration subscription is live + // from construction, long before application code builds a provider and registers a + // handler. The handler only fires on a change, so without this replay it never runs for a + // configuration that arrived first, and a caller waiting on it waits forever. + if (Volatile.Read(ref _evaluator) is null) + { + return; + } + + try + { + onNewConfig?.Invoke(); + } + catch (Exception ex) + { + Log.Warning(ex, "FeatureFlagsModule::RegisterOnNewConfigEventHandler -> Error in the configuration event handler"); + } } internal Evaluation Evaluate(string flagKey, ValueType resultType, object? defaultValue, string targetingKey, IDictionary? attributes) diff --git a/tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsModuleTests.cs b/tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsModuleTests.cs index 156cb960ee25..503def781b51 100644 --- a/tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsModuleTests.cs +++ b/tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsModuleTests.cs @@ -217,6 +217,44 @@ public void ApplyConfiguration_WhenTheEventHandlerThrows_ReportsTheConfiguration module.FirstConfigReceived.IsCompleted.Should().BeTrue(); } + [Fact] + public void RegisterOnNewConfigEventHandler_WhenConfigurationAlreadyArrived_InvokesTheHandler() + { + using var module = CreateModule(CreateSettings(), new MockRcmSubscriptionManager()); + + module.ApplyConfiguration(new ServerConfiguration()).Should().BeTrue(); + + // The handler registers after the configuration landed, which is the ordering an application + // gets whenever Remote Configuration delivers before it builds a provider. + var callbackInvoked = false; + module.RegisterOnNewConfigEventHandler(() => callbackInvoked = true); + + callbackInvoked.Should().BeTrue(); + } + + [Fact] + public void RegisterOnNewConfigEventHandler_WhenNoConfigurationYet_DoesNotInvokeTheHandler() + { + using var module = CreateModule(CreateSettings(), new MockRcmSubscriptionManager()); + + var callbackInvoked = false; + module.RegisterOnNewConfigEventHandler(() => callbackInvoked = true); + + callbackInvoked.Should().BeFalse(); + } + + [Fact] + public void RegisterOnNewConfigEventHandler_WhenTheHandlerThrowsOnReplay_DoesNotPropagate() + { + using var module = CreateModule(CreateSettings(), new MockRcmSubscriptionManager()); + + module.ApplyConfiguration(new ServerConfiguration()).Should().BeTrue(); + + Action register = () => module.RegisterOnNewConfigEventHandler(() => throw new InvalidOperationException("from application code")); + + register.Should().NotThrow(); + } + [Fact] public async Task InitializeAsync_OnTimeout_ReturnsWithoutThrowing() { From c4fe818f0e7e7255458906f76f170bbf928a83f1 Mon Sep 17 00:00:00 2001 From: Pavel Date: Thu, 17 Sep 2026 14:32:34 +0300 Subject: [PATCH 2/2] [FeatureFlags] Deliver each configuration to the handler exactly once --- .../FeatureFlags/FeatureFlagsModule.cs | 106 ++++++++++++++---- .../FeatureFlags/FeatureFlagsModuleTests.cs | 50 +++++++++ 2 files changed, 132 insertions(+), 24 deletions(-) diff --git a/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs b/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs index df31acf0e0e2..37c71137540f 100644 --- a/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs +++ b/tracer/src/Datadog.Trace/FeatureFlags/FeatureFlagsModule.cs @@ -31,6 +31,10 @@ internal sealed class FeatureFlagsModule : IDisposable // module, and a disposal interleaved with activation leaks the delivery path it started. private readonly object _stateLock = new(); + // Guards the configuration handler and its generation counters. Separate from _stateLock, + // which covers activation and disposal: a configuration event must not wait on either. + private readonly object _handlerLock = new(); + private readonly FeatureFlagsSettings _settings; // The agentless source targets flags by environment, which customers can change in code @@ -51,6 +55,13 @@ internal sealed class FeatureFlagsModule : IDisposable private ISubscription? _rcmSubscription; private Action? _onNewConfigEventHandler; + + // Counts configuration events, and records the last one handed to the current handler. A + // registration can run at the same time as an arriving configuration, and without these two + // the registration replay and the delivery path both hand the handler the same configuration. + private long _configGeneration; + private long _handlerDeliveredGeneration; + private FeatureFlagsEvaluator? _evaluator; private IFeatureFlagsDeliverySource? _agentlessSource; private ExposureApi? _exposureApi; @@ -277,25 +288,31 @@ internal async Task InitializeAsync(CancellationToken cancellationToken) internal void RegisterOnNewConfigEventHandler(Action? onNewConfig) { - _onNewConfigEventHandler = onNewConfig; + Action? replay; - // Configuration can already be held here: the Remote Configuration subscription is live - // from construction, long before application code builds a provider and registers a - // handler. The handler only fires on a change, so without this replay it never runs for a - // configuration that arrived first, and a caller waiting on it waits forever. - if (Volatile.Read(ref _evaluator) is null) + lock (_handlerLock) { - return; - } + if (!ReferenceEquals(_onNewConfigEventHandler, onNewConfig)) + { + // A handler that was never called cannot have received a configuration. + _handlerDeliveredGeneration = 0; + } - try - { - onNewConfig?.Invoke(); - } - catch (Exception ex) - { - Log.Warning(ex, "FeatureFlagsModule::RegisterOnNewConfigEventHandler -> Error in the configuration event handler"); + _onNewConfigEventHandler = onNewConfig; + + // Configuration can already be held here: the Remote Configuration subscription is + // live from construction, long before application code builds a provider and registers + // a handler. The handler only fires on a change, so without this replay it never runs + // for a configuration that arrived first, and a caller waiting on it waits forever. + // A withdrawal is not replayed: no configuration is the state a fresh handler assumes. + replay = onNewConfig is not null + && Volatile.Read(ref _evaluator) is not null + && _handlerDeliveredGeneration != _configGeneration + ? ClaimHandler() + : null; } + + InvokeConfigurationHandler(replay, "RegisterOnNewConfigEventHandler"); } internal Evaluation Evaluate(string flagKey, ValueType resultType, object? defaultValue, string targetingKey, IDictionary? attributes) @@ -325,20 +342,61 @@ internal bool ApplyConfiguration(ServerConfiguration configuration) return false; } - // The handler comes from application code, and the agentless source reads the return value - // to decide whether to advance its ETag. Reporting a failed apply because a handler threw - // would make every later poll re-download the whole payload instead of getting a 304, so - // the configuration is already applied by this point and the handler cannot change that. + NotifyNewConfiguration("ApplyConfiguration"); + + return true; + } + + /// + /// Hands one configuration event to the registered handler, whether a configuration arrived or + /// was withdrawn. Claiming the event under the lock and calling the handler outside it keeps + /// application code off the lock, and stops a concurrent registration from replaying the same + /// configuration the handler has just been given. + /// + private void NotifyNewConfiguration(string caller) + { + Action? handler; + + lock (_handlerLock) + { + _configGeneration++; + handler = _onNewConfigEventHandler is null ? null : ClaimHandler(); + } + + InvokeConfigurationHandler(handler, caller); + } + + /// + /// Records the current configuration as delivered to the current handler and returns that + /// handler. Callers hold . + /// + private Action? ClaimHandler() + { + _handlerDeliveredGeneration = _configGeneration; + return _onNewConfigEventHandler; + } + + /// + /// Calls the handler, which is application code. The agentless source reads the return value of + /// to decide whether to advance its ETag. Reporting a failed + /// apply because a handler threw would make every later poll re-download the whole payload + /// instead of getting a 304, and the configuration is applied before the handler runs anyway. + /// + private void InvokeConfigurationHandler(Action? handler, string caller) + { + if (handler is null) + { + return; + } + try { - _onNewConfigEventHandler?.Invoke(); + handler(); } catch (Exception ex) { - Log.Warning(ex, "FeatureFlagsModule::ApplyConfiguration -> Error in the configuration event handler"); + Log.Warning(ex, "FeatureFlagsModule::{Caller} -> Error in the configuration event handler", caller); } - - return true; } /// @@ -389,7 +447,7 @@ private void ApplyRemoteConfigurations(List invocations++; + module.RegisterOnNewConfigEventHandler(handler); + + module.ApplyConfiguration(new ServerConfiguration()).Should().BeTrue(); + + // The handler already has this configuration from the apply, so registering the same handler + // again must not replay it. Each handler gets each configuration exactly once. + module.RegisterOnNewConfigEventHandler(handler); + + invocations.Should().Be(1); + } + + [Fact] + public async Task RegisterOnNewConfigEventHandler_WhenRegistrationRacesWithApply_DeliversEachConfigurationOnce() + { + // Registration and delivery both look at the handler and the held configuration. One pass + // rarely interleaves them, so repeat: an interleaving that hands the same configuration to + // both paths invokes the handler twice and fails here. + for (var i = 0; i < 200; i++) + { + using var module = CreateModule(CreateSettings(), new MockRcmSubscriptionManager()); + + var invocations = 0; + using var start = new ManualResetEventSlim(false); + + var register = Task.Run(() => + { + start.Wait(); + module.RegisterOnNewConfigEventHandler(() => Interlocked.Increment(ref invocations)); + }); + + var apply = Task.Run(() => + { + start.Wait(); + module.ApplyConfiguration(new ServerConfiguration()); + }); + + start.Set(); + await Task.WhenAll(register, apply); + + invocations.Should().BeLessOrEqualTo(1, "iteration {0} delivered one configuration more than once", i); + } + } + [Fact] public async Task InitializeAsync_OnTimeout_ReturnsWithoutThrowing() {