Harden the retry classifier and pin per-attempt deadlines (closes #80)
check / check (push) Successful in 27s
check / check (push) Successful in 27s
A POST or PUT is replayed only when every errno in the cause chain is a connect errno and the walk reached the end of the chain, and postJSON/putJSON no longer follow redirects, so a redirect is an ApiError that is not retried. getRetryOptions() returns a copy. New tests pin every errno the classifier names, the cause-chain depth limit, a chain deeper than the limit, a two-error cycle, and a fresh deadline per attempt for every retrying entry point. The README endpoint list is now the one place naming the requests the replay rule covers; code comments point to it. Model: opus-5-5
This commit is contained in:
+91
-3
@@ -639,6 +639,24 @@ describe("ApiClient retries", () => {
|
||||
expect(policy.baseDelayMs).toBe(7);
|
||||
expect(policy.maxDelayMs).toBe(11);
|
||||
});
|
||||
|
||||
it("does not let a caller change its settings through that policy", async () => {
|
||||
const { fetch, calls } = scriptedFetch(
|
||||
textResponse("boom", 500),
|
||||
textResponse("boom", 500),
|
||||
textResponse("boom", 500),
|
||||
);
|
||||
const client = new ApiClient({
|
||||
fetch,
|
||||
retry: { ...noWait, attempts: 2 },
|
||||
});
|
||||
|
||||
client.getRetryOptions().attempts = 3;
|
||||
|
||||
expect(client.getRetryOptions().attempts).toBe(2);
|
||||
await expect(client.getJSON("/x")).rejects.toBeInstanceOf(ApiError);
|
||||
expect(calls).toHaveLength(2);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ApiClient timeouts", () => {
|
||||
@@ -693,6 +711,51 @@ describe("ApiClient timeouts", () => {
|
||||
expect(new Set(signals).size).toBe(3);
|
||||
}, 5000);
|
||||
|
||||
it("gives every retrying entry point a fresh deadline per attempt", async () => {
|
||||
// A refused connection is retried by every entry point, the
|
||||
// non-idempotent ones included. If the deadline were created once,
|
||||
// outside the retry, both attempts would carry the same signal.
|
||||
const entryPoints: [
|
||||
string,
|
||||
() => Response,
|
||||
(c: ApiClient) => unknown,
|
||||
][] = [
|
||||
["getJSON", () => jsonResponse({}), (c) => c.getJSON("/a")],
|
||||
["postJSON", () => jsonResponse({}), (c) => c.postJSON("/b", {})],
|
||||
["putJSON", () => jsonResponse({}), (c) => c.putJSON("/c", {})],
|
||||
[
|
||||
"putFile",
|
||||
() => new Response(null, { status: 200 }),
|
||||
(c) => c.putFile("https://s3.example/x", new Uint8Array([1])),
|
||||
],
|
||||
[
|
||||
"getFileStream",
|
||||
() => streamResponse(new Uint8Array([1])),
|
||||
(c) => c.getFileStream(1),
|
||||
],
|
||||
[
|
||||
"getThumbnailStream",
|
||||
() => streamResponse(new Uint8Array([1])),
|
||||
(c) => c.getThumbnailStream(1),
|
||||
],
|
||||
];
|
||||
for (const [name, success, call] of entryPoints) {
|
||||
const { fetch, calls } = scriptedFetch(
|
||||
errnoError("ECONNREFUSED", "connect ECONNREFUSED"),
|
||||
success(),
|
||||
);
|
||||
const client = new ApiClient({ fetch, retry: noWait });
|
||||
|
||||
await call(client);
|
||||
|
||||
expect(calls, name).toHaveLength(2);
|
||||
const [first, second] = calls.map((c) => c.init?.signal);
|
||||
expect(first, name).toBeInstanceOf(AbortSignal);
|
||||
expect(second, name).toBeInstanceOf(AbortSignal);
|
||||
expect(second, name).not.toBe(first);
|
||||
}
|
||||
});
|
||||
|
||||
it("recovers when a later attempt answers in time", async () => {
|
||||
const { fetch, calls } = scriptedFetch(HANG, jsonResponse({ ok: 1 }));
|
||||
const client = new ApiClient({
|
||||
@@ -820,9 +883,8 @@ describe("ApiClient error typing", () => {
|
||||
|
||||
describe("ApiClient non-idempotent requests", () => {
|
||||
/**
|
||||
* `postJSON` and `putJSON` carry quak's only requests that change server
|
||||
* state: `/users/srp/create-session`, `/users/two-factor/verify` — which
|
||||
* consumes one of a small number of 2FA attempts — and `/files/thumbnail`.
|
||||
* `postJSON` and `putJSON` carry quak's requests that can change server
|
||||
* state; the README lists them under "Endpoints used".
|
||||
*
|
||||
* They are retried only on a failure that establishes no TCP connection to
|
||||
* the server ever existed — DNS produced no address, or the peer refused
|
||||
@@ -922,4 +984,30 @@ describe("ApiClient non-idempotent requests", () => {
|
||||
await refusedClient.updateThumbnail(1, "key", "header");
|
||||
expect(refused.calls).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("does not follow or replay a redirect on POST or PUT", async () => {
|
||||
// The origin has already received a request it answers with a
|
||||
// redirect, so following it would let a refused connection to the
|
||||
// redirect target pass for a request that never went out.
|
||||
for (const send of [
|
||||
(c: ApiClient) => c.postJSON("/users/ott", {}),
|
||||
(c: ApiClient) => c.putJSON("/files/thumbnail", {}),
|
||||
]) {
|
||||
const { fetch, calls } = scriptedFetch(
|
||||
new Response(null, {
|
||||
status: 307,
|
||||
headers: { location: "https://elsewhere.example/" },
|
||||
}),
|
||||
jsonResponse({}),
|
||||
);
|
||||
const client = new ApiClient({ fetch, retry: noWait });
|
||||
|
||||
const err: unknown = await send(client).catch((e: unknown) => e);
|
||||
|
||||
expect(calls[0]?.init?.redirect).toBe("manual");
|
||||
expect(err).toBeInstanceOf(ApiError);
|
||||
expect((err as ApiError).status).toBe(307);
|
||||
expect(calls).toHaveLength(1);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user