`HttpClient.retryTransient` has inconsistent behavior

[lewxdev]

What version of Effect is running?

effect@3.19.14 @effect/platform@0.94.1

What steps can reproduce the bug?

see:

• https://github.com/lewxdev/effect-repro-1
• https://github.com/lewxdev/effect-repro-1/actions/workflows/test.yaml

import { HttpClient, HttpClientResponse } from "@effect/platform";
import { describe, expect, it } from "@effect/vitest";
import { Effect, Fiber, Layer, Ref, Schedule, TestClock } from "effect";

const DURATION = 1000;
const RECUR = 3;

const httpClient = HttpClient.make((request, url) =>
  Effect.sync(() => {
    const status = Number(url.pathname.slice(1));
    const response = new Response(null, { status });
    return HttpClientResponse.fromWeb(request, response);
  })
);

const retryTransient = Layer.succeed(
  HttpClient.HttpClient,
  httpClient.pipe(
    HttpClient.retryTransient({
      schedule: Schedule.spaced(DURATION),
      times: RECUR,
    }),
  ),
);

const retryTransientResponseOnly = Layer.succeed(
  HttpClient.HttpClient,
  httpClient.pipe(
    HttpClient.retryTransient({
      mode: "response-only",
      schedule: Schedule.spaced(DURATION),
      times: RECUR,
      while: (response) => response.status === 408,
    }),
  ),
);

const testResponse = ({ status, isTransient }: { status: number; isTransient: boolean }) =>
  Effect.gen(function*() {
    const attemptsExpected = isTransient ? RECUR + 1 : 1;
    const attemptsRef = yield* Ref.make(0);
    const client = yield* Effect.map(
      HttpClient.HttpClient,
      HttpClient.tapRequest(() => Ref.update(attemptsRef, (n) => n + 1)),
    );

    const fiber = yield* Effect.fork(client.get(`http://_/${status}`));
    yield* TestClock.adjust(DURATION * RECUR);

    const response = yield* Fiber.join(fiber);
    expect(response.status).toBe(status);
    const attempts = yield* Ref.get(attemptsRef);
    expect(attempts).toBe(attemptsExpected);
  });

describe("HttpClient.HttpClient", () => {
  const responses = [
    { status: 200, isTransient: false },
    { status: 408, isTransient: true },
    { status: 500, isTransient: true },
    { status: 501, isTransient: false },
  ];
  const tests = [
    { name: "retryTransient", layer: retryTransient },
    { name: "retryTransient mode: 'response-only'", layer: retryTransientResponseOnly },
  ];

  for (const test of tests) {
    describe(test.name, () => {
      it.layer(test.layer)(({ effect }) => {
        for (const opts of responses) {
          effect(`GET /${opts.status}`, () => testResponse(opts));
        }
      });
    });
  }
});

What is the expected behavior?

Based on documentation, naming, and common HTTP semantics:

• HTTP 408 (Request Timeout) should be considered transient.
• HTTP 500 (Internal Server Error) should be considered transient.
• HTTP 501 (Not Implemented) should NOT be considered transient.

HttpClient.retryTransient should ALWAYS retry transient responses, and the while Predicate should add conditions to the schedule, not suppress expected ones (as the current JSDoc implies).

What do you see instead?

• HTTP 408 is not retried, even when "response-only" mode is used and a while Predicate explicitly filters for responses with the 408 status.
• HTTP 500 is not retried in some configurations unless explicitly included in while.
• HTTP 501 is retried, even though it should not be considered transient.

The behavior differs significantly depending on whether while is provided, even for status codes not mentioned in the predicate.

Additional information

No response

[codewithkenzo]

Hi! I'd like to work on this.

Looking at packages/platform/src/internal/httpClient.ts line 619:

const isTransientResponse = (response) => response.status >= 429

This causes:

• 408 (Request Timeout) NOT retried — should be transient
• 501 (Not Implemented) IS retried — should NOT be transient (permanent)

The fix is to use an explicit allowlist matching industry standards (axios-retry, got, ky):

const isTransientResponse = (response) => {
  const status = response.status
  return status === 408 || status === 429 || 
         status === 500 || status === 502 || 
         status === 503 || status === 504
}

This follows the precedent from PR #4014 which fixed similar retry logic for TimeoutException.

I also noticed the while predicate uses AND for responses but OR for errors — should I address that inconsistency too, or just focus on the status codes?

Happy to submit a PR!