feat: add retry with exponential backoff in client.ts (DIS-143)#37
feat: add retry with exponential backoff in client.ts (DIS-143)#37
Conversation
Original prompt from Chris K |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
ckorhonen
left a comment
There was a problem hiding this comment.
Code Review
Overall: Request changes — All 162 tests pass and E2E works, but there are correctness issues that need code changes.
Critical Issues
-
AbortSignal reuse across retries — The same
AbortSignal.timeout(this.timeoutMs)is created once inget()/post()and shared across all retry attempts. The timeout clock runs from the first attempt, not per-retry. With 30s timeout and 3 retries + backoff, later retries will be aborted prematurely. Fix: create a fresh signal per attempt insidefetchWithRetry(). -
POST requests are retried on 5xx by default — POST is generally not idempotent. Retrying on a 500 could cause duplicate resource creation. Fix: only retry POST on 429 (request was never processed), not on 5xx.
Major Issues
-
Default
maxRetries=3is a breaking change for SDK consumers — Anyone usingnew OpenSeaClient({apiKey})as a library will now get automatic retries, changing error timing and fetch call counts. Fix: default tomaxRetries: 0for the SDK, keep CLI default at 3. -
response.body?.cancel()should be wrapped in try-catch — The stream may already be disturbed, which would throw and mask the retry logic.
Minor Suggestions
- Rename
retryBaseDelay→retryBaseDelayMsfor self-documenting units - Cap
Retry-Afterheader values at a reasonable maximum (e.g., 60s) to prevent indefinite stalls - Validate
--max-retriesand--no-retrymutual exclusivity
I've pushed a commit addressing issues 1-4.
|
Addressed all four issues in 0e3538a:
Added test for "does not retry post requests on 5xx". All 161 tests pass. |
Co-Authored-By: Chris K <ckorhonen@gmail.com>
Co-Authored-By: Chris K <ckorhonen@gmail.com>
Co-Authored-By: Chris K <ckorhonen@gmail.com>
…eam cancel safety) Co-Authored-By: Chris K <ckorhonen@gmail.com>
The healthCommand was added to src/commands/index.ts via PR #35 but these test files were not updated to include it in their mock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3e54b83 to
e4a6fbc
Compare
feat: add retry with exponential backoff in client.ts (DIS-143)
Summary
Adds automatic retry with exponential backoff to
OpenSeaClientfor transient failures. Bothget()andpost()now delegate to a sharedfetchWithRetry()private method that handles the retry loop.Config additions (
OpenSeaClientConfig):maxRetries(default 0) — number of retries before giving up. SDK defaults to 0 (no retries) for backwards compatibility; CLI defaults to 3 via--max-retries.retryBaseDelay(default 1000ms) — base delay for exponential backoffCLI flags:
--max-retries <n>— override max retries (default "3")--no-retry— disable retries entirely (sets maxRetries to 0)Retry behavior:
AbortSignal.timeoutso the per-request timeout applies independently per attempt--verboseis enabledUpdates since last revision
mainwhich introducedhealthCommand(DIS-144),--fields/--max-linesoptions (DIS-145),getApiKeyPrefix(), and User-Agent header (DIS-41).cli-api-error,cli-network-error,cli-rate-limit) to include the newhealthCommandexport.get()andpost()now usethis.defaultHeaders(which includes User-Agent) instead of inline header objects.Review & Testing Checklist for Human
isRetryableStatus(status, method)function gates this — confirm via the test "does not retry post requests on 5xx".maxRetriesdefaults to 0 inclient.ts, sonew OpenSeaClient({apiKey})has no retries (same as before). The CLI passesmaxRetries: 3explicitly. Confirm no SDK consumers are broken.Retry-AfterHTTP-date parsing:parseRetryAfterusesDate.parse()for HTTP-date format. Only the numeric-seconds path is unit tested. Verify this is acceptable or add a test for the date path.opensea collections get <slug> --verboseagainst the live API to confirm retry logging and normal request flow. Also verify--no-retrydisables retries and that--max-retries 0behaves equivalently.Notes