Preserve insertion order when listing server primitives

[PederHP]

Fixes #1923

Problem

McpServerPrimitiveCollection<T> enumerated its ConcurrentDictionary directly, so the built-in list handlers returned primitives in hash order:

• resources/list and resources/templates/list changed order on every process start, because UriTemplate.UriTemplateComparer uses randomized string hashing.
• tools/list and prompts/list were stable across runs, but in an arbitrary order that could also change as entries were added or removed.

Anything that needs stable ordering (e.g. dynamically constructed context) to avoid cache-busting was affected. Systems that snapshot for integrity might also be affected.

Change

Primitives are now listed in registration order. With WithTools<T>(), WithPrompts<T>() and WithResources<T>(), that is the order reflection discovers the methods in, which is declaration order in practice. This matches the Python and TypeScript SDKs. A primitive that is removed and added again moves to the end; the class remarks document this.

How the collection does it:

• Lookups by name still go through the ConcurrentDictionary and stay lock-free.
• A List<Entry> records insertion order. TryAdd, Remove and Clear take a mutation lock that keeps the dictionary and the list in sync, so every successful mutation has a single ordering point. A primitive that is already visible can't be reordered by a concurrent add.
• Enumeration reads a cached Entry[] snapshot. Mutations invalidate it under the lock and the next read rebuilds it, so repeated list requests don't lock, sort or allocate a copy. GetEnumerator walks the snapshot directly; ToArray, PrimitiveNames and CopyTo copy into caller-owned storage.
• Each entry stores the key it was added under. PrimitiveNames returns that key, and Remove reads primitive.Id once and uses the same key for lookup and removal, since Id is backed by mutable protocol properties.
• Remove removes only the exact entry it compared, so a primitive whose Equals reenters the collection and replaces the entry under the same key can't desynchronize the dictionary and the ordered list.
• Remove and Contains still match on both name and instance, and Changed is still raised outside the lock.

Ordering within a batch registered from a type or assembly stays as reflection returns it. Sorting by MetadataToken was considered and left out (see discussion).

Tests

• Unit tests in McpServerPrimitiveCollectionTests cover order for tools, prompts and resources, remove and re-add, Clear, removing a different instance with the same name, a reentrant Equals that replaces the entry during Remove, PrimitiveNames after a primitive is renamed, caller-owned copies from ToArray/PrimitiveNames, and CopyTo argument validation.
• Enumeration_AlreadyVisiblePrimitive_KeepsItsPosition uses a key comparer that reenters the collection before the first add is inserted, deterministically forcing the interleaving where a later add becomes visible first. It fails against the earlier sequence-number version of this PR and passes now.
• Enumeration_AddStalledMidway_LaterAddDoesNotOvertakeIt is best-effort threaded coverage of the same scenario: it stalls one add on another thread and gives a second add a window to complete. Waits are bounded so a regression fails rather than hangs.
• New McpServerPrimitiveListOrderTests checks all four list methods end to end, using WithTools<T>(), WithPrompts<T>() and WithResources<T>() with names deliberately not in alphabetical order.
• The full ModelContextProtocol.Tests suite passes on net10.0 (2408 passed, 5 skipped), and the collection and list-order tests also pass on net8.0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SNwgdcsZB2wxPqPZa2SC4y

[Copilot]

🟢 Approval recommended

The implementation consistently preserves insertion order while retaining thread-safe lookup and removal semantics.

0 open findings

What changed in this PR

Preserves insertion order when enumerating registered MCP server primitives.

Changes:

• Associates primitives with monotonic insertion sequence numbers.
• Returns ordered snapshots from enumeration APIs.
• Adds unit and end-to-end ordering tests.

File	Description
src/​ModelContextProtocol.Core/​Server/​McpServerPrimitiveCollection.cs	Implements ordered snapshots and safe removal.
tests/​ModelContextProtocol.Tests/​Server/​McpServerPrimitiveCollectionTests.cs	Tests collection ordering and re-add behavior.
tests/​ModelContextProtocol.Tests/​Configuration/​McpServerPrimitiveListOrderTests.cs	Tests ordering through MCP list endpoints.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tarekgh]

I am wondering, instead of paying the extra cost all the time, why don't expose something like GetOrderedEntries()? who cares about the order can use that without affecting the mainstream users.

@halter73 what do you think?

[PederHP]

I am wondering, instead of paying the extra cost all the time, why don't expose something like GetOrderedEntries()? who cares about the order can use that without affecting the mainstream users.

@halter73 what do you think?

