Repository navigation
rewrite: the CLOSE-during-mint test's yield poll can accuse the code, and its comment understates its coverage #991
Description
a_close_during_the_invite_mint_leaves_the_event_but_not_the_read (crates/nostream-relay/tests/handlers.rs) waits for the spawned mint to reach its held await with
for _ in 0..100 {
if fixture.invites.creates_started() == 1 { break; }
tokio::task::yield_now().await;
}
assert_eq!(fixture.invites.creates_started(), 1, "the invite mint is in flight");The loop is deterministic in the sense that matters — it cannot make the property pass late, because the property is asserted after the abort and the release is program-ordered, and a mint that never starts fails the assertion with a message rather than hanging. But a fixed iteration bound is a (small) flake source: on a loaded runner, 100 scheduler opportunities may not be enough for a spawned task to start, and the failure would look like the defect the test exists to catch.
What it needs
The test double already counts creates_started(); give it a notification as well — a tokio::sync::Notify or a oneshot fired when the mint first awaits — and await that instead of polling. Then the wait is on a state the code signals, not on the scheduler being generous.
Test support only (crates/nostream-relay/src/testing.rs), no production change. Same class as the abort observable #963's spec considered: the double should be able to say what it is doing rather than being sampled.
Activity
chappie-daemon commentedon Oct 25, 2026
Two follow-ups from #963's review, both test-comment sized, both on the test this issue already names (
crates/nostream-relay/tests/handlers.rs::a_close_during_the_invite_mint_leaves_the_event_but_not_the_read).1. Why the test authenticates is unexplained. It calls
connection.authenticate(...)before the handlers run, and nothing says why. The answer is insubscribe.rs:167-190: the invite path ignores a request from an unauthenticated client (authenticated.is_empty()→ return,:171-174) and then requires an authenticated pubkey oninviteRequestWhitelist(:181-190). So the call is required, not incidental setup — but a reader has to find that out from the production code. One sentence above it: the invite mint is behind the NIP-42 identity check, so the test authenticates before it subscribes.2. The comment should name where the wire coverage lives. It names the sibling wire test but not
wire_parity, and the reason the wire route was abandoned belongs beside the test that abandoned it, not only in #983: after #978,CLOSEis a prefix, so a wire test's timer could only say whether 50 ms was long enough, never whether the prefix ran.And, as the body says above: replace the
for _ in 0..100 { … yield_now() }poll with a signal from the invite double.[-]rewrite: replace the CLOSE-during-mint test's yield poll with a signal from the invite double[/-][+]rewrite: the CLOSE-during-mint test's yield poll can accuse the code, and its comment understates its coverage[/+]chappie-daemon commentedon Oct 25, 2026
Two follow-ups to
a_close_during_the_invite_mint_leaves_the_event_but_not_the_read(crates/nostream-relay/tests/handlers.rs), both from #963's review. One issue rather than two, because both are about the same test and a reader picking it up should see both halves.1. How it waits for the mint (the
yieldpoll)At
fa1f09b,handlers.rs:1262-1267:This is not a timer, and it is not merely a small flake source. On a multi-thread runtime the spawned mint may not have been polled at all by the hundredth yield, so the loop can exhaust and the test then fails on an assertion whose message says the mint is in flight — which a reader will take as a defect in the production code rather than in the test. The failure mode is a false accusation, not only a flake.
Fix: the invite double already counts
creates_started(); give it a notification (atokio::sync::Notify, or aoneshotfired when the mint first awaits) andawaitthat instead of sampling the counter. The wait then rests on a state the code signals rather than on the scheduler being generous. Test support only (crates/nostream-relay/src/testing.rs), no production change — the same principle as the abort observable #963's spec considered: the double should be able to say what it is doing.2. What the comment claims about coverage
The test's comment names the sibling wire test but not
wire_parity, and it does not say why the wire route was abandoned — which is the part a reader needs, because the obvious reaction to a handler-level test in a socket-level file is "where did the socket go".Fix: name
wire_parityas well, and state the reason in the same breath: after #978,CLOSEis a prefix running on the connection loop, so a wire test's timer could only say whether 50 ms was long enough, never whether the prefix ran. That reason lives in #983's commentary today; it belongs beside the test that gave the socket up.