Fallback host selection gaps: request timeout and CloudFront header (RSC15l, RSC15l4)

[paddybyers]

Spec points

• RSC15l: When a request times out after the connection is established (request-level timeout), the client should retry on a fallback host, just as it does for connection-level timeouts.
• RSC15l4: When a response includes a `Server: CloudFront` header with status >= 400, the client should treat it as a server error and retry on a fallback host.

Observed behaviour

RSC15l — Request timeout

Request-level timeouts propagate as errors without triggering fallback retry. Only connection-level errors (refused, DNS, timeout before connection) and HTTP 500–504 trigger fallback. The `shouldFallback` method does not consider request-level timeouts.

RSC15l4 — CloudFront header

`shouldFallback` in `http.ts` only checks for specific errno codes and HTTP 500–504 status codes. It does not inspect the `Server` response header. CloudFront errors with 4xx status codes are treated as non-retryable client errors.

Failing tests (2)

Test	Expected	Actual
`RSC15l - request timeout triggers fallback`	Retry on fallback host after request timeout	Error thrown, no retry
`RSC15l4 - CloudFront Server header triggers fallback`	Retry on fallback host on CloudFront 403	Error thrown, no retry

Reproduction

```bash
RUN_DEVIATIONS=1 npx mocha --grep "RSC15l" test/uts/rest/fallback.test.ts
```

From PR #2191, branch `uts-rest`.

┆Issue is synchronized with this Jira Task by Unito

[SimonWoolf]

Request-level timeouts propagate as errors without triggering fallback retry. Only connection-level errors (refused, DNS, timeout before connection) and HTTP 500–504 trigger fallback. The shouldFallback method does not consider request-level timeouts.

I don't follow this complaint. the web shouldFallback checks http 408, which is what XhrRequest exposes timeouts as; the node shouldFallback checks ETIMEDOUT; in both cases shouldFallback returns true

[paddybyers]

RSC15l — Request timeout: Simon is correct, mock has been fixed

Simon's comment is correct. ably-js does handle request timeouts in shouldFallback:

• Node (src/platform/nodejs/lib/util/http.ts:135): checks code === 'ETIMEDOUT'
• Web (src/platform/web/lib/http/http.ts:185): checks statusCode === 408

The test failure was caused by an incorrect mock. The mock's PendingRequest.respond_with_timeout() produced { code: 408, statusCode: 408 } (where code was a number), but the mock's own shouldFallback only checked for errno strings like 'ETIMEDOUT'. So 408 (number) ≠ 'ETIMEDOUT' (string), and 408 is not in 500–504 — shouldFallback returned false.

Fix applied (ably-js@6dc5eaf3):

• respond_with_timeout() now sets code: 'ETIMEDOUT' (matching Node error shape)
• Mock shouldFallback now also checks statusCode === 408 (matching web behaviour)
• RSC15l deviation removed — unit test now passes
• Added proxy integration test (test/uts/rest/integration/proxy/rest_fallback.test.ts) that confirms the real HTTP client retries on timeout through the proxy — also passes

[paddybyers]

RSC15l4 — CloudFront header handling

I think this is still an issue. shouldFallback only receives the error object (via RequestResultError), not the response headers, so the Server header is never inspected.

Suggested fix: Rather than widening the shouldFallback interface to accept headers (which would put platform-specific header inspection into non-portable code), the fix should go in common code. The intent of RSC15l4 is that a CloudFront error page with a 4xx status is actually reporting a server-side issue — the CDN is masking a 5xx. So:

1. In Http._handler (or equivalent per-platform response handler), before constructing the error: check if the response has Server: CloudFront (case-insensitive) and statusCode >= 400.
2. If so, modify the status code to 503 on the returned RequestResult.
3. The existing shouldFallback logic then naturally picks it up as a 5xx and triggers fallback.

A proxy integration test has been added (RSC15l4 - CloudFront Server header triggers fallback in test/uts/rest/integration/proxy/rest_fallback.test.ts) — currently skipped as a deviation, but will pass once this is implemented.