diff --git a/README.md b/README.md index 6145269..34ee16e 100644 --- a/README.md +++ b/README.md @@ -305,12 +305,18 @@ only guarded the initial request would leave the same hang one layer down. **Non-idempotent requests are not blindly replayed.** `postJSON` and `putJSON` reach `/users/srp/create-session`, `/users/two-factor/verify` — which consumes one of a small number of second-factor attempts — and `/files/thumbnail`. They -are retried only on failures that prove no request byte reached the server, -which means the connection was never established (`ECONNREFUSED`, `ENOTFOUND`, -and the like). A 5xx, a mid-flight reset and a deadline are all left to the -caller, because each of them can happen after the server has already acted. -`putFile` is exempt: a presigned PUT stores one whole object at one key in one -request, so replaying it has no partial state to damage. +are retried only on the three failures that establish no TCP connection to the +server ever existed, so no request byte can have been transmitted: `ENOTFOUND` +and `EAI_AGAIN` (name resolution produced no address) and `ECONNREFUSED` (the +peer refused the connection). A 5xx, a mid-flight reset and a deadline are all +left to the caller, because each of them can happen after the server has already +acted. The routing errnos `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` are +excluded for the same reason, despite looking like connect-time failures: on +Linux an ICMP unreachable arriving mid-flight, or a local interface going down +after the request was written, delivers them on an already-established socket. +They stay retryable for the idempotent calls. `putFile` is exempt: a presigned +PUT stores one whole object at one key in one request, so replaying it has no +partial state to damage. A download is retried as a whole — request, stream consumption, and decryption — because a socket reset after the response headers have arrived surfaces in the diff --git a/src/api/client.ts b/src/api/client.ts index 95ad995..f3b9996 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -227,11 +227,12 @@ export class ApiClient { // Idempotency: this reaches `/users/srp/create-session`, // `/users/two-factor/verify` and `/users/ott`, all of which change // server state — verifying a second factor consumes one of a small - // number of attempts. So a POST is replayed only when the failure - // proves the request never reached the server, which in practice means - // the connection was never established. A 5xx, a mid-flight reset and - // a timeout are all left to the caller, because each of them can occur - // after the server has already acted. + // number of attempts. So a POST is replayed only on a failure that + // establishes no TCP connection to the server ever existed: DNS + // produced no address, or the peer refused the connection. A 5xx, a + // mid-flight reset, a routing errno (which Linux also delivers on an + // established socket) and a timeout are all left to the caller, + // because each of them can occur after the server has already acted. return withRetry( async () => { const resp = await this._fetch(url, { diff --git a/src/retry.ts b/src/retry.ts index c425de8..4d5eb69 100644 --- a/src/retry.ts +++ b/src/retry.ts @@ -62,17 +62,22 @@ const TRANSPORT_CODES = new Set([ "ENETDOWN", ]); -// The subset of the above that can only happen before any request byte was -// written: name resolution failed, or the connection was refused or never -// routed. See `isSafeToReplay`. -const CONNECT_CODES = new Set([ - "ENOTFOUND", - "EAI_AGAIN", - "ECONNREFUSED", - "EHOSTUNREACH", - "ENETUNREACH", - "ENETDOWN", -]); +// The subset of the above that can only be reported before a TCP connection +// exists, and therefore before any request byte could have been written: name +// resolution produced no address (`ENOTFOUND`, `EAI_AGAIN`) or the peer +// refused the connection with an RST to the SYN (`ECONNREFUSED`). +// +// The routing errnos — `EHOSTUNREACH`, `ENETUNREACH`, `ENETDOWN` — are +// deliberately absent even though they look like connect-time failures. On +// Linux they are also delivered on an already-established socket: an ICMP +// destination-unreachable arriving mid-flight sets the socket error and the +// next read or write returns it, and a local interface going down after the +// request was fully written surfaces the same way. In those cases the server +// may already have received and acted on the request, which is exactly the +// ambiguity this set exists to exclude. They stay in `TRANSPORT_CODES`, so +// they remain retryable for idempotent calls; only replay eligibility is +// narrowed. See `isSafeToReplay`. +const CONNECT_CODES = new Set(["ENOTFOUND", "EAI_AGAIN", "ECONNREFUSED"]); // `cause` is an arbitrary user-settable property and nothing prevents it from // forming a cycle, so the walk is bounded. Hanging the process would be a @@ -148,13 +153,16 @@ export const isRetryable = (err: unknown): boolean => { // `isRetryable` is the wrong question for a request that changes state. // quak's non-idempotent calls are `/users/srp/create-session`, // `/users/two-factor/verify` — which consumes one of a small number of 2FA -// attempts — and `/files/thumbnail`. They are replayed only when the failure -// proves no request byte reached the server, which means the connection was -// never established. +// attempts — and `/files/thumbnail`. They are replayed only on the failures in +// `CONNECT_CODES`, which establish that no TCP connection to the server ever +// existed: there was no address to connect to, or the peer refused the +// connection outright. A request byte cannot have been transmitted, so the +// server cannot have acted. // // Everything else is ambiguous. A 5xx proves the server did process the // request. A reset or a broken pipe can arrive after it was fully sent and -// acted on. A deadline says nothing at all about the server's state. +// acted on. A routing errno can be delivered on an established socket. A +// deadline says nothing at all about the server's state. export const isSafeToReplay = (err: unknown): boolean => isRetryable(err) && causeCodes(err).some((code) => CONNECT_CODES.has(code)); diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 387da4f..2faf019 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -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. */ diff --git a/test/retry/retry.test.ts b/test/retry/retry.test.ts index 7ddd24b..7baf8c0 100644 --- a/test/retry/retry.test.ts +++ b/test/retry/retry.test.ts @@ -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);