Narrow the replay errno set to failures that prove no connection existed
All checks were successful
check / check (push) Successful in 4s
All checks were successful
check / check (push) Successful in 4s
`CONNECT_CODES` drives `isSafeToReplay`, which is the only thing standing between a transport failure and a replayed `POST /users/two-factor/verify`. It included `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` on the stated grounds that those errnos can only be reported before any request byte was written. That is not true on Linux: an ICMP destination-unreachable delivered on an already-established connection sets the socket error and the next read or write returns `EHOSTUNREACH` or `ENETUNREACH`, and a local interface going down after the request was fully written surfaces as `ENETDOWN` the same way. In each case the server may already have received and acted on the request -- exactly the ambiguity the rule exists to exclude, on the paths that consume a second-factor attempt or register a thumbnail. The three are dropped from `CONNECT_CODES` and stay in `TRANSPORT_CODES`, so they remain retryable for the idempotent calls; only replay eligibility narrows. What is left -- `ENOTFOUND`, `EAI_AGAIN`, `ECONNREFUSED` -- means no TCP connection to the server ever existed, so no request byte can have been transmitted. The justification is corrected everywhere it was stated: the comment on `CONNECT_CODES`, the one on `isSafeToReplay`, the `postJSON` call site, the README's idempotency section and the `client.test.ts` docblock. All of them now describe what the narrowed set actually establishes rather than claiming a proof it did not support. The narrowing is enforced by the suite rather than asserted in a comment: the three errnos join `ECONNRESET`/`EPIPE`/`ETIMEDOUT` in the `isSafeToReplay`-returns-false test, with companion `isRetryable` assertions so a future edit cannot make them non-retryable by accident. Putting the three back into `CONNECT_CODES` turns that test red (1 failure, verified).
This commit was merged in pull request #23.
This commit is contained in:
@@ -824,10 +824,11 @@ describe("ApiClient non-idempotent requests", () => {
|
||||
* state: `/users/srp/create-session`, `/users/two-factor/verify` — which
|
||||
* consumes one of a small number of 2FA attempts — and `/files/thumbnail`.
|
||||
*
|
||||
* They are retried only when the failure proves the request never reached
|
||||
* the server, which in practice means the connection was never
|
||||
* established. Everything else is ambiguous: a 5xx proves the server did
|
||||
* process the request, and a reset or a timeout can arrive after it did.
|
||||
* 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
|
||||
* the connection — so no request byte can have been transmitted.
|
||||
* Everything else is ambiguous: a 5xx proves the server did process the
|
||||
* request, and a reset or a timeout can arrive after it did.
|
||||
* Replaying under that ambiguity can burn a 2FA attempt or register a
|
||||
* thumbnail twice, and neither is worth the round trip it saves.
|
||||
*/
|
||||
|
||||
@@ -282,18 +282,13 @@ describe("isSafeToReplay", () => {
|
||||
* 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 failures that prove no request
|
||||
* byte ever reached the server — which means the connection was never
|
||||
* established.
|
||||
* 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.
|
||||
*/
|
||||
it("replays only failures where the connection was never established", () => {
|
||||
for (const code of [
|
||||
"ENOTFOUND",
|
||||
"EAI_AGAIN",
|
||||
"ECONNREFUSED",
|
||||
"EHOSTUNREACH",
|
||||
"ENETUNREACH",
|
||||
]) {
|
||||
for (const code of ["ENOTFOUND", "EAI_AGAIN", "ECONNREFUSED"]) {
|
||||
expect(isSafeToReplay(errnoError(code))).toBe(true);
|
||||
}
|
||||
// Also when undici has buried it, which is how it actually arrives.
|
||||
@@ -317,6 +312,22 @@ describe("isSafeToReplay", () => {
|
||||
expect(isSafeToReplay(errnoError("ECONNRESET"))).toBe(false);
|
||||
expect(isSafeToReplay(errnoError("EPIPE"))).toBe(false);
|
||||
expect(isSafeToReplay(errnoError("ETIMEDOUT"))).toBe(false);
|
||||
// The routing errnos look like connect-time failures but are not. On
|
||||
// Linux an ICMP destination-unreachable delivered on an established
|
||||
// connection sets the socket error, and the next read or write returns
|
||||
// `EHOSTUNREACH` or `ENETUNREACH`; a local interface going down after
|
||||
// the request was fully written surfaces as `ENETDOWN` the same way.
|
||||
// In each case the server may already have consumed the request — a
|
||||
// replayed `/users/two-factor/verify` would burn a second attempt.
|
||||
// They remain retryable for the idempotent calls; this asserts only
|
||||
// that they are not replayable.
|
||||
expect(isSafeToReplay(errnoError("EHOSTUNREACH"))).toBe(false);
|
||||
expect(isSafeToReplay(errnoError("ENETUNREACH"))).toBe(false);
|
||||
expect(isSafeToReplay(errnoError("ENETDOWN"))).toBe(false);
|
||||
// ...and that the narrowing did not make them non-retryable.
|
||||
expect(isRetryable(errnoError("EHOSTUNREACH"))).toBe(true);
|
||||
expect(isRetryable(errnoError("ENETUNREACH"))).toBe(true);
|
||||
expect(isRetryable(errnoError("ENETDOWN"))).toBe(true);
|
||||
expect(
|
||||
isSafeToReplay(new DOMException("timed out", "TimeoutError")),
|
||||
).toBe(false);
|
||||
|
||||
Reference in New Issue
Block a user