fix: WithMaxRetryWait bounds one wait, not the total

[karlwaldman]

Closes #42 (follow-up to #37)

The defect

WithMaxRetryWait bounded each individual wait. Nothing bounded their sum. With the default three retries, a server answering Retry-After just under the budget held the call for roughly three times the number the caller configured — and under context.Background() there is no deadline to rescue it.

Verified still reproducing on main (cd51e3a) before any change was written:

Path	Budget configured	Actual total wait on main
429 with Retry-After: 1, 5 retries	1.2s	5.007s (server hit 6 times)
transport failure, exponential backoff, 3 retries	1.5s	4.005s

Bounding how long a call can sit inside the SDK is the entire reason the option exists (#37), so bounding the total is the fix rather than adding a second option the caller has to discover.

The fix

doRequestWithHeaders tracks the wait already spent in this call and makes every decision against what is left:

• 429 / 5xx. A Retry-After longer than the remaining budget stops the retry loop and returns the typed error, exactly as a wait past the whole budget already did. Remaining budget is a stop condition, not a clamp, because waiting less than the server asked would not clear the limit.
• Transport failures. The exponential backoff is clamped to the remaining budget, so retries still happen while there is budget for them and stop when there is not. (Clamping rather than stopping here preserves the existing behaviour that a transport failure gets its retries; it just cannot overrun the total.)

Behaviour change

WithMaxRetryWait and DefaultMaxRetryWait now document a total automatic wait budget for one call. This can only make a call return sooner than before, never later. Two behaviours are explicitly preserved and pinned by tests:

• A retry that fits inside the budget still happens (TestRetryStillHappensWithinTheBudget — one 429, one success, server hit exactly twice).
• The fix(retry): restrict retries to safe requests and bound Retry-After (#33) #37 behaviour is unchanged: a single Retry-After past the budget returns *RateLimitError immediately, carrying the server's requested 3600 (TestSingleWaitLongerThanBudgetStillReturnsImmediately).

Red

Tests written first, run against unmodified main:

=== RUN   TestMaxRetryWaitBoundsTheTotalWaitNotEachOne
    retry_budget_test.go:52: total automatic wait was 5.006929292s against a 1.2s budget (server hit 6 times)
--- FAIL: TestMaxRetryWaitBoundsTheTotalWaitNotEachOne (5.01s)
=== RUN   TestMaxRetryWaitBoundsTheTotalWaitOnTransportFailures
    retry_budget_test.go:93: total automatic wait on transport failures was 4.004783791s against a 1.5s budget
--- FAIL: TestMaxRetryWaitBoundsTheTotalWaitOnTransportFailures (4.01s)
=== RUN   TestSingleWaitLongerThanBudgetStillReturnsImmediately
--- PASS: TestSingleWaitLongerThanBudgetStillReturnsImmediately (0.00s)
=== RUN   TestRetryStillHappensWithinTheBudget
--- PASS: TestRetryStillHappensWithinTheBudget (1.00s)
FAIL
FAIL	github.com/OilpriceAPI/oilpriceapi-go	10.026s

The two passing tests are the regression guards; they are green before and after by design.

The tests measure wall-clock time through the real request path against a real httptest server (and, for the transport case, a real listener that accepts and hangs up), not a field on the client — the defect is elapsed time, so elapsed time is what is asserted.

Green

=== RUN   TestMaxRetryWaitBoundsTheTotalWaitNotEachOne
--- PASS: TestMaxRetryWaitBoundsTheTotalWaitNotEachOne (1.00s)
=== RUN   TestMaxRetryWaitBoundsTheTotalWaitOnTransportFailures
--- PASS: TestMaxRetryWaitBoundsTheTotalWaitOnTransportFailures (1.50s)
=== RUN   TestSingleWaitLongerThanBudgetStillReturnsImmediately
--- PASS: TestSingleWaitLongerThanBudgetStillReturnsImmediately (0.00s)
=== RUN   TestRetryStillHappensWithinTheBudget
--- PASS: TestRetryStillHappensWithinTheBudget (1.00s)
PASS
ok  	github.com/OilpriceAPI/oilpriceapi-go	3.515s

5.007s → 1.00s, and 4.005s → 1.50s, each now inside its configured budget.

Full suite

Check	Result
go test ./...	ok github.com/OilpriceAPI/oilpriceapi-go 20.148s — 289 pass / 0 fail (285 baseline + 4 new)
go test -race ./...	ok github.com/OilpriceAPI/oilpriceapi-go 21.900s
go vet ./...	clean
gofmt -l .	clean

The existing retry-policy suite (retry_policy_test.go) is unchanged and still passes.

Scope

No version bump, no tag, no release.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 7e510844-d9d2-46f6-9821-dcb4405322b7

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}