[nissa-seru]

test/silent-heartbeat-rows.test.ts's "a reply retried after a failure past its first round keeps following the prompt" fails now and then. The provider failure it injects stated a 1 ms retry-after. When the framework's two reads of the clock fall in the same millisecond, it holds the retry for that wait instead of leaving it to the error policy, and the test then times out waiting for the retry's call, since nothing in it runs a retry once a hold releases it. The failure now states no wait, so the test always takes the error-policy path it describes. Test-only.

Problem

@Anarchid's review of #762 reported this test failing in its full-suite run of that PR merged into main, on Node 22.22.1, and in 1 of 10 runs of the test alone on main.

By reading main at 0a4a2c69:

• The failure: transientFailure() was a retryable 500 with retryAfterMs: 1 (test/silent-heartbeat-rows.test.ts:117-120).
• A stated wait is recorded: providerWaits.set reads the clock once and records until as that reading plus 1 ms (src/provider-waits.ts:296, :70).
• The hold check reads the clock again: holdProviderAcceleration passes the wait to holdForProviderWait (src/framework.ts:5093, :5107), which holds the turn's retry unless until <= Date.now() (:5327). So the retry is held when the second read is no later than the first: when both fall in the same millisecond, or when the clock is set back between them.
• A released hold goes to pendingRequests (:5250). A started framework's loop runs it. This test drives the framework with runUntilIdle() instead, and nothing runs it between those calls.

By run at 0a4a2c69, on Linux with Node 24.22.1 and 12 CPUs. Some runs used a copy of the test that logs each call, each trace event and the framework's state.

• Unforced, over 300 runs passed. They ran alone, in the whole file, on Node 22.22.1 as well, and with the test pinned to one CPU beside up to 20 busy loops. In the 70 runs whose timings were logged, the error policy retried 1.0 to 1.8 s after the failure, inside the first runUntilIdle(). On this host the two clock reads were 7 to 325 ms apart (median 38 ms, 40 runs), so the 1 ms wait never held the retry.
• With the hold check's clock read set to the millisecond the wait was recorded in, in the logging copy, the test failed 30 of 30 runs. In each, the first runUntilIdle() returned after 3 calls, with nothing queued, pending or streaming and the agent idle. Then the hold's 1 ms timer released the retry into pendingRequests, and waitFor timed out after 15 s with "timed out waiting for the retry's call".
• With the wait's clock 500 ms ahead, it failed 6 of 6 the same way.

Changes

• transientFailure() states no retryAfterMs. With no stated wait, recordProviderWait records nothing (src/framework.ts:5292) and nothing holds the retry. The default error policy retries after its base delay of 1 s. Every logged unforced run took that path, so the test's assertions are unchanged. transientFailure() has no other user.
• Its comment is removed. It said the default error policy retries "at once", and the policy waits 1 s first.

Tests

At 1a5596b8's tree, on Linux with Node 24.22.1 and 12 CPUs:

• The typecheck and build: clean.
• The same forcing that failed 36 of 36 at main: 30 of 30 and 6 of 6 pass. No wait is recorded, and the logs show no hold check at all.
• The test alone: 60 of 60 runs pass.
• Its file: 20 of 20 runs of the whole file pass.
• The full suite: 7,572 tests, 7,528 pass, 0 fail, 0 cancelled, 44 skipped.

Not verified

• The reported failures themselves. I haven't seen their output, and every unforced run here took the error-policy path. That they're this failure is my inference from the forced runs, which fail at the step and with the message described above.
• Node 22 at the fix. Only some unforced runs at main used Node 22.22.1. The forced runs and the runs at the fix used Node 24.22.1.

What each part is for, and what we cut

• Dropping retryAfterMs: the test is about the error policy's retry ("the error policy retries"), and the stated wait is what let a run hold the retry instead.
• Cut: letting the test pass on either path. Which path a run took would depend on the clock, so the test would check different things on different hosts.
• Cut: making the hold certain and driving its release. That would change the test's subject to a held retry, which other tests cover. In this file, "a reply resumed after a provider cooldown follows the message that arrived during it, not [Continue]" holds one on a 429 acceleration cooldown and drives the release itself. In test/held-turn-retry.test.ts, "a resident whose turn a stated wait holds for its retry is mid-turn: … the hold's end runs the retry" holds one on a stated wait under a controlled clock.

---

• No changelog fragment: the PR changes only tests (no-changelog).

Written by Kestrel-2409 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) chose its shape, and Effie-2289 (Claude Opus 5.5) read it.

Description checked and posted by Fiona-2757 (Claude Opus 5.5).

🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared nissa-seru account.

[github-actions]

Automated review

Recommendation: merge. The review found nothing of medium or high severity.

Reviewed commit 1a5596b with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits the project. This change fits. It is a one-line, test-only fix to a flaky existing test so that it always takes the error-policy retry path the test is about, and it follows the project's PR conventions.

4 reasons with sources

• It only fixes a test of behavior the framework already has, and makes no product change. The diff drops retryAfterMs: 1 from the test helper transientFailure() and removes that helper's comment. Nothing under src/ changes. (test/silent-heartbeat-rows.test.ts)
• The PR body has the Problem / Changes / Tests / Not verified shape and gives numeric suite results, as the guide asks. It reports 7,528 pass, 0 fail and 44 skipped, and lists what it did not verify, including the reported failures themselves and Node 22 at the fix. (CONTRIBUTING.md, What a PR should contain)
• Skipping a changelog fragment is correct for a test-only change. The changelog CI check applies only to changes that touch src/. This PR touches only test/ and declares no-changelog. (CONTRIBUTING.md, Changelog)
• It is one coherent change and carries no unrelated work. (test/silent-heartbeat-rows.test.ts)

Findings

No findings of medium or high severity.

1 more finding, less severe

• Low severity, documentation, test/silent-heartbeat-rows.test.ts:116-119. transientFailure() lost its comment, so nothing says why it must not state a retry-after. The PR deletes the inaccurate doc comment on transientFailure() and adds nothing in its place. The neighbouring helper, accelerationLimit() at line 109, keeps its comment saying what it is for. Leaving out retryAfterMs is now what keeps this test away from the provider-wait hold, which runUntilIdle() never drains, and no comment says so. Someone who later adds a realistic retryAfterMs back to the error would bring the intermittent timeout back without warning. A one-line comment would cover it, for example: no stated wait, so the default error policy alone schedules the retry after its base delay and no provider wait holds it.

For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.