[P2][Review] Do not retry exhausted durable quotas or shorten Retry-After into an early request #16

Activity

1. karlwaldman commented on Sep 5, 2026

   

   karlwaldman

   on Sep 5, 2026

   MemberAuthor

   More actions

   Adversarial sprint admission — rank 33/70

   Scheduled Monday, 2026-09-07 (America/New_York) in the approval-gated seven-night plan. Planning only; no implementation or merge yet.

   Narrow outcome: Remove futile quota retries and premature Retry-After requests.

   Required issue → PR → independent review → ordered merge evidence

   • Reproduce on the execution-day main and confirm exact files/callers; the review baseline was the current-main SHA recorded in this issue. If already fixed or unnecessary, record proof and close without a cosmetic PR.
   • Record why deletion alone does or does not solve this; delete redundant code before adding a helper/guard/automation.
   • A focused failing regression/fixture first where practical; test the relevant malformed, absent, stale, rejected or timeout path as well as success. No production customer mutations or raw-secret evidence.
   • One independently revertible PR, containing the narrow correction and targeted tests; no unrelated cleanup. Explain public/API compatibility and rollback.
   • Independent adversarial reviewer tests the failure case, challenges the assumptions and simpler alternatives, and resolves every blocking finding.
   • Refresh against the actual merge base and rerun checks before merging in this plan's numbered order. Preserve all existing required checks; no bypass to hit ten.
   • Link merge SHA plus approved deployed/published artifact and post-release proof where relevant; otherwise mark release pending rather than claiming shipped.

   Risk window: Only isolated low/medium-risk changes. If admission discovers authentication/entitlement, destructive migration, live ingestion semantics or a breaking interface change, hold and move to the next approved Friday/Saturday window.
2. karlwaldman commented on Sep 5, 2026

   

   karlwaldman

   on Sep 5, 2026

   MemberAuthor

   More actions

   Coverage-audit revision — current sprint admission

   Plan v2: oilpriceapi-api#8015 supersedes earlier scheduling comments. Rank 37/70; Monday, 2026-09-07, America/New_York. Low/medium-risk only; reclassify to a Friday/Saturday slot if boundary/compatibility risk is higher.

   Intended outcome: Remove futile quota retries and premature Retry-After requests.

   Coverage ledger: #8056. Inventory/pattern screening is complete; exhaustive semantic/runtime sign-off is not. Before implementation, review the complete affected file set and callers, revalidate against current main, supply the missing negative-path proof, and retain prior scope corrections. Prefer deletion, then simplification; correct behavior needs no PR.

   No implementation is approved yet. After Karl approves: focused reversible PR → independent adversarial review → passing affected checks → merge in plan order → separately approved release/publish and artifact/customer-path verification. Maximum ten planned slots per night, not ten forced merges. If the dated window elapses before approval, re-anchor the calendar rather than backdate execution.
3. karlwaldman

   added 2 commits that reference this issue on Sep 13, 2026

   docs: keep the changelog clear of the fixed-cadence claim guard

   ...

   98fa861

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

   ...

   21339d4
4. karlwaldman

   closed this as completedin #19on Sep 13, 2026
5. karlwaldman commented on Sep 13, 2026

   

   karlwaldman

   on Sep 13, 2026

   MemberAuthor

   More actions

   Confirming this issue against production, and flagging one edge case PR #19 does not close. Executed 2026-09-13.

   The real refusal envelope matches what #19 is built for. Live GET https://api.oilpriceapi.com/v1/demo/prices:

   HTTP/2 429
   retry-after: 26385
   x-ratelimit-limit-day: 25
   x-ratelimit-remaining-day: 0
   
   {"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 >> 30s case retryDelay() refuses to shorten. On main today this response costs 4 requests and 90 seconds of sleep against a limit that does not reset for 7.3 hours; under #19 it costs one request and no sleep. The policy is correct and it is aimed at the real production body, not a guess.

   Two notes:

   1. Not every zero-delay hot retry is closed by #19. The new guard rejects a negative Retry-After, but two other values still land on 0:

   Retry-After	(int) cast	main sleeps	#19 sleeps
   -5	-5	[0]	falls back to backoff (fixed)
   1e400	0 (INF cast to int)	[0]	[0]
   0.9	0	[0]	[0]

   is_numeric('1e400') is true and (int) (float) '1e400' is 0 in PHP 8, so a malformed header produces an immediate retry rather than a backoff. Suggest requiring the value to match /^\d+$/ (RFC 9110 delta-seconds is a non-negative integer) and treating anything else as absent.

   2. The body carries its own delay. The live 429 has error.retry_after: 26402 alongside retry-after: 26385 in the header. Only the header is read, which is fine, but they can disagree and the body's value is the one the customer sees in the JSON.

   Unrelated to the retry logic: #19 branches from pre-#17 main and edits isRetryable() and the retry loop adjacent to #17's new normalizeApiPath()/assertSameOrigin(), so expect a conflict in src/Client.php on rebase.