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 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, PrimitiveNames after a primitive is renamed, caller-owned copies from ToArray/PrimitiveNames, and CopyTo argument validation.
• Enumeration_AddStalledMidway_LaterAddDoesNotOvertakeIt uses a key comparer that stalls one add inside the collection while a second add runs, reproducing the interleaving where a later add could become visible first. It fails against the earlier sequence-number version of this PR and passes now.
• 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 (2405 passed, 5 skipped), and the collection and list-order tests 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]

Optional defensive hardening: could we conditionally remove the exact Entry captured above, rather than unconditionally removing by key?

EqualityComparer<T>.Default.Equals can invoke a custom primitive's Equals while the mutation monitor is held. Since the monitor is reentrant, an equality implementation that calls Clear() and adds a replacement under the same key leaves entry stale. This line then removes the replacement from the dictionary, while _orderedEntries.Remove(entry) targets the original entry and leaves the replacement in the ordered list. The resulting collection can have Count == 0 and a failed lookup while ToArray() still returns one primitive.

This requires unusual side-effectful equality and is not a blocker for ordinary built-in primitives, but the safeguard is small:

if (!((ICollection<KeyValuePair<string, Entry>>)_primitives).Remove(new(key, entry)))
{
    return false;
}

_orderedEntries.Remove(entry);
_snapshot = null;

That way the ordered list, snapshot invalidation, and Changed notification are updated only when the exact compared entry was removed.

[tarekgh]

Nit: could we make the stalled-add test failure-safe?

This Release.Wait() is unbounded and does not observe cancellation, while the test does not guarantee Release.Set() if an earlier assertion or cancellation fails. That can leave a worker permanently blocked. A regression that makes snapshot enumeration wait for the mutation lock can also hang the test rather than produce a useful failure.

Please bound the comparer wait using TestConstants.DefaultTimeout and the test cancellation token, ensure release in a finally block, and await the worker tasks during cleanup. The synchronization objects should be disposed after the workers have finished. The initial hardcoded 10-second wait can also use TestConstants.DefaultTimeout.

[tarekgh]

Nit: the 200 ms delay gives the second add a chance to run, but does not establish that it ran before the snapshot was read. If it is scheduled only after the first add is released, whileStalled is empty and the prefix assertion is vacuous; the earlier sequence-number implementation can then pass too. Checking addSecond.IsCompleted instead would not solve that scheduling gap.

Could we retain this as best-effort threaded coverage and add the following deterministic ordering-invariant regression? It deliberately uses a reentrant comparer as a test fixture to force the insertion interleaving; it is not a claim of actual multithreaded execution. The earlier sequence-number implementation observes [second] and then reorders it to [first, second], whereas the current implementation preserves [second, first].

These members fit into this test class and use the existing CreateTool helper:

[Fact]
public void Enumeration_AlreadyVisiblePrimitive_KeepsItsPosition()
{
    var comparer = new InsertionInterleavingComparer();
    var collection = new McpServerPrimitiveCollection<McpServerTool>(comparer);
    string[] whileFirstPending = [];

    comparer.BeforeFirstInsertion = () =>
    {
        collection.Add(CreateTool("second"));
        whileFirstPending = collection.ToArray()
            .Select(tool => tool.ProtocolTool.Name)
            .ToArray();
    };

    collection.Add(CreateTool("first"));

    string[] afterward = collection.ToArray()
        .Select(tool => tool.ProtocolTool.Name)
        .ToArray();

    Assert.Equal(["second"], whileFirstPending);
    Assert.Equal(["second", "first"], afterward);
    Assert.Equal(
        whileFirstPending,
        afterward.Take(whileFirstPending.Length));
}

private sealed class InsertionInterleavingComparer : IEqualityComparer<string>
{
    private bool _callbackInvoked;

    public Action? BeforeFirstInsertion { get; set; }

    public bool Equals(string? x, string? y) =>
        StringComparer.Ordinal.Equals(x, y);

    public int GetHashCode(string value)
    {
        if (value == "first" && !_callbackInvoked)
        {
            // Re-enter once, before the first primitive is inserted.
            _callbackInvoked = true;
            BeforeFirstInsertion?.Invoke();
        }

        return StringComparer.Ordinal.GetHashCode(value);
    }
}

The threaded test should not be described as a deterministic reproducer while it still depends on the scheduling window.