From b9096f7c575c4744059fab85ad06008040d65f31 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 6 Nov 2026 16:07:35 -0500 Subject: [PATCH] fix(client-dotnet): size the safe-unary retry budget for every attempt (#197) ExecuteSafeUnaryAsync capped the whole retry pipeline at one DefaultCallTimeout while each attempt carried the same deadline, so a first attempt ending in DeadlineExceeded spent the entire budget and the retry never ran. The outer cap is now MaxAttempts x DefaultCallTimeout plus MaxDelay per backoff (MxGatewayClientRetryPolicy.SafeUnaryBudget), in both MxGatewayClient and GalaxyRepositoryClient; a budget too large for a cancellation timer leaves only the per-attempt deadlines. Tests: MxGatewayClientWireTests.SafeUnaryRetry_AfterDeadlineExceededFirstAttempt_RunsTheRetry (real gRPC loopback server stalls the first Invoke; fails with Cancelled on the old code), plus two SafeUnaryBudget unit tests. --- clients/dotnet/DotnetClientDesign.md | 5 ++ clients/dotnet/README.md | 4 +- .../MxGatewayClientOptionsTests.cs | 33 +++++++++++++ .../MxGatewayClientWireTests.cs | 35 ++++++++++++++ .../WireFakeGatewayServer.cs | 48 +++++++++++++++++-- .../GalaxyRepositoryClient.cs | 9 +++- .../MxGatewayClient.cs | 9 +++- .../MxGatewayClientRetryPolicy.cs | 23 +++++++++ 8 files changed, 159 insertions(+), 7 deletions(-) diff --git a/clients/dotnet/DotnetClientDesign.md b/clients/dotnet/DotnetClientDesign.md index 4124f3d..005e5d8 100644 --- a/clients/dotnet/DotnetClientDesign.md +++ b/clients/dotnet/DotnetClientDesign.md @@ -122,6 +122,11 @@ The .NET client applies a bounded Polly retry policy only to idempotent calls: streams, writes, secured writes, authentication, registration, item management, or subscription changes because those calls can partially succeed in MXAccess. +Each attempt carries its own `DefaultCallTimeout` deadline. The overall budget +for a retried call is `MaxAttempts × DefaultCallTimeout` plus `MaxDelay` for each +backoff, so an attempt that ends in `DeadlineExceeded` leaves the retry its own +full deadline instead of consuming the whole budget. + API key may be loaded from `MXGATEWAY_API_KEY` by the CLI, not implicitly by the library constructor unless a helper explicitly says it does that. diff --git a/clients/dotnet/README.md b/clients/dotnet/README.md index 897f990..a652fc1 100644 --- a/clients/dotnet/README.md +++ b/clients/dotnet/README.md @@ -157,7 +157,9 @@ worker; call until empty). `DisposeAsync` closes the session best-effort and doe not throw when the gateway is unreachable or the client is already disposed. Retry applies only to idempotent calls (`CloseSessionRawAsync`, ping, -session-state and worker-info commands, Galaxy reads). `AcknowledgeAlarmAsync` +session-state and worker-info commands, Galaxy reads). Each attempt gets its own +`DefaultCallTimeout`; a retried call can take up to `Retry.MaxAttempts × +DefaultCallTimeout` plus the backoff between attempts. `AcknowledgeAlarmAsync` is never retried because it is a non-idempotent operator write that re-sends the operator password and can double-acknowledge in the Historian. diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientOptionsTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientOptionsTests.cs index 4abf1ee..8d2704c 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientOptionsTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientOptionsTests.cs @@ -2,6 +2,39 @@ namespace ZB.MOM.WW.MxGateway.Client.Tests; public sealed class MxGatewayClientOptionsTests { + /// + /// The safe-unary budget covers every attempt's deadline plus the backoff between them, so a + /// first attempt that hits its deadline does not consume the retry's time (#197). + /// + [Fact] + public void SafeUnaryBudget_CoversEveryAttemptAndBackoff() + { + var options = new MxGatewayClientOptions + { + Endpoint = new Uri("http://localhost:5000"), + ApiKey = "test-api-key", + DefaultCallTimeout = TimeSpan.FromSeconds(30), + Retry = new MxGatewayClientRetryOptions { MaxAttempts = 3, MaxDelay = TimeSpan.FromSeconds(2) }, + }; + + Assert.Equal(TimeSpan.FromSeconds(94), MxGatewayClientRetryPolicy.SafeUnaryBudget(options)); + } + + /// A budget beyond what a cancellation timer accepts means no outer cap rather than a throw. + [Fact] + public void SafeUnaryBudget_WhenTooLargeForATimer_ReturnsNull() + { + var options = new MxGatewayClientOptions + { + Endpoint = new Uri("http://localhost:5000"), + ApiKey = "test-api-key", + DefaultCallTimeout = TimeSpan.FromDays(40), + Retry = new MxGatewayClientRetryOptions { MaxAttempts = 2 }, + }; + + Assert.Null(MxGatewayClientRetryPolicy.SafeUnaryBudget(options)); + } + /// Verifies that options with valid endpoint and API key pass validation. [Fact] public void Validate_WithAbsoluteEndpointAndApiKey_Succeeds() diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientWireTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientWireTests.cs index e093a89..943175c 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientWireTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientWireTests.cs @@ -90,6 +90,41 @@ public sealed class MxGatewayClientWireTests server.Service.AuthorizationByMethod); } + /// + /// A safe unary call whose first attempt runs into its deadline must still get its retry: + /// the outer budget covers every attempt, not just one DefaultCallTimeout (#197). + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task SafeUnaryRetry_AfterDeadlineExceededFirstAttempt_RunsTheRetry() + { + await using WireFakeGatewayServer server = await WireFakeGatewayServer.StartAsync(service => + service.StallFirstInvokes = 1); + await using MxGatewayClient client = server.CreateClient( + ApiKey, + TimeSpan.FromMilliseconds(500), + new MxGatewayClientRetryOptions + { + MaxAttempts = 2, + Delay = TimeSpan.FromMilliseconds(10), + MaxDelay = TimeSpan.FromMilliseconds(10), + UseJitter = false, + }); + + MxCommandReply reply = await client.InvokeAsync(new MxCommandRequest + { + SessionId = WireFakeGatewayServer.FakeGatewayService.SessionId, + Command = new MxCommand + { + Kind = MxCommandKind.Ping, + Ping = new PingCommand(), + }, + }); + + Assert.Equal(ProtocolStatusCode.Ok, reply.ProtocolStatus.Code); + Assert.Equal(2, server.Service.InvokeCount); + } + /// /// Verifies the gateway's replay-gap sentinel survives serialization and is /// surfaced as a typed, non-terminal stream item. diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/WireFakeGatewayServer.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/WireFakeGatewayServer.cs index 3cfc9bd..97b2c02 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/WireFakeGatewayServer.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/WireFakeGatewayServer.cs @@ -88,12 +88,26 @@ internal sealed class WireFakeGatewayServer : IAsyncDisposable /// API key the client should present. /// A client that talks to this server over h2c. public MxGatewayClient CreateClient(string apiKey) => + CreateClient(apiKey, TimeSpan.FromSeconds(30), new MxGatewayClientRetryOptions()); + + /// + /// Creates a client bound to this server's endpoint with an explicit call timeout and retry policy. + /// + /// API key the client should present. + /// Per-attempt unary deadline. + /// Retry options for safe unary calls. + /// A client that talks to this server over h2c. + public MxGatewayClient CreateClient( + string apiKey, + TimeSpan defaultCallTimeout, + MxGatewayClientRetryOptions retry) => MxGatewayClient.Create(new MxGatewayClientOptions { Endpoint = Endpoint, ApiKey = apiKey, UseTls = false, - DefaultCallTimeout = TimeSpan.FromSeconds(30), + DefaultCallTimeout = defaultCallTimeout, + Retry = retry, }); /// Stops and disposes the underlying web application. @@ -145,6 +159,17 @@ internal sealed class WireFakeGatewayServer : IAsyncDisposable /// public ReplayGap? ReplayGap { get; set; } + /// + /// Gets or sets how many leading Invoke calls stall until the client's deadline + /// expires, so the client sees for them. + /// + public int StallFirstInvokes { get; set; } + + /// Gets the number of Invoke calls the server received. + public int InvokeCount => Volatile.Read(ref _invokeCount); + + private int _invokeCount; + /// /// Gets the last Invoke request the client sent, as decoded from the wire. /// @@ -179,17 +204,32 @@ internal sealed class WireFakeGatewayServer : IAsyncDisposable } /// - public override Task Invoke(MxCommandRequest request, ServerCallContext context) + public override async Task Invoke(MxCommandRequest request, ServerCallContext context) { Record(context); InvokeRequest = request; + if (Interlocked.Increment(ref _invokeCount) <= StallFirstInvokes) + { + // Hold the call past the client's deadline; the cancellation is the deadline + // firing (or the client giving up), either way the caller has stopped waiting. + try + { + await Task.Delay(Timeout.InfiniteTimeSpan, context.CancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + } + + throw new RpcException(new Status(StatusCode.DeadlineExceeded, "stalled attempt")); + } + if (DenyInvoke) { throw new RpcException(new Status(StatusCode.PermissionDenied, "invoke scope required")); } - return Task.FromResult(new MxCommandReply + return new MxCommandReply { SessionId = request.SessionId, CorrelationId = request.ClientCorrelationId, @@ -197,7 +237,7 @@ internal sealed class WireFakeGatewayServer : IAsyncDisposable ProtocolStatus = Ok(), Hresult = 0, Register = new RegisterReply { ServerHandle = ServerHandle }, - }); + }; } /// diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/GalaxyRepositoryClient.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/GalaxyRepositoryClient.cs index a259fe3..6f4473e 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/GalaxyRepositoryClient.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/GalaxyRepositoryClient.cs @@ -517,8 +517,15 @@ public sealed class GalaxyRepositoryClient : IAsyncDisposable Func> call, CancellationToken cancellationToken) { + // Each attempt carries its own DefaultCallTimeout deadline, so the outer cap must cover + // every attempt plus the backoff between them; capping it at one DefaultCallTimeout let a + // DeadlineExceeded first attempt spend the whole budget and the retry never ran (#197). using CancellationTokenSource timeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - timeout.CancelAfter(Options.DefaultCallTimeout); + TimeSpan? budget = MxGatewayClientRetryPolicy.SafeUnaryBudget(Options); + if (budget is not null) + { + timeout.CancelAfter(budget.Value); + } return await _safeUnaryRetryPipeline.ExecuteAsync( async token => await call(token).ConfigureAwait(false), diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClient.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClient.cs index cb33125..154b2f6 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClient.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClient.cs @@ -375,8 +375,15 @@ public sealed class MxGatewayClient : IAsyncDisposable Func> call, CancellationToken cancellationToken) { + // Each attempt carries its own DefaultCallTimeout deadline, so the outer cap must cover + // every attempt plus the backoff between them; capping it at one DefaultCallTimeout let a + // DeadlineExceeded first attempt spend the whole budget and the retry never ran (#197). using CancellationTokenSource timeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - timeout.CancelAfter(Options.DefaultCallTimeout); + TimeSpan? budget = MxGatewayClientRetryPolicy.SafeUnaryBudget(Options); + if (budget is not null) + { + timeout.CancelAfter(budget.Value); + } return await _safeUnaryRetryPipeline.ExecuteAsync( async token => await call(token).ConfigureAwait(false), diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClientRetryPolicy.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClientRetryPolicy.cs index 4d01bbb..d912154 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClientRetryPolicy.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayClientRetryPolicy.cs @@ -41,6 +41,29 @@ internal static class MxGatewayClientRetryPolicy .Build(); } + /// + /// Returns the overall budget for one retried safe unary call: every attempt's + /// deadline plus the longest backoff + /// between attempts. Returns when the budget is too large for a + /// cancellation timer, in which case the per-attempt deadlines and the bounded attempt count + /// already bound the call. + /// + /// The client options supplying the call timeout and retry settings. + /// The overall budget, or for no outer cap. + public static TimeSpan? SafeUnaryBudget(MxGatewayClientOptions options) + { + ArgumentNullException.ThrowIfNull(options); + + int attempts = Math.Max(1, options.Retry.MaxAttempts); + double milliseconds = (options.DefaultCallTimeout.TotalMilliseconds * attempts) + + (options.Retry.MaxDelay.TotalMilliseconds * (attempts - 1)); + + // CancellationTokenSource.CancelAfter rejects anything above uint.MaxValue - 1 ms. + return milliseconds >= uint.MaxValue - 1 + ? null + : TimeSpan.FromMilliseconds(milliseconds); + } + /// Returns whether a command kind is eligible for automatic retry on transient failures. /// The command kind to check. /// if the command kind may be automatically retried; otherwise, .