[FeatureFlags] Replay the configuration handler on registration and bump the OpenFeature package

tracer/src/Datadog.FeatureFlags.OpenFeature/Datadog.FeatureFlags.OpenFeature.csproj

@@ -17,7 +17,7 @@
     <!-- OpenFeature v2.3.0  = Datadog.FeatureFlags.OpenFeature 2.3.1 (patch release)-->    
     <!-- OpenFeature v2.3.1  = Datadog.FeatureFlags.OpenFeature 2.3.100 (Open Feature patch)-->    
     <!-- OpenFeature v2.3.1  = Datadog.FeatureFlags.OpenFeature 2.3.101 (Open Feature patch + dd patch)-->    
-    <Version>2.3.0</Version>
+    <Version>2.3.1</Version>
     <!-- These target frameworks should match the values exposed in the OpenFeature package referenced below-->
     <TargetFrameworks>net462;netstandard2.0;net8.0;net9.0</TargetFrameworks>
   </PropertyGroup>

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,7 +288,31 @@ internal async Task InitializeAsync(CancellationToken cancellationToken)
 
         internal void RegisterOnNewConfigEventHandler(Action? onNewConfig)
         {
-            _onNewConfigEventHandler = onNewConfig;
+            Action? replay;
+
+            lock (_handlerLock)
+            {
+                if (!ReferenceEquals(_onNewConfigEventHandler, onNewConfig))
+                {
+                    // A handler that was never called cannot have received a configuration.
+                    _handlerDeliveredGeneration = 0;
+                }
+
+                _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<string, object?>? attributes)
@@ -307,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;
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        private void NotifyNewConfiguration(string caller)
+        {
+            Action? handler;
+
+            lock (_handlerLock)
+            {
+                _configGeneration++;
+                handler = _onNewConfigEventHandler is null ? null : ClaimHandler();
+            }
+
+            InvokeConfigurationHandler(handler, caller);
+        }
+
+        /// <summary>
+        /// Records the current configuration as delivered to the current handler and returns that
+        /// handler. Callers hold <see cref="_handlerLock"/>.
+        /// </summary>
+        private Action? ClaimHandler()
+        {
+            _handlerDeliveredGeneration = _configGeneration;
+            return _onNewConfigEventHandler;
+        }
+
+        /// <summary>
+        /// Calls the handler, which is application code. The agentless source reads the return value of
+        /// <see cref="ApplyConfiguration"/> 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.
+        /// </summary>
+        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<string>(ex, "FeatureFlagsModule::{Caller} -> Error in the configuration event handler", caller);
             }
-
-            return true;
         }
 
         /// <summary>
@@ -371,7 +447,7 @@ private void ApplyRemoteConfigurations(List<KeyValuePair<string, ServerConfigura
                     // from here on. The handler is notified either way, and reads HasConfiguration to
                     // tell a withdrawal from an update.
                     Interlocked.Exchange(ref _evaluator, null);
-                    _onNewConfigEventHandler?.Invoke();
+                    NotifyNewConfiguration("ApplyRemoteConfigurations");
                 }
             }
             catch (Exception ex)

tracer/test/Datadog.Trace.Tests/FeatureFlags/FeatureFlagsModuleTests.cs

@@ -217,6 +217,94 @@ 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 void RegisterOnNewConfigEventHandler_AfterAConfigurationWasDelivered_DoesNotDeliverItAgain()
+    {
+        using var module = CreateModule(CreateSettings(), new MockRcmSubscriptionManager());
+
+        var invocations = 0;
+        Action handler = () => 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()
     {