[nissa-seru]

Two tests in test/provider-acceleration-cooldown.test.ts could fail now and then for the same reason as #807's: each gives a resident a 429 stating a 1 ms retry-after, and whether that wait holds the retry depends on the clock. The test about the stated wait now holds Date, so the wait always binds, and it runs the framework until the retry comes. The test about the providerHold hook now injects a 429 that states no wait, and runs both hooks its name describes. Test-only.

Problem

The two tests:

• "ordinary 429 does not enter acceleration cooldown: its stated wait passes, then one retry"
• "providerHold returning nothing, or throwing, leaves the ordinary retry path untouched"

Each fed a resident GenericRateLimitThenSuccessMembrane's 429 with retryAfterMs: 1 (:61 at 0a4a2c69), drove it only with runUntilIdle(), and asserted right after that the retry had been made (2 calls).

By reading main at 0a4a2c69:

• The wait is recorded with its end at the recording's clock reading plus 1 ms (src/provider-waits.ts:296, :70).
• The hold check reads the clock again and holds the turn's retry unless that end is at or before its own Date.now() (src/framework.ts:5325-5327). So the retry is held when the second read is no later than the first.
• A held retry's release puts it in pendingRequests (:5250). A started framework's loop runs it, but these tests drive the framework with runUntilIdle(), which had already returned.

By run on Linux with Node 24.22.1 and 12 CPUs, at 0a4a2c69 or at #807's head, which differs from it only in test/silent-heartbeat-rows.test.ts:

• Unforced, each test passed 10 of 10.
• With the hold check's Date.now() set to the millisecond the wait was recorded in (a preload on holdForProviderWait; nothing else changed), each failed 3 of 3, and 10 of 10 under an earlier form of the preload. The message was "actual: 1, expected: 2".
• A sweep of all 11 test files that state a retryAfterMs, each run whole under a forcing that holds every wait, found no other test with this race besides test(silent-heartbeat-rows): the retried reply's failure states no wait #807's. One more test failed under it, but its stated wait is negative, so only a clock set back can hold it, and it passes under the exact forcing above.

Changes

• "ordinary 429 … its stated wait passes, then one retry" is about the stated wait, so it now holds Date with t.mock.timers.enable({ apis: ['Date'] }), as test/held-turn-retry.test.ts does. The 1 ms wait then binds every time.
  • After the first runUntilIdle(), it checks that the retry hasn't been made, that providerWaitSnapshot('resident') lists the wait, and that healthSnapshot() shows the resident held until the wait's end. An acceleration cooldown lasts at least 1 s (src/framework.ts:5017-5025), so it would fail that check.
  • Then it moves the clock past the wait and runs the framework until the second call.
  • Its earlier checks stay: two calls, nothing held, no wait left.
• "providerHold returning nothing, or throwing, …" is about the hook: its check reads "ordinary policy retry happened", and its 429 predates stated waits holding a resident's retry. So its 429 now states no wait, through an option on the fixture, and the error policy retries inside runUntilIdle() every time.
• Both hooks the name describes: until now only a throwing hook was set up. Each variant now counts its hook's calls and checks it was asked once about the failure.
• waitFor runs runUntilIdle() while it waits, since a released hold's request waits in pendingRequests for a drain. The test drains once more after it before counting calls, so a second retry queued during waitFor's last sleep would still show in the count. Its 5 s limit turns a retry that never comes into a failure that names it, and reads performance.now(), which the Date mock doesn't touch.

Tests

At a8eefd6a, on Linux with Node 24.22.1 and 12 CPUs:

• The typecheck and build: clean.
• Each test alone: 60 of 60 runs pass.
• The same forcing that failed both at main: 10 of 10 runs of each pass.
• The file: 20 of 20 runs pass.
• The full suite: 7,572 tests, 7,528 pass, 0 fail, 0 cancelled, 44 skipped.
• Removals, one per new check, each run once and each failing that check:
  • the hold check never holding (src/framework.ts:5327): "the retry waits for the stated wait", actual 2, expected 1;
  • the stated wait never recorded (src/provider-waits.ts:309): "the stated wait binds while the retry waits", actual [];
  • this 429 taken for an acceleration limit (src/framework.ts:5101): "held until the stated wait ends, not for an acceleration cooldown", the hold ending about 4.5 s after the wait;
  • a hook returning nothing treated as a hold (src/framework.ts:5061): "ordinary policy retry happened, with the hook returning nothing", actual 1, expected 2;
  • the hook never asked (src/framework.ts:5060): "the hook was asked about the failure, returning nothing", actual 0, expected 1.

Not verified

• Node 22. Every run used Node 24.22.1.
• The flake unforced. Neither test failed here without forcing the clock, so the case it fixes rests on the forced runs.