Well the problem is that the caller is an MCP client which expects a stable ordering (and could use any SDK), so the server has to make a choice whether to have that or not. So exposing this at the code level doesn't solve the issue for a protocol-level client, it just allows server authors to write a filter and sort the entries, which is extra busy work and should really be the default, imo.

I think it would be good to align with the Python and TypeScript SDKs, especially now that caching has become more important than it used to be.

[tarekgh]

I am wondering, instead of paying the extra cost all the time, why don't expose something like GetOrderedEntries()? who cares about the order can use that without affecting the mainstream users.
@halter73 what do you think?

Well the problem is that the caller is an MCP client which expects a stable ordering (and could use any SDK), so the server has to make a choice whether to have that or not. So exposing this at the code level doesn't solve the issue for a protocol-level client, it just allows server authors to write a filter and sort the entries, which is extra busy work and should really be the default, imo.

I think it would be good to align with the Python and TypeScript SDKs, especially now that caching has become more important than it used to be.

Make sense to me. Thanks!

[tarekgh]

The current implementation has concurrency and performance issues that work against the stability this PR is trying to provide.

TryAdd assigns the sequence number before calling ConcurrentDictionary.TryAdd. With concurrent additions, A can receive sequence 1 and stall while B receives sequence 2 and completes insertion. A client can then observe [B], followed by [A, B] after A completes. An already visible primitive moves even though the collection is intended to preserve stable insertion order.

Remove also reads primitive.Id twice, once for TryGetValue and again for the conditional dictionary removal. Since built-in primitive names are backed by mutable protocol properties, a concurrent name change can cause the lookup to succeed with one key and the removal to use another. Remove then returns false and leaves the entry indexed under its original key.

PrimitiveNames has a related consistency problem because it now reads Primitive.Id instead of returning the key captured at insertion. A renamed primitive can therefore appear under one name in PrimitiveNames while remaining accessible only through its original dictionary key.

Finally, GetOrderedEntries snapshots and sorts the complete dictionary on every enumeration. The built-in tools, prompts, resources, and resource-template list handlers take this path on every list request, even when the collection has not changed.

Could we instead use:

• A ConcurrentDictionary<string, Entry> for lock-free O(1) lookup.
• A List<Entry> that records insertion order.
• A normal mutation lock protecting successful add, remove, and clear operations.
• A private cached Entry[] snapshot that is invalidated under the mutation lock.
• An immutable insertion key stored in each Entry.

Serializing mutations gives each successful add or remove a clear ordering point and removes the need for sequence numbers. Add remains amortized O(1). Removal from the ordered list is O(n), but primitive collections are normally small and mutations are uncommon.

The first enumeration after a mutation would rebuild the snapshot in O(n). Subsequent enumerations would traverse the cached snapshot without locking, sorting, or additional allocation. The snapshot array must remain private: GetEnumerator may traverse it directly, while ToArray, PrimitiveNames, and CopyTo should copy into caller-owned storage so users cannot mutate the cached structure.

Remove should capture primitive.Id once and use that same key for lookup and removal. PrimitiveNames should use the immutable key stored in Entry. Changed should continue to be raised only after releasing the mutation lock.

This design would provide reliable behavior under concurrent mutations while retaining lock-free lookup and efficient repeated listing.

[PederHP]

The current implementation has concurrency and performance issues that work against the stability this PR is trying to provide.

TryAdd assigns the sequence number before calling ConcurrentDictionary.TryAdd. With concurrent additions, A can receive sequence 1 and stall while B receives sequence 2 and completes insertion. A client can then observe [B], followed by [A, B] after A completes. An already visible primitive moves even though the collection is intended to preserve stable insertion order.

Remove also reads primitive.Id twice, once for TryGetValue and again for the conditional dictionary removal. Since built-in primitive names are backed by mutable protocol properties, a concurrent name change can cause the lookup to succeed with one key and the removal to use another. Remove then returns false and leaves the entry indexed under its original key.

PrimitiveNames has a related consistency problem because it now reads Primitive.Id instead of returning the key captured at insertion. A renamed primitive can therefore appear under one name in PrimitiveNames while remaining accessible only through its original dictionary key.

Finally, GetOrderedEntries snapshots and sorts the complete dictionary on every enumeration. The built-in tools, prompts, resources, and resource-template list handlers take this path on every list request, even when the collection has not changed.

Could we instead use:

• A ConcurrentDictionary<string, Entry> for lock-free O(1) lookup.
• A List<Entry> that records insertion order.
• A normal mutation lock protecting successful add, remove, and clear operations.
• A private cached Entry[] snapshot that is invalidated under the mutation lock.
• An immutable insertion key stored in each Entry.

