Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions packages/stack-shared/src/interface/client-interface.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ function createKnownErrorResponse(error: InstanceType<typeof KnownErrors[keyof t
});
}

function createTextResponse(body: string, options: { status: number, headers?: Record<string, string> }): Response {
return new Response(body, options);
}

function getRequestBody(fetchMock: { mock: { calls: unknown[][] } }): Record<string, unknown> {
const requestInit = fetchMock.mock.calls[0]?.[1];
if (requestInit == null || typeof requestInit !== "object" || !("body" in requestInit)) {
Expand Down Expand Up @@ -437,6 +441,63 @@ describe("_withFallback", () => {
expect(log.every(u => urlIndex(urls, u) === 0)).toBe(true);
});

it("does not retry or fall back on non-KnownError 4xx responses", async () => {
const urls = urlList(3);
const log: string[] = [];
vi.stubGlobal("fetch", vi.fn(async (input: RequestInfo | URL) => {
log.push(input.toString());
return createTextResponse("Payments are not set up", { status: 402 });
}));

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);
Comment on lines +452 to +455
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0480409 by matching { name: "Error" } in the no-retry/fallback test.

});

it("wraps non-KnownError 4xx responses as normal errors", async () => {
const response = createTextResponse("Payments are not set up", { status: 402 });
vi.stubGlobal("fetch", vi.fn(async () => response));

const iface = createClientInterface({ apiUrls: urlList(1) });
await expect(sendRequest(iface)).rejects.toMatchObject({
name: "Error",
message: expect.stringContaining("402 Payments are not set up"),
cause: response,
});
});

it("does not retry non-KnownError 5xx responses on a single URL", async () => {
let attempts = 0;
vi.stubGlobal("fetch", vi.fn(async () => {
attempts++;
return createTextResponse("Server unavailable", { status: 503 });
}));

const iface = createClientInterface({ apiUrls: urlList(1) });
await expect(sendRequest(iface)).rejects.toThrow("503 Server unavailable");
expect(attempts).toBe(1);
});

it("falls back on non-KnownError 5xx responses", async () => {
const urls = urlList(3);
const log: string[] = [];
vi.stubGlobal("fetch", vi.fn(async (input: RequestInfo | URL) => {
const url = input.toString();
log.push(url);
if (urlIndex(urls, url) === 0) {
return createTextResponse("Server unavailable", { status: 503 });
}
return createJsonResponse({ display_name: "test" });
}));

const iface = createClientInterface({ apiUrls: urls });
await sendRequest(iface);
expect(log.length).toBe(2);
expect(urlIndex(urls, log[0])).toBe(0);
expect(urlIndex(urls, log[1])).toBe(1);
});

it("makes 2 passes × N URLs attempts before throwing", async () => {
for (const n of [2, 3, 5]) {
const urls = urlList(n);
Expand Down
25 changes: 19 additions & 6 deletions packages/stack-shared/src/interface/client-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,8 @@ export class HexclaveClientInterface {
* - Sticky URL fails → exit sticky mode, do a full iteration.
*
* In both modes, a full iteration tries every URL once per pass for 2
* passes before giving up. KnownErrors are never retried (they're
* application-level, not network-level).
* passes before giving up. KnownErrors and 4xx API responses (except 429)
* are never retried (they're application-level, not network-level).
*
* Single-URL lists skip all of this and use 5-retry behavior directly.
*/
Expand All @@ -243,6 +243,15 @@ export class HexclaveClientInterface {
return await this._iterateUrls(apiUrls, cb);
}

private _shouldSkipFallback(error: unknown) {
return error instanceof KnownError || this._isNonRetryableApiResponseError(error);
}

private _isNonRetryableApiResponseError(error: unknown) {
const cause = error instanceof Error ? error.cause : undefined;
return cause instanceof Response && cause.status >= 400 && cause.status < 500;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Attempts the sticky URL, optionally probing primary first.
* Returns the result on success, or `undefined` if we should fall through to full iteration.
Expand All @@ -260,7 +269,7 @@ export class HexclaveClientInterface {
this._sticky = null;
return result;
} catch (e) {
if (e instanceof KnownError) throw e;
if (this._shouldSkipFallback(e)) throw e;
sticky.probeRate = Math.max(sticky.probeRate * 0.5, 0.01);
}
}
Expand All @@ -269,7 +278,7 @@ export class HexclaveClientInterface {
try {
return await cb(apiUrls[sticky.index], { maxAttempts: 1, skipDiagnostics: true });
} catch (e) {
if (e instanceof KnownError) throw e;
if (this._shouldSkipFallback(e)) throw e;
this._sticky = null;
return undefined;
}
Expand All @@ -294,7 +303,7 @@ export class HexclaveClientInterface {
}
return result;
} catch (e) {
if (e instanceof KnownError) throw e;
if (this._shouldSkipFallback(e)) throw e;
lastError = e instanceof Error ? e : new Error(String(e));
}
}
Expand Down Expand Up @@ -457,7 +466,7 @@ export class HexclaveClientInterface {

if (!response.data.ok) {
const body = await response.data.text();
throw new Error(`Failed to send refresh token request: ${response.status} ${body}`);
throw new Error(`Failed to send refresh token request: ${response.status} ${body}`, { cause: response.data });
}

return response.data;
Expand Down Expand Up @@ -777,6 +786,10 @@ export class HexclaveClientInterface {
} else {
const error = await res.text();

// Do not retry, throw error instead of returning one
if (res.status >= 400 && res.status < 500) {
throw new Error(`Failed to send request to ${url}: ${res.status} ${error}`, { cause: res });
}
const errorObj = new HexclaveAssertionError(`Failed to send request to ${url}: ${res.status} ${error}`, { request: params, res, path });

if (res.status === 508 && error.includes("INFINITE_LOOP_DETECTED")) {
Expand Down
Loading