Repository navigation
test(net): pin cross-node close flapping and its residuals - #5458
Conversation
|
Outcome: the quest is done. The drill passes on main, and the positive controls fail on the release before #5376 and on main before #5375. The residuals are pinned, and the quest is deleted in favor of Routes and announces. Open decisions for the maintainer:
(Written by Claude Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 333d7cdb2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| /// A link between two relays, by index, and its one-way latency in ms. | ||
| type Link = ((usize, usize), u64); |
There was a problem hiding this comment.
These fixtures depend on exact timing to reproduce route hunting, but Link stores the latency as an untyped u64 and only assigns milliseconds at the call site, making a future fixture or consumer able to silently interpret the value in another unit. Use Duration in Link and construct the constants with Duration::from_millis so the unit is encoded where each value is declared.
AGENTS.md reference: rs/AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: Link now carries a Duration, and the tables use a const fn ms so the unit sits at each value (ee9da29).
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 333d7cd against ab09ae8.
No actionable correctness bugs found in the test/quest changes. The zero-hold watching client, deterministic latency fixtures, repeated close rounds, and quiet window beyond the quarantine provide useful regression coverage. Pinning the known residual sequences while leaving #4349 closure with the routing rewrite is a sound, clearly documented direction.
Coverage limit: mesh_withdraw.rs:518–524 pins counts of affected relays, not the number of toggles on each. Extra toggles on an already-affected relay can leave (179, 3) unchanged; per-seed event counts would strengthen that sweep. The reduced fixtures already assert exact sequences.
Verification: GitHub-only static review. No tests or historical positive controls run independently; the reported test counts and timings are author results.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe simulator adds an Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 79.31% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly summarizes the main change: adding tests that pin cross-node close flapping and residual route behavior. Description check The description directly explains the tested regressions, simulator changes, quest updates, test results, and maintainer decisions.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
ee9da29 to
276cdd1
Compare
|
@codex review Since the last review: (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 276cdd18c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async fn drain(watched: &mut mpsc::UnboundedReceiver<(String, Kind)>) -> HashMap<String, Vec<Kind>> { | ||
| let mut updates = HashMap::<String, Vec<Kind>>::new(); | ||
| while let Ok(Some((prefix, kind))) = moq_net_sim::timeout(Duration::from_secs(1), watched.next()).await { | ||
| while let Ok(Some((prefix, kind))) = moq_net_sim::timeout(Duration::from_secs(3), watched.next()).await { |
There was a problem hiding this comment.
When a stale announcement is quarantined on successive relays, it can be scheduled more than three simulated seconds after this watcher’s last event because each quarantine lasts two seconds in addition to link and update-hold delays. This timeout then wins while later simulator timers remain armed; since Cluster::close drains the client and relays sequentially and never revisits earlier receivers, a delayed Start can remain unread and the tests can incorrectly report a final End or zero revival. Drive the mesh to deterministic global convergence before draining every receiver instead of treating three seconds of inactivity as convergence.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in ae7beba. moq_net_sim::idle() completes once every task is parked and no timer or source is armed, the point the run loop used to report only as a deadlock. Each drain in mesh_withdraw now awaits it first, then reads every receiver without blocking, so a release armed after a quarantine always lands before anything is read. The quiet-period timeout is gone.
The test origins use cache::Pool::unbounded(): the idle-expiry sweep re-arms every 15 s, so the mesh never went idle otherwise, and nothing here carries media. Every pinned sequence holds unchanged.
(Written by Claude Opus 5.5)
A client publishing on one relay closes a broadcast, and a client watching from another relay must see one retraction. The drill reproduces #4349's retract/announce/retract on the release before the retraction quarantine and passes on main; a zero-hold control shows it still detects the toggle. Residual counterexamples pin what main does today: a stale path outlasting the 300 ms hold on a six-relay ring, a revival the quarantine misses on nine relays, and 179 hunting relays (3 reviving) over 200 seeded random meshes. The route rewrite must bring each to a single retraction. Drains now wait out the two-second quarantine, so a late revival is seen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dates Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq_net_sim::idle completes once every task is parked and nothing is armed, the point the run loop used to report only as a deadlock. mesh_withdraw waits for it before draining any receiver, so a stale path released after a quarantine can no longer land after its relay was read. Test origins use an unbounded cache pool: the idle-expiry sweep would otherwise stay armed forever. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
276cdd1 to
ae7beba
Compare
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Problem
#4349: after a clean close, a client watching from another relay saw the path retracted and announced repeatedly. Main carries #4642's 300 ms hold, #5375's two-second retraction quarantine and #5391's queue dedup, but nothing pinned the client-visible case or the residuals #5375 measured.
Approach
All in
rs/moq-net/tests/mesh_withdraw.rs, time-mocked:Cluster: relays over latency links, a client publishing on one relay and a client watching on another (holding nothing, like a browser), plus every relay watched.cross_node_close_retracts_once_{lite06,lite07}: the drill. Eight relays (the existingretracted_relay_ignores_a_stale_pathgraph, nowSTALE_PATHS), three publish/close rounds; the watching client sees exactlyStartthenEnd, and no relay revives.cross_node_close_without_hold_reannounces: positive control; with the hold (and so the quarantine) at zero the client seesEnd, Start, End.residual_slow_ring_reannounces: a six-relay ring, links under 150 ms, where an equal-cost stale copy outlives the hold (84 ms vs 446 ms); the client seesEnd, Start, End.residual_mesh_revives: nine relays reduced from a seeded mesh; two relays revive after retracting, the client sees two toggles.residual_random_meshes: fix(net): hold back a stale path to a prefix a cursor just retracted #5375's search made reproducible: 200 splitmix-seeded meshes of 8-17 relays, links 1-150 ms, pinned at 179 hunting relays, 3 reviving, 192 updates beyond their retraction (2 s in debug).mesh_withdrawfirst awaits the newmoq_net_sim::idle(), which completes once every task is parked and no timer is armed (the point the run loop used to report only as a deadlock). This replaces the quiet-period timeout, which could fire while a later quarantine release was still armed and miss it. Test origins usecache::Pool::unbounded(), since the idle-expiry sweep keeps a timer armed forever and nothing here carries media. Existing tests still pass, and every pinned sequence is unchanged.The residual tests pin today's sequences, so a change in either direction fails and the route rewrite flips them. The quest is deleted; Routes and announces now names these tests as the #4349 acceptance.
Positive controls (same fixtures, ported to each harness):
046bfd4ba)End, Start, EndUpdate, Endd68bd0a7e)End, Start, EndEnd, Start, End42ca851af)EndUpdate, EndEndEnd, Start, EndOn main, a stale-path switch for an epochless broadcast is a
Restart, and lite-06 sends that as a retraction then an announcement. The release sends an in-place update there, so on these residuals a lite-06 client sees a toggle on main that it does not see on the release.Impact
moq-net-sim(the unpublished test executor) gainsidle(). No public API or wire change.Alternatives
#[ignore]d until the rewrite passes them. Not run in CI, so a regression in the meantime goes unseen.Decisions
Maintainer, 2026-10-13:
Restartdifference versus the release is accepted, with no release fix and no quest (2026-10-13). Main is correct: an epochless replacement is a new instance, so lite-06 peers restart cleanly. The release's in-place update kept them on the old route, and the next release carriesRestartplus epochs.Restart, and a lite-06 client seesStart, End, Start) is now a fixture requirement inquest/m1/cluster-routing/routes.md.Follow-ups
Updates per relay beforeEnd.Checks
just rs test -p moq-net: 2003 passed.mesh_withdrawpasses with every pinned sequence unchanged, before and after the switch toidle().just checkon the final head: 6286 tests passed, plusmoq-net-sim's newidle_waits_for_every_timer. One rerun hit a wall-clock flake inmoq-cli::export::an_export_failure_with_the_broadcast_up_exits_without_lingering, which passed alone and in the other run; this PR touches no code it runs.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)