Reject duplicate SignalR upload stream IDs

[javiercn]

Fixes #68301

Overview

SignalR upload streams were registered with dictionary assignment, so a protocol-invalid duplicate stream ID replaced the active channel and could leave the displaced hub argument permanently unreachable. This change makes registration atomic and rejects an active duplicate immediately, while preserving valid multi-stream uploads and reuse after completion. The cross-cutting constraint is ownership-safe cleanup: a rejected invocation must roll back streams it registered without completing a stream owned by another invocation.

Design

There is no public API change. Internally, each upload-stream invocation receives one allocation-free owner ID, and the stream dictionary remains keyed only by the protocol stream ID because StreamItem and Completion messages contain no owner information. Its value is an allocation-free tuple containing the owner and converter:

// src/SignalR/server/Core/src/StreamTracker.cs
// Owner IDs need only be unique within this connection's StreamTracker.
private readonly ConcurrentDictionary<string, (long Owner, IStreamConverter Converter)> _lookup = new();
private long _nextStreamOwner;

public long GetNextStreamOwner()
{
    return Interlocked.Increment(ref _nextStreamOwner);
}

Alternatives considered:

• Use dictionary Add instead of assignment: this detects duplicates, but does not solve cleanup. The rejected invocation's existing finally path would still complete the pre-existing stream by ID. TryAdd also allows the deterministic Stream ID '7' is already in use. protocol error.
• Use (streamId, owner) as the dictionary key: this would permit duplicate protocol stream IDs under different owners and make item/completion lookup ambiguous because those messages carry only the stream ID.
• Track every successful registration in a list: correct, including partial rollback, but adds invocation plumbing and a list allocation. One owner ID lets cleanup use the existing StreamIds while proving ownership.
• Use an object or Guid owner: both work. A connection-local monotonic long is allocation-free and cheaper; the tuple dictionary value is also a value type.
• Return null on collision: possible, but argument binding would need a separate failure-propagation path to prevent invoking the hub with a null stream. Throwing HubException reuses the dispatcher's existing invocation-error handling for this rare protocol violation.

Implementation

Registration now uses TryAdd; a collision leaves the current entry untouched and aborts argument binding through the existing error path:

// src/SignalR/server/Core/src/StreamTracker.cs
public object AddStream(string streamId, Type itemType, Type targetType, long streamOwner)
{
    var newConverter = CreateConverter(itemType); // reflection details omitted
    var reader = newConverter.GetReaderAsObject(targetType);

    if (!_lookup.TryAdd(streamId, (streamOwner, newConverter)))
    {
        throw new HubException($"Stream ID '{streamId}' is already in use.");
    }

    return reader;
}

The dispatcher lazily assigns one numeric owner ID when it encounters the invocation's first streaming argument. All streams from that invocation share it:

// src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs
arguments[parameterPointer] = connection.StreamTracker.AddStream(
    hubMethodInvocationMessage.StreamIds[streamPointer],
    itemType,
    descriptor.OriginalParameterTypes[parameterPointer],
    streamOwner ??= connection.StreamTracker.GetNextStreamOwner());

Cleanup separates lookup, ownership validation, and mutation. TryRemove(KeyValuePair) atomically removes only the exact key/value observed; if the stream was completed and its ID reused before older asynchronous cleanup ran, the tuple no longer matches and the newer stream remains intact:

// src/SignalR/server/Core/src/StreamTracker.cs
public bool TryComplete(string streamId, long streamOwner)
{
    if (!_lookup.TryGetValue(streamId, out var registration) || registration.Owner != streamOwner)
    {
        return false;
    }

    if (!_lookup.TryRemove(KeyValuePair.Create(streamId, registration)))
    {
        return false;
    }

    registration.Converter.TryComplete(null);
    return true;
}

The regression tests cover the distinct behavior classes once each: duplicate IDs within one invocation under a buffer capacity of one while a later invocation still completes; a second invocation attempting to reuse an active ID without disturbing the original; reuse after completion; and multiple unique upload streams continuing to work.

Outcome

Behavior	Result
Duplicate ID in one invocation	Prompt deterministic completion error; connection continues processing
Duplicate ID across active invocations	Existing stream remains registered and receives its items/completion
Partial registration failure	Cleanup completes only entries owned by the rejected invocation
ID reuse after completion	Supported unchanged
Multiple unique upload streams	Supported unchanged
Allocation impact	No owner or registration object allocation

Validation: the complete Microsoft.AspNetCore.SignalR.Tests project passes with zero warnings and errors via eng\build.cmd -nobuildnative -noBuildJava -projects .\src\SignalR\server\SignalR\test\Microsoft.AspNetCore.SignalR.Tests\Microsoft.AspNetCore.SignalR.Tests.csproj -test.

[Copilot]

Pull request overview

This PR enforces Hub Protocol uniqueness for client upload stream IDs by making stream registration atomic and ownership-aware, preventing active streams from being replaced and avoiding cleanup that could affect other invocations (fixes #68301).

Changes:

• Make StreamTracker.AddStream reject duplicate active stream IDs via ConcurrentDictionary.TryAdd, throwing a deterministic HubException.
• Track per-invocation successful stream registrations and use them during cleanup to avoid completing/removing streams owned by other invocations.
• Add regression tests covering duplicate IDs, connection progress under buffer pressure, ownership preservation, unique streams, and ID reuse after completion.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs	Adds a hub method accepting two upload streams for duplicate/unique stream ID test scenarios.
src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs	Adds regression tests for duplicate upload stream IDs, non-blocking connection behavior, and ID reuse after completion.
src/SignalR/server/Core/src/StreamTracker.cs	Implements atomic stream registration and introduces an ownership token for safe cleanup.
src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs	Uses per-invocation stream registration tracking to ensure cleanup is ownership-safe.

---

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