fix: do not retry exhausted durable quotas or shorten Retry-After (#16)

[karlwaldman]

Closes #16.

Confirmed live on origin/main (6879501) — and one part already safe

Mock transport with a request counter and an injected sleeper:

scenario	origin/main
429 MONTHLY_QUOTA_EXCEEDED, Retry-After: 120	4 requests, sleeps [30, 30, 30] — confirmed live
429 with Retry-After: 120	request sent after 30s — confirmed live
429 with Retry-After: -30	sleeps 0 — already non-negative (parseRetryAfter clamps with max(0, ...)); does not reproduce the Python negative-sleep bug
maxRetries = 0	1 request, 0 sleeps — already correct; does not reproduce the Python "ignored entirely" bug

Both Python-specific defects named in the issue were checked rather than assumed and neither ports to PHP. The two that are real here are the durable-quota retry and the shortened Retry-After.

Quota classification, from the API's own error contract

Codes taken from oilpriceapi-api origin/main (107f140ec), app/controllers/v1/base_controller.rb and app/controllers/v1/demo_controller.rb — not guessed:

Durable (never retried): MONTHLY_QUOTA_EXCEEDED, DAILY_QUOTA_EXCEEDED, QUOTA_EXCEEDED, TRIAL_LIMIT_EXCEEDED, TRIAL_EXPIRED, EMAIL_CONFIRMATION_REQUIRED, DEMO_RATE_LIMIT_EXCEEDED.

Recoverable (still retried): RATE_LIMIT_EXCEEDED, HOURLY_CIRCUIT_BREAKER_EXCEEDED, and every 5xx.

MONTHLY_QUOTA_EXCEEDED is a known misnomer for daily-window free users (api#5788) and is deliberately stable as a client contract, so matching on it is correct for both windows. Codes are read from error_code (top level, used by the quota path), error.code (nested, used by the demo path) and code, compared case-insensitively.

Retry-After

• longer than the 30-second budget → do not sleep less; stop and raise RateLimitException carrying the server's real delay and the upgrade URL, so the caller schedules it properly
• negative delta-seconds → malformed, not "retry now": fall back to exponential backoff instead of a zero-second hot retry, and report retryAfter as null rather than 0
• HTTP-date already in the past → genuinely means now, becomes 0
• unparseable → exponential backoff with jitter, as before

Every sleep remains non-negative and <= 30s; a test asserts that across a mixed sequence.

Red / green

Red, proven against pre-fix code (git checkout origin/main -- src/Client.php, rerun, restore):

Tests: 43, Assertions: 364, Failures: 10.

1) RetryPolicyTest::testExhaustedDurableQuotaIsNotRetried@monthly quota, top-level error_code
A spent durable quota must cost exactly one request.
Failed asserting that 4 is identical to 1.

7) RetryPolicyTest::testRetryAfterLongerThanTheBudgetIsNotShortened
A 120s Retry-After must not produce a request after 30s.
Failed asserting that 4 is identical to 1.

Green, with the fix:

OK (43 tests, 375 assertions)

Baseline on clean origin/main before any change: OK (26 tests, 323 assertions). PHP 8.5.8, Composer 2.10.2, PHPUnit 13.3.3.

The maxRetries = 0 and negative-Retry-After tests pass on origin/main too — they are regression cover for behaviour that was already correct, kept so the Python failure mode cannot appear here later.

Compatibility

Public API unchanged; no new dependency and no scheduler. isRetryable() and backoffDelay() were private. The only behaviour change is fewer requests and no early retry.

Not merged, no auto-merge, no release tagged.

🤖 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: fe1a7c12-6397-459a-a812-f6ba3898438f

---

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

[karlwaldman]

---

Updated for main at 3feb505fa (after #17 merged)

origin/main was merged into this branch. This one had a real code conflict, unlike #18: #17 inserted its path-normalization helpers immediately before isRetryable(), and this branch changed that method's signature from int $statusCode to HttpResponse $response. The two hunks were adjacent, so git could not pick.

Resolved by hand, keeping everything from both sides:

• from fix(security): reject raw paths that change the authenticated API origin (#14) #17: normalizeApiPath(), assertSameOrigin(), originOf(), offOriginPath(), and both call sites inside request() — $path = $this->normalizeApiPath($rawPath) and $this->assertSameOrigin($url, $rawPath)
• from this branch: isRetryable(HttpResponse), isDurableQuotaExhausted(), retryDelay(), the revised parseRetryAfter(), the DURABLE_QUOTA_CODES list, and the retry loop that breaks instead of shortening Retry-After

CHANGELOG.md also conflicted; both entries are kept. Nothing had to be dropped.

New baseline, clean origin/main at 3feb505f: OK (50 tests, 380 assertions) — up from OK (26 tests, 323 assertions).

Re-proven against the new main (git checkout origin/main -- src/Client.php, rerun, restore):

Tests: 67, Assertions: 421, Failures: 10.

1) RetryPolicyTest::testExhaustedDurableQuotaIsNotRetried@monthly quota, top-level error_code
A spent durable quota must cost exactly one request.
Failed asserting that 4 is identical to 1.

2) RetryPolicyTest::testExhaustedDurableQuotaIsNotRetried@trial limit, top-level error_code
A spent durable quota must cost exactly one request.
Failed asserting that 4 is identical to 1.

All 10 failures are RetryPolicyTest; RawPathOriginTest reports OK (24 tests, 57 assertions) in that same pre-fix state, so #17's guard is intact and the red is this defect alone.

