fix(ssm): wait on the condition, not on 5ms (#61) - #68
Merged
Conversation
Four assertions in `src/ssm/session.test.ts` slept `setTimeout(r, 5)` for an
async chain whose duration is a property of the machine, not of the code: the
inbound handler awaits WebCrypto digests before ACKing, and `markReady` is
invoked with `void`, so nothing awaits its `flushPending`. 5ms was enough on an
idle laptop and not enough alongside 29 other test files in parallel CI workers.
Each sleep is replaced by a wait on the condition it was actually about:
- `s.ready` after HandshakeComplete — which sends no frame back, so `ready`
itself is the only observable effect. The `waitForSends(ws, 3).catch(() => {})`
hack that stood in for it goes away with it.
- `s.ready` in `makeReady` before clearing `ws.sent` — a premature clear would
drop the flushed frames the *caller's* assertions then read, which is why the
helper needs the session and not just the socket.
- a flushed `Output` frame in the queue-then-flush test.
- `onClose` having fired and the socket being closed.
`waitFor(pred, describe)` polls on a 1ms tick with a 2s ceiling, so the test's
duration tracks the machine while its verdict tracks only correctness. It takes
a `describe` string because a CI timeout message is the only diagnostic anyone
will see. `waitForSends` is now a thin wrapper over it.
`sentFrames`/`inputFrames` extract the repeated deserialize-and-filter. Note the
session sends `Uint8Array`s, not `ArrayBuffer`s — an `instanceof ArrayBuffer`
filter silently matches nothing, which is a quiet way to write a test that can
never fail.
This matters more than an ordinary flake: the flakiest assertion covers input
being *queued rather than dropped*, which is the terminal's core correctness
property. A test everyone learns to re-run is how a real regression in the flush
path gets waved through.
Verified by slowing the flush path to 25ms — the old sleeps fail exactly the
test #61 names, the waits pass — and by four mutations of `session.ts`
(no-op flush, never-ready, no onClose, no socket close), each still caught.
Ten full-suite runs under 8-core load: 591 passing, no flake.
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.
Closes #61.
Four assertions in
src/ssm/session.test.tssleptsetTimeout(r, 5)for an async chain whose duration is a property of the machine, not of the code: the inbound handler awaits WebCrypto digests before ACKing, andmarkReadyis invoked withvoid, so nothing awaits itsflushPending. 5 ms was enough on an idle laptop and not enough alongside 29 other test files in parallel CI workers.What each sleep was actually waiting for
Each is now a wait on that condition instead of on a duration:
:141—waitForSends(ws, 3).catch(() => {})+ 5 mss.ready. HandshakeComplete sends no frame back, soreadyis the only observable effect — the.catch(() => {})on a send-count wait was standing in for a condition it couldn't express, and goes away with it.makeReadys.readybefore clearingws.sent. A premature clear drops the flushed frames the caller's assertions then read, which is why the helper now takes the session and not just the socket.OutputframeonClosehaving fired and the socket closedwaitFor(pred, describe)polls on a 1 ms tick with a 2 s ceiling. It takes adescribestring because a CI timeout message is the only diagnostic anyone will ever see of a failure here.waitForSendsbecomes a thin wrapper over it.sentFrames/inputFramesextract the repeated deserialize-and-filter. Worth noting: the session sendsUint8Arrays, notArrayBuffers — my first cut ofsentFramesfiltered oninstanceof ArrayBufferand silently matched nothing. That's a quiet way to write a test that can never fail, so the helper filters out the one string send (the token JSON) rather than filtering in on a binary type.Why this is worth more than an ordinary flake fix
The flakiest of the four covers input being queued rather than dropped — the terminal's core correctness property. A test everyone learns to re-run is precisely how a real regression in the flush path gets waved through.
Verification
session.ts, each still caught: no-opflushPending(1 fail),handshakeDonenever set (3 fails), noonCloseon ChannelClosed (1 fail), socket not closed (1 fail). The waits fail on broken, not merely on slow.Other
setTimeout(r, 0)calls insrc/ui/*.test.tsare left alone: a zero-delay yield is a deterministic macrotask boundary, not a bet on how long something takes.