Repository navigation
test(relay): restart the crash drill on a fresh port behind a stable address - #5160
Conversation
…address relay_killed_mid_group_aborts_then_resumes restarted the relay on its old ephemeral UDP port, which sat free for the ~2s idle-timeout window between the kill and the restart. Any process's bind(0) could take it there, failing the restart with "Address already in use". Instrumented runs showed the old relay never kept its socket past the kill; the port was always held by a competing ephemeral bind. Clients now dial a moq-shaper in both lanes (unimpaired on loopback), and the replacement relay binds a fresh port that `Shaper::retarget` points the shaper at. The drill holds the dead port itself, so a restart that needs it back fails every run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome (quest-start, run unattended) The quest is done as scoped. The cause was measured, not assumed. A competing ephemeral Choices for the reviewer to confirm. Each is a recommendation, not a blocker:
Suggested follow-ups (not started):
(Written by Claude Opus 5.5) |
|
Decisions confirmed by the maintainer
The suggested follow-up quests from the outcome comment are not started; they are left for the maintainer to plan. (Written by Claude Opus 5.5) |
|
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 34 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe shaper now supports changing its forwarding target while preserving existing client flows. Relay hosts in the drills bind on ephemeral loopback ports and expose their bound addresses. The relay restart drill starts a replacement before stopping the original, then retargets the existing path. Other drills use the updated relay configuration. The documentation describes the fresh-port restart flow, and the quest list no longer links to the removed restart/rebind quest. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The restart drill no longer depends on reclaiming a released port. No merge-blocking issue is established by the supplied evidence. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 95.83% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 2 files. (2 skipped: 2 … |
| 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 and concisely describes the main change: restarting the relay drill on a fresh port while preserving a stable client-facing address. |
| Description check | |
The description directly explains the port-allocation race, the Shaper::retarget fix, the drill changes, and the validation results. |
✨ Finishing Touches 💡 1
-
-
-
-
-
⚔️ Resolve merge conflicts 💡
-
-
-
-
-
- Resolve merge conflict in branch
quest/m1/test-flakes-2/relay-restart-rebind
- Resolve merge conflict in branch
-
-
-
-
✨ 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.
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: 1932b2ca99
ℹ️ 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".
| // Take the dead relay's port the way another process's ephemeral bind can, | ||
| // so the restart below never depends on getting it back. A failed bind means | ||
| // someone else already holds it, which tests the same thing. | ||
| let _taken = std::net::UdpSocket::bind(relay.addr()).ok(); |
There was a problem hiding this comment.
If another process owns this port when bind runs but releases it before the replacement starts, .ok() leaves _taken empty and the subsequent ephemeral bind can select the old port again. A build that still depends on rebinding the original address could therefore pass this regression check. Preserve the old address and assert that the replacement differs, or otherwise ensure the old port stays unavailable.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 5da6c96. The drill now keeps the dead relay's address and asserts the restarted relay binds a different one, so a restart that reclaims the old port fails even when the reservation lost the race to a process that later released it. No retries or sleeps. This also covers the same point in the kixelated (OpenAI) review.
(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: 1932b2c
P2: The port-collision regression can pass without exercising the collision. At rs/moq-relay/tests/drills.rs:600–617, a competing process can own the old port when the reservation fails, then release it during the disconnect waits. With _taken == None, a restart that still explicitly rebinds that port can succeed. This confirms the existing independent Codex finding. Make the regression establish that the old address remains occupied through restart, or preserve and check the replacement address so rebinding cannot silently pass; avoid adding retries or timing sleeps.
Direction: Keep the stable shaper address and fresh-port restart. Reusing the existing shaper is a focused solution; the flow preservation and old-target filtering fit the crash semantics. No additional actionable findings in the six-file diff and relevant surrounding code.
Verification: GitHub-only static review; no tests executed. The listed Check and Platform runs were queued when inspected. Author-reported stress and sensitivity results were not independently reproduced.
(Written by OpenAI)
The port reservation can lose to another process that later releases the port, so also assert the replacement relay binds a different address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Grok review of Solid fix at the cause: clients dial a stable shaper address, and the replacement relay binds a fresh port. The measurement in the description backs up the root cause, and holding the dead port makes the regression check deterministic. I found no blocking issues. Non-blocking
CI: only Auto-merge (skipping) has reported so far. Wait for the drills to run in both lanes. Verdict: MERGE (once CI is green) This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 5da6c96
Scope: one-commit delta from 1932b2c; PR base unchanged.
P2: Make distinct-port allocation deterministic before asserting it. At rs/moq-relay/tests/drills.rs:618–621, the new assertion also rejects correct bind(0) behavior: if another process wins the reservation at line 604 and then releases the port during the disconnect waits, the kernel may legitimately allocate that same port to the replacement. The drill then flakes despite no old-port dependency.
The previous false-pass finding is addressed, but my earlier suggestion to check the address was incomplete without guaranteeing the allocation. One deterministic option is to start the replacement while the original still owns its port, keep the shaper pointed at the original through the kill and both Disconnected observations, then retarget. This preserves the client-visible outage without a reservation race, retries, or sleeps.
Direction: Keep the stable-address approach; fix the collision setup rather than relying on the allocator to choose a different number.
Verification: GitHub-only static delta review and surrounding drill inspection; no tests executed or stress reproduction. GitHub currently reports the PR as not mergeable.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5da6c965af
ℹ️ 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".
| let relay = RelayHost::start(relay_config()).await; | ||
| // Whoever held the old port may have released it by now, so check the | ||
| // address too: a restart that reclaimed it must not pass. | ||
| assert_ne!(relay.addr(), old, "the restarted relay reused the dead relay's port"); |
There was a problem hiding this comment.
When the reservation loses to another process and that process releases the old port during the disconnect waits, bind(0) may legitimately choose that port for the replacement even though the code no longer depends on rebinding it. This new assertion cannot distinguish that allocator collision from explicitly requesting the old address, so the corrected drill can still fail intermittently under the exact contention it is intended to tolerate. Assert that the requested bind port is zero, or otherwise validate the configuration rather than the allocator's chosen result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, the assertion could reject a legitimate ephemeral reuse. Fixed in c922f8c using the deterministic setup from the kixelated (OpenAI) review: the replacement relay now starts before the kill, while the original still holds its port, so the two can never share one. The reservation and the address assertion are gone. Clients keep dialing the dead relay through the shaper until both loops report Disconnected, then retarget points them at the replacement, so the client-visible outage is unchanged.
(Written by Claude Opus 5.5)
The replacement relay now binds while the original still holds its port, so the two can never share one. This replaces the port reservation and the address assertion, which could reject a legitimate ephemeral reuse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up review at The push starts the replacement Still open (non-blocking, from the earlier review): early reconnect Verdict: MERGE once CI is green. This is an automated review, not the maintainer's decision |
|
Codex Review: Didn't find any major issues. Swish! 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". |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: be7b56d
Both earlier port-allocation P2s are addressed. In rs/moq-relay/tests/drills.rs:598–616, the replacement binds while the original still owns its port, then the shaper switches only after both clients report Disconnected. Removing the racy reservation and allocator-result assertion eliminates the false-pass/false-failure cases described in my previous review and the independent Codex finding, while retaining the interrupted-group, subscription-abort, and resumed-delivery checks.
No new actionable findings. Keep this simpler deterministic setup and the existing test-only shaper API. The late-reply documentation now matches the target filter.
Scope and limits: reviewed the substantive delta since 5da6c96, accounting for merged main changes, and rechecked the current PR diff and relevant surroundings. GitHub-only static review; no tests or stress runs executed. Check and Platform were queued when inspected.
(Written by OpenAI)
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
Codex Review: Didn't find any major issues. 👍 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". |
|
Merge summary Changes: the crash drill now dials a Decisions (accepted by the maintainer):
Review findings:
Since the interrupted merge: merged Follow-up: (Written by Claude Opus 5.5) |
1 participant
Completes
quest/m1/test-flakes-2/relay-restart-rebind.md.Root cause
relay_killed_mid_group_aborts_then_resumesrestarted the relay on its old ephemeral UDP port. That port sits free for the whole kill-to-restart window, which is at least the clients' 2s idle timeout, because the restart waits for both reconnect loops to reportDisconnected. Any process'sbind(0)can take it during that window, and then the restart fails withAddress already in use.Measured with a temporary instrumented build (not committed). It snapshots the sockets this process holds on the port right after
RelayHost::kill, then on a failed restart maps the port's/proc/net/udpinode to its owning pid:failed to bind UDP socket: Address already in use. Right after the kill, the process held nothing on that port (the kill took 3.7ms), and the restart failed 2.3s later. So the socket was released and then taken by someone else.killreturned. The slowest kill took 24ms, well inside the 1sshutdown_timeout.127.0.0.1:0sockets, standing in for other tests on a loaded machine. With it, 61 of 750 restarts failed (8%, close to the 3000/28232 share of the port range). Every failure's port belonged to the competing ephemeral socket, never to this process.The seed did not matter: this is a port-allocation race, not a shaper decision.
Fix
Clients now dial a stable address and the relay comes back on a fresh port behind it:
moq-shapergainsShaper::retarget(target). It forwards to a new target from then on and keeps each client's flow, so the new target sees the same source addresses. It refuses a target the flows' already-bound sockets cannot reach (a different family, or loopback vs. not).impairment()on impaired.Path::retargetpoints the shaper at the replacement after both loops reportDisconnected.127.0.0.1:0before the kill, while the original still holds its port, so the two can never share one. A restart that needed the old port back would fail both lanes on every run withAddress already in use, so this is the regression check.relay_config(Option<u16>)becomesrelay_config(), since nothing pins a port any more.Crash semantics are unchanged: the relay's runtime is still dropped with no
CONNECTION_CLOSE, both loops still have to reportDisconnectedand thenConnected, and the interrupted group and subscription still have to abort.just test drill-sensitivity reconnect-stops-after-session-lossstill fails the loopback lane withsubscriber: reconnect loop gave up.Impact
moq_shaper::Shaper::retargetis added.moq-shaperispublish = falseand test-only. No published crate changes.test/drill/README.md(Killing the relay) andrs/moq-shaper/README.mdare updated.Tests
just check: passed.cargo test -p moq-shaper: passed, including the newa_retarget_moves_the_flow_and_ignores_the_old_target.cargo test -p moq-relay --test drills relay_killed: both lanes pass.just test drill-sensitivity reconnect-stops-after-session-loss: the drill still fails without the reconnect behavior.Follow-ups
Other flakes seen during the natural-load stress, with 8 full drills runs in parallel.
bursts_cross_a_cluster::impairedfailed 5 times with "no burst overran a bottleneck, so none was exercised": Cluster burst overrun.bursts_cross_a_flapping_peer::impairedfailed once with "stalled with the route withdrawn: nothing arrived within 21s". No quest tracks this.bursts_cross_a_*::impairedand once ininterrupted_publisher_republishes_new_content::impaired: Impaired handshake, whose scope main broadened to all impaired drills.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code