Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications

[Copilot]

Fixes #1688

Registering a batch of N tools/prompts/resources emits N list_changed wire notifications -- one per TryAdd -- instead of one. This can trip clients that debounce or rate-limit notifications, and is wasteful for progressive-disclosure patterns.

Changes

• DeferChanges() on McpServerPrimitiveCollection<T> -- returns an IDisposable scope that suppresses Changed events. A single notification fires when the outermost scope disposes, only if at least one mutation occurred. Nesting is supported via a depth counter.
• Thread-safe implementation -- _deferralDepth and _pendingChange are coordinated with Interlocked/Volatile; RaiseChanged() defers when depth > 0, and the DeferralScope dispose is idempotent via Interlocked.Exchange.
• Unit tests covering: no-mutation scope, single/multiple mutations, mixed add+remove, Clear, nested scopes, idempotent dispose, no-handler, and baseline non-deferred behavior.

Usage

using (options.ToolCollection.DeferChanges())
{
    foreach (var tool in group.Tools)
        options.ToolCollection.TryAdd(tool);
} // exactly one notifications/tools/list_changed emitted here

Purely additive -- no behavior change for callers that never call DeferChanges(). Applies uniformly to tool, prompt, and resource collections.

[tarekgh]

Faithful implementation of the agreed design, and it picks up the gate-at-RaiseChanged and net-zero-fires points. One correctness issue on the concurrency handling, plus a small doc gap, noted inline.

[tarekgh]

The deferral check here and EndDeferral are not atomic together, so a mutation racing with disposal of the last scope can be dropped. If this line reads depth > 0, but EndDeferral then runs Decrement to 0 and Exchange(_pendingChange, 0) before line 79 sets _pendingChange to 1, the pending change is left set with no active scope to flush it, so no notification is ever sent. That is exactly the missed list_changed this PR aims to prevent, so the Interlocked/Volatile approach does not fully deliver the thread-safety claim.

Suggest guarding the depth/pending transitions with a small lock and invoking Changed outside it. Mutation frequency is low, so the lock only wraps a few integer ops and does not affect ConcurrentDictionary throughput:

private readonly object _deferralLock = new();

protected void RaiseChanged()
{
    lock (_deferralLock)
    {
        if (_deferralDepth > 0) { _pendingChange = true; return; }
    }
    Changed?.Invoke(this, EventArgs.Empty);
}

private void EndDeferral()
{
    bool raise;
    lock (_deferralLock)
    {
        raise = --_deferralDepth == 0 && _pendingChange;
        if (raise) _pendingChange = false;
    }
    if (raise) Changed?.Invoke(this, EventArgs.Empty);
}

DeferChanges() should increment _deferralDepth under the same lock. A multithreaded test (N threads adding inside one shared scope, asserting a single Changed) would lock this in.

[Copilot]

Fixed in the latest commit. Replaced the Interlocked/Volatile approach with a lock (_deferralLock) guard around all _deferralDepth and _pendingChange transitions -- DeferChanges(), RaiseChanged(), and EndDeferral(). _pendingChange is now a bool rather than an int. The Changed invocation still happens outside the lock. The concurrency tests (DeferChanges_ConcurrentMutations_FiresExactlyOneChanged and DeferChanges_MutationRacingWithDispose_NotificationNotLost) lock this in.

[tarekgh]

Please add a remark that mutations from any thread during an open scope are coalesced, and the single notification fires when the outermost scope disposes. The concurrent behavior is asserted in the PR description but not documented on the API.

[Copilot]

Done in the latest commit -- added a <para> block to the <remarks> on DeferChanges() documenting that mutations from any thread during an open scope are coalesced, the single notification fires on the thread that disposes the outermost scope, and that all deferral state transitions are guarded by an internal lock.

[tarekgh]

A couple of tests are missing to back the review points, plus a few worth adding:

Required:

