diff --git a/relay/test/harness.ts b/relay/test/harness.ts index e139894..0da8b3e 100644 --- a/relay/test/harness.ts +++ b/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 { + await runInDurableObject(hub, (instance) => { + const withEnv = instance as unknown as { env: Record } + withEnv.env = { ...withEnv.env, HANDSHAKE_TIMEOUT_MS: ms } + }) +} + export function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)) } diff --git a/relay/test/hub.test.ts b/relay/test/hub.test.ts index 3c7c95b..6963400 100644 --- a/relay/test/hub.test.ts +++ b/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 } - 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) diff --git a/relay/vitest.config.ts b/relay/vitest.config.ts index 877decc..8aba76b 100644 --- a/relay/vitest.config.ts +++ b/relay/vitest.config.ts @@ -5,11 +5,27 @@ 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: [ @@ -17,7 +33,7 @@ export default defineConfig({ 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