fix(clients): reject outstanding requests to dropped clients

[ReubenBond]

Fixes #9462

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull Request Overview

This pull request addresses the issue of rejecting outstanding requests to dropped clients by refining client disconnection handling and updating related test cases. Key changes include:

• Fixing a spelling mistake in a comment from "immideately" to "immediately" in the timeout tests.
• Introducing extensive client disconnection tests in the Tester project.
• Updating methods in production code (SiloControl, Gateway, and others) to handle client disconnection more robustly and to use the new OnResponse callback pattern.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/TesterInternal/TimeoutTests.cs	Corrects a spelling mistake in a comment.
test/Tester/ClientConnectionTests/ClientDisconnectionTests.cs	Adds tests to validate client disconnection scenarios and message rejections.
src/Orleans.TestingHost/InProcTestCluster.cs	Introduces multi-client support and related nullability enhancements.
src/Orleans.Runtime/Silo/SiloControl.cs	Adds a DropDisconnectedClients method using a new parameter to optionally exclude recent disconnects.
src/Orleans.Runtime/Networking/GatewayConnectionListener.cs	Removes unused logger initialization for simplicity.
src/Orleans.Runtime/Messaging/Gateway.cs	Implements new client message handling via WorkItemType and updates message tracking.
src/Orleans.Runtime/Core/ManagementGrain.cs	Exposes a DropDisconnectedClients method to orchestrate client disconnection among silos.
src/Orleans.Runtime/Core/InsideRuntimeClient.cs, HostedClient.cs, OutsideRuntimeClient.cs, CallbackData.cs, EventSourceEvents.cs	Updates callback methods from DoCallback to OnResponse and adjusts disposal behavior.
src/Orleans.Core/SystemTargetInterfaces/*	Updates ISiloControl and IManagementGrain interfaces to reflect the new DropDisconnectedClients API.
src/Orleans.Core/GrainReferences/GrainReferenceActivator.cs	Minor refactoring by removing an unused injected dependency.
src/Orleans.Core/Networking/ConnectionManager.cs	Updates the shutdown exception message for clarity.
src/Orleans.Core.Abstractions/IDs/ClientGrainId.cs	Adds a helper method for client equality evaluation.

[Copilot]

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 8
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

src/Orleans.Runtime/Core/InsideRuntimeClient.cs:656

• InsideRuntimeClient.Dispose completes callbacks by calling CallbackData.OnResponse without first removing entries from the callbacks dictionary. Since CallbackData.DoCallback does not unregister itself, this leaves completed callbacks in the dictionary (unbounded growth / inaccurate GetRunningRequestsCount) and can keep other shutdown paths from observing proper cleanup.

        public void Dispose()
        {
            foreach (var callback in callbacks)
            {
                var message = callback.Value.Message;

src/Orleans.TestingHost/InProcTestCluster.cs:604

• If starting the default client host throws, InitializeClientAsync leaves the new host stored in _clientHosts["default"] and undisposed. Wrap StartAsync in a try/catch to dispose the host and clear ClientHost on failure.

This issue also appears on line 663 of the same file.

        var clientHost = CreateClientHost("default");
        ClientHost = clientHost;
        await ClientHost.StartAsync();
    }

src/Orleans.TestingHost/InProcTestCluster.cs:681

• RemoveClientAsync disposes the host only if StopAsync succeeds. If StopAsync throws (eg, due to a failing hosted service), the host will leak and the client will remain partially removed. Use try/finally to ensure disposal happens regardless of StopAsync outcome.

        if (_clientHosts.Remove(name, out var host))
        {
            await host.StopAsync();
            await DisposeAsync(host);
        }

src/Orleans.TestingHost/InProcTestCluster.cs:665

• GetClientAsync has the same exception-path leak as InitializeClientAsync: if host.StartAsync throws, the partially-created host remains in _clientHosts and is never disposed. This can also cause later GetClientAsync calls to return a broken host instance.

            host = CreateClientHost(name, configure);
            _clientHosts[name] = host;
            await host.StartAsync();

• Files reviewed: 18/18 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (5)

Previously missed (4) — in code that hasn't changed since the last review.

src/Orleans.Runtime/Networking/GatewayInboundConnection.cs:173

• reason is a non-nullable string parameter here, so reason ?? "Connection terminated." is redundant and can trigger analyzers complaining about unnecessary null coalescing. Passing reason directly keeps the exception message consistent with the rejection text.

                this.messageCenter.SendRejection(
                    msg,
                    Message.RejectionTypes.Transient,
                    $"Silo {this.myAddress} is rejecting message: {msg}. Reason = {reason}",
                    new ClientNotAvailableException(reason ?? "Connection terminated."));

test/Orleans.Runtime.Internal.Tests/TimeoutTests.cs:13

• This comment contains non-breaking spaces and a trailing space, which can cause noisy diffs and can be flagged by whitespace analyzers.

    // if we parallelize tests, this should run in isolation 

test/Orleans.Runtime.Internal.Tests/TimeoutTests.cs:101

• This comment contradicts the test behavior: the next line asserts that the first call throws TimeoutException. It should describe that the grain work continues but the client times out and does not receive the response.

            // First call should be successful, but client will not receive the response

src/Orleans.Runtime/Networking/GatewayConnectionListener.cs:47

• The ILogger<GatewayConnectionListener> logger parameter is now unused after removing the logger field assignment. With code-style analyzers enabled during build, this can surface as an unused-parameter warning/error. Either remove the parameter (and update DI registration) or explicitly consume it in the constructor body.

            this.messageCenter = messageCenter;
            this.connectionShared = connectionShared;
            this.connectionPreambleHelper = connectionPreambleHelper;
            this.endpointOptions = endpointOptions.Value;

src/Orleans.TestingHost/InProcTestCluster.cs:780

• After configuring TestClusterFatalErrorHandler, the created silo host also needs to be attached so the handler can stop the specific host on fatal errors (instead of leaving it unattached and throwing if triggered).

            var host = appBuilder.Build();
            InitializeTestHooksSystemTarget(host);
            await host.StartAsync();

• Files reviewed: 18/18 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

src/Orleans.Runtime/Core/InsideRuntimeClient.cs:655

• Dispose() completes pending callbacks by invoking user continuations via CallbackData.OnResponse, but it is not wrapped in try/catch. If any callback throws, disposal can fail and potentially surface an unhandled exception during host shutdown. The existing BreakOutstandingMessages() path already treats callback execution as untrusted and logs+continues; Dispose() should do the same.

            foreach (var callback in callbacks)
            {
                var message = callback.Value.Message;
                var response = messageFactory.CreateRejectionResponse(message, Message.RejectionTypes.Unrecoverable, "Host is shutting down.", null);
                callback.Value.OnResponse(response);

src/Orleans.TestingHost/InProcTestCluster.cs:654

• GetClient reads from _clientHosts without synchronization. Since _clientHosts is mutated by GetClientAsync/RemoveClientAsync, this can race and throw due to concurrent Dictionary access. Consider guarding reads with the same synchronization mechanism used for writes.

This issue also appears in the following locations of the same file:

• line 659
• line 674

    public IClusterClient? GetClient(string name)
    {
        if (_clientHosts.TryGetValue(name, out var host))
        {
            return host.Services.GetRequiredService<IInternalClusterClient>();
        }

        return null;
    }

src/Orleans.TestingHost/InProcTestSiloSpecificOptions.cs:43

• Using the null-forgiving operator here can mask a real initialization issue and will produce a NullReferenceException later if it ever occurs. Since PortAllocator is required for assigning new ports, it’s clearer to throw a targeted exception when it is missing.

            var (siloPort, gatewayPort) = testCluster.PortAllocator!.AllocateConsecutivePortPairs(1);

src/Orleans.TestingHost/InProcTestCluster.cs:663

• _clientHosts is a Dictionary and is accessed without synchronization. If tests call GetClientAsync concurrently for the same name (or interleave with RemoveClientAsync), this can race and create multiple hosts or throw due to concurrent Dictionary mutation. Consider synchronizing access (eg, lock) or switching to ConcurrentDictionary + GetOrAdd semantics.

    public async Task<IClusterClient> GetClientAsync(string name, Action<IHostApplicationBuilder>? configure = null)
    {
        if (!_clientHosts.TryGetValue(name, out var host))
        {
            host = CreateClientHost(name, configure);

src/Orleans.TestingHost/InProcTestCluster.cs:680

• RemoveClientAsync mutates _clientHosts without synchronization. Interleaving with GetClient/GetClientAsync can cause races and (since Dictionary is not thread-safe) can throw or leave clients in an inconsistent state. Consider removing the host under a lock and then stopping/disposing it outside the lock.

    public async Task RemoveClientAsync(string name)
    {
        if (_clientHosts.Remove(name, out var host))
        {
            await host.StopAsync();
            await DisposeAsync(host);
        }

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Lite

[ReubenBond]

The post-review CI run exposed a PR-specific multi-client lifecycle regression: both InProcessTestCluster_Client_RemainsUnavailableWhenStartupFails and InProcessTestCluster_Client_IsAvailableOnlyWhileDeployed failed on every OS/TFM because client hosts were published before startup completed and the public unavailable-state message changed. Commit 5465c0c3d now publishes hosts only after successful startup, disposes failed hosts, and preserves the established client availability contract. Run: https://github.com/dotnet/orleans/actions/runs/32148333736

[Copilot]

Copilot review overview

Review tier: Lite
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	src/​Orleans.Core/​Runtime/​RequestContextExtensions.cs — RequestContextExtensions.Import now filters gateway headers using LINQ + ToDictionary, which will…
	src/​Orleans.Runtime/​Messaging/​Gateway.cs — ClientState.RunMessageLoop uses _pendingToSend.Count to snapshot the drain size.…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Review tier: Lite
Findings: None

Issues resolved since last review (2)

Severity	Finding
	src/​Orleans.Runtime/​Messaging/​Gateway.cs — ClientState.RunMessageLoop uses _pendingToSend.Count to snapshot the drain size.… View resolved comment
	src/​Orleans.Core/​Runtime/​RequestContextExtensions.cs — RequestContextExtensions.Import now filters gateway headers using LINQ + ToDictionary, which will… View resolved comment

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Orleans.TestingHost/InProcTestCluster.cs:820

• InitializeClientAsync(CancellationToken) documents that the token can cancel initialization, but the token is not used when creating/starting the default client host (GetClientAsync is called without any cancellation support). This can cause hangs during test teardown/startup to ignore cancellation/timeouts.

Consider plumbing CancellationToken through to GetClientAsync/host.StartAsync (and the semaphore WaitAsync) or, if cancellation is intentionally unsupported, remove the CancellationToken overload/documentation to avoid a misleading API.

        await GetClientAsync("default");