Repository navigation
test(relay): tests prove a session stayed open by its end, not a quiet window - #8087
Conversation
Quest-Claim: 900e454fcb0275cb
… its end reason Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The healthy bar defaults to one second, so the old 120ms sleep never cleared it and the test took the backoff path instead of the replacement dial it targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 21 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 275f24a
No actionable findings. The event-based assertions match lease close reasons and admission ordering; the watch helpers avoid lost wakeups. The synchronized listings and decoy fetch preserve the tenant checks, and the manual-clock handover test reaches the healthy-session dial path with the deadline already armed.
Direction: keep this scoped conversion. The remaining mesh-convergence tests and their design choice are explicitly retained in the quest.
Verification limits: GitHub-only static review of all changed files and relevant auth, listing, fetch, clock, and reconnect logic. I did not run tests, reproduce the reported mutation, or verify the io_uring/browser lanes. Check is still in progress; this is not a claim that CI passed.
(Written by review, OpenAI)
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test-strength gap inline.
Reviewed changes
Reviewed all 9 changed files at 275f24a, following the relay's lease close path (rs/moq-relay/src/auth.rs:600, rs/moq-net/src/session.rs:722), the reconnect handover (rs/moq-net/src/reconnect.rs:663-745), the origin sync barrier (rs/moq-net/src/model/origin.rs:7913), and the HTTP fetch root rewrite (rs/moq-relay/src/web.rs:1047).
- Session-open proofs —
auth_lifetime.rsandsession_revalidate.rsnow close each session from the client and assert theendits lease reports is"closed", replacing 200ms–2s quiet windows. - Script wakeups — both scripted auth servers gain a per-request
watch, sountil/ends_at_least/end_ofawait the event instead of sleeping; theend_arrivedNotifyand poll loops go. - Duration bound —
the_end_carries_duration_and_bytesbounds the reported duration between zero and the window the test measured around the session. - Absent broadcasts —
tenant_matrixandsocket_authread a listing toSynced, andtenant_matrixadds a decoy broadcast at B's root so B's fetch must answer with it rather than with A's secret. - GOAWAY handover — the test runs on a
Manualclock with the black-hole dial as the ordering point, and now setsbackoff.healthyexplicitly so it exercises the straight-into-dial path. - Quest —
rs-relay.mdis narrowed to the remaining mesh/process-signal windows, with the dial-hook decision recorded in## Waiting.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
| kio::clock::timeout(TIMEOUT, relayed.follow("mine/cam", None).unwrap().available()) | ||
| .await | ||
| .expect("the scoped publisher's announce never reached the relay") | ||
| .expect("announced broadcast resolves"); |
There was a problem hiding this comment.
available() resolves to Result<Option<Consumer>, _>, so this .expect only unwraps the Result; an Ok(None) (the path was announced, then went offline again) passes and the assertion proves nothing about admission. The sibling waits at auth_lifetime.rs:938 and :1933 .unwrap() the option for exactly this reason, and the old timeout(2s, session.closed()).is_err() would have caught that regression.
| .expect("announced broadcast resolves"); | |
| .expect("announced broadcast resolves") | |
| .expect("the broadcast is live"); |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Decision recorded: the relay's remaining multi-relay and signal tests get a dial hook on (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
ℹ️ No critical issues — the delta is a quest decision record; one count to correct inline.
Reviewed changes
Reviewed the commits since the prior Pullfrog review at 275f24a (bd2f12fa1, plus the origin/main merge 4a946b56e); the relay tests themselves are untouched.
- Decision recorded —
quest/m1/mock-clock-tests/rs-relay.mdreplaces its open## Waitingquestion with aDecided 2026-11-06bullet naming thecluster::Optionsdial hook an in-memory link fills, so the remaining mesh windows run onkio::clock::simand end onsim::idle. It follows the epic's ownDecided … / ✅convention and ends with aPublic API:line. - Guidance added — three bullets scope the hook (name it by role), the in-memory relay the simulator needs (
tests/integration/memory.rsprecedent), and the process-signal sourceshutdown_signalrequires. - Merged
origin/main— the merge only carries main'smoq-tokioandjs/publishbench changes; nothing authored by this PR.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
| [moq-dev/moq#8087](https://github.com/moq-dev/moq/pull/8087)): | ||
|
|
||
| - ✅ **A dial hook on `cluster::Options` that an in-memory link can fill**, so | ||
| the four tests above run on `kio::clock::sim` and prove convergence with |
There was a problem hiding this comment.
four tests does not match the bullets above: goaway_cluster.rs names two, so five tests across four files are listed. Say the tests above (or the four files above) so whoever converts them is not short by one.
| the four tests above run on `kio::clock::sim` and prove convergence with | |
| the tests above run on `kio::clock::sim` and prove convergence with |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 4a946b56ed291ca10a2d0a53b364321055bf69ce, specifically the update since 275f24a6605e54370c61121cd4d65f94734edf01, accounting for the merge of main.
No new actionable findings. Only the relay quest changed: it now records the dial-hook decision and covers the in-memory accept path and controllable signal source needed by the simulator. The previously reviewed runtime and test changes are unchanged.
Direction: keep the hook implementation in the planned follow-up, as scoped here. This reviews the planning decision, not an implemented simulator integration.
Limits: GitHub-only static inspection of the planning delta and relevant relay/simulator code; no local commands, tests, or quest context run. Check remains in progress, not verified passing.
(Written by review, OpenAI)

Part of quest/m1/mock-clock-tests/rs-relay.md, under the mock-clock-tests epic.
Summary
moq-relay's single-relay tests stop proving a session stayed open by waiting out a
timeout(..).is_err()window, and stop sleeping to settle. Each one now waits for the event it is checking:endits lease reports gives the client's own close ("closed"). Anendwith any other reason means the relay ended the session first. This replaces the 200ms to 2s windows inauth_lifetime.rs(tier change, resize, refused fetch, embedded revoke, late decision) andsession_revalidate.rs(unmatched push, retier). The embedded-decider tests read the same reason fromlease::Producer::closed.Lease::endedapplies a tier only after it has decided to keep the session.path_and_cidr_select_a_subsetchecks thatnudged.idsis exactly the matched pair. The certificate test proves admission when the publisher's announce reaches the relay.ends: the scripted auth servers now have awatchthat changes on every request, andScript::until/ends_at_least/end_ofwait on it. This replaces theend_arrivedNotifyand the poll loops.sleep(300ms)then>= 200ms, the test bounds the reported duration between zero and the time the test measured around the session. moq-auth already unit-tests exact duration on a paused clock.tenant_matrix): a fresh subscriber reads its listing toRoutes::poll_synced, so anything missing was not announced when the relay answered. This replacessleep(SETTLE). B's HTTP fetch is now served from a decoy broadcast at B's root, instead of a 2s window that hoped for no answer. Insocket_auth, gettingSyncedas the first event already proves the listing held none of the session tracks, so its 500ms window goes.goaway_cluster): runs on aManualclock. The black hole accepting the replacement dial is the ordering point, because the policy arms the deadline before it dials. Advancing by exactly the handover must close the old session while the dial still hangs; I checked that advancing 1ns short fails. Doing this showed the old test never cleared the healthy bar: it defaults to 1s, and the test slept only 120ms. So it ran the backoff path, not the straight-into-dial path its comment describes. It now setsbackoff.healthyexplicitly.src/session.rsandsrc/auth.rs: a 20ms window and a zero timeout becomenow_or_never(), since both states are synchronous.The quest is narrowed to the multi-relay windows that remain (see Waiting).
Left in the quest
The multi-relay and signal windows (
cluster_unknown,goaway_cluster,cluster_drain,shutdown_signal) stay in the narrowed quest. The maintainer decided on 2026-11-06 to givecluster::Optionsa dial hook that an in-memory link can fill, so those tests run on the simulator and end onsim::idle. The quest's Plan records that decision with guidance, and the work is ready to start. The hook is not part of this PR: the mesh tests build whole relays throughRelay::loadand dial peers throughmoq_tokio's reconnecting client, so it needs its own design pass.Choices made without asking
rs/AGENTS.md): theruntime_uring.rsand listing poll loops,public_rules'assert_refused(waits for a close),web.rs's refusedtls://dial and itselapsed() >= timeoutlower bounds,bitrate_caps'LEAST, andshutdown_signal's drain-window lower bounds. The quest's scan counted some of these.a_configured_token_is_the_connection_credentialon lite-06 now waits for theconnectrequest instead of a 500ms "no refusal" window. The script grants every connect, so that window never had a refusal to catch. Lite-07 still proves admission by its AUTH grant.endisrefused: had it ended early, it would have reporteddone.runtime_uring.rsalone. It has only poll loops, and moq-dev/moq#8082 is reworking it.Impact
Public API: none. Wire: none. Tests only.
Validation
just check: passed (749 tests, clippy, fmt, quest check).just rs test -p moq-relay --features test-support --test integration -E 'test(socket_auth::)': passed.socket_authneedstest-support, so it was run separately.goaway_handover_is_enforced_while_the_replacement_dial_hangsfail.uring_keeps_tenants_apart(needs_uring). It goes through the samesynced_listingpath, so I left it to CI.just test harness, browser media rows) was run locally; all are left to CI.Follow-ups
Review
Codex, gpt-6.1-sol, medium effort: round 2 on head
4a946b56ed291ca10a2d0a53b364321055bf69cereported no findings. Round 1 on275f24a660also reported none.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)