Test what the terminal-waiter fix only reasoned about

[aaylward]

What

Follow-up to #173. That PR's fix rested on four claims that no test touched — each one checked by reading, which is exactly how the original bug survived in two transports at once. Tests only, plus one CHANGELOG line.

1. The composition that actually broke. A Detached loop parked in Receive, its Share() handle in an async_delivery registry, and a peer closing while a fan-out delivery is parked on the wire — the hub shape the downstream hang was reported against. No suite had it, because the halves live in different files: the transport suites park waiters with no registry above them (beast_websocket_test, the contract suite), and the registry suites park chains with no coroutine loop below (TeardownWithAParkedChainNeverHangs destroys the registry, not the session, and has no AsyncEventStream at all). The bug needed both at once. Now in session_registry_test.

2. The branch the fix created. Firing the send first means a coroutine's own parked co_await Send can resume a loop that ends and runs the entire revocation drain inline — underneath Fire, before Fire reaches its receive branch. That's safe only because a coroutine's awaits are sequential, so the branch Fire returns to is empty. The contract suite covered lone-receive and both-parked, but not lone-send, so the hazard the fix relocated was the one case it didn't exercise. If a handle ever grows a Receive, this fails as a use-after-free rather than a hang.

3. That a third party can use any of this. TerminalWaiters and the contract suite were made public specifically for out-of-tree implementors, and nothing demonstrated one could reach them. The consumer module now implements a WebSocket in consumer code, runs its terminal transition through TerminalWaiters, and is held to the same contract suite across the module boundary. That also closes an assumption I'd flagged but not tested: that the testonly websocket_contract_test_support target is reachable from a consuming module at all.

4. That JsonRpcStreamSocket parks nothing. This is why the fix needed no change there — its async twins forward to the socket underneath — and it was checked by reading. Instantiating the suite for it keeps that true if the decorator ever grows slots of its own, and incidentally shows the suite works for a decorator rather than only for real transports.

Four implementations now run the shared suite: both in-repo transports, the JSON-RPC decorator, and the consumer's.

Testing

Verified the same way the fix was: flipping the order in TerminalWaiters::Fire — one edit — and confirming the new tests fail. The registry composition test wedges ("the close wedged: a parked fan-out delivery was never completed"), and so does the ordering test in all four instantiations, including the out-of-tree one — which is the point of #3: the third-party socket inherits the rule without its author knowing the rule exists.

suite
async_event_stream_test	34/34 (ASan+UBSan, TSan)
beast_websocket_test	68/68
session_registry_test	53/53 (ASan+UBSan, TSan)
jsonrpc_stream_socket_test	20/20
websocket_contract_consumer_test	5/5
websocket_pair_test	16/16
event_stream_test	19/19

Same caveat as #173: bazel test can't fetch the bats-core archive through this environment's proxy, so suites were compiled and run directly against vendored headers. Two things here are therefore unexercised by a real bazel run and want CI:

• the new consumer cc_test and its dep on @smithy_cpp//runtime:websocket_contract_test_support — i.e. the very cross-module visibility claim item 3 is meant to prove. CI's bazel consumer jobs are the real check.
• the added :websocket_contract_test_support dep on jsonrpc_stream_socket_test.

One judgement call worth flagging: the lone-parked-send test detects "the loop has parked" by watching its send counter go stable, because how many writes a wire accepts before wedging is the transport's business, not the suite's. That's a timing heuristic — two consecutive equal readings 500ms apart, 30s budget. It's the one thing here that could flake on a very slow runner, and it would flake as a clean "the loop never parked in Send" failure rather than a hang.

Checklist

• Tests added/updated for the change
• bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally — see the caveat; suites run directly, bazel could not fetch a toolchain dep here
• Formatting clean (clang-format verified; buildifier not available in this environment)
• Architectural decisions recorded as an ADR (if applicable) — n/a, no decisions changed

---

Generated by Claude Code

[aaylward]

Both macOS jobs failed Beast/WebSocketContractTest/0.ATerminalTransitionCompletesALoneParkedCoroutineSend — the timing heuristic I flagged in the description. Fixed in f9a7705, but the cause was worse than a slow runner, and worth writing down because it briefly made a passing test meaningless.

The detector could not detect a parked send. WaitUntilStable required the counter to be non-zero before believing it had settled:

stable = (now == last && now > 0) ? stable + 1 : 0;

With a large enough message the first send parks, so the counter never leaves zero and stability is never reported. The guard made the helper blind to the most decisively parked state there is. Zero is a fine stable value here: a Detached coroutine starts eagerly and issues its first send before the launch expression returns, so a settled counter can't mean "not started yet" — it means parked, or finished. The old predicate was conflating those and getting it wrong; the test now rules out "finished" with an explicit ASSERT_FALSE(loop_ended).

The Beast driver's messages were too small to wedge a macOS wire. It absorbed 1 MiB writes for the full 30s without ever parking. The more interesting part: WedgeThenPark's 2s-per-attempt timeout read those slow-but-progressing writes as parked, so the sibling ...FiresTheParkedSendBeforeTheParkedReceive passed on macOS against a wire that had never wedged — it was asserting on a session with nothing actually parked. 8 MiB exceeds any loopback's buffers, so the first write parks and both detectors now agree for the right reason.

