Repository navigation
test(relay): the embed socket check matches each port's own protocol - #8236
Conversation
Quest-Claim: e74eada298d59aa4
`bound` collected every UDP socket and TCP listener on a port number, so the test's own QUIC client, still draining after `drop(subscriber)`, counted as the relay's whenever its UDP port matched the HTTP listener's TCP port. UDP and TCP number ports separately: match UDP on the QUIC port and TCP listeners on the HTTP port. Re-enables `embed::worker_tokio_custom_route_and_quic`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
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 4 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. |
…2/embed-sockets-outlived # Conflicts: # sh/gh/test-inventory.allow
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed e452261.
No actionable findings. Filtering by socket type as well as port fixes the false ownership match: the HTTP listener's TCP port must not collect a draining QUIC client's UDP socket with the same number. The shared helper still captures every matching QUIC socket and only TCP listeners, then checks those original inodes after run returns. This preserves the shutdown assertion across shared Tokio, worker Tokio and io_uring rather than adding a delay or changing runtime teardown.
Direction: correcting the ownership check and restoring the quarantined test is appropriate. The forced-collision and stress results in the PR remain author-provided evidence.
Verification limits: GitHub-only static review of the four-file diff and complete embedding test, including descriptor duplication, protocol selection and shutdown assertions. No tests, stress reproduction or runtime teardown validation performed. Rechecked open/non-draft state, full head and published reviews immediately before this COMMENT.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
boundnow takes a socket type —rs/moq-relay/tests/integration/embed.rsmatches DGRAM sockets only for the QUIC port and STREAM listeners only for the HTTP port, so a still-draining test client's UDP socket can no longer be counted as the relay's when its ephemeral port coincides with the HTTP listener's TCP port. Keepingis_listener()also preserves the rule that accepted TCP connections are not the owner's.- Quarantine removed — the
#[ignore]is gone fromworker_tokio_custom_route_and_quic, so the Linux CI lane exercises it again. Capturingownedafter the session still catches worker-modeSO_REUSEPORTsockets, which are bound lazily at serve time. - Inventory and quest tree cleaned up — the
sh/gh/test-inventory.allowentry and its now-empty header are removed (the script errors once an allow-listed test is no longer ignored, and it validates the quest path), the completed quest file is deleted, its## Requiredbullet is dropped, and the epic README records the diagnosis with a link to this PR. No remaining references to the deleted file were found.
The branch advanced to ce14d50 (a merge of origin/main 544f144) during this run; comparing that head against the new base shows the same four files and the same line counts, so this review covers it. CI Check, the only lane that runs this test, was still in progress on that head, so the re-enabled test had not yet completed a CI run.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Only
ce14d50landed since the prior pullfrog review — a merge oforigin/main(544f1441), not a code change. Compared against the new base it touches the same four files with the same +14/−57, and the test binary (rs/moq-relay/tests/integration/embed.rs) is byte-identical to the reviewed head. - The hand-resolved docs kept main's additions — the merge preserved main's new
moq-gststale-completion entries insh/gh/test-inventory.allowand the epic README while still removing the embed entry, its "Flaky" header, the## Requiredbullet, and the completed quest file.
The review of the actual fix stands unchanged.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Finishes quest/m0/test-flakes-2/embed-sockets-outlived.md.
Summary
The relay doesn't leak a socket. The test's ownership check was wrong.
bound(port)inrs/moq-relay/tests/integration/embed.rscollected every UDP socket and every TCP listener on a port number, for both the QUIC port and the HTTP port. UDP and TCP number their ports separately. So the test's own QUIC client counted as "owned by the relay" whenever its ephemeral UDP port happened to equal the relay's ephemeral TCP HTTP port. That client is still draining afterdrop(subscriber), so it is still open whenrunreturns, and the check fails with "the owner's sockets outlivedrun". The chance is about 1 in 28k per run (the size of the ephemeral range), which fits a single CI hit with no local repro.The fix:
bound(kind, port)matches UDP sockets only on the QUIC port and TCP listeners only on the HTTP port. The#[ignore]and itssh/gh/test-inventory.allowentry are removed. The allow file's "Flaky" header had no entries left, so it is gone too.Evidence
embed::module, then 1920 single-test runs of the worker variant (32 concurrent processes on a 24-core host at load ~27, like nextest's process-per-test). Zero socket failures.moq_tokio::worker::Group::shutdown, the worker thread's runtime drop,EndpointDriver'sSlot::release, and the shardBound's dup'd fds) found no socket that outlives the join.127.0.0.1:<http port>fails bothshared_tokio_custom_route_and_quicandworker_tokio_custom_route_and_quicevery time, with the exact CI panic. The same forced setup passes all three variants (shared, worker, uring) once the fix is in.RLIMIT_MEMLOCK, whichsh/rs/nextest.shraises in CI, and none of those failures came from the socket check.cargo fmt --all --checkandcargo clippy -p moq-relay --all-targets --all-features -D warningsboth pass.moq-dev/moq#8208 (transport close) doesn't overlap: this was a test-check bug, not a transport-close bug, and the two PRs touch different files.
Choices made without asking
run, because the quest allowed either and the evidence points to the check.socket2::Typerather than splitting into two helpers, which keeps one function.Public API: none. Wire: none.
Models
Size: S
Implementer: Claude Code, claude-opus-5-5, medium effort
(Written by Claude Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1b8e8-885b-7461-a97a-f0ee903b57ef, round 1The protocol-specific filtering fixes the false ownership match while preserving relay socket-closure assertions, and appears sound from source inspection; builds and tests were not run as instructed.Head ce14d50 only merges origin/main, hand-resolving
sh/gh/test-inventory.allowto keep main's new gst quarantine. Its added and removed lines match the reviewed head e452261 exactly.