Serializing mutations gives each successful add or remove a clear ordering point and removes the need for sequence numbers. Add remains amortized O(1). Removal from the ordered list is O(n), but primitive collections are normally small and mutations are uncommon.

The first enumeration after a mutation would rebuild the snapshot in O(n). Subsequent enumerations would traverse the cached snapshot without locking, sorting, or additional allocation. The snapshot array must remain private: GetEnumerator may traverse it directly, while ToArray, PrimitiveNames, and CopyTo should copy into caller-owned storage so users cannot mutate the cached structure.

Remove should capture primitive.Id once and use that same key for lookup and removal. PrimitiveNames should use the immutable key stored in Entry. Changed should continue to be raised only after releasing the mutation lock.

This design would provide reliable behavior under concurrent mutations while retaining lock-free lookup and efficient repeated listing.

Good points. It was the part of the PR I was the most uncomfortable and unsure about. I'll do an update to address this. Should I fit in the sorting by MetadataToken in the builder extensions while I'm doing it? That makes the ordering stable not just incidentally (although I assume the underlying .NET code that leads to this isn't likely to change in a major way), but using mechanics that guarantee it, so this won't suddenly break due a .NET change down the road.

[halter73]

I’d leave the MetadataToken sorting out of this PR. I think the contract should be preserving registration order, whether primitives are added individually or discovered together from a type or assembly. A batch should occupy its place in that registration order without reordering other registrations.

Within a batch, the existing discovery order seems sufficient. GetMethods already provides deterministic ordering for a given compiled assembly on .NET 7+, and explicitly sorting by metadata tokens wouldn’t guarantee stability across compilations. I don’t think the additional sorting buys enough to justify it here.

[PederHP]

The current implementation has concurrency and performance issues that work against the stability this PR is trying to provide.

TryAdd assigns the sequence number before calling ConcurrentDictionary.TryAdd. With concurrent additions, A can receive sequence 1 and stall while B receives sequence 2 and completes insertion. A client can then observe [B], followed by [A, B] after A completes. An already visible primitive moves even though the collection is intended to preserve stable insertion order.

Remove also reads primitive.Id twice, once for TryGetValue and again for the conditional dictionary removal. Since built-in primitive names are backed by mutable protocol properties, a concurrent name change can cause the lookup to succeed with one key and the removal to use another. Remove then returns false and leaves the entry indexed under its original key.

PrimitiveNames has a related consistency problem because it now reads Primitive.Id instead of returning the key captured at insertion. A renamed primitive can therefore appear under one name in PrimitiveNames while remaining accessible only through its original dictionary key.

Finally, GetOrderedEntries snapshots and sorts the complete dictionary on every enumeration. The built-in tools, prompts, resources, and resource-template list handlers take this path on every list request, even when the collection has not changed.

Could we instead use:

• A ConcurrentDictionary<string, Entry> for lock-free O(1) lookup.
• A List<Entry> that records insertion order.
• A normal mutation lock protecting successful add, remove, and clear operations.
• A private cached Entry[] snapshot that is invalidated under the mutation lock.
• An immutable insertion key stored in each Entry.

Serializing mutations gives each successful add or remove a clear ordering point and removes the need for sequence numbers. Add remains amortized O(1). Removal from the ordered list is O(n), but primitive collections are normally small and mutations are uncommon.

The first enumeration after a mutation would rebuild the snapshot in O(n). Subsequent enumerations would traverse the cached snapshot without locking, sorting, or additional allocation. The snapshot array must remain private: GetEnumerator may traverse it directly, while ToArray, PrimitiveNames, and CopyTo should copy into caller-owned storage so users cannot mutate the cached structure.

Remove should capture primitive.Id once and use that same key for lookup and removal. PrimitiveNames should use the immutable key stored in Entry. Changed should continue to be raised only after releasing the mutation lock.

This design would provide reliable behavior under concurrent mutations while retaining lock-free lookup and efficient repeated listing.

I had Claude do an update to the PR and it looks sensible to me in terms of addressing your concerns and following your outlined solution. It did have this interesting comment to you:

On the stalled-add race: my first attempt at a regression test was a multi-threaded stress test, and it passed against the old code too, because the window between Interlocked.Increment and TryAdd is too small to hit reliably. The test in the PR, Enumeration_AddStalledMidway_LaterAddDoesNotOvertakeIt, uses a key comparer that blocks inside one add while a second add runs. That reproduces your A/B scenario deterministically: it fails on the previous version and passes now. There's also a test that PrimitiveNames keeps the name a primitive was added under after it's renamed.

[tarekgh]

Thanks @PederHP for providing the changes. I'll wait the CI to finish and then merge.