Repository navigation
feat: per-method [Retry] for the one flaky endpoint - #2
Merged
Merged
Conversation
WithRetry is a client-wide policy. This is the other granularity: a method
declaring its own, by the person who knows the endpoint.
[Get("/reports/{id}")]
[Retry(5, DelayMilliseconds = 250)]
Task<Report> GetReportAsync(int id, CancellationToken ct = default);
It refines the client's registered settings rather than replacing them —
backoff curve, jitter, status-code set and Retry-After handling still come
from the registration, and only what the attribute names changes.
The precedence rule is the part worth arguing about, because it inverts
[Transport], which resolves scope > registration > source. That inversion is
deliberate, not an oversight. A transport is a *competing* claim about one
fact — which stack sends this request — so exactly one side can hold and the
tie-break favours the operator, who knows the deployment. A retry count is a
*refinement*: "how should this endpoint behave" narrows "how should this
client behave by default". There is no tie to break, so the more specific
statement simply applies. The library already resolves refinements this way
in two places: a per-request Timeout wins over the client's (the line
immediately above the one this change touches), and a method's [Header] wins
over a client-wide default header.
The alternative was decisive against itself. If registration won, then
WithRetry(3) anywhere on the client would silently flatten a [Retry(5)] the
author put on one flaky endpoint — and since retrying is off by default,
enabling it at all requires WithRetry, so the attribute would be useless
exactly when it was configured.
A declaration therefore also turns retrying on by itself, with no client-wide
WithRetry first. That is still "only retry when someone asked": the attribute
is someone asking, in source, deliberately.
WithoutRetries() keeps the last word for whoever runs it, ignoring every
declaration. A client that cannot be stopped from retrying is a liability in
the middle of an incident. It is distinct from WithRetry(1), which only says
the client has no policy of its own and leaves a declaration free to supply
one — collapsing the two would remove the kill switch.
The safety defaults are not bypassable. A streamed body is still never
retried, since the first attempt consumed it; that is physics, not policy.
A non-idempotent method still needs AllowNonIdempotent, which is on the
attribute as well as at registration because the interface author is the one
who knows whether a given endpoint carries an idempotency key — the
client-wide RetryNonIdempotentMethods opts in every method at once and is the
blunter tool of the two.
Attribute surface is deliberately small: attempt count, DelayMilliseconds
(milliseconds because a TimeSpan cannot be an attribute argument, with -1
rather than 0 as the inherit sentinel since zero is a legitimate delay), and
AllowNonIdempotent. Backoff, MaxDelay, jitter, Retry-After handling and the
status-code set are left to registration — they describe how hard to lean on
a struggling service, which is a deployment property, and an enum like
BackoffStrategy cannot express "inherit" without a sentinel member polluting
a public type.
Method-level only, not interface-level. A client-wide declaration is already
WithRetry, which belongs to whoever runs the thing; an interface author
turning retries on for every consumer of their client is precisely the
"retries without being asked" liability the off-by-default stance exists to
prevent.
Two diagnostics. RXH020 (warning) catches [Retry] on a POST or PATCH without
AllowNonIdempotent, where the safety default would leave the attribute doing
nothing at all and nothing else in the build would mention it. RXH021 catches
an attempt count below one.
The executor change is 7 lines: the effective options are resolved once
before the loop, so the loop body is untouched and knows nothing about where
its settings came from.
1203 tests; build, format and native AOT clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WithRetryis a client-wide policy. This adds the other granularity — a method declaring its own,for the one flaky endpoint on an otherwise healthy API.
It refines the client's registered settings rather than replacing them: backoff curve, jitter,
status-code set and
Retry-Afterhandling still come from the registration, and only what theattribute names changes.
Closes the Per-method retry roadmap item.
Changes
RetryAttributeinRunax.Http.Abstractions— attempt count,DelayMilliseconds,AllowNonIdempotent. Method-level only.RetryOverridecarries what the generator found;RetryOptions.Resolvefolds it into a copyof the client's options.
RetryOptions.Suppressedis the new kill switch.WithoutRetries()onIRunaxClientBuilder<T>.ClientRequest.Retry, alongside the existing per-requestTimeout.RetryOverride— no reflection.RXH020(warning) andRXH021(error), recorded under New Rules inAnalyzerReleases.Unshipped.md.retries.md,diagnostics.md,src/Runax.Http/README.md,CHANGELOG,ROADMAP.Type
Checklist
Notes for review
The precedence rule
A method's
[Retry]wins over the client's attempt count.WithoutRetries()wins overeverything.
This inverts
[Transport], which resolves scope → registration → source. The inversion isdeliberate:
one side can hold, so a tie-break is needed, and it favours the operator because they know the
deployment (and must be able to decline a transport package they don't want installed).
client behave by default". There is no tie to break, so the more specific statement applies.
The library already resolves refinements this way in two places — a per-request
Timeoutwins overthe client's (literally the line above the one this PR touches in
HttpContractExecutor), and amethod's
[Header]wins over a client-wide default header.[Transport]is the odd one out, notthis.
The alternative was decisive against itself: if registration won,
WithRetry(3)anywhere on theclient would silently flatten a
[Retry(5)]an author put on one flaky endpoint — and sinceretrying is off by default, enabling it at all requires
WithRetry, so the attribute would beuseless precisely when anyone configured the client.
A declaration therefore also turns retrying on by itself, with no client-wide
WithRetryfirst.That is still "only retry when someone asked" — the attribute is someone asking, in source,
deliberately.
The kill switch
WithoutRetries()ignores every declaration, because an operator needs a client that provablystops retrying during an incident. It is not the same as
WithRetry(1), which only says theclient has no policy of its own and leaves a declaration free to supply one — collapsing the two
would remove the switch. There is a test asserting they differ.
Every precedence test would fail if the rule silently flipped, in both directions: declaration over
registration,
[Retry(1)]under aWithRetry(5)client, andWithoutRetries()over both.Attribute surface, and what was left out
Exposed: attempt count (ctor),
DelayMilliseconds,AllowNonIdempotent.Milliseconds because a
TimeSpancannot be an attribute argument. The inherit sentinel is-1rather than
0, since zero is a legitimate delay and the tests rely on it.Left to registration:
Backoff,MaxDelay,UseJitter,RespectRetryAfter,MaxRetryAfterDelay,RetryableStatusCodes. Two reasons — they describe how hard to lean on astruggling service, which is a deployment property rather than an endpoint one; and an enum like
BackoffStrategycannot express "inherit" without adding a sentinel member to a public type.RetryableStatusCodeswould additionally need inherit-vs-replace semantics for a rare need.Method-level only, not interface-level. A client-wide declaration is already
WithRetry, whichbelongs to whoever runs the service. An interface author enabling retries for every consumer of
their client is exactly the "retries without being asked" liability the off-by-default stance
exists to prevent.
Safety defaults are not bypassable
A streamed body is still never retried whatever the attribute says — the first attempt consumed
the stream. That is physics, not policy, and there is a test pinning it.
A non-idempotent method still needs
AllowNonIdempotent. That sits on the attribute as well asat registration because the interface author is the one who knows whether a given endpoint carries
an idempotency key; the client-wide
RetryNonIdempotentMethodsopts in every method at once andis the blunter of the two.
RXH020warns when a[Retry]on aPOST/PATCHlacks it, becausethe safety default would otherwise leave the attribute doing nothing and nothing else in the build
would mention it.
Conflict footprint
HttpContractExecutor.csis +7/−3: the effective options are resolved once before the loop, sothe loop body is untouched.
CHANGELOG.mdadds one block at the top of[Unreleased]. Those arethe two files the parallel pipeline PR also touches.
Verification
1203 tests (from 1119) across three target frameworks.
dotnet build --warnaserrorclean,dotnet format --verify-no-changesclean, native AOT publishes with no trim warnings and thebinary runs. All internal doc links and anchors resolve.