Two things I'd call out from this:

• The bug reproduced on Linux only after I raised the message size — the 1 MiB value was masking it by letting a few sends land first. Different symptom per platform, one wrong predicate.
• Side effect: the Beast contract group went from 39s to 8s, since the tests park immediately rather than grinding through their budgets. That also pulls beast_websocket_test well back from the size = "small" timeout it was creeping toward at 48s.

Re-verified locally: beast_websocket_test 68/68, async_event_stream_test 34/34 (ASan+UBSan and TSan), session_registry_test 53/53, jsonrpc_stream_socket_test 20/20, consumer contract 5/5 — and flipping TerminalWaiters::Fire still fails the ordering test in every instantiation, so the guard is intact rather than merely quiet.

---

Generated by Claude Code

[cursor]

Approve with nits. Tests-only follow-up to #173 that closes real coverage gaps; CI is green on f9a7705 (including consumer + macOS after the detector fix).

What works

The four claims map cleanly onto tests:

1. Hub composition — APeerCloseWithAParkedFanOutEndsTheLoopAndTheDelivery is the important one. Transport suites and registry suites each had half the stack; this is the shape that actually hung.
2. Shared contract expansion — lone-parked-send + AwaitLoopEnd + JsonRpc/consumer instantiations make the suite the real implementor contract, not an in-repo convenience.
3. Consumer module — ConsumerSocket is a solid reference: parks under the lock, std::exchanges into TerminalWaiters, fires unlocked. Cross-module visibility of websocket_contract_test_support is proven by CI’s consumer jobs.
4. macOS fix — WaitUntilStable allowing zero, plus 8 MiB Beast payloads, are the right fixes. The PR comment’s diagnosis (detector blind to first-send park; WedgeThenPark false-positive on slow progress) is accurate and more serious than “slow runner.”

Nits

Lone-send test overclaims what it catches. The comment says send-first can run pin drain under Fire and would UAF if a handle grew Receive. The test never Share()s, so there are no pins, and with only a send parked, flipping Fire’s order still passes (receive slot is empty either way). What it actually guards today is “terminal transition must complete a lone parked send / loop must unwind.” Worth tightening the comment so the next reader doesn’t think this is the ordering tripwire — that’s still ...FiresTheParkedSendBeforeTheParkedReceive.

WaitUntilStable floor is 3s (three 1s samples). Fine given 8 MiB parks immediately, but every instantiation pays it. Not blocking; just the remaining timing surface after the macOS fix.

Registry test still hand-rolls the unwind wait instead of AwaitLoopEnd. Harmless duplication; abort-on-timeout posture matches.

Verdict

Mergeable. The composition test and the consumer/JsonRpc suite wiring are the load-bearing bits; the detector/size fix makes the new Beast coverage real rather than accidentally green. Only ask I’d make before merge: tone down the lone-send comment so it matches the failure mode it actually has.

[aaylward]

Nit 1 was right on both counts, and I checked rather than took it on faith — f04e50c.

The test never called Share(), so view_ held no state, so ~AsyncEventStream → End() hit its if (state_ == nullptr) return; and did nothing. There was no revocation drain at all, and the comment's talk of one was fiction. And with no receive parked, Fire's order is unobservable, so flipping it does not fail this test — it is not the ordering tripwire, as you say.

I took the stronger fix rather than only softening the prose: the loop now Share()s, which is what makes the case worth a test. Firing the lone parked send resumes the loop inline, so the loop's exit — revoke the shared view, close the session, drain pins — runs underneath Fire on the completing thread, and that close reenters the transition that is still running (on the pair, a nested Fire inside the outer one). That is a real reentrancy path; the version you reviewed was exercising a bare resume. The comment now states exactly that and points at ...FiresTheParkedSendBeforeTheParkedReceive for ordering.

Worth noting this is the second time on this PR that a green test turned out to be asserting less than it claimed — the first was WedgeThenPark reading slow macOS writes as parked. Both were mine, and both were only visible by asking what the test would do if the thing it guards were broken.

On the other two:

WaitUntilStable's 3s floor — keeping it. Parking is decisive now, so the samples are pure margin, but the failure it guards against (misreading slow progress as parked) is exactly what produced a meaningless green last round. 3s × 4 instantiations is a cheap price for not re-learning that. Happy to drop to two samples if you'd rather have the 4s back.

Registry test hand-rolling the unwind wait — leaving it. session_registry_test has no other reason to depend on websocket_contract_test_support, and taking the dep for one 8-line helper seemed a worse trade than the duplication. The abort-on-timeout posture matches deliberately.

Re-verified after the change: beast_websocket_test 68/68, async_event_stream_test 34/34 (ASan+UBSan and TSan), jsonrpc_stream_socket_test 20/20, consumer 5/5, and the lone-send test still completes in ~4s.

---

Generated by Claude Code