Repository navigation
feat(client): opt-in retries that follow the shared retry policy, off by default - #58
gonzalo-zenrows wants to merge 1 commit into
Conversation
|
Same retry policy across all clients (429/502/503/504 and pre-response connection errors; 2 retries by default; full jitter; Retry-After capped at 30 s; 90 s total budget; timeouts never retried):
|
jonathan-zenrows
left a comment
There was a problem hiding this comment.
Can't be merged as is.
1. Conflicts with #49, and the fallback in this branch is now dead code
Main is on 2.5.0 with #49, which removed the extract() AutoParse fallback and added prepareDomain/preparedDomainStatus. This branch still has fallbackToAutoparse = true and the shared clock (src/index.ts:189-208), and src/index.ts conflicts. If the conflict is resolved by keeping this side, #49 gets reverted without anyone noticing: the fallback comes back on and the prepared-domain exports disappear. After the rebase, delete the CallClock fallback path, the tests at tests/retries.test.ts:375-415, and the README sentence about the fallback sharing the budget.
2. Retries hold queue slots
The backoff sleep runs inside the fastq worker (src/index.ts:130, 306). With the default concurrency: 5, five calls that get 429 + Retry-After: 30 block every other call on that client for 30 s. Before this PR nobody hit it, because retries defaulted to 0. Repro: concurrency 5, a server that answers 429 Retry-After: 30 to the first 5 requests, and fire a 6th call: it waits 30 s before it even leaves. It should release the slot while it waits, or at minimum document this.
3. Release note
0 to 2 retries, plus RangeError on bad options (src/retry.ts:57-76), ships as 2.6.0 with no note under feat. It needs feat!/BREAKING CHANGE: or a written changelog line.
Small things:
{"Idempotency-Key": 123}from plain JS throws aTypeErroratvalue.trim()(src/retry.ts:116). UseString(value).trim().ENETUNREACH/EHOSTUNREACHare retried here but not in the other SDKs. They're fine to retry, but let's make all 5 the same.localhost(happy-eyeballs →AggregateError) isn't tested, only127.0.0.1. On Node 26 the code arrives correctly; add a test so we don't depend on that.
… by default With retries above 0, fetch(), get() and extract() retry a 429, 502, 503, 504, a 422 other than no_data_extracted, and a connect failure before the request was sent. post(), a network error after sending, a response timeout, an abort and target-site statuses are never retried. Waits follow Retry-After or full jitter (new retryMaxWait), within a 90 s budget (new retryBudget). Every retries value 2.6.0 accepted still builds a client with the same count; NaN and Infinity now turn retries off with a warning.
5a582d8 to
d10c3cb
Compare
Merge after #65, which adds the changelog section this PR's changelog lines use.
This brings
fetch(),get()andextract()in line with the shared retry policy.retriesstill defaults to 0, so a caller who never sets it sees no change: every request is sent once, with the same headers, answers and errors.When retries are on (
retriesabove 0)no_data_extracted; a connection that failed before the request was sent (ECONNREFUSED,ENOTFOUND,EAI_AGAIN,ENETUNREACH,EHOSTUNREACH,UND_ERR_CONNECT_TIMEOUT, orETIMEDOUTfromconnect; anAggregateErroronly when every inner error is one of these).post(); a 500 or any other status; a network error after sending, a response timeout or an abort; any status withoriginal_status, or one listed inallowed_status_codes.Retry-Afterin whole seconds, clamped to 0.25-30 s; otherwise a random 0.25 s to min(retryMaxWait, 2^attempt) s. NewretryMaxWait, default 30.retryBudget(new, default 90 s).client.batch,client.crawl,prepareDomain()andpreparedDomainStatus()are untouched.If you already set
retriesno_data_extractedis still retried.post()is never retried.original_status,allowed_status_codes) is no longer retried.Retry-After(capped at 30 s) or full jitter instead of a fixed 1, 2, 4 s, within the 90 s budget.retriesvalue 2.6.0 accepted still builds a client with the same count (a numeric string is its number, a fraction rounds up, 0 or less is off).NaN,Infinityor another non-number now turns retries off with one warning; before, it retried without limit.Tests
Retry-Aftervalues, jitter bounds, statuses and network errors, plus one test per rule (target-site statuses, budget,post(), prepared-domain calls).retriesvalue 2.6.0 accepted and checks the requests sent and the warning; it matches a probe run against 2.6.0.fetchagainst a local server; with noretries, the refused connection is sent once.pnpm check,pnpm test -- --run,pnpm buildandtsc --noEmitpass; the built.d.tsadds the two options and one private field, nothing else.BEGIN_COMMIT_OVERRIDE
feat(client): opt-in retries following the shared retry policy, with Retry-After, full jitter and a 90 s budget; still off by default
behaviour(client): with retries set, a 429 and a 502 are now retried; post(), errors after sending, response timeouts, aborts and target-site statuses no longer are; waits follow Retry-After or full jitter; NaN or Infinity turns retries off
END_COMMIT_OVERRIDE