Repository navigation
test(relay): forged-dial test waits for bob's announce - #8254
Conversation
Quest-Claim: 132349b968f1d0f3
…ing it Mallory's request could sync ahead of bob's server endpoint announce and report Unroutable. Wait for the hidden announce to reach mallory's origin, and do the same for the client-grant test's carried() helper, which shares the race. Un-ignore the test and drop its allowlist entry. 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 5 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
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 08a774a.
No actionable findings. The hidden-route announcement watch establishes the missing ordering before requesting Bob's endpoint, and replays an already-present route, so it does not miss an early announcement. Applying the same helper to carried() addresses the sibling request race. The forged-accept and unauthorized-read assertions remain intact, and removing the quarantine and its inventory allowance accompanies the causal fix.
Direction: await the endpoint's observable arrival rather than delaying the request by a timing budget. The timeout remains a bound on real network progress, not the proof that discovery has completed.
Verification limits: GitHub-only static review of the complete diff, surrounding socket-auth tests and origin announcement API. No tests, load/stress runs or relay reproduction 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 critical issues — one stale epic note this PR leaves behind.
Reviewed changes
- Announce wait helper —
announced()awaits aPresentevent on the origin's announced listing (hidden routes included), so the request cannot sync ahead of the announcement. - Forged-dial test — waits for mallory's origin to learn
.server/bobbefore the forged request, and drops its#[ignore]. - Client-grant test —
carried()now waits for the endpoint's announce before requesting it from bob's origin, covering the same race. - Quarantine cleanup — removes the
sh/gh/test-inventory.allowentry, deletes the fixed child quest, and drops its## Requiredline.
The ordering fix is sound. The origin's default scope head is "", so .server/bob is hidden and with_hidden(true) is needed; lite-07 already carries hidden routes without a per-session opt-in. routes() replays the current routes as its initial events, so an announce that landed before the helper subscribes is still observed, and a Present in the table guarantees the subsequent request finds a covering route instead of concluding Unroutable.
ℹ️ Epic README still records the test as quarantined
quest/m0/test-flakes-2/README.md:191-195 says socket_auth::a_forged_dial_never_connects "is quarantined with #[ignore] until its child fixes it", but this PR deletes that child and removes the #[ignore], so the epic now asserts a quarantine that no longer exists. The sibling fix for embed::worker_tokio_custom_route_and_quic rewrote its "Added" entry into a cause paragraph ending "Fixed by #8236", which is the convention the other resolved flakes in this section follow.
Technical details
# Stale quarantine record in the epic README
## Affected sites
- `quest/m0/test-flakes-2/README.md:191-195` — the "Added ... /quest-complete"
entry for `socket_auth::a_forged_dial_never_connects` ends with "It is
quarantined with `#[ignore]` until its child fixes it." after the child is
deleted.
## Required outcome
- The entry stays as history but reflects that the test is fixed and re-enabled,
matching how the same section records other resolved flakes (e.g. the
`embed::worker_tokio_custom_route_and_quic` entry rewritten by #8236, or the
"re-enabled in #8110" note above it).
## Suggested approach
Replace the final sentence, e.g.:
mallory's request for bob's endpoint synced before his announce arrived.
The request now waits for `.server/bob` to reach mallory's origin, so it
cannot conclude absence first, and `carried()` waits the same way. Fixed by
[moq-dev/moq#8254](https://github.com/moq-dev/moq/pull/8254).deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
…2/forged-dial-announce
…2/forged-dial-announce
There was a problem hiding this comment.
ℹ️ No new issues in the delta. One carried-over doc nit from the prior review still stands.
Reviewed changes
- Base sync — merged
origin/maintwice (7e7519cc5,0cb1f86c6); the PR's own diff is unchanged from the previous review, and the resolution preserved main's new## Requiredentries and thetest-inventory.allowgrouping comment. - No behavior change —
git diff origin/main..HEADon the touched files is exactly the intended four-file delta (23 additions, 35 deletions), andsocket_auth.rsis byte-identical to the version reviewed at08a774ae3.
ℹ️ Epic README still records the test as quarantined
quest/m0/test-flakes-2/README.md:192-196 still says socket_auth::a_forged_dial_never_connects "is quarantined with #[ignore] until its child fixes it", but this PR deletes that child quest and removes the #[ignore]. The merges of main did not touch that paragraph, so the epic now asserts a quarantine that no longer exists.
Technical details
# Stale quarantine record in the epic README
## Affected sites
- `quest/m0/test-flakes-2/README.md:192-196` — the "Added ... /quest-complete"
entry for `socket_auth::a_forged_dial_never_connects` ends with "It is
quarantined with `#[ignore]` until its child fixes it." after the child is
deleted and the attribute is gone.
## Required outcome
- The entry stays as history but reflects that the test is fixed and re-enabled,
matching how the same section records other resolved flakes (e.g. the
`embed::worker_tokio_custom_route_and_quic` entry rewritten by #8236, or the
"re-enabled in #8110" note above it).
## Suggested approach
Replace the final sentence, e.g.:
mallory's request for bob's endpoint synced before his announce arrived.
The request now waits for `.server/bob` to reach mallory's origin, so it
cannot conclude absence first, and `carried()` waits the same way. Fixed by
[moq-dev/moq#8254](https://github.com/moq-dev/moq/pull/8254).deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Summary
Fixes the
socket_auth::a_forged_dial_never_connectsflake (bob's endpoint: Unroutable, run 42430262086). Mallory's request could sync before bob's.server/bobannounce crossed the relay. The test now waits for that announce to reach mallory's origin before it requests the endpoint. It uses awith_hidden(true)announced listing, since.server/...is a hidden route.announced(origin, id)helper inrs/moq-relay/tests/integration/socket_auth.rswaits on an awaited announce event, not a sleep.a_client_grant_neither_serves_nor_reads_clients'scarried()had the same race (bob requests.client/aliceright after alice dials), so it now waits on the announce first. The other tests in the file request only from sessions that connect after the endpoints exist, so they don't share the race.#[ignore], thesh/gh/test-inventory.allowentry, and the quest file and its link.The diff touches only these two tests and a new helper, so it should merge cleanly with moq-dev/moq#8205.
Verification
just checkpasses.--test-threads=8, plus 60 more with the CPU saturated: 0 failures.Choices made without asking
carried()in the client-grant test, since it shares the race.Follow-ups
None.
Models
🤖 Generated with Claude Code
(Written by Claude Opus 5.5 with low)
Review
Rewritten by
quest reviewafter each round.01a1bc6f-88ae-7af2-b0e0-c0d7d6a15905, round 1The tests now await endpoint announcements before requesting them, addressing the discovery race without changing production behavior; source review found no actionable defects, though tests were not run as instructed.