What each part is for, and what we cut

• Holding Date in the first test: the test is about the stated wait being honoured, and only a held clock makes the wait bind every time. With a real clock the wait binds only when the second read is no later than the first, and otherwise the error policy's own retry makes the second call.
• No stated wait in the second test: the test is about the hook, and with no stated wait nothing else can hold the retry.
• The hook count: without it, a variant would pass even if the hook were never asked.
• waitFor's limit: without it, a retry that never came would wait for the suite's ten-minute test timeout (--test-timeout=600000), and fail without saying what didn't happen.
• Cut: dropping the stated wait from the first test, as test(silent-heartbeat-rows): the retried reply's failure states no wait #807 did. That test is about the wait.
• Cut: holding Date in the second test as well. It would then check a stated-wait hold beside the hook, which isn't what its name says.

---

• 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 Abigail-2676 (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 a8eefd6 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. The change fits. It only touches tests: two flaky tests for behavior the project already has are made deterministic and their checks are tightened, using the clock-mocking pattern the suite already uses.

4 reasons with sources

• It is a test-only fix for existing provider-wait and providerHold behavior, and nothing in src/ changes. The diff touches only this test file. It changes one fixture's constructor, adds a waitFor helper and edits two tests. The framework's behavior is unchanged. (test/provider-acceleration-cooldown.test.ts)
• Holding Date with t.mock.timers follows how other tests in the suite already pin the clock. The same call, t.mock.timers.enable({ apis: ['Date'], now: Date.now() }), already appears in held-turn-retry.test.ts:116, provider-waits.test.ts:655 and tool-result-guard.test.ts. (test/held-turn-retry.test.ts)
• It meets the guide's rules on evidence and test substance: numeric baselines, forced-failure removals and a Not verified section. The guide asks for numeric test baselines, an honest list of what wasn't verified, and tests that can fail on broken code. The description gives suite counts, the forced runs that fail on main, and one removal per new check. It also lists Node 22 and the unforced flake under Not verified. (CONTRIBUTING.md, What a PR should contain)
• It needs no changelog fragment, because it doesn't touch src/ and changes no behavior. The guide requires fragments for behavior-affecting changes and has CI check this only when src/ is touched. (CONTRIBUTING.md, Changelog)

Findings

No findings of medium or high severity.

4 more findings, less severe

• Low severity, maintainability, test/provider-acceleration-cooldown.test.ts:80-87. waitFor is copied from held-turn-retry.test.ts instead of shared, and the copy behaves differently. test/held-turn-retry.test.ts:142-149 already has a waitFor(fw, done, what, boundMs) that polls while driving the framework, with a deadline based on performance.now(). This change adds a second copy with a fixed 5 s bound, a different error message and a different way of driving the framework. The two copies will drift apart. A shared helper under test/helpers/ (next to mock-membrane.ts) would keep one implementation.
• Low severity, test coverage, test/provider-acceleration-cooldown.test.ts:82-86. The 5 s limit in waitFor never fires if runUntilIdle itself hangs. The loop awaits framework.runUntilIdle() before it checks the deadline again. If a regression makes runUntilIdle() block, for example on a long retry wait or a hold the drain waits on, the loop never gets back to the performance.now() > deadline check. The test then hangs until the suite's 600 s timeout. That is the failure mode the PR says the limit exists to prevent. The helper in held-turn-retry.test.ts avoids this by starting the drain with void drive(fw) and polling on its own. This copy should do the same.
• Low severity, test coverage, test/provider-acceleration-cooldown.test.ts:414-426. A 429 with no stated wait makes each hook variant sleep the policy's 1 s rate-limit backoff. GenericRateLimitThenSuccessMembrane(null) now raises a rate_limit error with no retryAfterMs. DefaultErrorPolicy.onInferenceError then uses membrane's base schedule for the rate_limit class. test/default-error-policy.test.ts:49 pins that schedule's first delay at 1000 ms. waitForRetry sleeps that delay with a real setTimeout inside runUntilIdle(), so the two variants add about 2 s of wall-clock time to this test, where the old 1 ms stated wait added almost none. The PR description does not mention this cost. A custom errorPolicy with a short delay would keep the test about the hook and keep it fast.
• Low severity, test coverage, test/provider-acceleration-cooldown.test.ts:406-432. The two hook variants share one test body, so a failure in the first hides the second. The 'returning nothing' and 'throwing' cases run one after the other in a for loop inside one test(...). If an assertion fails for 'returning nothing', the loop exits and the 'throwing' variant never runs, so the report shows only one of the two behaviours the test name describes. Running each variant as a subtest (t.test(how, ...)) would report both cases on their own and name them in the output.

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