Green, with the fix, on top of the new main: OK (67 tests, 432 assertions). Packaged Composer smoke passes.

On the merge order

This branch is based on origin/main, not on #18, so its diff stays one concern. I test-merged #18 and #19 against each other to check what happens after #18 lands: src/Client.php auto-merges cleanly — priceOrFail, normalizeApiPath and isDurableQuotaExhausted all coexist, and the file passes php -l. The only conflict between the two is the order of two bullets under ## Unreleased in CHANGELOG.md.

So merging #18 first will mark this PR conflicted again, but the resolution is one changelog bullet list, not code. Say the word once #18 is in and I will merge main again and re-verify.

[karlwaldman]

---

Updated for main at 3537cc46 (after #18 merged)

Merged origin/main again. It went exactly as predicted: src/Client.php auto-merged, and the only conflict was the bullet list under ## Unreleased → ### Fixed.

All three entries are kept, ### Security (#17) first, then under ### Fixed the malformed-row entry (#18, landed first) ahead of this branch's two.

Verified, not assumed — php -l src/Client.php clean, and all three feature sets coexist in the merged file with every call site intact:

 47:    private const DURABLE_QUOTA_CODES = [
126:    $this->priceOrFail($price, $body, '/v1/prices/latest')        #18
184:    $this->priceListOrFail($data, $body, '/v1/demo/prices')       #18
240:    private function priceListOrFail(...)                         #18
261:    private function priceOrFail(...)                             #18
292:    $path = $this->normalizeApiPath($rawPath);                    #17
305:    $this->assertSameOrigin($url, $rawPath);                      #17
324:    if (!$this->isRetryable($response) ...                        this PR
328:    $delay = $this->retryDelay($attempt, $response);              this PR
355:    private function normalizeApiPath(...)                        #17
395:    private function assertSameOrigin(...)                        #17
435:    private function isRetryable(HttpResponse $response)          this PR
447:    private function isDurableQuotaExhausted(...)                 this PR
476:    private function retryDelay(...)                              this PR

New baseline, clean origin/main at 3537cc46: OK (91 tests, 426 assertions) — was 50/380 after #17, 26/323 before either.

Re-proven against the new main (git checkout origin/main -- src/Client.php, rerun, restore):

Tests: 108, Assertions: 467, Failures: 10.

1) RetryPolicyTest::testExhaustedDurableQuotaIsNotRetried@monthly quota, top-level error_code
A spent durable quota must cost exactly one request.
Failed asserting that 4 is identical to 1.

2) RetryPolicyTest::testExhaustedDurableQuotaIsNotRetried@trial limit, top-level error_code
A spent durable quota must cost exactly one request.
Failed asserting that 4 is identical to 1.

All 10 failures are RetryPolicyTest. In that same pre-fix state both siblings stay green — RawPathOriginTest OK (24 tests, 57 assertions) and MalformedPriceRowTest OK (41 tests, 46 assertions) — so this red is this defect alone and borrows nothing from #17 or #18.

Green, with the fix: OK (108 tests, 478 assertions). Packaged Composer smoke passes. MERGEABLE / CLEAN.

Nothing failed to merge as predicted. The Price::fromArray / currency default is untouched.

[karlwaldman]

PHP expert review of this branch as proposed (98fa861). Suite green here: OK (43 tests, 375 assertions) on PHP 8.5.8. Not touching the branch — rebase in progress.

The policy is right and, unusually for a retry change, it is verifiable against production. I hit the live demo endpoint today and got the real refusal:

HTTP/2 429 · retry-after: 26385
{"status":"fail","error":{"code":"DEMO_RATE_LIMIT_EXCEEDED", ... "retry_after":26402 ...}}

That is exactly the nested error.code shape isDurableQuotaExhausted() reads and exactly the Retry-After >> MAX_BACKOFF_SECONDS case retryDelay() refuses to shorten. On main today that response costs 4 requests and 90s of sleep against a limit that resets in 7.3 hours; here it costs one request and no sleep. Case-insensitive comparison across error_code / code / error.code covers the envelopes I can see, and in_array(..., true) is the right strictness.

Four notes, none blocking:

1. Two zero-delay hot retries survive the new guard. It rejects a negative Retry-After, but is_numeric('1e400') is true and (int) (float) '1e400' is 0 in PHP 8, so 1e400 — and 0.9 — still produce retryDelay() === 0.0 and an immediate retry. RFC 9110 delta-seconds is a non-negative integer; a /^\d+$/ check would close all three cases in one line instead of special-casing the negative.

2. MAX_BACKOFF_SECONDS is now doing two jobs. It is both the backoff ceiling and "this client's retry budget", the threshold above which a request is refused rather than retried. It is a private const, so a caller who sets maxRetries: 10 cannot see or influence the point at which the SDK stops retrying. Worth deriving from $timeout/$maxRetries, or at least documenting on the constructor.

3. DAILY_QUOTA_EXCEEDED and QUOTA_EXCEEDED are in DURABLE_QUOTA_CODES but missing from the changelog list. Cosmetic.

4. Rebase. This branches from pre-#17 main and edits isRetryable() and the retry loop immediately adjacent to #17's new normalizeApiPath()/assertSameOrigin(). Expect a conflict in src/Client.php; the two changes are logically independent.

Also noted while here: the live 429 body carries its own error.retry_after (26402) alongside the header (26385). Only the header is read, which is fine, but they can disagree and the body's value is the one the customer reads.

Full review notes in review-php.md.