fix(messaging): reject gateway requests to dead silos - #10539
fix(messaging): reject gateway requests to dead silos#10539ReubenBond wants to merge 29 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a messaging gap in the Orleans gateway: gateway-forwarded external client requests were not tracked by destination silo, so when membership declared a target silo dead those requests could linger until normal client timeout instead of failing promptly with SiloUnavailableException. The change integrates per-client in-flight request tracking into the gateway and adds a regression test which exercises an external client through its gateway.
Changes:
- Track gateway-forwarded (non-local) client requests after addressing, expire them via existing maintenance, and clear them on disconnect/shutdown.
- Listen for
SiloStatus.Deadnotifications and reject tracked requests to the dead silo via the existing client response path. - Add a functional liveness test which ensures a gateway-forwarded request breaks promptly when the destination silo is killed.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Runtime.Tests/MembershipTests/SilosStopTests.cs | Adds a regression which forces a gateway-forwarded in-flight request, then kills the destination silo and asserts prompt SiloUnavailableException. |
| src/Orleans.Runtime/Messaging/MessageCenter.cs | Hooks gateway request tracking into the outbound send path. |
| src/Orleans.Runtime/Messaging/Gateway.cs | Adds per-client in-flight request tracking, periodic expiry, dead-silo rejection, and lifecycle cleanup for forwarded requests. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
src/Orleans.Runtime/Messaging/Gateway.cs:536
- RejectRequestsToSilo includes the full request Message in the SiloUnavailableException text ("... for message: {request}"). Message.ToString() appends BodyObject, so this can leak request payload details back to external clients and also inflate rejection strings. Prefer a sanitized message which does not embed the full request (e.g., include only CorrelationId).
var exception = new SiloUnavailableException(
$"The target silo {deadSilo} became unavailable for message: {request}.");
_gateway.messageCenter.RejectMessage(
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (1)
test/Orleans.Runtime.Tests/MembershipTests/SilosStopTests.cs:65
- Test name is inconsistent with the existing pattern in this file ("...RequestsBreak" vs "...RequestBreaks"). Consider aligning the naming to reduce confusion when scanning similar tests.
public async Task SiloUngracefulShutdown_GatewayForwardedRequestBreaks()
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (2)
test/Orleans.Runtime.Tests/MembershipTests/SilosStopTests.cs:134
- LongRunningTaskObserver stores the call id in a mutable field and completes a non-generic TaskCompletionSource. If OnCallStarted were invoked more than once (e.g., due to retries or reentrancy), _callId could be overwritten after _started is completed, making the final Assert nondeterministic. Capture the call id as the TaskCompletionSource result instead.
private readonly TaskCompletionSource _started = new(TaskCreationOptions.RunContinuationsAsynchronously);
private Guid _callId;
public void OnCallStarted(Guid callId)
{
src/Orleans.Runtime/Messaging/Gateway.cs:463
- ClientState.TrackRequest reads the current connection via the Connection property, which returns the backing field without a volatile read. Since _connection is updated via Interlocked.Exchange, a non-volatile read here can observe a stale value and either skip tracking while connected (breaking the feature) or track after disconnect (leaking entries until TTL cleanup). Use Volatile.Read for both reads in this method.
public void TrackRequest(Message message)
{
var connection = Connection;
if (connection is null)
{
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Orleans.Runtime/Messaging/Gateway.cs:478
- CreateRequestSnapshot assigns CacheInvalidationHeader by reference, which can alias the original message's mutable List. That list can be appended to later during forwarding/cache invalidation, so the snapshot can observe concurrent mutations and potentially race during serialization of the synthesized rejection. Copy the list when snapshotting to avoid sharing mutable state between messages.
TargetSilo = message.TargetSilo,
TargetGrain = message.TargetGrain,
SendingSilo = message.SendingSilo,
SendingGrain = message.SendingGrain,
CacheInvalidationHeader = message.CacheInvalidationHeader,
src/Orleans.Runtime/Messaging/Gateway.cs:492
- ClearPendingRequests() is called on disconnect and gateway shutdown, but not when a client is dropped/removed (ClientState.Drop()). Since dropped clients are removed from Gateway.clients, the maintenance loop will no longer call DropExpiredRequests() for that ClientState, so any tracked _pendingRequests entries can be retained for the lifetime of the ClientState. Consider clearing _pendingRequests when dropping the client as well.
public void ClearPendingRequests()
{
lock (_pendingRequests)
{
_pendingRequests.Clear();
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Orleans.Runtime/Messaging/Gateway.cs:236
TrackRequestwill also record SystemTarget requests if they have a non-localTargetSiloand a clientSendingGrain. However,Gateway.TryToRerouteexplicitly allows SystemTarget routing via gateway addresses (not membershipSiloAddressvalues), so these entries will never matchSiloStatusChangeNotification(which reports membership silo addresses) and therefore cannot be rejected promptly when the silo dies. Consider skipping SystemTarget messages here to avoid tracking overhead and misleading entries.
if (message.Direction != Message.Directions.Request
|| message.TargetSilo is not { } targetSilo
|| targetSilo.Matches(siloAddress)
|| !ClientGrainId.TryParse(message.SendingGrain, out var clientId)
|| !clients.TryGetValue(clientId, out var client))
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
65a9806 to
475d795
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Orleans.Runtime/Messaging/Gateway.cs:78
requestMaintenancePeriodcan become 1ms whenmessagingOptions.ResponseTimeout <= 0(becauseMinreturns a non-positive value andMaxclamps to 1ms). Since non-positive response timeouts are explicitly supported (and tracking may still occur via per-message TTL), this can create an unnecessarily tight maintenance loop and avoidable CPU usage.
var requestMaintenancePeriod = Max(
TimeSpan.FromMilliseconds(1),
Min(messagingOptions.ResponseTimeout, TimeSpan.FromSeconds(1)));
src/Orleans.Runtime/Messaging/Gateway.cs:132
PerformRequestMaintenancelogs exceptions usingLogErrorGatewayMaintenanceError, which emits the message "Error performing gateway maintenance". That makes it hard to distinguish request-tracking maintenance failures from the existing gateway maintenance loop when diagnosing issues.
catch (Exception exception)
{
LogErrorGatewayMaintenanceError(logger, exception);
}
}
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c483dd99-07c0-4a20-8bf7-04613353a821
6f7387f to
b40660c
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes core messaging/forwarding and response-routing behavior with new concurrency and lifecycle interactions that warrant final human validation beyond automated review.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
test/Orleans.Runtime.Tests/MembershipTests/SilosStopTests.cs — using Orleans.Messaging; appears unused in this test file (it is only present in the using list).… View resolved comment |

Fixes #10165.
External client callbacks are owned by the client process, so a gateway previously had no record of requests it forwarded after placement selected a destination silo. When that silo died, silo-originated callbacks failed promptly while gateway-forwarded client requests waited for the client response timeout.
Track non-one-way external-client requests at the concrete remote transport enqueue. The gateway binds each inbound connection to its existing
ClientState, and the tracking callback follows initial addressing and later invalid-activation forwarding. Callback recovery is restricted to the ingress gateway recorded inSendingSilo, preventing another gateway connected to the same client from creating a duplicate owner.Responses remain routed through the live ingress gateway so terminal responses, rejections, and diagnostic status responses update the owning tracker. When the ingress gateway is dead, another gateway connected to the client can deliver the response locally.
Each
ClientStateserializes registration, the post-registration membership check, andConnection.Sendwith one per-client lock. Notification-first ordering rejects without sending, while send-first ordering completes the enqueue before dead-silo removal. A shutdown gate prevents late registrations, and gateway shutdown drains every client state after stopping membership notifications and maintenance.Tracking stores a lightweight request snapshot without retaining the request body or request context. Mutable cache-invalidation headers are copied. Explicit message TTL is preserved; TTL-less requests use the configured response timeout only for tracker retention, so synthetic rejections remain sendable. A sparse concurrent index contains only clients with active tracked requests and unregisters clients after terminal completion, rejection, expiry, disconnect, drop, or shutdown.
When membership declares a destination dead, the gateway removes matching entries first, records rejection instrumentation, and enqueues transient rejections backed by
SiloUnavailableExceptionthrough the owningClientState. Gateway-addressed system targets and one-way messages bypass tracking.Deterministic coverage exercises eligibility, response classification, duplicate-attempt replacement, membership-before-send and send-before-membership ordering, destination removal, cleanup, snapshot isolation, TTL/default-timeout expiry, ingress-response routing, cancellation-versus-silo-death races, successful completion, client shutdown, and prompt dead-silo rejection.
Microsoft Reviewers: Open in CodeFlow