Address review feedback on DeferChanges concurrency and tests

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>

Author: Copilot

src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs

@@ -12,11 +12,14 @@ public class McpServerPrimitiveCollection<T> : ICollection<T>, IReadOnlyCollecti
     /// <summary>Concurrent dictionary of primitives, indexed by their names.</summary>
     private readonly ConcurrentDictionary<string, T> _primitives;
 
+    /// <summary>Lock protecting <see cref="_deferralDepth"/> and <see cref="_pendingChange"/>.</summary>
+    private readonly object _deferralLock = new();
+
     /// <summary>Depth counter for active <see cref="DeferChanges"/> scopes. Positive means notifications are deferred.</summary>
     private int _deferralDepth;
 
-    /// <summary>Whether a change occurred while notifications were deferred. 1 means pending, 0 means none.</summary>
-    private int _pendingChange;
+    /// <summary>Whether a change occurred while notifications were deferred.</summary>
+    private bool _pendingChange;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="McpServerPrimitiveCollection{T}"/> class.
@@ -59,10 +62,19 @@ public McpServerPrimitiveCollection(IEqualityComparer<string>? keyComparer = nul
     /// The scope is exception-safe: even if an exception is thrown inside the <c>using</c> block,
     /// the deferral is ended on dispose. If any mutation occurred before the exception, a single
     /// <see cref="Changed"/> notification is raised.
+    /// <para>
+    /// Mutations from any thread during an open scope are coalesced. A single <see cref="Changed"/>
+    /// notification fires on the thread that disposes the outermost scope, only if at least one
+    /// mutation occurred. All deferral state transitions are guarded by an internal lock, so
+    /// concurrent mutations and concurrent scope disposal are both safe.
+    /// </para>
     /// </remarks>
     public IDisposable DeferChanges()
     {
-        Interlocked.Increment(ref _deferralDepth);
+        lock (_deferralLock)
+        {
+            _deferralDepth++;
+        }
         return new ChangeDeferralScope(this);
     }
 
@@ -74,21 +86,33 @@ public IDisposable DeferChanges()
     /// </remarks>
     protected void RaiseChanged()
     {
-        if (Volatile.Read(ref _deferralDepth) > 0)
+        lock (_deferralLock)
         {
-            Interlocked.Exchange(ref _pendingChange, 1);
-            return;
+            if (_deferralDepth > 0)
+            {
+                _pendingChange = true;
+                return;
+            }
         }
 
         Changed?.Invoke(this, EventArgs.Empty);
     }
 
     private void EndDeferral()
     {
-        if (Interlocked.Decrement(ref _deferralDepth) == 0 &&
-            Interlocked.Exchange(ref _pendingChange, 0) == 1)
+        bool raise;
+        lock (_deferralLock)
         {
-            RaiseChanged();
+            raise = --_deferralDepth == 0 && _pendingChange;
+            if (raise)
+            {
+                _pendingChange = false;
+            }
+        }
+
+        if (raise)
+        {
+            Changed?.Invoke(this, EventArgs.Empty);
         }
     }
 

tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsPromptsTests.cs

@@ -173,6 +173,51 @@ public async Task Can_Be_Notified_Of_Prompt_Changes()
         Assert.DoesNotContain(prompts, t => t.Name == "NewPrompt");
     }
 
+    [Fact]
+    public async Task DeferChanges_BatchAddPrompts_EmitsExactlyOneNotification()
+    {
+        // Under the 2026-07-28 protocol, list-changed notifications are delivered only over a
+        // subscriptions/listen stream. Pin the legacy revision to test the session-wide broadcast.
+        await using McpClient client = await CreateMcpClientForServer(new McpClientOptions
+        {
+            ProtocolVersion = McpHttpHeaders.November2025ProtocolVersion,
+        });
+
+        var serverOptions = ServiceProvider.GetRequiredService<IOptions<McpServerOptions>>().Value;
+        var serverPrompts = serverOptions.PromptCollection;
+        Assert.NotNull(serverPrompts);
+
+        int notificationCount = 0;
+        var firstNotification = new TaskCompletionSource();
+
+        await using (client.RegisterNotificationHandler(NotificationMethods.PromptListChangedNotification, (notification, cancellationToken) =>
+            {
+                if (Interlocked.Increment(ref notificationCount) == 1)
+                {
+                    firstNotification.TrySetResult();
+                }
+                return default;
+            }))
+        {
+            using (serverPrompts.DeferChanges())
+            {
+                serverPrompts.Add(McpServerPrompt.Create([McpServerPrompt(Name = "BatchPrompt1")] () => "1"));
+                serverPrompts.Add(McpServerPrompt.Create([McpServerPrompt(Name = "BatchPrompt2")] () => "2"));
+                serverPrompts.Add(McpServerPrompt.Create([McpServerPrompt(Name = "BatchPrompt3")] () => "3"));
+            }
+
+            await firstNotification.Task.WaitAsync(TestContext.Current.CancellationToken);
+
+            // Do a round-trip so that any second (erroneous) notification has time to arrive.
+            var prompts = await client.ListPromptsAsync(cancellationToken: TestContext.Current.CancellationToken);
+            Assert.Contains(prompts, t => t.Name == "BatchPrompt1");
+            Assert.Contains(prompts, t => t.Name == "BatchPrompt2");
+            Assert.Contains(prompts, t => t.Name == "BatchPrompt3");
+
+            Assert.Equal(1, notificationCount);
+        }
+    }
+
     [Fact]
     public async Task AttributeProperties_Propagated()
     {

tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsResourcesTests.cs

@@ -207,6 +207,51 @@ public async Task Can_Be_Notified_Of_Resource_Changes()
         Assert.DoesNotContain(resources, t => t.Name == "NewResource");
     }
 
