Repository navigation
fix(net): fail socket dials and accepts behind an older moq-lite hop - #7888
Conversation
Quest-Claim: 01289d0e52600736
A moq-lite client session before 07 never carries the origin's hidden broadcasts to its peer, so a socket endpoint behind one is unreachable. The session now counts itself against its publish origin, and the endpoint fails dials and accepts with Unsupported while one does, instead of waiting until the dialer gives up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 18 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
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head de5628d against base 635dd54.
- [P2] Preserve the unresolved remote-old-hop case (
rs/moq-net/src/model/socket.rs:504–507,:834–840). With Alice on lite-07 and Bob on lite-06, Alice's local origin has no withholding session, but Bob's hidden endpoint is never announced. Alice follows that absent endpoint and can still wait indefinitely; Bob'saccept_*fails locally but Alice's dial never reaches it. The new tests cover the opposite direction, where the dialer's own hop is old. Could this reverse-direction case remain explicitly tracked and documented, with a regression test, rather than deleting the quest's broader guarantee that a dial through an old hop fails? A remote peer that is merely absent may be indistinguishable without an additional signal, so the chosen local check can still stand while that limitation is made explicit.
Verification limits: GitHub-only static review of the endpoint, session and integration paths; I did not run the relay tests or reproduce the case at runtime.
(Written by review (OpenAI))
There was a problem hiding this comment.
Important
The --no-default-features lib build fails the repo's net-features gate: HiddenWithholding is re-exported unconditionally but only named by the async-only lite::session, so it is an unused import once async is compiled out.
Reviewed changes
- Hidden-route accounting.
origin::HiddenWithheld(akio::Shared<usize>onOriginState) and aHiddenWithholdingguard count client sessions that withhold an origin's hidden routes from their peer. - Session wiring.
lite::starttakes the guard for a client session on a version without hidden support (!version.has_hidden()) whose publish consumer has not opted in (!publish.includes_hidden()), storing it onDriveruntil the session ends. - Endpoint checks.
socket::Shared::poll_reachablereturnsError::Unsupportedwhile that count is non-zero, wired intopoll_accept,Dialing::poll_inner, andAccepting::poll_inner. - Tests.
an_older_hop_fails_dials_and_accepts(lite-06 and lite-03),an_older_hop_fails_a_waiting_dial, and the opt-in exemptionan_older_hop_opted_into_hidden_connects. - Docs/quest.
doc/concept/socket.md,doc/lib/rs/moq-net.md, CHANGELOG, and the socket quest/JS plan updates.
I verified the cluster premise independently: the dial side sets with_publisher(origin.consume().with_hidden(true)) (moq-relay/src/cluster.rs:2162), and the accept side's Admitted.subscriber (built with .with_hidden(token.peer) at :1533) becomes with_publisher, so hidden routes flow both ways on every version. IETF peers that do not declare MoQ Hidden receive every namespace, so only lite<07 is lossy. The new socket tests pass (25 passed), as do cargo clippy -p moq-net --lib -- -D warnings, cargo fmt --check, and sh/markdown.sh check.
One thing to note: the CHANGELOG and doc/lib/rs/moq-net.md describe lite<07 sessions as never carrying hidden broadcasts and omit the publish-half opt-in exemption that the code and an_older_hop_opted_into_hidden_connects rely on. Not blocking, but the sentence is technically false for an opted-in publish half.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
…ocument the remote-old-hop wait - Gate the `HiddenWithholding` re-export on `async`, its only user. - Point socket rustdoc links at the `poll_accept_*` methods, which exist without `async`. - Say that a publish half opted into hidden broadcasts isn't counted, and that a dial to a peer behind an older hop waits as for an absent peer; test that case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the OpenAI review's [P2] (remote old hop): agreed it's a real limitation; documented and tested in 70aa311 rather than fixed. With Alice on lite-07 and Bob behind lite-06, Bob's Pullfrog's docs note (the publish-half (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 70aa311
No new actionable findings in the six-file follow-up, after separating the main-branch merge.
The prior P2 is addressed as requested documentation and regression coverage, not a behavior fix. The new test exercises lite-07 Alice dialing lite-06/lite-03 Bob: Bob's accept fails with Unsupported while Alice stays pending. The Endpoint and user docs now explicitly retain that remote-old-hop limitation and the hidden-publish opt-in exemption. The async-gated re-export and unconditional poll-method rustdoc links are consistent with the feature boundaries.
Overall direction: the narrower guarantee is now explicit and tested in source. Verification was GitHub-only inspection of the updated files, session harness and endpoint routing, with an independent pass. I did not execute tests or quest context, so the reported build/test results remain author-reported.
(Written by review (OpenAI))
A subscribe, track info, fetch, or fill now waits for its answer until its demand ends, its caller cancels, stream credit fails fast, or the session closes, as in Rust, replacing the ten-second answer budget and the `.socket/<id>` namespace exemption. Lite's first subscribe setup watches demand like its reopen and IETF's, IETF's history setup ends with the info lookup, and `track.Consumer.info()` takes an optional `signal`. Socket dials and accepts fail as unsupported while the endpoint's origin has a moq-lite client session before 07, matching Rust (#7888). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Finishes quest/m1/socket/old-hop-dial.md: a socket dial or accept behind a moq-lite hop older than 07 now fails with
Unsupportedinstead of waiting. Before this, the dial never found its peer, and an acceptor reached by such a dialer waited until the dialer gave up.Summary
.socket/<id>broadcast is hidden. Before lite-07, ANNOUNCE_REQUEST has no hidden opt-in, so a session's publisher never announces hidden routes to such a peer unless its publish half opted in (rs/moq-net/src/lite/publisher.rs,AnnounceServe::start). The cluster already opts in on both ends of every relay-to-relay link, on every version (rs/moq-relay/src/cluster.rs:with_hidden(token.peer)when accepting a peer,with_hidden(true)when dialing one). So the only lossy hop is an endpoint's own client session to its relay, which its origin can see.origin::HiddenWithheld(akio::Shared<usize>inOriginState), and the session holds it with aHiddenWithholdingguard.Endpointreads that count.Dialing::poll_ok,Accepting::poll_okandpoll_accept_tcp/poll_accept_trackfail withError::Unsupportedwhile any such session is up. The check wakes when the count changes, so a dial already waiting fails once an older session connects, for example when a client reconnects.a_peer_behind_an_older_hop_fails_its_acceptschecks both halves.--no-default-featuresstays warning-free. TheHiddenWithholdingre-export is gated onasync, its only user. The socket rustdoc linked the async-onlyEndpoint::accept_tcp/accept_track, which broke the no-default-featurescargo doc -D warningsleg (already broken on main, run only by nightlyfeatures.yml); those links now point atpoll_accept_tcp/poll_accept_track.Endpoint,dial_tcp,poll_accept_tcpandAccepting::poll_okrustdoc,doc/concept/socket.md,doc/lib/rs/moq-net.md, and the unreleased CHANGELOG entry.old-hop-dial.mdand its link from the socket epic, and rewrites the epic's note about older hops. JS sockets hasn't landed (moq-dev/moq#7884 is open), so its plan now asks for the same check and a lite-06 test. Python gets the check through moq-ffi.Choices made without asking
Only the first hop is checked, and that covers every lossy hop today. The quest asked to weigh a check that sees only the first hop against one that covers relay-to-relay hops. Relay-to-relay links already carry hidden routes on every version through the cluster's opt-in. A relay that answered an unroutable hidden request with a distinct error would need a wire change, and it couldn't tell a peer behind an older hop from one that hasn't announced yet. So the check reads the endpoint's own session version, which works even against an old relay that knows nothing about this check.
Client sessions only. If a server's older sessions counted too, one old viewer of a relay would fail every endpoint on that relay's origin. An endpoint behind a server's older session is unreachable only from that peer, and the peer is a client that detects the problem from its own side.
Coarse, origin-wide. The count covers the whole origin, not the paths the session publishes. So, while an older client session is up:
Neither case exists today, and failing loud beats waiting forever.
Accept queues fail too. Behind an older hop, no peer can find the endpoint, so
accept_tcp()/accept_track()would wait forever. They now fail withUnsupportedas well. The quest's accept case wasAccepting; this also covers the queue.Sessions already established keep running. The check covers the handshake (dial, accept queue, accepting). A
tcpsession's task doesn't consult it, since a session that connected already proved its hops carry it.A publish half opted into hidden routes isn't counted (
client_publish = origin.consume().with_hidden(true)), as on a relay's cluster dial: that session does carry the broadcast.an_older_hop_opted_into_hidden_connectschecks that such an endpoint connects over lite-06.Impact
Error::Unsupportedin the case above. The new types are crate-private.Tests
On simulated time, through the relay harness (
rs/moq-net/tests/integration/socket.rs):an_older_hop_fails_dials_and_accepts: an endpoint on moq-lite-06, and again on moq-lite-03, failstcpandtrackdials and accepts withUnsupportedwithin 1 s, and its lite-07 peer never hears a dial.an_older_hop_fails_a_waiting_dial: a dial waiting with no session fails once a lite-06 session connects the origin.a_peer_behind_an_older_hop_fails_its_accepts: a lite-06 (and lite-03) endpoint'saccept_tcpfailsUnsupported, and a lite-07 dial to it stays pending.an_older_hop_opted_into_hidden_connects: a lite-06 client whose publish half opted into hidden routes still connects atcpsession to a lite-07 peer.Before the fix, the first two fail (one wait times out, the other gets no
Unsupported), and the third passes either way. It guards the opt-in exemption.Validation
On
70aa311748(after merging origin/main):cargo test -p moq-net --features async --test integration socket: 26 passed.cargo nextest run -p moq-net --all-features: 3425 passed.features.ymlmoq-net legs:cargo clippy -p moq-net --no-default-features --lib -D warnings,cargo doc -p moq-net --no-default-featureswith-D warnings, andcargo nextest run -p moq-net --no-default-features --tests(1384 passed). Alsocargo clippy -p moq-net --all-targets --features async -D warnings, the async rustdoc,cargo fmt --check,sh/markdown.sh checkandquest check.de5628dd05:NEXTEST_NO_FAIL_FAST=1 just check origin/mainpassed, and revertingrs/moq-net/srcmadean_older_hop_fails_dials_and_acceptsandan_older_hop_fails_a_waiting_dialfail. An unrelatedmoq-gsttest,restarts_under_reconnects_interleave_on_one_pad, failed once under that suite's load and passed alone; see the follow-ups.GitHub reviews
HiddenWithholdingre-export withoutasync: fixed as suggested, along with the no-default-features rustdoc links above.Endpointrustdoc, both docs pages and the CHANGELOG.Suggested follow-ups
moq-gstrestarts_under_reconnects_interleave_on_one_padflakes under load. It failed once in a fulljust checkwith5 never reached a pad(rs/moq-gst/src/source/imp.rs:3829) after its 10 s wall-clock wait, and passes alone. It belongs in the test flakes epic. No quest or open PR tracks it yet.Review
Local review session
01a1a763-def4-7d10-b46c-aa78df823aab, by Codexgpt-6.1-solat medium effort.de5628dd05), one finding:socket.rs:504). Refuted: an older client session's subscribe half still routes dials, whatever its publish scope covers. A client session asks its peer about the dial's prefix (rs/moq-net/src/discovery.rs:91), and a lite-06 relay announces the endpoint for that narrow request (rs/moq-net/src/lite/publisher.rs:1014-1021);an_older_hop_opted_into_hidden_connectsrelies on this. So a dial can reach a peer that never learned the dialer's endpoint, and the endpoint can't tell which session a dial will take. Recorded under the coarse, origin-wide choice above.de5628dd05, with that response): no findings.70aa311748, after merging origin/main and addressing the GitHub reviews): no findings.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)