mailboxrpc: honor the operator's retry-after hint

[Roasbeef]

In this PR, we teach the mailbox retry helper to wait as long as the operator
asks when it sheds a request, instead of falling back on a delay we picked
ourselves.

Retry already backs off on ResourceExhausted with jittered exponential
backoff, which is the right shape when we know nothing. But the operator does
know something: its rate limiter can say exactly when the next token arrives.
The lumos side now attaches that as a google.rpc.RetryInfo detail
(lightninglabs/lumos#726), and this reads it. A client that returns too early
gets shed again, and because the operator can only afford to answer roughly one
shed per second, that second shed is silent, so the client burns a full deadline
learning nothing. Honoring the hint is what turns an ongoing loop into a single
wasted round trip.

The hint rides on the status detail rather than an envelope header on purpose.
DecodeErrorHeaders already reconstructs the status, so RetryAfter reads it
off the error the retry loop is already holding. A bare header would never reach
here: the call closure returns only an error, and the transport drops the
rest of the headers, so we would have had to plumb them through the facade for
this one field.

Treating the hint as untrusted

A retry-after is a number from someone else, so backoffFor does not take it at
face value. Three cases, each deliberately different rather than folded together:

An absurd but well-formed hint, say a hundred years, is clamped to
RetryPolicy.MaxRetryAfter (30s by default) instead of ignored. A hostile or
buggy operator gets to slow us down, which is its right, but not to park us
forever.

A malformed hint, meaning a duration outside the protobuf range or a detail we
cannot resolve, is treated as no hint at all and falls through to the computed
backoff. Garbage should not be honored, and it should not be clamped either,
because clamping garbage silently invents a number.

A non-positive hint is also treated as no hint. This one matters most: honoring
a zero would collapse the backoff into a hot loop aimed at a server that just
told us it is overloaded, which is worse than having no hint at all.

The clamped hint is used without jitter, which is a departure from the computed
path. Jitter exists to break up a fleet-wide schedule, and there is no fleet-wide
schedule here: the hint comes from the caller's own per-client bucket, so two
clients already get different values. Jittering downward would just return early
and defeat the point.

TestRetryClampsHostileServerHint holds the ceiling. Deleting the clamp makes it
hang past its timeout rather than fail an assertion, which is the honest
signature of what the bug would be.

A rebase note

This began stacked on the idempotency-key work in #1056, now merged. Rebasing it
onto main hit one conflict: that branch deleted DefaultRetryPolicy as dead
code, and this branch had added MaxRetryAfter to it. We kept the deletion, and
dropped the one test assertion that called it. The property it checked, that the
default ceiling is applied, is already asserted two lines above through
RetryPolicy{}.normalize(), which is the documented zero-value contract and the
thing production actually goes through.

Scope

This is the client half of a cheap interim, not a fix. The underlying problem is
that a shed response costs a durable mailbox write, which is why the operator
rate-limits how often it can afford to answer at all. Removing that constraint
needs a non-durable delivery class, tracked in #1054. Until then this makes the
answers we do get more useful, and it does nothing for the sheds that stay
silent.

go test ./mailbox/... -race, make fmt-changed, make lint-changed-local
(0 issues) and make commitmsg-lint are all clean.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[Roasbeef]

@claude review this

[claude]

Claude finished @Roasbeef's task in 2m 16s —— View job

---

Review: mailboxrpc — honor the operator's retry-after hint

• Read the changed files (retry.go, retry_test.go, CLAUDE.md)
• Analyze retry-after decoding & clamping logic
• Review tests
• Verify the wire round-trip claim
• Post review feedback

Overall this is a clean, well-reasoned change. The logic is correct, the "untrusted number" handling is careful, the doc comments explain the why, and the test coverage is thorough (extract/wrap/absent/malformed/non-positive/clamp/fallback/normalize). The CLAUDE.md update is accurate. I found no blocking issues — just a few observations.

What I verified

• The wire round-trip actually preserves the hint. The tests build a status directly, so they don't exercise the encode/decode path the PR body leans on. I checked grpc_status.go: EncodeErrorHeaders marshals st.Proto() (which includes the Details []*anypb.Any), and DecodeErrorHeaders rebuilds it via status.FromProto. errdetails.RetryInfo is registered in the global proto registry (retry.go imports the package), so st.Details() resolves it after decode. The claim holds. ✅
• RetryAfter(nil) is safe. status.FromError(nil) returns (nil, true), and (*Status)(nil).Details() is nil-guarded, so the loop is skipped and you get (0, false). Matches the test. ✅
• Malformed vs. absurd split is right. durationpb.Duration{Seconds: 1<<60} fails IsValid() → falls through (malformed); 100*365*24h passes IsValid() → clamped to MaxRetryAfter (absurd). Both paths are covered. ✅
• backoffFor runs on a normalized policy (RetryWithKey calls normalize() before the loop), so MaxRetryAfter is always populated. ✅

Observations (non-blocking)

1. Jitter is dropped for hints on the trust assumption that hints are per-client — but the clamp exists precisely because that assumption can be violated. backoffFor returns the hint verbatim (no jitter), justified by "the hint comes from the caller's own per-client bucket, so there's no fleet-wide schedule." That's true for an honest operator. A buggy/hostile one — the same threat model the clamp defends against — can hand every client an identical short hint (e.g. 50ms) and re-create a synchronized wake-up, a thundering herd aimed at its own server. It's low severity: the herd only hurts the operator, and MaxAttempts bounds total amplification to 4 sends. Worth a one-line comment acknowledging that the no-jitter choice trusts the hint's provenance even though the magnitude is treated as untrusted, so a future reader doesn't mistake it for an oversight.

2. A hint longer than the remaining context deadline turns a retryable shed into a terminal failure with no final attempt. If the hint (up to 30s) exceeds the ctx deadline, waitBackoff returns ctx.Err() and the loop returns the shed error without a last send. This is arguably correct — "come back in 30s" when you have 5s left means retrying is pointless — but it's a behavior change worth a sentence in the Retry doc, since a hint-free shed with the same deadline would have gotten one more (shorter, computed) attempt in.

3. RetryAfter keeps scanning after a malformed RetryInfo. If a status carried two RetryInfo details and the first were invalid, the loop continues and could honor the second. Harmless and arguably more robust; just flagging the intent isn't obvious from the code.

4. Timing tests are wall-clock based. Lower-bound asserts (>= hint) are safe (scheduling only adds delay). The upper bounds have generous margins — TestRetryPrefersShorterServerHint needs < 250ms against a 5ms hint (the computed floor it's distinguishing from is 250ms), and the clamp test needs < 1s against a 40ms ceiling. These should hold on normal CI, but they're the ones to watch if flakiness ever shows up. No change needed now.

None of these require action to merge. Nice work — the design notes in the doc comments and PR body make the reasoning easy to follow.
· indexer-honor-retry-after