1. Concurrency test: N threads each TryAdd a distinct tool inside one shared DeferChanges() scope, then assert exactly one Changed and Count == N after all scopes dispose. Add a variant where one thread mutates while another disposes the last scope, asserting the notification is not lost. This exercises the thread-safety claim and pairs with the locking fix above (it may be flaky against the current lock-free code and becomes deterministic once locked).
2. Derived-type coalescing: a subclass that calls RaiseChanged() directly inside a scope, asserting it coalesces to one notification. Nothing currently covers the gate-at-RaiseChanged path that McpServerResourceCollection relies on.

Recommended:

3. End-to-end: every test counts the in-process Changed event, not the wire message. Add one through a real client/server (see the list_changed pattern in McpServerBuilderExtensionsToolsTests) that batches inside DeferChanges() and asserts the client receives exactly one notifications/tools/list_changed.
4. Exception-safety: mutate, throw inside the using block, then assert one Changed fired and the next mutation after the scope fires immediately (deferral fully reset). This validates the guarantee stated in the XML docs.
5. Prompt/resource coverage: all tests use McpServerTool; add one on a prompt or resource collection to confirm the uniform behavior the PR claims.

[jeffhandley]

@copilot Please address the following excellent feedback from @tarekgh:

1. Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications #1689 (comment), where it's proven that the Interlocked.Exchange approach was insufficient and we need to use a lock instead. That means we can use a bool for flagging when there are pending changes vs. the esoteric int approach. This also addresses the idempotent Changed invocation outside a lock feedback noted in Add batched change notifications to McpServerPrimitiveCollection<T> #1688 (comment)
2. Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications #1689 (comment), which requests adding a <remarks> blurb about threading behavior.
3. Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications #1689 (comment), which requests:
   a. Adding concurrency tests
   b. Derived-type coalescing, which is also captured as a design shortfall in the Add batched change notifications to McpServerPrimitiveCollection<T> #1688 (comment) comment on the backing issue (after this PR was created)
   c. Add end-to-end functional tests in the appropriate test classes that covers list_changed event payloads for all collections that gain this behavior by using this collection type
   d. Exception-exercising tests that assert the disposal still functions as expected raising the events

[Copilot]

@copilot Please address the following excellent feedback from @tarekgh:

1. Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications #1689 (comment), where it's proven that the `Interlock...

All addressed in the latest commit:

1. Concurrency fix -- _deferralDepth and _pendingChange are now guarded by _deferralLock throughout DeferChanges(), RaiseChanged(), and EndDeferral(). _pendingChange is a bool. Changed is invoked outside the lock.
2. Threading remarks -- added a <para> block to DeferChanges() documenting coalescing across threads and the lock guarantee.
3. Tests added to McpServerPrimitiveCollectionTests:
   • DeferChanges_ConcurrentMutations_FiresExactlyOneChanged -- 10 threads each adding inside a shared scope; asserts one Changed and Count == 10.
   • DeferChanges_MutationRacingWithDispose_NotificationNotLost -- 200 iterations of a mutation racing concurrently with scope disposal; asserts exactly one Changed every time.
   • DeferChanges_DerivedTypeCallsRaiseChanged_Coalesces / DeferChanges_DerivedTypeRaisesChanged_OutsideScope_FiresImmediately -- subclass calls RaiseChanged() directly; coalesces inside a scope, fires immediately outside.
   • DeferChanges_ExceptionDuringScope_StillFiresChanged / DeferChanges_ExceptionDuringScope_ResumesImmediateNotificationsAfterward -- scope disposes on exception; deferral fully reset afterward.
   • DeferChanges_PromptCollection_MultipleMutations_FiresOneChanged -- prompt collection coverage.
4. End-to-end tests -- DeferChanges_BatchAddTools_EmitsExactlyOneNotification, DeferChanges_BatchAddPrompts_EmitsExactlyOneNotification, and DeferChanges_BatchAddResources_EmitsExactlyOneNotification added to the respective McpServerBuilderExtensions*Tests classes. Each pins the legacy protocol, batches three adds inside DeferChanges(), waits for exactly one wire notification, does a List* round-trip to let any second notification arrive, then asserts notificationCount == 1.

[PranavSenthilnathan]

The naming makes this sound like mutations to the collection are being deferred until the scope closes. Something like DeferChangedEvents might be clearer.

[PranavSenthilnathan]

_activeDeferralScopes seems more self-describing