Fix client interface 4xx retry handling#1492
Conversation
🤖 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:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends the HTTP client's fallback and retry logic to treat 4xx responses as non-retryable, matching existing Changes4xx Response Non-Retryable Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
Greptile SummaryThis PR fixes a bug where non- Confidence Score: 4/5The change is safe to merge; logic is consistent across all three catch sites and the cause: Response detection pattern is well-scoped. The implementation correctly handles all edge cases — 429 rate-limiting is intercepted before the new 4xx branch, KnownError headers are resolved by _processResponse before status-based branching, and the single-URL path simply propagates the throw without needing _shouldSkipFallback. Both changed files are straightforward; no files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_withFallback called] --> B{Single URL?}
B -- Yes --> C[cb with maxAttempts=5]
B -- No --> D{Sticky mode?}
D -- Yes --> E[_tryStickyUrl]
D -- No --> F[_iterateUrls]
E --> G{Error thrown?}
G -- Yes --> H{_shouldSkipFallback?}
H -- Yes: KnownError or 4xx --> I[rethrow immediately]
H -- No: network error --> J[clear sticky, fall to _iterateUrls]
G -- No --> K[return result]
F --> L{Error thrown per URL?}
L -- Yes --> M{_shouldSkipFallback?}
M -- Yes: KnownError or 4xx --> I
M -- No --> N[try next URL / pass]
N --> O{Exhausted?}
O -- Yes --> P[throw lastError]
O -- No --> L
L -- No --> K
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/stack-shared/src/interface/client-interface.test.ts:452-455
The first new test uses `toThrow(Error)`, which matches any `Error` subclass including `HexclaveAssertionError`. Before this fix, the old code also threw a `HexclaveAssertionError` (not returned `Result.error`), so the `toThrow(Error)` assertion would have passed even without the fix. The `log.length === 1` assertion is the load-bearing check here. Using `toMatchObject({ name: "Error" })` would make the assertion explicitly target the plain-`Error` requirement and catch a regression if the type reverts.
```suggestion
const iface = createClientInterface({ apiUrls: urls });
await expect(sendRequest(iface)).rejects.toMatchObject({ name: "Error" });
expect(log.length).toBe(1);
expect(urlIndex(urls, log[0])).toBe(0);
```
Reviews (1): Last reviewed commit: "Preserve 5xx API fallback behavior" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const iface = createClientInterface({ apiUrls: urls }); | ||
| await expect(sendRequest(iface)).rejects.toThrow(Error); | ||
| expect(log.length).toBe(1); | ||
| expect(urlIndex(urls, log[0])).toBe(0); |
There was a problem hiding this comment.
toThrow(Error), which matches any Error subclass including HexclaveAssertionError. Before this fix, the old code also threw a HexclaveAssertionError (not returned Result.error), so the toThrow(Error) assertion would have passed even without the fix. The log.length === 1 assertion is the load-bearing check here. Using toMatchObject({ name: "Error" }) would make the assertion explicitly target the plain-Error requirement and catch a regression if the type reverts.
| const iface = createClientInterface({ apiUrls: urls }); | |
| await expect(sendRequest(iface)).rejects.toThrow(Error); | |
| expect(log.length).toBe(1); | |
| expect(urlIndex(urls, log[0])).toBe(0); | |
| const iface = createClientInterface({ apiUrls: urls }); | |
| await expect(sendRequest(iface)).rejects.toMatchObject({ name: "Error" }); | |
| expect(log.length).toBe(1); | |
| expect(urlIndex(urls, log[0])).toBe(0); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/stack-shared/src/interface/client-interface.test.ts
Line: 452-455
Comment:
The first new test uses `toThrow(Error)`, which matches any `Error` subclass including `HexclaveAssertionError`. Before this fix, the old code also threw a `HexclaveAssertionError` (not returned `Result.error`), so the `toThrow(Error)` assertion would have passed even without the fix. The `log.length === 1` assertion is the load-bearing check here. Using `toMatchObject({ name: "Error" })` would make the assertion explicitly target the plain-`Error` requirement and catch a regression if the type reverts.
```suggestion
const iface = createClientInterface({ apiUrls: urls });
await expect(sendRequest(iface)).rejects.toMatchObject({ name: "Error" });
expect(log.length).toBe(1);
expect(urlIndex(urls, log[0])).toBe(0);
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 0480409 by matching { name: "Error" } in the no-retry/fallback test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/stack-shared/src/interface/client-interface.test.ts (1)
458-466: 💤 Low valueConsider verifying the Response is attached as error cause.
The test correctly checks the error message format, but per the PR description the 4xx Response should be attached as
cause. Explicitly verifying this would make the test more complete.💡 Optional enhancement to verify cause
const iface = createClientInterface({ apiUrls: urlList(1) }); - await expect(sendRequest(iface)).rejects.toMatchObject({ - name: "Error", - message: expect.stringContaining("402 Payments are not set up"), - }); + try { + await sendRequest(iface); + throw new Error("Expected request to fail"); + } catch (error) { + expect(error).toMatchObject({ + name: "Error", + message: expect.stringContaining("402 Payments are not set up"), + }); + expect(error).toHaveProperty("cause"); + expect((error as Error).cause).toBeInstanceOf(Response); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack-shared/src/interface/client-interface.test.ts` around lines 458 - 466, The test should also assert that the thrown Error includes the original Response as its cause: create the Response instance (e.g., const resp = createTextResponse("Payments are not set up", { status: 402 })), stub fetch to return that resp, then update the assertion to verify the rejection includes a cause property equal to resp (or matching its status/text) when calling sendRequest(iface); reference sendRequest, createClientInterface, fetch stub, and createTextResponse to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack-shared/src/interface/client-interface.ts`:
- Around line 221-223: Update the comment describing retry behavior to exclude
429: change the sentence that currently states "KnownErrors and 4xx API
responses are never retried (they're application-level, not network-level)" to
explicitly say "KnownErrors and 4xx API responses (except 429) are never
retried" so it matches the implementation in sendClientRequestInner() which
treats 429 specially and honors Retry-After. Ensure the updated wording appears
where the retry contract is documented (the block describing "full iteration
tries every URL once per pass for 2 passes...").
- Around line 246-253: The helper
_shouldSkipFallback/_isNonRetryableApiResponseError currently treats 4xx as
non-retryable only when the thrown Error carries a Response in error.cause, but
_fetchNewAccessTokenInner throws a plain Error on non-OK token responses so
falls through to retry logic; update _fetchNewAccessTokenInner (used by
fetchNewAccessToken and _withFallback) to throw an Error that includes the HTTP
response as the cause (e.g. throw new Error('token fetch failed', { cause:
response }) or include response.data as the cause) so
_isNonRetryableApiResponseError can detect 4xx token responses, or alternatively
limit the scope by moving/renaming _isNonRetryableApiResponseError and its usage
so it only applies to sendClientRequestInner.
---
Nitpick comments:
In `@packages/stack-shared/src/interface/client-interface.test.ts`:
- Around line 458-466: The test should also assert that the thrown Error
includes the original Response as its cause: create the Response instance (e.g.,
const resp = createTextResponse("Payments are not set up", { status: 402 })),
stub fetch to return that resp, then update the assertion to verify the
rejection includes a cause property equal to resp (or matching its status/text)
when calling sendRequest(iface); reference sendRequest, createClientInterface,
fetch stub, and createTextResponse to make the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6096d64c-1def-4256-b0d9-a66b91b028b0
📒 Files selected for processing (2)
packages/stack-shared/src/interface/client-interface.test.tspackages/stack-shared/src/interface/client-interface.ts
|
Post-merge note: |
1 participant
Prevents ordinary non-KnownError 4xx API responses from retrying or failing over, and wraps them as normal Errors instead of HexclaveAssertionError.
Tests:
pnpm -C /home/ubuntu/repos/stack-auth test run packages/stack-shared/src/interface/client-interface.test.tspnpm -C /home/ubuntu/repos/stack-auth --filter @stackframe/stack-shared typecheckpnpm -C /home/ubuntu/repos/stack-auth --filter @stackframe/stack-shared lintLink to Devin session: https://app.devin.ai/sessions/ad50447fe1de425888f6d54a66376297
Requested by: @N2D4
Summary by cubic
Stop retry/failover for non-KnownError 4xx API responses (including refresh-token requests) and surface them as normal Errors. 5xx still fall back to the next URL (no retry for single-URL clients) and keep the 508 INFINITE_LOOP_DETECTED special-case.
Result.error(HexclaveAssertionError)to enable cross-URL fallback; no retry on single-URL clients.error.causeis a 4xxResponse.Written for commit c6944cb. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests