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); + }); +}