Harden the retry classifier and pin per-attempt deadlines (closes #80)
check / check (push) Successful in 17s
check / check (push) Successful in 17s
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:
@@ -149,8 +149,11 @@ describe("isRetryable: transport failures", () => {
|
||||
});
|
||||
|
||||
it("retries an errno carried on the error itself", () => {
|
||||
// Every errno the classifier names, so none can be reclassified
|
||||
// unnoticed.
|
||||
for (const code of [
|
||||
"ECONNRESET",
|
||||
"ECONNABORTED",
|
||||
"ETIMEDOUT",
|
||||
"EPIPE",
|
||||
"ENOTFOUND",
|
||||
@@ -158,6 +161,8 @@ describe("isRetryable: transport failures", () => {
|
||||
"ECONNREFUSED",
|
||||
"EHOSTUNREACH",
|
||||
"ENETUNREACH",
|
||||
"ENETRESET",
|
||||
"ENETDOWN",
|
||||
]) {
|
||||
expect(isRetryable(errnoError(code))).toBe(true);
|
||||
}
|
||||
@@ -215,6 +220,27 @@ describe("isRetryable: transport failures", () => {
|
||||
looped.cause = looped;
|
||||
expect(isRetryable(looped)).toBe(false);
|
||||
});
|
||||
|
||||
it("terminates on a cause chain that loops through two errors", () => {
|
||||
const first: Error & { cause?: unknown } = new Error("first");
|
||||
const second = new Error("second", { cause: first });
|
||||
first.cause = second;
|
||||
expect(isRetryable(first)).toBe(false);
|
||||
});
|
||||
|
||||
it("reads the error and at most seven causes below it", () => {
|
||||
// The walk is bounded at eight links. An errno at the eighth link is
|
||||
// found; one at the ninth is not.
|
||||
const buried = (causes: number): Error => {
|
||||
let err = errnoError("ECONNRESET");
|
||||
for (let i = 0; i < causes; i++) {
|
||||
err = new Error(`wrapper ${i}`, { cause: err });
|
||||
}
|
||||
return err;
|
||||
};
|
||||
expect(isRetryable(buried(7))).toBe(true);
|
||||
expect(isRetryable(buried(8))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isRetryable: stream truncation versus corruption", () => {
|
||||
@@ -279,10 +305,9 @@ describe("isSafeToReplay", () => {
|
||||
* that is not the whole question: the other half is "could the first
|
||||
* attempt already have taken effect on the server?".
|
||||
*
|
||||
* quak's non-idempotent calls are `/users/srp/create-session`,
|
||||
* `/users/two-factor/verify` (which consumes one of a limited number of
|
||||
* 2FA attempts) and `/files/thumbnail`. A blind replay of any of them can
|
||||
* do real damage, so they retry only on the failures that establish no TCP
|
||||
* The calls this guards are the `POST` and `PUT` requests listed in the
|
||||
* README under "Endpoints used". A blind replay of some of them can do
|
||||
* real damage, so they retry only on the failures that establish no TCP
|
||||
* connection to the server ever existed — DNS produced no address, or the
|
||||
* peer refused the connection — and therefore that no request byte can
|
||||
* have been transmitted.
|
||||
@@ -333,6 +358,52 @@ describe("isSafeToReplay", () => {
|
||||
).toBe(false);
|
||||
expect(isSafeToReplay(new TypeError("fetch failed"))).toBe(false);
|
||||
});
|
||||
|
||||
it("does not replay any other errno the classifier names", () => {
|
||||
for (const code of [
|
||||
"ECONNRESET",
|
||||
"ECONNABORTED",
|
||||
"ETIMEDOUT",
|
||||
"EPIPE",
|
||||
"EHOSTUNREACH",
|
||||
"ENETUNREACH",
|
||||
"ENETRESET",
|
||||
"ENETDOWN",
|
||||
]) {
|
||||
expect(isSafeToReplay(errnoError(code))).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("does not replay a chain that also shows the request may have gone out", () => {
|
||||
// A connect errno somewhere in the chain is not enough: any other
|
||||
// errno beside it is doubt, and doubt is not replayed.
|
||||
const reset = Object.assign(
|
||||
new Error("read ECONNRESET", { cause: errnoError("ECONNREFUSED") }),
|
||||
{ code: "ECONNRESET" },
|
||||
);
|
||||
const mixed = new TypeError("fetch failed", { cause: reset });
|
||||
expect(isRetryable(mixed)).toBe(true);
|
||||
expect(isSafeToReplay(mixed)).toBe(false);
|
||||
});
|
||||
|
||||
it("does not replay a chain longer than the walk reads", () => {
|
||||
// Eight connect errnos, then a reset at the ninth link, below the
|
||||
// limit. The walk never sees the reset, so it cannot rule it out.
|
||||
const refusedChain = (below: Error | undefined): Error => {
|
||||
let err = below;
|
||||
for (let i = 0; i < 8; i++) {
|
||||
err = Object.assign(new Error(`refused ${i}`, { cause: err }), {
|
||||
code: "ECONNREFUSED",
|
||||
});
|
||||
}
|
||||
return err as Error;
|
||||
};
|
||||
expect(isSafeToReplay(refusedChain(errnoError("ECONNRESET")))).toBe(
|
||||
false,
|
||||
);
|
||||
// The same eight links with nothing below them are replayable.
|
||||
expect(isSafeToReplay(refusedChain(undefined))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user