Harden the retry classifier and pin per-attempt deadlines #83

Merged
clawbot merged 1 commits from issue-80-retry-hardening into next2 2026-09-23 02:38:02 +02:00
Collaborator

Implements #80 (items 3 to 9 of #24).

Changes

  • isSafeToReplay now replays a POST or PUT only when every errno in the cause chain is a connect errno (ENOTFOUND, EAI_AGAIN, ECONNREFUSED). Before, one connect errno anywhere in the chain was enough, even next to a reset.
  • postJSON and putJSON send redirect: "manual". A redirect comes back as an ApiError with its 3xx status and is not retried.
  • getRetryOptions() returns a copy.
  • The README's "Endpoints used" list is now the one place that names the requests the replay rule covers. It gains the missing POST /files/data/fetch. The idempotency section and the code comments point to it.
  • New tests pin every errno in both sets, the cause-chain depth limit of eight links, a cycle through two errors, and a fresh deadline on every attempt for all six retrying entry points.

Things to know

  • The redirect, copy and replay-rule tests each fail when their fix is reverted. The errno, depth, cycle and deadline tests pin behaviour that was already correct, so there was no fix to revert for them.
  • The new deadline test checks that each attempt gets its own signal, so it does not depend on the clock. The older getJSON timeout tests still use a real 20 ms deadline. I left them alone.
  • In a browser, redirect: "manual" gives a response with status 0. It still becomes an ApiError that is not retried.

make check green.

Model: opus-5-5

Implements https://git.eeqj.de/sneak/quak/issues/80 (items 3 to 9 of https://git.eeqj.de/sneak/quak/issues/24). ## Changes - `isSafeToReplay` now replays a `POST` or `PUT` only when every errno in the `cause` chain is a connect errno (`ENOTFOUND`, `EAI_AGAIN`, `ECONNREFUSED`). Before, one connect errno anywhere in the chain was enough, even next to a reset. - `postJSON` and `putJSON` send `redirect: "manual"`. A redirect comes back as an `ApiError` with its 3xx status and is not retried. - `getRetryOptions()` returns a copy. - The README's "Endpoints used" list is now the one place that names the requests the replay rule covers. It gains the missing `POST /files/data/fetch`. The idempotency section and the code comments point to it. - New tests pin every errno in both sets, the cause-chain depth limit of eight links, a cycle through two errors, and a fresh deadline on every attempt for all six retrying entry points. ## Things to know - The redirect, copy and replay-rule tests each fail when their fix is reverted. The errno, depth, cycle and deadline tests pin behaviour that was already correct, so there was no fix to revert for them. - The new deadline test checks that each attempt gets its own signal, so it does not depend on the clock. The older `getJSON` timeout tests still use a real 20 ms deadline. I left them alone. - In a browser, `redirect: "manual"` gives a response with status 0. It still becomes an `ApiError` that is not retried. `make check` green. Model: opus-5-5
clawbot added the needs-review label 2026-09-23 01:59:32 +02:00
clawbot self-assigned this 2026-09-23 01:59:32 +02:00
Author
Collaborator

FAIL on f788c0a against next2 b7d6ab9.

  1. Conflicts with next2. TODO.md "Completed Steps": the issue 9 and issue 10 entries landed at the top of that list since this branch was cut, so the rebase stops there. Acceptable: rebased onto current next2, with all three entries kept.

  2. isSafeToReplay still replays in doubt when the chain is longer than the walk. src/retry.ts:168-175: causeCodes stops after eight links, and every() judges only the codes it collected. A chain whose first eight links carry only ECONNREFUSED and whose ninth carries ECONNRESET is replayed. That breaks the rule #80 item 7 asks for ("a chain containing any error that shows the request went out is not replayable"): a walk that did not reach the end of the chain has not ruled that out. Acceptable: when the walk stops at the limit before the chain ends, isSafeToReplay returns false, and a test pins it: a chain deeper than the limit with a reset below link eight is not replayable.

Judgement call: I read "no test depends on wall-clock time" as covering the new tests only. The existing 20 ms getJSON deadline tests the PR body mentions are not counted as a finding.

Model: opus-5-5

FAIL on `f788c0a` against `next2` `b7d6ab9`. 1. **Conflicts with `next2`.** `TODO.md` "Completed Steps": the issue 9 and issue 10 entries landed at the top of that list since this branch was cut, so the rebase stops there. Acceptable: rebased onto current `next2`, with all three entries kept. 2. **`isSafeToReplay` still replays in doubt when the chain is longer than the walk.** `src/retry.ts:168-175`: `causeCodes` stops after eight links, and `every()` judges only the codes it collected. A chain whose first eight links carry only `ECONNREFUSED` and whose ninth carries `ECONNRESET` is replayed. That breaks the rule https://git.eeqj.de/sneak/quak/issues/80 item 7 asks for ("a chain containing any error that shows the request went out is not replayable"): a walk that did not reach the end of the chain has not ruled that out. Acceptable: when the walk stops at the limit before the chain ends, `isSafeToReplay` returns false, and a test pins it: a chain deeper than the limit with a reset below link eight is not replayable. Judgement call: I read "no test depends on wall-clock time" as covering the new tests only. The existing 20 ms `getJSON` deadline tests the PR body mentions are not counted as a finding. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-09-23 02:09:48 +02:00
clawbot force-pushed issue-80-retry-hardening from f788c0adc8 to 7dd2abc480 2026-09-23 02:19:35 +02:00 Compare
clawbot force-pushed issue-80-retry-hardening from 7dd2abc480 to 80d45ebc4e 2026-09-23 02:21:08 +02:00 Compare
Author
Collaborator

Rework, now 80d45eb on next2 b44c4ba:

  1. Rebased onto current next2. TODO.md keeps every entry, newest first, with this one on top.
  2. isSafeToReplay returns false when the cause walk stops at the depth limit with more chain below it. New test: eight ECONNREFUSED links with ECONNRESET below them is not replayable, and the same eight links alone still are. It fails with the fix reverted.

Model: opus-5-5

Rework, now `80d45eb` on `next2` `b44c4ba`: 1. Rebased onto current `next2`. `TODO.md` keeps every entry, newest first, with this one on top. 2. `isSafeToReplay` returns false when the cause walk stops at the depth limit with more chain below it. New test: eight `ECONNREFUSED` links with `ECONNRESET` below them is not replayable, and the same eight links alone still are. It fails with the fix reverted. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-09-23 02:21:15 +02:00
Author
Collaborator

PASS on 80d45eb rebased onto next2 b44c4ba.

Model: opus-5-5

PASS on `80d45eb` rebased onto `next2` `b44c4ba`. Model: opus-5-5
clawbot added needs-rebase and removed needs-review labels 2026-09-23 02:34:20 +02:00
clawbot added 1 commit 2026-09-23 02:36:10 +02:00
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
clawbot force-pushed issue-80-retry-hardening from 80d45ebc4e to 435c84fc35 2026-09-23 02:36:10 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-09-23 02:36:14 +02:00
clawbot merged commit d545dcd8b1 into next2 2026-09-23 02:38:02 +02:00
clawbot deleted branch issue-80-retry-hardening 2026-09-23 02:38:02 +02:00
Sign in to join this conversation.