The handshake reaper stops firing behind the tests' backs - #6
Merged
Conversation
`test/hub.test.ts` failed twice in CI this week, in two different tests,
and neither reproduced locally. One root cause.
vitest.config.ts bound HANDSHAKE_TIMEOUT_MS at 50 ms so the reap tests
could run in real time. That made the reaper ambient for the whole file.
Almost every test dials a client, most have no reason to send at once,
and a loaded runner stretches dial-to-first-byte past 50 ms — at which
point alarm() does exactly what it should to a client that looks idle:
close 4001, and tell the daemon `closed{channel}`.
Both failures are that, seen from different angles. `daemon disconnect
closes every client 1012` got 4001, the reaper's code arriving before
the teardown's. The oversized-frame test asked for the frame on channel
2 and got channel 0, because `Leg` queues control and data together and
the reaper's `closed` control had landed in front of it.
So the default is inverted. The config now binds ten minutes — past any
deadline vitest lets a test reach — and the two tests that are *about*
reaping bind 50 ms for themselves through a new `handshakeDeadline()`
helper, before they dial. It has to be before: the deadline is read once
to arm the alarm at accept and once in alarm() to judge who is overdue,
and setting it later arms minutes out and then waits for a reap that
never comes.
The channel-cap test loses its own copy of this workaround, which it had
carried since it hit the same wall first. Its sleep stays: it is now the
assertion that the default really does outlast a test.
Verified by reproducing rather than by re-running. Binding the deadline
at 1 ms fails four tests including both CI signatures verbatim; at ten
minutes the suite is 90/90 five runs over. Neutering the new helper to a
no-op fails the two reap tests and only those, which is what says the
other nineteen no longer depend on the reaper's timing at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failures
relay/test/hub.test.tswent red twice this week, in two different tests, and neither reproduced locally.8b7a07cc—daemon disconnect closes every client 1012 "daemon gone":expected { code: 4001, … } to deeply equal { code: 1012, reason: 'daemon gone' }closes only the client that sent an oversized frame:expect(f.channel).toBe(2)got0Different tests, different assertions. One cause.
Cause
vitest.config.tsboundHANDSHAKE_TIMEOUT_MS: 50so the reap tests could run in real time. That made the reaper ambient for the entire file. Nearly every test dials a client; most have no reason to send immediately; a loaded runner stretches dial-to-first-byte past 50 ms.alarm()(src/hub.ts:297) then does precisely what it should to a client that has never spoken —ws.close(4001, 'handshake timeout')andretireClient(ws, true), which sends the daemonclosed{channel}.Both failures are that, seen from two angles:
hub.ts:308verbatim.Legbuffers control and data frames in one queue, so the reaper'sclosed{channel:2}control (channel 0) sat in front of the data frame the test was waiting for andnextFrame()handed it back.Fix
Invert the default. The config binds ten minutes — past any deadline vitest will let a test reach — and the two tests that are about reaping bind 50 ms for themselves via a new
handshakeDeadline()harness helper.It must be called before anything dials: the deadline is read twice, once to arm the alarm as each client is accepted (
hub.ts:102) and once insidealarm()to decide who is overdue. Setting it after the dial arms minutes out and then waits on a reap that never lands. The helper's doc comment says so.The channel-cap test drops its own inline copy of this workaround — it hit this wall first and had been carrying a bespoke
runInDurableObjectpoke to 600_000 ever since. Itssleepstays, now as the assertion that the default really does outlast a test.A test that says nothing about the deadline is no longer making a silent bet on how fast the runner is.
Verification — reproduced, not re-run
4001vs1012, and the oversized-frame test). That is the diagnosis confirmed rather than inferred.handshakeDeadline()to a no-op fails exactly the two reap tests and nothing else. That is what establishes both halves: the opt-in is load-bearing where it is used, and the other 19 tests in the file no longer depend on the reaper's timing at all.make lintandmake testclean: Go allok, web 601/601, relay 90/90.No production code changed —
src/is untouched.🤖 Generated with Claude Code