Repository navigation
Conversation
Quest-Claim: 56cec0b9a8b9ec20
ROUTE_START and ROUTE_UPDATE carry no RTT, the draining flag moves to 0x2, and RouteInfo::rtt is gone from Rust, JS, and the bindings, with the per-link min-RTT sampler in both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e quest 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 30 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. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two small leftovers from the edit, inline.
Reviewed changes
I reviewed the full removal of lite-07's Route Offer RTT across the draft, Rust, JS, the FFI bindings, docs, and tests. I also checked that no readers remain, and that the flag renumbering is safe for relays on mixed versions (release has no Route Offer at all, so moving draining to 0x2 costs nothing).
- Wire and draft: the Route Offer drops
[RTT (i)], and the flags become 0x1 down-only and 0x2 draining. Any other bit is refused. The JS test refuses 0x4 and 0x8, and the Rust test refuses 0x8 and 0x86. - Rust
moq-net: removesLinkRtt,Router::rtt,Stream::rtt, theRoutedsampler and its timer,Offer/Served/RouteInfo::rtt, and the RTT slot inRouteMeta. - JS
@moq/net: removeslink_rtt.tsand thegetStats()sampler.startRouteis now synchronous, andrttis gone fromOffer,RouteInfo,WireRoute, androutesEqual. - Bindings:
MoqRouteInfo.rttis removed. Dart is regenerated, and the dropped blank lines between imports are harmless becausecheck.shdiffs with-B. The Go, Kotlin, Swift, and Dart alias docs now nameavailable. - Tests, bench, and corpus: the RTT tests, the
routes/path/drainedbench, andPathRouteDrain::idleare gone. Thelite.jsoncorpus is re-encoded for the new offer layout.
claude-opus-5-5 ｜ 𝕏
|
|
||
| /// Encode a Route Offer: the path, then flags, priority, metric, and RTT in | ||
| /// milliseconds. No node offers an empty path or one past the hop limit, nor names Node | ||
| /// Encode a Route Offer: the path, then flags, priority, and metric. No node offers an empty path or one past the hop limit, nor names Node |
There was a problem hiding this comment.
The edit merged two doc lines into one that runs about 140 columns, past the 120-column max_width. rustfmt doesn't wrap comments, so nothing will fix it automatically. Reflow it to match the surrounding docs.
| /// Encode a Route Offer: the path, then flags, priority, and metric. No node offers an empty path or one past the hop limit, nor names Node | |
| /// Encode a Route Offer: the path, then flags, priority, and metric. No node offers an | |
| /// empty path or one past the hop limit, nor names Node |
| ); | ||
| let before = router.work(); | ||
| router.rtt(idle, Some(Duration::from_millis(5))); | ||
| router.disconnect([idle], Instant::ZERO); |
There was a problem hiding this comment.
The third measurement now disconnects the idle session, but the test's doc (L303, "any later update that changes no route") and its assertion message (L386, "an idle update: {costs:?}") still describe the RTT update this replaced. Update both so a failure names what was actually measured, e.g. "closing an idle session: {costs:?}".
Finishes quest/m2/route-rtt.md: lite-07's ROUTE_START and ROUTE_UPDATE no longer carry an RTT, and
RouteInfo::rttis gone from Rust, JS, and every binding, along with the per-link min-RTT samplers that fed it.Summary
drafts/draft-lcurley-moq-lite.md): the Route Offer loses[RTT (i)], the routing section loses its RTT paragraph, ROUTE_UPDATE no longer lists RTT among its triggers, and the changelog entry for the RTT flag is gone. The draining flag moves from 0x4 to 0x2, so the defined bits stay contiguous (0x1 down-only, 0x2 draining).moq-net): removesroute::LinkRtt(route/rtt.rs) and its tests,path::Router::rtt,route::node::Stream::rtt,Offer::rtt,Served::rtt, and theRoutedsampler: its timer,sample, andread_link. Also removesRouteInfo::rttandwith_rtt, and drops the RTT from the origin's route metadata tuple. Encode and decode lose the field and the RTT flag.@moq/net): removeslink_rtt.ts, thegetStats()sampler inlite/subscriber.ts(which polled once a second while a route existed),Offer.rtt,RouteInfo.rtt, andWireRoute.rtt.startRouteno longer waits on a first link read, so it is now synchronous.MoqRouteInfo.rttis removed frommoq-ffi, and the Dart bindings are regenerated. The RouteInfo doc comments in Go, Swift, Kotlin, and Dart droprtt. They also drop the stale "relay hop ids", whichMoqRouteInfonever carried, and now nameavailable.doc/bin/relay/cluster.mdand the RTT mention in the verify-route skill.moq-tokiointegration suite. It also drops theroutes/path/drainedbench andfuzz::PathRouteDrain::idle, because their only operation was an RTT change on an idle session, and no periodic idle update remains.drain_news_is_linear_in_neighboursnow measures closing an idle session after the drain where it used to measure an RTT change. The sharedfixtures/lite.jsoncorpus drops its RTT cases and moves its draining case to 0x2.Impact
Public API (breaking):
moq_net::origin::RouteInfo::rtt()is removed.RouteInfo.rttis removed from@moq/net.MoqRouteInfo.rttis removed (C, C++, Go, Python, Kotlin, Swift, Dart).Migration: nothing ranked by this value. An application that wants a round trip should read its own session's
stats().rtt. A path-wide sum is not available any more.Wire:
moq-lite-07-wiponly, which is unpublished and opt-in. The Route Offer loses its RTT field, and the draining flag moves from 0x4 to 0x2. The released lite-07-wip has no Route Offer at all, so relays on the previous release are not affected. lite-06 and earlier are unchanged.Decisions
Choices made without the maintainer (the user is away), each easy to undo in review:
routes/path/drainedbench is deleted instead of being repointed. Its only operation was the removed RTT update. Once that is gone, no recurring "update that changes no route" is left to measure. In the matching scope unit test, the third measurement now closes the idle session.Caused by
Validation
just checkover the branch: everything passes except one Python test,test_socket.py::test_abort_and_endpoint_close_are_errors. That test flakes about 1 run in 3 with the same failure at line 90, and it runs only on a local origin with no lite-07 session, so it never touches this change. It is filed as a follow-up below.rs/moq-net/fixtures/lite.jsoncorpus is replayed byte for byte by both the Rust (lite::corpus) and JS (corpus.test.ts) decoders, so both agree on the new Route Offer encoding. Separately, the Rust and JS unit tests pin the exact Route Offer bytes, including draining under 0x2.just test interop --allpasses across Rust, JS (node and bun), and the Python sockets, overmoq-lite-07-wipand the older versions.@moq/net: 3331 tests pass.Follow-ups
just check, added to the open test-flakes epic. It has no open decisions.Models
Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort (arm A)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.Head: b27db51
Base: origin/main at ca88fc1
Risk: high,
Changes to the wire format and public bindings could break interoperability or downstream consumers, and reverting cannot undo versions already adopted externally.Classified: medium by default
Reviewer: Codex, gpt-6.1-sol, medium effort
Session:
01a1df9e-743a-7a53-8a57-f36d9440d16f, round 1Score: 9/10,
RTT removal is consistent across Rust, JS, bindings, and the draft wire format, with no actionable regression found in the source-only review; builds and tests were not run.Raised the risk to high:
Changes to the wire format and public bindings could break interoperability or downstream consumers, and reverting cannot undo versions already adopted externally.Rounds: 1; first round 0; findings 0: fixed 0, refuted 0, open 0
Findings: none