Skip to content

test(relay): tests prove a session stayed open by its end, not a quiet window - #8087

Merged
kixelated merged 9 commits into
mainfrom
quest/m1/mock-clock-tests/rs-relay
Nov 7, 2026
Merged

kixelated merged 9 commits into
mainfrom
quest/m1/mock-clock-tests/rs-relay

Conversation

@kixelated

@kixelated kixelated commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  • A session stayed open: the test closes it from the client and checks that the end its lease reports gives the client's own close ("closed"). An end with any other reason means the relay ended the session first. This replaces the 200ms to 2s windows in auth_lifetime.rs (tier change, resize, refused fetch, embedded revoke, late decision) and session_revalidate.rs (unmatched push, retier). The embedded-decider tests read the same reason from lease::Producer::closed.
  • Ordering points: the retier checks wait for the stats tier to move, because Lease::ended applies a tier only after it has decided to keep the session. path_and_cidr_select_a_subset checks that nudged.ids is exactly the matched pair. The certificate test proves admission when the publisher's announce reaches the relay.
  • Settle sleeps before reading ends: the scripted auth servers now have a watch that changes on every request, and Script::until / ends_at_least / end_of wait on it. This replaces the end_arrived Notify and the poll loops.
  • End duration: instead of 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.
  • Absent broadcasts (tenant_matrix): a fresh subscriber reads its listing to Routes::poll_synced, so anything missing was not announced when the relay answered. This replaces sleep(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. In socket_auth, getting Synced as the first event already proves the listing held none of the session tracks, so its 500ms window goes.
  • GOAWAY handover (goaway_cluster): runs on a Manual clock. 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 sets backoff.healthy explicitly.
  • src/session.rs and src/auth.rs: a 20ms window and a zero timeout become now_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 give cluster::Options a dial hook that an in-memory link can fill, so those tests run on the simulator and end on sim::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 through Relay::load and dial peers through moq_tokio's reconnecting client, so it needs its own design pass.

Choices made without asking

  • Kept on the real clock, since they only wait for progress or bound timing from below (both allowed by the epic and rs/AGENTS.md): the runtime_uring.rs and listing poll loops, public_rules' assert_refused (waits for a close), web.rs's refused tls:// dial and its elapsed() >= timeout lower bounds, bitrate_caps' LEAST, and shutdown_signal's drain-window lower bounds. The quest's scan counted some of these.
  • a_configured_token_is_the_connection_credential on lite-06 now waits for the connect request 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.
  • The "fetch must not end while its body streams" check is folded into the later assertion that the fetch's end is refused: had it ended early, it would have reported done.
  • I left runtime_uring.rs alone. 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_auth needs test-support, so it was run separately.
  • Mutation: advancing the handover clock 1ns short makes goaway_handover_is_enforced_while_the_replacement_dial_hangs fail.
  • Not run locally: uring_keeps_tenants_apart (needs _uring). It goes through the same synced_listing path, so I left it to CI.
  • Browser checks: none apply to this change. No browser-driven lane (Playwright, just test harness, browser media rows) was run locally; all are left to CI.

Follow-ups

  • None beyond the narrowed quest, which already holds the dial-hook work.

Review

Codex, gpt-6.1-sol, medium effort: round 2 on head 4a946b56ed291ca10a2d0a53b364321055bf69ce reported no findings. Round 1 on 275f24a660 also reported none.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 7 commits November 6, 2026 13:31
… 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>
@kixelated kixelated changed the title moq-relay tests run on a mocked clock test(relay): tests prove a session stayed open by its end, not a quiet window Nov 6, 2026
@kixelated
kixelated marked this pull request as ready for review November 6, 2026 22:54
@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e650539d-5129-4f2f-8017-074f6d850a0b
Commits

Reviewing files that changed from the base of the PR and between 636ba9f and 4a946b5.

Files selected for processing: 9

  • quest/m1/mock-clock-tests/README.md
  • quest/m1/mock-clock-tests/rs-relay.md
  • rs/moq-relay/src/auth.rs
  • rs/moq-relay/src/session.rs
  • rs/moq-relay/tests/integration/auth_lifetime.rs
  • rs/moq-relay/tests/integration/goaway_cluster.rs
  • rs/moq-relay/tests/integration/session_revalidate.rs
  • rs/moq-relay/tests/integration/socket_auth.rs
  • rs/moq-relay/tests/integration/tenant_matrix.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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.rs and session_revalidate.rs now close each session from the client and assert the end its lease reports is "closed", replacing 200ms–2s quiet windows.
  • Script wakeups — both scripted auth servers gain a per-request watch, so until / ends_at_least / end_of await the event instead of sleeping; the end_arrived Notify and poll loops go.
  • Duration bound — the_end_carries_duration_and_bytes bounds the reported duration between zero and the window the test measured around the session.
  • Absent broadcasts — tenant_matrix and socket_auth read a listing to Synced, and tenant_matrix adds 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 Manual clock with the black-hole dial as the ordering point, and now sets backoff.healthy explicitly so it exercises the straight-into-dial path.
  • Quest — rs-relay.md is narrowed to the remaining mesh/process-signal windows, with the dial-hook decision recorded in ## Waiting.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using 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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
.expect("announced broadcast resolves");
.expect("announced broadcast resolves")
.expect("the broadcast is live");

kixelated and others added 2 commits November 6, 2026 15:13
@kixelated

Copy link
Copy Markdown
Collaborator Author

Decision recorded: the relay's remaining multi-relay and signal tests get a dial hook on cluster::Options that an in-memory link can fill, so they run on the simulator. Not chosen: real-clock exceptions. The narrowed quest's Plan records this with guidance. The hook itself is that quest's work, not this PR's.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Nov 6, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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.md replaces its open ## Waiting question with a Decided 2026-11-06 bullet naming the cluster::Options dial hook an in-memory link fills, so the remaining mesh windows run on kio::clock::sim and end on sim::idle. It follows the epic's own Decided … / ✅ convention and ends with a Public API: line.
  • Guidance added — three bullets scope the hook (name it by role), the in-memory relay the simulator needs (tests/integration/memory.rs precedent), and the process-signal source shutdown_signal requires.
  • Merged origin/main — the merge only carries main's moq-tokio and js/publish bench changes; nothing authored by this PR.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Merged via the queue into main with commit 2db6151 Nov 7, 2026
9 of 10 checks passed
@kixelated
kixelated deleted the quest/m1/mock-clock-tests/rs-relay branch November 7, 2026 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant