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 need stable ordering (e.g. dynamically constructed context) to avoid cache-busting was affected. Systems that snapshot for integrity might also be affected.

Change

• Each entry in the dictionary now records a sequence number when it is added. Lookups by name work as before.
• GetEnumerator, ToArray, CopyTo and PrimitiveNames take a snapshot and sort it by that number. Primitives are listed in the order they were added, which with WithTools<T>(), WithPrompts<T>() and WithResources<T>() is declaration order. This matches the Python and TypeScript SDKs.
• A primitive that is removed and added again moves to the end. The class remarks document this.
• Remove and Contains still match on both name and instance.

Enumeration now reads a snapshot (ConcurrentDictionary.Values) instead of the live dictionary. Listing therefore briefly takes the dictionary's locks instead of being lock-free, which should be negligible at these sizes.

Insertion order still depends on reflection returning methods in declaration order. .NET does this in practice but doesn't guarantee it. Sorting by MetadataToken in the builder extensions would make that deterministic and could be a follow-up.

Tests

• Unit tests in McpServerPrimitiveCollectionTests cover order for tools, prompts and resources, remove and re-add, Clear, and removing a different instance with the same name.
• 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.
• With the old collection code, 8 of the new tests fail; with the fix they all pass.
• The full ModelContextProtocol.Tests suite passes on net10.0, apart from DockerEverythingServerTests.Sampling_Sse_EverythingServer, which fails locally because the Docker "everything" server image no longer has a trigger-sampling-request tool.

🤖 Generated with Claude Code

[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.