Skip to content

Add DeferChangedEvents() to McpServerPrimitiveCollection for batched change notifications#1689

Open
jeffhandley with Copilot wants to merge 10 commits into
mainfrom
copilot/add-batched-change-notifications
Open

Add DeferChangedEvents() to McpServerPrimitiveCollection for batched change notifications#1689
jeffhandley with Copilot wants to merge 10 commits into
mainfrom
copilot/add-batched-change-notifications

Conversation

Copilot AI commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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

  • DeferChangedEvents() 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 -- _activeDeferralScopes and _hasDeferredChangeEvents are guarded by _deferralLock. RaiseChanged() defers when _activeDeferralScopes > 0, and ChangeDeferralScope.Dispose() is idempotent via Interlocked.Exchange. Changed is invoked outside the lock.
  • Unit tests covering: no-mutation scope, single/multiple mutations, mixed add+remove, Clear, nested scopes, idempotent dispose, no-handler, baseline non-deferred behavior, concurrent mutations, mutation racing with dispose, derived-type coalescing, exception-safety, and prompt/resource collection coverage.
  • End-to-end tests in McpServerBuilderExtensionsToolsTests, McpServerBuilderExtensionsPromptsTests, and McpServerBuilderExtensionsResourcesTests asserting exactly one wire notification for a batch add.

Usage

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

… notifications

- Add _deferralDepth and _pendingChange int fields for thread-safe deferral tracking
- Update RaiseChanged() to defer notification when _deferralDepth > 0
- Add DeferChanges() public method returning a DeferralScope : IDisposable
- Add EndDeferral() private method using Interlocked ops for atomicity
- Add unit tests: no-mutation scope, single/multi-mutations, mixed add+remove,
  Clear, nested scopes, idempotent dispose, no-handler case, baseline behavior

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI changed the title [WIP] Add batched change notifications to McpServerPrimitiveCollection Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications Jul 2, 2026
Copilot AI requested a review from jeffhandley July 2, 2026 02:59
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
- Rename DeferralScope to ChangeDeferralScope
- Add preexisting Changed behavior tests (TryAdd, Remove, Clear -- with and without success)
- Add DeferChanges tests for duplicate TryAdd (with and without prior entry)
- Add DeferChanges test for TryAdd-then-Remove same tool (fires one Changed even though net content is unchanged)

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI requested a review from jeffhandley July 2, 2026 03:54
@jeffhandley jeffhandley marked this pull request as ready for review July 2, 2026 03:58
Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI requested a review from jeffhandley July 2, 2026 04:00
Copilot stopped work on behalf of jeffhandley due to an error July 2, 2026 04:45
Copilot AI requested a review from jeffhandley July 2, 2026 04:45

@tarekgh tarekgh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
@tarekgh

tarekgh commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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:

  1. 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.
  2. 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.
  3. Prompt/resource coverage: all tests use McpServerTool; add one on a prompt or resource collection to confirm the uniform behavior the PR claims.

@jeffhandley

Copy link
Copy Markdown
Contributor

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

  1. Add DeferChangedEvents() 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 DeferChangedEvents() to McpServerPrimitiveCollection for batched change notifications #1689 (comment), which requests adding a <remarks> blurb about threading behavior.
  3. Add DeferChangedEvents() 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

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
…erralDepth->_activeDeferralScopes

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI requested a review from jeffhandley July 3, 2026 01:54
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI requested a review from jeffhandley July 3, 2026 02:14
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
Comment thread src/ModelContextProtocol.Core/Server/McpServerPrimitiveCollection.cs Outdated
…in exception-safety sentence

Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Copilot AI requested a review from jeffhandley July 3, 2026 04:22
@jeffhandley jeffhandley changed the title Add DeferChanges() to McpServerPrimitiveCollection for batched change notifications Add DeferChangedEvents() to McpServerPrimitiveCollection for batched change notifications Jul 3, 2026

@jeffhandley jeffhandley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot add unit tests that verify out-of-order disposals result in the Changed event being raised once all deferrals have been disposed. This can be achieved by capturing the IDisposable instances and calling Dispose() rather than with using blocks.

These tests should ensure re-disposing the same deferral instance multiple times also does not result in the count decrementing more than once (potentially leading to the Changed event being raised while there are still undisposed deferrals.

With this, we should change the "outermost" phrasing to say once all deferrals have been disposed, as the nesting isn't a requirement.

If you find bugs with these new tests, fix them.

…rmost phrasing

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

Copilot AI commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add unit tests that verify out-of-order disposals result in the Changed event being raised once all deferrals have been disposed. This can be achieved by capturing the IDisposable instances...

Done in the latest commit. Added four new tests:

  • DeferChangedEvents_OutOfOrderDisposal_FiresOnceWhenAllScopesDisposed -- two scopes created in order 1, 2, disposed in reverse order 2, 1; verifies Changed does not fire after scope 2 disposes (scope 1 still active) and fires exactly once after scope 1 disposes.
  • DeferChangedEvents_DoubleDisposeSingleScope_DoesNotDecrementCountTwice -- scope 1 and scope 2 both active; scope 1 disposed twice; verifies Changed does not fire (scope 2 still active) and the second dispose of scope 1 is a no-op that does not decrement the counter.
  • DeferChangedEvents_OutOfOrderDisposalNoMutation_DoesNotFireChanged -- same out-of-order pattern with no mutations; verifies no spurious Changed.
  • DeferChangedEvents_Nested_FiresOnceWhenAllScopesDisposed -- renamed from ...OutermostDispose to match the updated contract.

Updated the DeferChangedEvents() XML docs to say "fires once all active scopes have been disposed" (removed "outermost" and the nesting framing), and updated the RaiseChanged() remarks similarly. All 30 tests pass.

Copilot AI requested a review from jeffhandley July 3, 2026 04:34
@jeffhandley jeffhandley requested a review from tarekgh July 3, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add batched change notifications to McpServerPrimitiveCollection<T>

5 participants