fix(retry): restrict retries to safe requests and bound Retry-After (#33)

[karlwaldman]

Closes #33.

Rebased onto main 6ceca9b6 (the #36 origin guard) on 2026-09-13. Both changes are kept: doRequestWithHeaders now validates the retry configuration, resolves and origin-checks the path via resolveEndpoint, and only then enters the retry loop, which builds every request from the validated requestURL. Every number below was re-measured on the rebased branch against the new main. See Rebase at the bottom.

Confirmed live, twice over

Production, 2026-09-13. The keyless demo endpoint answers 429 with a Retry-After counting down to the next daily reset:

$ curl -sD- https://api.oilpriceapi.com/v1/demo/prices/latest | grep -iE '^(HTTP|retry-after|x-ratelimit-remaining-day)'
HTTP/2 429
x-ratelimit-remaining-day: 0
retry-after: 28197

28,197s = 7h 50m. The issue recorded 31,612s (8h 47m) earlier the same day; the number is whatever is left until midnight UTC, so the worst case is ~24h.

Through the real client, client.go:745 (verified — that is the Retry-After read, :747 the uncapped delay =) passed that straight to time.After. A caller on context.Background() has no deadline to rescue it.

Measured before / after — the 7.8-hour block

Driven through GetLatestPrices against an httptest server replaying the production header, with context.Background():

	before	after
time to return	still blocked at 10s (test timeout; the real wait is 28,197s = 7.83h)	961.583µs
requests sent	1, then a 7.8-hour sleep	1
what the caller gets	nothing, for hours	*RateLimitError{RetryAfter: 28197}
cancellable	no (context.Background())	n/a — it returns

The test asserts on observable behaviour — a real 429 with a real huge header driven through the public method, returning promptly with the typed error — not on an internal delay variable.

The other four, also confirmed on main 3b24c6d

Each was reproduced with a counting test server before anything was written.

defect	measured on main	after
Non-idempotent POST replayed on 5xx	CreateSubscription sent POST /v1/subscriptions 4 times on a 503	1
Non-idempotent POST replayed on ambiguous transport failure	CreateWebhook sent POST /v1/webhooks 3 times after the connection died mid-response	1
Negative Retry-After	Retry-After: -3600 passed strconv.Atoi; time.After on a negative duration fires at once — 4 requests in 1.01ms	falls back to exponential backoff, 2 requests over 1.00s
Durable quota retried	429 with X-RateLimit-Remaining-Day: 0 retried 4 times	1, returns *RateLimitError
Invalid retry config	WithRetries(-1) sent 0 requests and returned request failed after -1 retries: %!w(<nil>) — a nil-wrap formatting artefact, not a diagnosis	*ConfigurationError, 0 requests
HTTP-date Retry-After	not parsed at all (RFC 9110 permits it); the client slept 7s of exponential backoff instead and RateLimitError.RetryAfter read 0	parsed, bounded, reported
Overflowing Retry-After	Retry-After: 9223372036854775807 × time.Second wraps int64 nanoseconds; 4 requests sent	clamped, 1 request

One claim from the issue that did not hold

max_retries=0 being ignored was a Python defect (max_retries=0 still retried 3 times). It does not reproduce in Go — WithRetries(0) already sent exactly one request on main. The Go analogue is WithRetries(-1), which disabled the client entirely and reported it as a %!w(<nil>) format error. Both are now covered by a regression test.

The fix

retry.go holds one classifier used by every request. Three rules, in order:

1. Only safe requests are replayed. GET/HEAD/OPTIONS/TRACE. A retried POST or PATCH is a duplicate write whenever the server committed and the response was lost, and the API offers no idempotency key that would make it safe.
2. Only recoverable rate limits are retried. X-RateLimit-Remaining-Day or -Month at zero means the allowance is spent and will not refill inside any retry budget, so the typed *RateLimitError comes back immediately and the caller can surface an upgrade path instead of stalling. The per-minute X-RateLimit-Remaining burst window is still retried.
3. Every wait is bounded. Retry-After is parsed in both RFC 9110 forms, rejected when absent, unparseable, zero, negative or in the past (falling back to exponential backoff rather than to a zero or negative delay), and clamped at one year so the × time.Second conversion cannot overflow. A delay beyond WithMaxRetryWait (new option, default 60s) or beyond the caller's own context deadline ends the retry loop and returns the typed error carrying the server's requested wait — it does not sleep, and it does not re-send early. Context cancellation still interrupts every wait.

handleError now uses the same parser, so a date-form Retry-After reaches RateLimitError.RetryAfter instead of 0.

PUT and DELETE are deliberately excluded despite being idempotent by definition. Idempotent means a repeat has the same effect on the resource, not that the caller observes the same outcome; a replayed DELETE can remove a resource recreated in between. This makes DeleteWebhook, DeleteAlert and DeleteSubscription non-retrying. Say the word if you would rather they retried.

Red / green

Red, re-run against the new origin/main 6ceca9b6 — client.go taken from that commit (origin guard included) plus the new option/field declarations only, so the file compiles. #36's path changes did not alter the retry defect; the output is the same as before the rebase:

$ go test -run 'TestHugeRetryAfter|TestRetryAfter|TestNegativeRetryAfter|TestOverflow|TestDurableQuota|TestNonIdempotent|TestSafeRequest|TestContextCancellation|TestRetryWaitBounded|TestRetryConfiguration' -v .
    retry_policy_test.go:72: still blocked after 10s: Retry-After 28197s (7.83 hours) was honoured uncapped; requests sent=1
--- FAIL: TestHugeRetryAfterDoesNotBlock (10.00s)
--- PASS: TestRetryAfterWithinBudgetIsHonoured (1.00s)
    retry_policy_test.go:127: retried after 309.25µs; backoff collapsed to zero (hot retry loop)
    retry_policy_test.go:127: retried after 292.708µs; backoff collapsed to zero (hot retry loop)
--- FAIL: TestNegativeRetryAfterDoesNotCollapseBackoff (2.00s)
    --- FAIL: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After=-3600 (0.00s)
    --- FAIL: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After=0 (0.00s)
    retry_policy_test.go:146: blocked 7.003931625s on a 6-hour HTTP-date Retry-After
--- FAIL: TestRetryAfterHTTPDate (7.00s)
    retry_policy_test.go:180: sent 4 requests, want 1
--- FAIL: TestOverflowRetryAfterDoesNotWrap (0.00s)
    retry_policy_test.go:201: durable quota 429 sent 4 requests, want 1
--- FAIL: TestDurableQuotaIsNotRetried (3.00s)
--- PASS: TestBurstRateLimitIsStillRetried (1.00s)
    retry_policy_test.go:248: POST /v1/subscriptions sent 4 times on 503, want 1
--- FAIL: TestNonIdempotentWriteNotReplayedOn503 (7.00s)
    retry_policy_test.go:280: POST /v1/webhooks sent 3 times after an ambiguous transport failure, want 1
--- FAIL: TestNonIdempotentWriteNotReplayedOnTransportError (3.00s)
--- PASS: TestSafeRequestStillRetriesOnTransportError (1.00s)
--- PASS: TestContextCancellationInterruptsRetryWait (0.15s)
    retry_policy_test.go:353: waited 2.004543s against a 2s deadline for a 30s Retry-After
--- FAIL: TestRetryWaitBoundedByContextDeadline (2.00s)
    retry_policy_test.go:387: got *fmt.wrapError (request failed after -1 retries: %!w(<nil>)), want *ConfigurationError
--- FAIL: TestRetryConfiguration (2.00s)
    --- PASS: TestRetryConfiguration/zero_retries_sends_exactly_one_request (0.00s)
    --- FAIL: TestRetryConfiguration/negative_retries_is_rejected,_not_silently_fatal (0.00s)
    --- PASS: TestRetryConfiguration/explicit_retry_count_is_honoured (2.00s)
FAIL	github.com/OilpriceAPI/oilpriceapi-go	39.193s

The four already-passing tests are deliberate regression guards: a Retry-After inside the budget is still honoured, a burst 429 is still retried, a safe GET still retries through a transport error, and context cancellation still interrupts a wait. They must stay green on both sides, and they do.

Green, with the fix:

$ go test -run '...same set...' -v .
    retry_policy_test.go:70: returned in 961.583µs with rate limit exceeded: {"error":"rate limit exceeded"} (retry after 28197 seconds)
--- PASS: TestHugeRetryAfterDoesNotBlock (0.00s)
--- PASS: TestRetryAfterWithinBudgetIsHonoured (1.00s)
--- PASS: TestNegativeRetryAfterDoesNotCollapseBackoff (4.01s)
    --- PASS: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After=-3600 (1.00s)
    --- PASS: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After=0 (1.00s)
    --- PASS: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After=not-a-number (1.00s)
    --- PASS: TestNegativeRetryAfterDoesNotCollapseBackoff/Retry-After= (1.00s)
--- PASS: TestRetryAfterHTTPDate (0.00s)
--- PASS: TestOverflowRetryAfterDoesNotWrap (0.00s)
--- PASS: TestDurableQuotaIsNotRetried (0.00s)
--- PASS: TestBurstRateLimitIsStillRetried (1.00s)
--- PASS: TestNonIdempotentWriteNotReplayedOn503 (0.00s)
--- PASS: TestNonIdempotentWriteNotReplayedOnTransportError (0.00s)
--- PASS: TestSafeRequestStillRetriesOnTransportError (1.00s)
--- PASS: TestContextCancellationInterruptsRetryWait (0.15s)
--- PASS: TestRetryWaitBoundedByContextDeadline (0.00s)
--- PASS: TestRetryConfiguration (2.00s)
ok  	github.com/OilpriceAPI/oilpriceapi-go	9.179s

Red-capability re-verified after the rebase by restoring the pre-fix doRequestWithHeaders from 6ceca9b6 and re-running: the same nine tests fail again, then pass on restore.

The #36 origin tests inherited from main also stay green on this branch:

$ go test -run 'TestRawRejectsOffOriginPaths|TestRawAllowsLegitimatePaths|TestCustomBaseURLStillWorks|TestTypedEndpointsRejectOffOriginIDs' .
ok  	github.com/OilpriceAPI/oilpriceapi-go	0.012s

Full suite

Baseline re-measured on the new main myself, not carried over:

	baseline origin/main 6ceca9b6	this branch
go test ./...	ok ... 1.314s	ok ... 16.489s
passing tests	265	285 (+20)
failing tests	0	0
go test -race ./...	—	ok ... 18.232s

(265 is the post-#36 baseline; it was 237 before #36 landed.)

go vet ./... clean, gofmt -l . clean. Go 1.26.5 darwin/arm64. The suite is slower because several new tests exercise real one-second backoff waits rather than asserting on an internal timer.

Reviewer notes

• Behaviour change for writes. POST, PATCH and DELETE no longer retry. That is the point of the issue, but it means a transient 503 on CreateSubscription now surfaces to the caller instead of being papered over. Minor-version material, not a patch.
• WithMaxRetryWait default is 60s. Chosen so a normal burst limit (per-minute window) is still absorbed while a daily reset never is. A caller who genuinely wants to wait longer can raise it, and cancellation still works.
• Durable-quota detection reads X-RateLimit-Remaining-Day / -Month, which production sends on every 429 (see the curl above). It does not parse the error body, so a wording change cannot break it.
• No CHANGELOG entry: release_contract_test.go requires the top changelog heading to equal Version, so the entry has to land with the version bump at release time.
• Not merged, no auto-merge, no release tagged.

Rebase onto 6ceca9b6

Two conflicts, both the "both sides added" shape, both resolved by keeping each side whole:

• errors.go — InvalidPathError (fix(security): reject raw paths that change the authenticated API origin (#32) #36) and ConfigurationError (fix(retry): restrict retries to safe requests and bound Retry-After (#33) #37) were appended at the same point. Both kept.
• client.go — the top of doRequestWithHeaders. fix(security): reject raw paths that change the authenticated API origin (#32) #36 inserted the resolveEndpoint call there; fix(retry): restrict retries to safe requests and bound Retry-After (#33) #37 inserted the retry-config validation and method classification. Resolved as: config validation, then path resolution, then method classification, then the loop. Config is checked first because a negative WithRetries is a programming error the caller should hear about regardless of the path.

The request-construction line resolved to #36's http.NewRequestWithContext(ctx, method, requestURL, body), so every attempt — including every retry — is built from the origin-validated URL rather than from c.baseURL + endpoint. That is the property that had to survive, and it is asserted by the #36 tests, which are on this branch and green.

Nothing was dropped from either change. The two flagged judgement calls are unchanged by the rebase and still open: DELETE excluded from retries, and .. segments rejected.

🤖 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: 7e73cceb-1bcb-4eab-80a8-3e16e6b23598

---

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.}