fix(sdk): request timeout, network-error normalization, idempotent-only retries, Retry-After, bounded iterate - #274
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6f00ef4fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 257d4da83b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Triaged the automated review feedback (Codex + Greptile) plus the known residuals. Most items were already handled in Addressed now (51a9ad5):
Already addressed in
Intentionally skipped:
Green gate from the worktree: SDK 85 pass / 2 skip, MCP 77 pass; both builds + type-checks clean. No public SDK surface change in this commit, so no docs regeneration needed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51a9ad5638
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ly retries, Retry-After, bounded iterate Harden the TypeScript SDK transport against the failure modes that can wedge or corrupt a caller in production: - Request timeout: every request is now bounded by an AbortController-based timeout (configurable via `timeoutMs`, default 30s, `0` disables). A hung upstream is aborted and rejected with a new `TimeoutError`. Applied once in Transport so it covers both the openapi-fetch client and `executeManual`. (new `client/timeout.ts`) - Network-error normalization + retry: a thrown `fetch` failure (DNS, ECONNRESET, "fetch failed", ...) is normalized to a `Terminal49Error` (`NetworkError`) and run through the same retry policy as a 5xx, via the RetryInterceptor `onError` hook. (`errors.ts`, `interceptors.ts`) - Idempotent-only retries: retries are gated to GET/HEAD (and writes that carry an `Idempotency-Key`). A POST/PATCH that hits a transient 5xx/network error is no longer silently replayed, which could have created duplicate tracking requests. (new pure `client/retry-policy.ts`) - Retry-After: 429 backoff honors the server `Retry-After` header (delta-seconds or HTTP-date) instead of fixed exponential only. - Bounded iterate(): `BaseManager.createIterator` now stops at documented `maxPages` / `maxRows` safety caps so a no-op/overly-broad filter cannot walk the entire dataset. (`managers/base.ts`) - Search path: `executeManual` reads success bodies with `readSuccessBody` (new `client/body.ts`) so a non-JSON 200 is surfaced instead of being silently collapsed to `undefined`, and a thrown network error is normalized. - Documented the load-bearing interceptor registration order (Retry registered last so it runs before error-mapping on the reverse onResponse/onError pass). Pure logic is extracted into unit-testable modules (retry-policy, timeout, body). New + updated mock-transport tests cover: timeout abort, network-error normalize+retry, Retry-After wait, write-not-retried, and the iterate bound. Closes DEV-10663 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…manual-path error normalization Addresses review feedback on the transport-resilience PR: - Expose `maxPages`/`maxRows` on the public `iterate()` signatures by adding them to `ListOptions`, so TypeScript callers can raise the iterator safety caps with types (previously type-unreachable). Regenerated SDK reference docs for the new `ListOptions` properties. - Normalize a network error thrown from a retry kicked off inside `RetryInterceptor.onResponse` in the manual `/search` path: wrap the `retry.onResponse` call in `executeManual` so the caller always sees a `NetworkError` rather than a raw `TypeError`. - Cap a honored `Retry-After` delay at 60s (`MAX_RETRY_AFTER_MS`) so an adversarial/misbehaving upstream cannot wedge the caller in a multi-hour sleep (the request timeout guards `fetch`, not the backoff sleep). - Tighten network-error detection: a bare `TypeError` is no longer treated as retryable unless its message looks network-ish, so a programming-bug `TypeError` is not retried up to `maxRetries` and masked. - Document why `search()` uses `executeManual` (no `/search` entry in the generated OpenAPI types, so it cannot route through the typed client). Added unit tests for the Retry-After cap and the tightened TypeError guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… state across onError->onResponse Two transport-resilience gaps in the typed-client retry path remained after the prior review pass: - A network error thrown by a response-triggered retry inside `RetryInterceptor.onResponse` propagated as a raw `TypeError`. openapi-fetch does not route an `onResponse` throw back through `onError`, so the caller saw an un-normalized error instead of a `NetworkError`. Wrap the retry `fetch` and normalize via `toNetworkError`, matching the initial-fetch path. - `onError` deleted the replay entry in its `finally` even when it returned a recovered Response. openapi-fetch then runs `onResponse` for that same request id, but with the replay state gone it could not retry a subsequent 429/5xx, so a transient network-failure -> 500 -> success sequence failed despite remaining retry budget. Only delete the replay entry on the terminal error paths; the `onResponse` chain deletes it once the response is settled. Added transport tests for both paths (verified red against the prior code). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
51a9ad5 to
dcfd05d
Compare
|
Rebased onto current Re-audited all 9 inline Codex/Greptile review comments against the current code on the branch tip ( Verified fixed in code:
Confirmed intentional skips (documented in thread replies, still correct):
No further code changes were needed — everything substantive had already landed in Checks after rebase (from the worktree):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcfd05df63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // would otherwise surface as a raw TypeError. Normalize it to a | ||
| // NetworkError so the caller sees the same error shape as the | ||
| // initial-fetch path. | ||
| throw toNetworkError(error); |
There was a problem hiding this comment.
Keep retrying retryable failures during response replay
When a 429/5xx response triggers this retry loop, any network failure from the replayed fetch is thrown immediately, so a sequence like 503 -> TypeError('fetch failed') -> 200 fails even with maxRetries: 2. Fresh evidence in the current diff is that the catch still exits via throw toNetworkError(error) before incrementing the attempt or checking whether remaining retry budget can cover a retryable network failure.
Useful? React with 👍 / 👎.
| } catch (retryError) { | ||
| attempt++; | ||
| if (attempt >= this.maxRetries) { |
There was a problem hiding this comment.
Stop replaying after non-retryable retry errors
After the initial error passes isRetryableNetworkError, subsequent retry failures are retried unconditionally until the budget is exhausted. If the first replay is aborted by the SDK timeout or caller cancellation, or throws a non-network TypeError, maxRetries > 1 will still sleep and replay again even though the retry policy explicitly treats those errors as non-retryable; re-check retryError before continuing the loop.
Useful? React with 👍 / 👎.
Summary
Hardens the
@terminal49/sdktransport layer against failure modes that can hang or corrupt a caller in production. Pure logic is extracted into small unit-testable modules behind the existing interceptor seam.AbortController-based timeout (timeoutMs, default30000,0disables). A hung upstream is aborted and rejected with a newTimeoutError. Applied once inTransportso it covers both the openapi-fetch client and the manualsearch()path. (newclient/timeout.ts)fetchfailure (DNS,ECONNRESET,"fetch failed", ...) is normalized to aTerminal49Error(NetworkError) and run through the same retry policy as a 5xx, via theRetryInterceptor.onErrorhook.GET/HEAD(and writes that carry anIdempotency-Key). APOST/PATCHhitting a transient 5xx/network error is no longer silently replayed (which could create duplicate tracking requests). (new pureclient/retry-policy.ts)Retry-Afterheader (delta-seconds or HTTP-date) instead of fixed exponential only.iterate()—BaseManager.createIteratorstops at documentedmaxPages/maxRowscaps so a no-op / overly-broad filter cannot walk the entire dataset. (managers/base.ts)executeManualreads success bodies viareadSuccessBody(newclient/body.ts) so a non-JSON 200 is surfaced instead of being silently collapsed toundefined, and a thrown network error is normalized like the typed path.onResponse/onErrorpass).Behavior change worth a reviewer's eye
The previous SDK retried
POSTtracking-request creation on a 5xx; that is now intentionally blocked (duplicate-write risk). The corresponding test inclient.test.tswas updated to document the new safe contract.Issues
Closes DEV-10663
Green gate
All six gate commands pass (baseline SDK 51 pass/2 skip, MCP 77 pass):
build @terminal49/sdkbuild @terminal49/mcptype-check @terminal49/sdktype-check @terminal49/mcptest @terminal49/sdk --runtest @terminal49/mcp --runoxlint+oxfmt --checkclean on changed files.Notes
err.message-leak fix lives in the MCP server (a separate PR);api/mcp.ts/packages/mcpare intentionally untouched here.🤖 Generated with Claude Code
Greptile Summary
This PR hardens the
@terminal49/sdktransport layer with five targeted reliability improvements:AbortController-based request timeouts, network-error normalization toNetworkError, idempotent-only retries (blocking unsafe replay ofPOSTwrites),Retry-After-aware backoff, andmaxPages/maxRowscaps onBaseManager.createIterator.withTimeoutwraps every fetch (typed and manual paths) and a newtoNetworkErrorhelper ensures thrown fetch errors always surface asTerminal49Errorsubclasses rather than rawTypeErrors.shouldRetryRequestgates retries toGET/HEAD/OPTIONSor writes carrying anIdempotency-Key, preventing duplicate tracking-request creation on transient 5xx — a behavioral change explicitly tested.createIteratornow stops atDEFAULT_ITERATE_MAX_PAGES(1000) andDEFAULT_ITERATE_MAX_ROWS(100k) so a broad filter cannot silently page the entire dataset.Confidence Score: 3/5
Safe to merge once the uncaught-error gap in
executeManualis addressed; the idempotency change is intentional and well-tested.The manual fetch pipeline (
executeManual, used bysearch()) normalizes network errors on the initial fetch but leaves an uncovered path: if the initial call returns a 5xx and the subsequent retry insideretry.onResponseitself throws a network error, that rawTypeErrorescapes to the caller without going throughtoNetworkError. Every other change in the PR is straightforward and well-tested. TheRetry-Afterparsing also lacks an upper-bound cap, which could stall a process for an arbitrarily long period if the API returns a large delta-seconds value.sdks/typescript-sdk/src/client/transport.ts(executeManual error handling path) andsdks/typescript-sdk/src/client/retry-policy.ts(Retry-After cap, TypeError breadth).Important Files Changed
executeManual), and interceptor ordering. Network errors insideretry.onResponseretries escape the normalization guard.Retry-Afterparsing has no upper-bound cap;isRetryableNetworkErrormatches allTypeErrorinstances regardless of message.onErrorhook toRetryInterceptorfor network-error recovery, integrates idempotency gating, and addsRetry-Afterbackoff. Logic and cleanup (finally-delete) are correct.AbortController-based timeout wrapper. Caller signal forwarding and timer cleanup infinallyare correct.maxPages/maxRowssafety caps tocreateIterator. Row-count and page-count checks are correct; yields exactlymaxRowsitems.readSuccessBodyhelper surfaces non-JSON and empty bodies correctly usingresponse.clone().text()to avoid consuming the stream.NetworkErrorandTimeoutErrorclasses with properinstanceof Terminal49Errorhierarchy fortoNetworkErrorpass-through.UpstreamError.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "docs(sdk): regenerate reference for tran..." | Re-trigger Greptile