+    [Fact]
+    public async Task DeferChanges_BatchAddResources_EmitsExactlyOneNotification()
+    {
+        // Under the 2026-07-28 protocol, list-changed notifications are delivered only over a
+        // subscriptions/listen stream. Pin the legacy revision to test the session-wide broadcast.
+        await using McpClient client = await CreateMcpClientForServer(new McpClientOptions
+        {
+            ProtocolVersion = McpHttpHeaders.November2025ProtocolVersion,
+        });
+
+        var serverOptions = ServiceProvider.GetRequiredService<IOptions<McpServerOptions>>().Value;
+        var serverResources = serverOptions.ResourceCollection;
+        Assert.NotNull(serverResources);
+
+        int notificationCount = 0;
+        var firstNotification = new TaskCompletionSource();
+
+        await using (client.RegisterNotificationHandler(NotificationMethods.ResourceListChangedNotification, (notification, cancellationToken) =>
+            {
+                if (Interlocked.Increment(ref notificationCount) == 1)
+                {
+                    firstNotification.TrySetResult();
+                }
+                return default;
+            }))
+        {
+            using (serverResources.DeferChanges())
+            {
+                serverResources.Add(McpServerResource.Create([McpServerResource(Name = "BatchResource1", UriTemplate = "test://batch1")] () => "1"));
+                serverResources.Add(McpServerResource.Create([McpServerResource(Name = "BatchResource2", UriTemplate = "test://batch2")] () => "2"));
+                serverResources.Add(McpServerResource.Create([McpServerResource(Name = "BatchResource3", UriTemplate = "test://batch3")] () => "3"));
+            }
+
+            await firstNotification.Task.WaitAsync(TestContext.Current.CancellationToken);
+
+            // Do a round-trip so that any second (erroneous) notification has time to arrive.
+            var resources = await client.ListResourcesAsync(cancellationToken: TestContext.Current.CancellationToken);
+            Assert.Contains(resources, t => t.Name == "BatchResource1");
+            Assert.Contains(resources, t => t.Name == "BatchResource2");
+            Assert.Contains(resources, t => t.Name == "BatchResource3");
+
+            Assert.Equal(1, notificationCount);
+        }
+    }
+
     [Fact]
     public async Task AttributeProperties_Propagated()
     {

tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs

@@ -232,6 +232,51 @@ public async Task Can_Be_Notified_Of_Tool_Changes()
         Assert.DoesNotContain(tools, t => t.Name == "NewTool");
     }
 
+    [Fact]
+    public async Task DeferChanges_BatchAddTools_EmitsExactlyOneNotification()
+    {
+        // Under the 2026-07-28 protocol, list-changed notifications are delivered only over a
+        // subscriptions/listen stream. Pin the legacy revision to test the session-wide broadcast.
+        await using McpClient client = await CreateMcpClientForServer(new McpClientOptions
+        {
+            ProtocolVersion = McpHttpHeaders.November2025ProtocolVersion,
+        });
+
+        var serverOptions = ServiceProvider.GetRequiredService<IOptions<McpServerOptions>>().Value;
+        var serverTools = serverOptions.ToolCollection;
+        Assert.NotNull(serverTools);
+
+        int notificationCount = 0;
+        var firstNotification = new TaskCompletionSource();
+
+        await using (client.RegisterNotificationHandler(NotificationMethods.ToolListChangedNotification, (notification, cancellationToken) =>
+            {
+                if (Interlocked.Increment(ref notificationCount) == 1)
+                {
+                    firstNotification.TrySetResult();
+                }
+                return default;
+            }))
+        {
+            using (serverTools.DeferChanges())
+            {
+                serverTools.Add(McpServerTool.Create([McpServerTool(Name = "BatchTool1")] () => "1"));
+                serverTools.Add(McpServerTool.Create([McpServerTool(Name = "BatchTool2")] () => "2"));
+                serverTools.Add(McpServerTool.Create([McpServerTool(Name = "BatchTool3")] () => "3"));
+            }
+
+            await firstNotification.Task.WaitAsync(TestContext.Current.CancellationToken);
+
+            // Do a round-trip so that any second (erroneous) notification has time to arrive.
+            var tools = await client.ListToolsAsync(cancellationToken: TestContext.Current.CancellationToken);
+            Assert.Contains(tools, t => t.Name == "BatchTool1");
+            Assert.Contains(tools, t => t.Name == "BatchTool2");
+            Assert.Contains(tools, t => t.Name == "BatchTool3");
+
+            Assert.Equal(1, notificationCount);
+        }
+    }
+
     [Fact]
     public async Task Can_Call_Registered_Tool()
     {