test(engine): stop the listen-session harness from dropping its own socket - #864
Merged
Conversation
…ocket
The "re-arms the alarm" assertion failed once in CI with `expected null not to
be null` and never reproduced in 11 local runs. Reproduced and fixed: the cause
is the harness, not the poll loop.
`connect` returns a Response carrying the CLIENT end of the WebSocketPair. Three
of the six `expectArmed` call sites destructured only `{ stub }`, dropping that
end unreferenced. workerd is then free to collect it, which tears down the pair,
fires `webSocketClose` on the DO, and `stopIfIdle` deletes the alarm — so
`getAlarm()` reads null for a reason unrelated to the code under test. In
production a real remote peer holds that end for the life of the session, so
this is a condition only the test could create.
It never reproduced because it is a GC race and an idle laptop does not collect.
Under heap churn, connecting and firing the alarm:
client end dropped → 25 nulls in 200 runs (12.5%)
Response retained → 0 nulls in 300 runs
retained + accepted → 0 nulls in 300 runs
What identified it was the socket count added to `alarmState` last week: every
null came with `sockets: 0` BEFORE the alarm ran, which eliminated everything
inside `alarm()` and pointed at the socket vanishing beforehand.
`connect` now retains the Response, released per-test — the one choke point every
session passes through, so a future test that destructures carelessly cannot
reintroduce this. Two guards keep the retention honest: a deterministic one that
fails immediately on any machine if it is unwired, and a churn sweep that fails
~99.5% of the time if it stops working. Verified by removing the retention: both
fail, the sweep in 18ms. The sweep is built so a workerd that collects less
eagerly makes it greener, never spuriously red.
Engine suite: 460 passed / 35 files (was 458; +2 guards).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae
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.
Reproduces and fixes the last unexplained flake from the local-dev-parity lane.
The failure
ListenSession — fail-safe alarm + idle > never throws on a poll error: re-arms the alarmfailedonce in CI on 2026-07-29 with
expected null not to be null, and never reproduced in 11 local runs.testis a required check withbypass_actors: [], so a live flake here blocks every PR.Two earlier hypotheses were falsified: a wall-clock race against the 2s re-arm, and
reauthReasonfailing closed on a missing
boundAtMs.Root cause — the harness, not the poll loop
connectreturns a Response carrying the client end of the WebSocketPair. Three of the sixexpectArmedcall sites destructured only{ stub }, dropping that end unreferenced. workerd is thenfree to collect it, which tears down the pair, fires
webSocketCloseon the DO, andstopIfIdledeletes the alarm — so
getAlarm()reads null for a reason unrelated to the code under test.In production a real remote peer holds that end for the life of the session, so this is a condition
only the test could create. It never reproduced because it is a GC race: an idle laptop running one
file does not collect.
Re-deriving from source, the assertion can only see null three ways —
alarm()early-returning on anempty socket set,
terminate()on a revoked binding (the falsified one), orstopIfIdle. Bothsurvivors require the socket to be gone, which turned the question from "why is the alarm null" into
"can the socket vanish first".
What settled it was the socket count added to
alarmStatelast week: every null came withsockets: 0before the alarm ran, eliminating everything insidealarm().Measured
Connecting and firing the alarm under heap churn:
The fix
connectretains the Response, released per-test. That is the one choke point every session passesthrough, so a future test that destructures carelessly cannot reintroduce this — it was never one
flaky test, it was a latent class with three exposed call sites.
Two guards keep the retention honest, because otherwise it is an invisible crutch a future edit
deletes silently:
collects less eagerly makes it greener, never spuriously red
Verified by mutation: removing the retention fails both, the sweep in 18ms.
Test plan
pnpm --filter @webhook-co/engine test→ 460 passed / 35 files (was 458; +2 guards)pnpm lint→ 997 pass / 0 failCaveat
This is a reproduction of a mechanism that produces exactly that message and symptom, not a recording
of the original CI run. If the same assertion fails again with
sockets: 1, that is a genuinelydifferent bug in the poll loop — and the message now says so.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae