From 6ab7f8bc5536d4f393195b1012ee3a96a295c543 Mon Sep 17 00:00:00 2001 From: Maksym Kacharaba Date: Sat, 7 Nov 2026 13:02:48 +0200 Subject: [PATCH] feat: bound the whole call with WithTotalTimeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Timeout bounds a single attempt, which reads like a call budget and is not one. The transport request carrying it is built once, before the retry loop, so every attempt gets the same allowance and the caller waits for all of them plus the backoff between. Five attempts at thirty seconds is over two and a half minutes, nothing capped it, and nothing in the configuration looked wrong. Timeout's own documentation never said "per attempt" either; it does now. WithTotalTimeout is the budget a caller actually experiences. Implemented as a linked token rather than a timeout handed to the transport, because the budget has to cover the waits between attempts and those happen in the executor rather than inside a send. The translation sits outside the retry loop on purpose. Thrown inside, a TransportException would be caught by the loop's own retry guard and the budget would buy further attempts instead of ending the call — the exact opposite of its job. Exceeding the budget throws TransportException rather than surfacing as a cancellation, matching how a per-attempt timeout is already reported: "the deadline passed" and "the caller changed their mind" lead to different retry and alerting decisions. A caller's own cancellation still surfaces as a cancellation, and a test pins that the budget does not relabel it. Unbounded by default, since adding a budget nobody asked for would change the behaviour of every existing client on upgrade. 1386 tests; build clean with warnings-as-errors, format clean, native AOT publishes and runs. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 5 + CHANGELOG.md | 36 ++++ docs/configuration.md | 6 +- docs/retries.md | 20 ++ .../IRunaxClientBuilder.cs | 21 ++- .../DependencyInjection/RunaxClientBuilder.cs | 10 + src/Runax.Http/HttpContractExecutor.cs | 27 ++- src/Runax.Http/README.md | 5 + src/Runax.Http/RunaxClientOptions.cs | 30 ++- tests/Runax.Http.Tests/TotalTimeoutTests.cs | 174 ++++++++++++++++++ 10 files changed, 327 insertions(+), 7 deletions(-) create mode 100644 tests/Runax.Http.Tests/TotalTimeoutTests.cs diff --git a/.gitignore b/.gitignore index 35622d0..cb15f0a 100644 --- a/.gitignore +++ b/.gitignore @@ -207,3 +207,8 @@ apm_modules/ # Claude Code personal/local settings .claude/settings.local.json + +# Agent worktrees. Each is a live git worktree of this repository, so a `git add -A` from the +# root otherwise commits them as embedded repositories — which is what happened once before +# this line existed. +.claude/worktrees/ diff --git a/CHANGELOG.md b/CHANGELOG.md index eb8d1e7..78baa8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,24 @@ All packages in the repository are versioned together. ### Added +- **`WithTotalTimeout`** — a budget for the whole call, retries and the waits between them + included. `Timeout` bounds a single *attempt*, which reads like a call budget and is not one: the + transport request carrying it is built once, before the retry loop, so every attempt gets the + same allowance and the caller waits for all of them plus the backoff. Five attempts at thirty + seconds is over two and a half minutes with nothing in the configuration looking wrong, and + nothing capped it. + - Exceeding the budget throws `TransportException` rather than surfacing as a cancellation, + matching how a per-attempt timeout is already reported. A caller's own cancellation still + surfaces as a cancellation. + - Implemented as a linked token rather than a timeout handed to the transport, because the budget + has to cover the waits *between* attempts, which happen in the executor rather than inside a + send. Translated outside the retry loop on purpose: thrown inside, a `TransportException` would + be caught by the loop's own retry guard and the budget would buy more attempts instead of + ending the call. + - Unbounded by default. Adding a budget nobody asked for would change the behaviour of every + existing client on upgrade. + - `Timeout`'s documentation now says "per attempt", which it never did. + - **`[Retry]` on a method**, for the one flaky endpoint on an otherwise healthy API. Surfaces an attempt count, `DelayMilliseconds` (milliseconds because a `TimeSpan` cannot be an attribute argument) and `AllowNonIdempotent`. It refines the client's registered settings rather than @@ -102,6 +120,24 @@ All packages in the repository are versioned together. ### Added +- **`WithTotalTimeout`** — a budget for the whole call, retries and the waits between them + included. `Timeout` bounds a single *attempt*, which reads like a call budget and is not one: the + transport request carrying it is built once, before the retry loop, so every attempt gets the + same allowance and the caller waits for all of them plus the backoff. Five attempts at thirty + seconds is over two and a half minutes with nothing in the configuration looking wrong, and + nothing capped it. + - Exceeding the budget throws `TransportException` rather than surfacing as a cancellation, + matching how a per-attempt timeout is already reported. A caller's own cancellation still + surfaces as a cancellation. + - Implemented as a linked token rather than a timeout handed to the transport, because the budget + has to cover the waits *between* attempts, which happen in the executor rather than inside a + send. Translated outside the retry loop on purpose: thrown inside, a `TransportException` would + be caught by the loop's own retry guard and the budget would buy more attempts instead of + ending the call. + - Unbounded by default. Adding a budget nobody asked for would change the behaviour of every + existing client on upgrade. + - `Timeout`'s documentation now says "per attempt", which it never did. + - **`Runax.Http.TestKit`** — `RunaxClientHarness.For()` stands up a real container, the real generated client and the real runtime over a `TestTransport` that records instead of sending. - A transport rather than a mock, because that is where the interesting state is. Mocking the diff --git a/docs/configuration.md b/docs/configuration.md index 085843a..7412dbf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -27,7 +27,8 @@ earns one. See [Serialization](serialization.md). services.AddRunaxClient() .WithBaseAddress("https://api.example.com/v3") // or a Uri .UseTransport(RestSharpTransport.TransportName) - .WithTimeout(TimeSpan.FromSeconds(10)) + .WithTimeout(TimeSpan.FromSeconds(10)) // per attempt + .WithTotalTimeout(TimeSpan.FromSeconds(30)) // the whole call, retries included .WithDefaultHeader("User-Agent", "my-service/1.0") .WithRetry(3) // or WithRetry(options => …) .UseSerializer(legacySerializer) @@ -45,7 +46,8 @@ Everything `Configure(...)` can reach. | --- | --- | --- | | `BaseAddress` | from `[BaseAddress]` | the absolute address request paths resolve against | | `TransportName` | from `[Transport]` | which transport this client's requests go over | -| `Timeout` | none | how long a request may take when the method does not say | +| `Timeout` | none | how long a single **attempt** may take when the method does not say | +| `TotalTimeout` | none | how long the whole **call** may take, retries and backoff included | | `Serializer` | the shared one | this client's serializer | | `DefaultHeaders` | from interface `[Header]`s | headers sent with every request | | `Retry` | off | see [Retries](retries.md) | diff --git a/docs/retries.md b/docs/retries.md index dc7a92e..07a8d3d 100644 --- a/docs/retries.md +++ b/docs/retries.md @@ -103,6 +103,26 @@ ignored and the backoff curve decides. Not retried: a capability gap, a cancelled request, or anything else thrown before the request reached the transport. +## Bound the call, not just the attempt + +`WithTimeout` bounds a **single attempt**. Retries multiply it and the backoff is on top, so five +attempts at thirty seconds is over two and a half minutes — with nothing in the configuration +looking wrong. Set a budget for the call: + +```csharp +services.AddRunaxClient() + .WithRetry(5) + .WithTimeout(TimeSpan.FromSeconds(10)) // per attempt + .WithTotalTimeout(TimeSpan.FromSeconds(20)); // the whole call, retries and waits included +``` + +Exceeding the budget throws `TransportException`, not a cancellation — "the deadline passed" and +"the caller changed their mind" lead to different retry and alerting decisions. Your own +`CancellationToken` still surfaces as a cancellation. + +Worth setting on any client with retries on. It is unbounded by default, because adding a budget +nobody asked for would change the behaviour of every existing client on upgrade. + ## Where it sits Above the transport boundary. Every transport gets the same retry behaviour, none of them diff --git a/src/Runax.Http/DependencyInjection/IRunaxClientBuilder.cs b/src/Runax.Http/DependencyInjection/IRunaxClientBuilder.cs index ebabc37..792f7e8 100644 --- a/src/Runax.Http/DependencyInjection/IRunaxClientBuilder.cs +++ b/src/Runax.Http/DependencyInjection/IRunaxClientBuilder.cs @@ -42,11 +42,28 @@ public interface IRunaxClientBuilder /// IRunaxClientBuilder UseTransport(string name); - /// Sets how long a request may take when the method does not say. - /// The timeout. + /// Sets how long a single attempt may take when the method does not say. + /// The per-attempt timeout. /// This builder. + /// + /// Per attempt. With retries on, the caller waits this multiplied by the attempt count plus + /// the backoff — see for the budget that bounds the call. + /// IRunaxClientBuilder WithTimeout(TimeSpan timeout); + /// + /// Sets how long the whole call may take, including every retry and the waits between them. + /// + /// The total budget. + /// This builder. + /// + /// The budget a caller actually experiences. Worth setting on any client with retries on: a + /// per-attempt timeout bounds an attempt, and retries multiply it with backoff on top, so a + /// client can block for minutes with nothing in its configuration looking wrong. Exceeding it + /// throws , not a cancellation. + /// + IRunaxClientBuilder WithTotalTimeout(TimeSpan timeout); + /// Adds a header sent with every request from this client. /// The header name. /// The header value. diff --git a/src/Runax.Http/DependencyInjection/RunaxClientBuilder.cs b/src/Runax.Http/DependencyInjection/RunaxClientBuilder.cs index 3ed0f0a..dd82ae8 100644 --- a/src/Runax.Http/DependencyInjection/RunaxClientBuilder.cs +++ b/src/Runax.Http/DependencyInjection/RunaxClientBuilder.cs @@ -67,6 +67,16 @@ public IRunaxClientBuilder WithTimeout(TimeSpan timeout) return this; } + /// + public IRunaxClientBuilder WithTotalTimeout(TimeSpan timeout) + { + ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(timeout, TimeSpan.Zero); + + _options.TotalTimeout = timeout; + + return this; + } + /// public IRunaxClientBuilder WithDefaultHeader(string name, string value) { diff --git a/src/Runax.Http/HttpContractExecutor.cs b/src/Runax.Http/HttpContractExecutor.cs index bc2be7e..89b3a3f 100644 --- a/src/Runax.Http/HttpContractExecutor.cs +++ b/src/Runax.Http/HttpContractExecutor.cs @@ -228,7 +228,32 @@ private async Task SendCoreAsync(ClientRequest request, CancellationTo // token on a retry while a correlation id stays put. See RequestContext. var context = new RequestContext(request, transportRequest, transport); - return await SendWithRetriesAsync(context, retry, cancellationToken).ConfigureAwait(false); + if (_options.TotalTimeout is not { } budget) + return await SendWithRetriesAsync(context, retry, cancellationToken).ConfigureAwait(false); + + // A linked source rather than a timeout handed to the transport: the budget has to cover + // the waits between attempts too, and those happen here rather than inside a send. + using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + deadline.CancelAfter(budget); + + try + { + return await SendWithRetriesAsync(context, retry, deadline.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested + && deadline.IsCancellationRequested) + { + // Translated outside the retry loop on purpose. Thrown inside, a TransportException + // would be caught by the loop's retry guard and the budget would buy more attempts + // instead of ending the call. + throw new TransportException( + $"{request.Method.ToString().ToUpperInvariant()} {transportRequest.RequestUri} " + + $"exceeded its total budget of {budget}.") + { + TransportName = transport.Name, + RequestUri = transportRequest.RequestUri + }; + } } /// diff --git a/src/Runax.Http/README.md b/src/Runax.Http/README.md index bc177e4..e498be2 100644 --- a/src/Runax.Http/README.md +++ b/src/Runax.Http/README.md @@ -207,6 +207,11 @@ without it every client that failed at the same moment retries at the same momen service gets hit by a synchronized wave. A capability gap is never retried — waiting will not give a transport an ability it lacks. +`WithTimeout` bounds one **attempt**; `WithTotalTimeout` bounds the **call**, retries and backoff +included. Worth setting whenever retries are on — otherwise five attempts at thirty seconds is over +two and a half minutes, and nothing in the configuration looks wrong. Exceeding the budget throws +`TransportException`, not a cancellation. + Retries live above the transport boundary, so every transport gets the same behaviour and none of them implements it. Swapping a transport does not change your retry semantics. diff --git a/src/Runax.Http/RunaxClientOptions.cs b/src/Runax.Http/RunaxClientOptions.cs index 1cd7a09..bc4fc54 100644 --- a/src/Runax.Http/RunaxClientOptions.cs +++ b/src/Runax.Http/RunaxClientOptions.cs @@ -33,11 +33,37 @@ public sealed class RunaxClientOptions public string? TransportName { get; set; } /// - /// How long a request may take when the method does not say. leaves it - /// to the transport. + /// How long a single attempt may take when the method does not say. + /// leaves it to the transport. /// + /// + /// Per attempt, not per call. With retries on, the wall-clock a caller waits is this + /// multiplied by the attempt count, plus the backoff between them — so five attempts at thirty + /// seconds is over two and a half minutes. Use to bound the call + /// itself. + /// public TimeSpan? Timeout { get; set; } + /// + /// How long the whole call may take, including every retry and the waits between them. + /// means unbounded. + /// + /// + /// + /// The budget a caller actually experiences, and the one worth setting. + /// bounds an attempt, which is not the same thing: retries multiply it, and the backoff is on + /// top. A client with retries on and no total budget can block for minutes with nothing in its + /// configuration looking wrong. + /// + /// + /// Exceeding it surfaces as rather than a cancellation, + /// matching how a per-attempt timeout is reported: "the deadline passed" and "the caller + /// changed their mind" lead to different handling, and conflating them sends retry logic and + /// alerting down the wrong path. A caller's own cancellation still surfaces as a cancellation. + /// + /// + public TimeSpan? TotalTimeout { get; set; } + /// /// The serializer for request and response bodies. uses the one /// registered for the application. diff --git a/tests/Runax.Http.Tests/TotalTimeoutTests.cs b/tests/Runax.Http.Tests/TotalTimeoutTests.cs new file mode 100644 index 0000000..3fca3f2 --- /dev/null +++ b/tests/Runax.Http.Tests/TotalTimeoutTests.cs @@ -0,0 +1,174 @@ +using System.Diagnostics; +using Microsoft.Extensions.DependencyInjection; +using Runax.Http.DependencyInjection; +using Runax.Http.Retry; +using Runax.Http.Tests.Contracts; +using Runax.Http.Tests.Fakes; +using Runax.Http.Transports; + +namespace Runax.Http.Tests; + +/// +/// The budget that bounds a whole call, retries and backoff included. +/// +/// +/// bounds an attempt, which reads like a call +/// budget and is not one: the transport request carrying it is built once, before the retry loop, +/// so every attempt gets the same allowance and the caller waits for all of them plus the waits +/// between. Five attempts at thirty seconds is over two and a half minutes with nothing in the +/// configuration looking wrong. +/// +public class TotalTimeoutTests +{ + private static (IRepositoryClient Client, BlockingTransport Transport) Build( + Func> respond, + Action> configure) + { + var transport = new BlockingTransport(respond); + var services = new ServiceCollection(); + + var shared = services.AddRunaxHttp(); + shared.UseSystemTextJson(); + shared.AddTransport(transport); + shared.UseDefaultTransport(transport.Name); + + configure(services.AddRunaxClient()); + + return (services.BuildServiceProvider().GetRequiredService(), transport); + } + + /// + /// A transport that can take as long as a test needs and honours cancellation. + /// + /// + /// Standalone rather than built on RecordingTransport, which is sealed and answers + /// synchronously — and a synchronous responder cannot be made to outrun a deadline. + /// + private sealed class BlockingTransport( + Func> respond) : IHttpTransport + { + public string Name => "blocking"; + + public TransportCapabilities Capabilities => + TransportCapabilities.CancellationMidFlight | TransportCapabilities.PerRequestTimeout; + + public List Requests { get; } = []; + + public Task SendAsync( + TransportRequest request, + CancellationToken cancellationToken = default) + { + lock (Requests) + Requests.Add(request); + + return respond(request, cancellationToken); + } + } + + [Fact] + public async Task A_call_that_outruns_its_budget_fails_as_a_timeout_not_a_cancellation() + { + // The distinction matters: "the deadline passed" and "the caller changed their mind" lead + // to different retry and alerting decisions. + var (client, _) = Build( + async (_, token) => + { + await Task.Delay(TimeSpan.FromSeconds(30), token); + return Responses.Json("{}"); + }, + c => c.WithTotalTimeout(TimeSpan.FromMilliseconds(150))); + + var thrown = await Should.ThrowAsync(() => client.GetAsync("o", "n")); + + thrown.ShouldNotBeAssignableTo(); + thrown.Message.ShouldContain("total budget"); + } + + [Fact] + public async Task The_budget_covers_the_retries_and_not_just_one_attempt() + { + // The whole point. Each attempt is fast and fails, so a per-attempt timeout would never + // fire; only a call budget stops the loop. + var attempts = 0; + + var (client, _) = Build( + (_, _) => + { + attempts++; + return Task.FromResult(Responses.Error(503)); + }, + c => c + .WithTotalTimeout(TimeSpan.FromMilliseconds(250)) + .WithRetry(options => + { + options.MaxAttempts = 100; + options.Delay = TimeSpan.FromMilliseconds(40); + options.Backoff = BackoffStrategy.Constant; + options.UseJitter = false; + })); + + var started = Stopwatch.StartNew(); + + await Should.ThrowAsync(() => client.GetAsync("o", "n")); + + started.Stop(); + + // Stopped well short of a hundred attempts, and quickly. Generous bounds because CI + // timing is not a precision instrument; the assertion is "bounded", not "exactly 250ms". + attempts.ShouldBeLessThan(100); + attempts.ShouldBeGreaterThan(1); + started.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10)); + } + + [Fact] + public async Task The_callers_own_cancellation_stays_a_cancellation() + { + // The budget must not swallow the caller's intent and relabel it a timeout. + using var cancellation = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)); + + var (client, _) = Build( + async (_, token) => + { + await Task.Delay(TimeSpan.FromSeconds(30), token); + return Responses.Json("{}"); + }, + c => c.WithTotalTimeout(TimeSpan.FromSeconds(30))); + + await Should.ThrowAsync( + () => client.GetAsync("o", "n", cancellation.Token)); + } + + [Fact] + public async Task A_call_inside_its_budget_is_untouched() + { + var (client, transport) = Build( + (_, _) => Task.FromResult(Responses.Json("""{"name":"n","stars":0}""")), + c => c.WithTotalTimeout(TimeSpan.FromSeconds(30))); + + var repository = await client.GetAsync("o", "n"); + + repository.Name.ShouldBe("n"); + transport.Requests.Count.ShouldBe(1); + } + + [Fact] + public async Task No_budget_means_unbounded_as_before() + { + // The default has to stay what it was: adding a budget nobody asked for would change the + // behaviour of every existing client on upgrade. + var (client, _) = Build( + (_, _) => Task.FromResult(Responses.Json("""{"name":"n","stars":0}""")), + _ => { }); + + (await client.GetAsync("o", "n")).Name.ShouldBe("n"); + } + + [Fact] + public void A_nonsensical_budget_is_refused() + => Should.Throw(() => + { + var services = new ServiceCollection(); + services.AddRunaxHttp().UseSystemTextJson(); + services.AddRunaxClient().WithTotalTimeout(TimeSpan.Zero); + }); +}