The handshake reaper stops firing behind the tests' backs

relay/test/harness.ts

@@ -4,7 +4,7 @@
 // Not a `.test.ts` file, so vitest does not collect it — importing one test
 // file from another would re-register its describe blocks.
 
-import { env } from 'cloudflare:test'
+import { env, runInDurableObject } from 'cloudflare:test'
 import { expect } from 'vitest'
 
 import { decodeFrame, encodeFrame } from '../src/frame'
@@ -57,6 +57,25 @@ export function freshHub(): DurableObjectStub {
   return env.HUB.get(env.HUB.idFromName(crypto.randomUUID()))
 }
 
+/**
+ * Bind one hub's handshake deadline, for the tests that are about reaping.
+ *
+ * vitest.config.ts binds a deadline no test can outlive, so the reaper never
+ * fires behind a test's back; a test that wants it to fire asks here. See that
+ * file for why the default runs that way round.
+ *
+ * Call this *before* anything dials the hub. The deadline is read twice — once
+ * to arm the alarm as each client is accepted, once inside `alarm()` to decide
+ * who is overdue — and only the second would see a later change, which leaves
+ * an alarm armed minutes out and a test waiting on a reap that never lands.
+ */
+export async function handshakeDeadline(hub: DurableObjectStub, ms: number): Promise<void> {
+  await runInDurableObject(hub, (instance) => {
+    const withEnv = instance as unknown as { env: Record<string, unknown> }
+    withEnv.env = { ...withEnv.env, HANDSHAKE_TIMEOUT_MS: ms }
+  })
+}
+
 export function sleep(ms: number): Promise<void> {
   return new Promise((resolve) => setTimeout(resolve, ms))
 }

relay/test/hub.test.ts

@@ -9,13 +9,18 @@ import {
   encoder,
   frame,
   freshHub,
+  handshakeDeadline,
   MACHINE,
   open,
   sleep,
   within,
 } from './harness'
 
-/** Mirrors the HANDSHAKE_TIMEOUT_MS binding in vitest.config.ts. */
+/**
+ * The handshake deadline the tests below bind for themselves with
+ * `handshakeDeadline()`. It is not what vitest.config.ts binds — that is ten
+ * minutes, so the reaper only ever fires where a test asked it to.
+ */
 const TIMEOUT_MS = 50
 
 afterEach(() => {
@@ -281,17 +286,12 @@ describe('the message-size cap', () => {
 describe('the channel cap', () => {
   it('refuses the 65th concurrent client: 503 relay full', async () => {
     const hub = freshHub()
-    // None of the 64 clients below ever handshakes, and on a slow runner the
-    // 50 ms deadline this config binds would reap them before the 65th dial —
-    // the cap check would then find free slots and hand out a 101. Reap-then-
-    // accept is the hub behaving correctly; this test is about the cap alone,
-    // so give this one instance a deadline the test cannot outlive. The sleep
-    // stands in for the slow runner, and keeps the reap from ever going quiet
-    // here by accident.
-    await runInDurableObject(hub, (instance) => {
-      const hubInstance = instance as unknown as { env: Record<string, unknown> }
-      hubInstance.env = { ...hubInstance.env, HANDSHAKE_TIMEOUT_MS: 600_000 }
-    })
+    // None of the 64 clients below ever handshakes. Were the reaper live they
+    // would be reaped before the 65th dial, the cap check would find free slots
+    // and hand out a 101, and this test would fail for a reason that has nothing
+    // to do with the cap — reap-then-accept is the hub behaving correctly. It
+    // needs no opt-out to say so any more: the default deadline outlasts every
+    // test. The sleep stays as the assertion that it does.
     const daemon = await dial(hub, `/daemon/${MACHINE}`)
     await Promise.all(Array.from({ length: 64 }, () => dial(hub, `/client/${MACHINE}`)))
     await sleep(TIMEOUT_MS + 30)
@@ -329,6 +329,7 @@ describe('hibernation', () => {
 describe('the handshake deadline', () => {
   it('reaps a client that never sends, tells the daemon, spares one that did', async () => {
     const hub = freshHub()
+    await handshakeDeadline(hub, TIMEOUT_MS)
     const daemon = await dial(hub, `/daemon/${MACHINE}`)
     const seen = await dial(hub, `/client/${MACHINE}`) // channel 1
     seen.ws.send(Uint8Array.of(1))
@@ -356,6 +357,7 @@ describe('the handshake deadline', () => {
 
   it('re-arms while unseen clients remain, then reaps them too', async () => {
     const hub = freshHub()
+    await handshakeDeadline(hub, TIMEOUT_MS)
     const daemon = await dial(hub, `/daemon/${MACHINE}`)
     const b1 = await dial(hub, `/client/${MACHINE}`)
     await sleep(40)

relay/vitest.config.ts

@@ -5,19 +5,35 @@ import { cloudflareTest } from '@cloudflare/vitest-pool-workers'
 import { defineConfig } from 'vitest/config'
 
 /**
- * HANDSHAKE_TIMEOUT_MS and PAIR_TIMEOUT_MS are the hub's test seams: short here
- * so the deadline tests run in real time; unset in production, where code
- * defaults to 30 s and 10 s. The pairing deadline is the looser of the two
- * because the tests that must *not* hit it run a whole HTTP request through a
- * WebSocket round trip first.
+ * HANDSHAKE_TIMEOUT_MS and PAIR_TIMEOUT_MS are the hub's test seams; both are
+ * unset in production, where code defaults to 30 s and 10 s.
+ *
+ * The handshake deadline is bound *long* — ten minutes, far past any deadline
+ * vitest itself will allow a test to reach. It used to be bound at 50 ms so the
+ * reap tests could run in real time, and that made the reaper ambient: every
+ * test dials clients, most of them have no reason to send immediately, and a
+ * loaded runner stretches the gap between dial and first byte past 50 ms. The
+ * reaper then fired mid-test and closed a healthy client 4001, or slipped a
+ * `closed{channel}` control onto the daemon leg ahead of the frame the test was
+ * waiting for. Two different CI failures in `test/hub.test.ts` came from that,
+ * and neither reproduced locally.
+ *
+ * So the default is now "no test can outlive the deadline", and the three tests
+ * that are *about* reaping bind their own short one with `handshakeDeadline()`
+ * before they dial. A test that says nothing about the deadline is no longer
+ * making a silent bet on how fast the runner is.
+ *
+ * The pairing deadline stays short and stays the looser of the two, because the
+ * tests that must *not* hit it run a whole HTTP request through a WebSocket
+ * round trip first.
  */
 export default defineConfig({
   plugins: [
     cloudflareTest({
       wrangler: { configPath: './wrangler.jsonc' },
       miniflare: {
         bindings: {
-          HANDSHAKE_TIMEOUT_MS: 50,
+          HANDSHAKE_TIMEOUT_MS: 600_000,
           PAIR_TIMEOUT_MS: 250,
           DAEMON_SECRET: 'test-secret',
           // The deploy stamps this (internal/relaydeploy, VersionVar); binding