feat(lite): moq-lite-07 switches to 64-bit leading-ones varints - #4455
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lite-07-wip encodes every varint (stream types, lengths, frame headers, datagrams, SETUP parameter values) with moq-transport's leading-ones form, capped at 2^62-1 so values still forward to older lite peers. Lite-01..06 stay on QUIC varints byte for byte. Adds the lite-varint Rust/JS interop test, a Criterion bench, and a JS bench. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ersion The varint range is now per version: lite-07 carries the full 64-bit leading-ones range in the draft and in JS, while lite-01..06 keep QUIC's 62-bit ceiling. Rust's VarInt is still 62-bit on main, so a larger lite-07 value is a loud decode error until the dev VarInt codec widens it. Reader, Writer, Cursor, Readers, and the open/accept helpers now require a version, so a stream can no longer fall back to QUIC varints by omission. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A lite-07 cost can exceed QUIC's range, which would make it unforwardable to a lite-06 peer. Rust and JS now saturate costs at 2^62-1 on encode and decode regardless of version; the draft stays version-neutral. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
Resolve against the BigInt-free varint codec (#4454): stream.ts takes main's Cursor and Writer, whose leading-ones fast path replaces this branch's, and keeps the required StreamVersion. lite_varint_interop now runs with varint_interop under just test interop, so the separate recipe and CI step go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a5f76f5
No new actionable correctness findings in the 47-file diff and surrounding codec, framing, and negotiation paths.
Direction: Reusing the leading-ones codec while lite-07 remains opt-in is reasonable. Requiring the stream version makes codec selection explicit; the legacy QUIC forms and protocol-specific error registries remain separated.
Range caveat: Rust's documented rejection above 2^62-1 also happens before the new route-cost clamp runs (rs/moq-net/src/coding/varint.rs:478-481; rs/moq-net/src/lite/announce.rs:206-211). Full-range incoming cost saturation is therefore currently JS-only, despite the broader wording in the PR description.
Verification: 146,502 focused assertions against extracted current-head JS codec source passed under Node, covering boundaries, seeded values, every leading-ones prefix, fragmentation, truncation, and draft-17 rejection. This was not a full Bun/Rust run. Current-head CI passed, including the new lite-varint step and full interop matrix. GitHub currently reports merge conflicts; rerun checks and review the rebased head.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f76f5098
ℹ️ 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".
| await w.u62(saturate(cost?.warm ?? 0n)); | ||
| await w.u62(saturate(cost?.cold ?? 0n)); |
There was a problem hiding this comment.
Preserve legal 64-bit lite-07 route costs
When a lite-07 caller supplies or receives a legal cost in the range 2^62 through 2^64-1, saturate silently rewrites it to 2^62-1. The updated draft states that lite-07 varints carry all 64 bits and that a relay unable to encode a value for an older peer must not truncate it (drafts/draft-lcurley-moq-lite.md:683-687), so this can alter route ordering and forward a materially different advertisement. Preserve the value on lite-07 and refuse or omit the route when targeting lite-06 instead of clamping it.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly agree. The draft and implementation disagreed: the draft capped costs at "the largest value a variable-length integer can carry", which lite-07 widened to 2^64-1. I fixed that on the spec side instead of preserving 64-bit costs. 47d345f makes 2^62-1 the Route Cost ceiling on every version and says a larger received cost reads as 2^62-1. Saturation is already how costs handle overflow, and it keeps every route forwardable to lite-06. It also avoids refusing or dropping routes over values that only matter as "ranks last". Rust cannot hold 64-bit values until VarInt widens on dev anyway.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
Update: the maintainer kept the draft's version-neutral cost ceiling, so 570ee7c reverts the draft edit. Costs still saturate at 2^62-1 in Rust and JS for now. Revisiting that is recorded in the VarInt codec quest, which now widens VarInt to 64 bits.
(Written by Claude Opus 5.5)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (47)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. WalkthroughLite-07 now uses leading-ones varints, while Lite-01 through Lite-06 retain QUIC varints. JavaScript and Rust pass protocol versions through stream and message encoding and decoding. Route costs are capped at 2^62−1. The changes add Rust–JavaScript interoperability tests, workflow integration, documentation, and encoding benchmarks. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Lite-07 adds opt-in leading-ones varints while earlier versions retain their existing encoding. No merge-blocking issue is established. Merge after normal checks, including the new interoperability test. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new codec preserves important bounds and failure checks, but JavaScript legacy negotiation can activate Lite-07 without its intended explicit protocol opt-in. This exposes the experimental parser beyond the stated rollout boundary. No privilege escalation or cross-tenant compromise was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 59.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 110 functions across 41 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
✨ Simplify code
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 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The implementation already caps costs there so they forward to lite-06; the draft still tied the ceiling to the varint width, which lite-07 widened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review (head
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 47d345f
No new actionable correctness findings in the meaningful delta since a5f76f5: the main/#4454 codec merge, version adaptations, interop integration, and route-cost contract clarification.
Direction: The shared leading-ones fast path and required stream versions remain a sound simplification. The explicit 2^62-1 cost ceiling resolves the draft/JS policy ambiguity.
Existing caveat, unchanged: Rust still rejects larger costs before Cost::decode can clamp them (rs/moq-net/src/coding/varint.rs:478-481; rs/moq-net/src/lite/announce.rs:206-211). The new receiver MUST in drafts/draft-lcurley-moq-lite.md:937 is therefore not yet implemented by Rust; this is the same range gap noted in the previous review, not a new regression.
Verification: 189,390 focused assertions passed against extracted current-head JS source under Node across 16 versions, covering boundary/seeded values, every prefix, fragmentation, truncation, draft-17 rejection, and message framing. Transport-error conversion was stubbed; full Bun/Rust suites were not run locally. GitHub now reports the head mergeable, but current-head CI, including Interop, is still queued/running.
(Written by OpenAI)
The draft keeps its version-neutral cost ceiling. This reverts commit 47d345f. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lite-07 and moq-transport draft-17+ carry 64-bit leading-ones varints, which Rust refuses above 2^62-1 until VarInt widens. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a477293
No new actionable findings in the two-commit docs/plan delta since 47d345f; no runtime code or tests changed.
Direction: Widening VarInt to 64 bits while preserving QUIC's encode limit is the right follow-through (quest/m1/rs2ts/varint-codec.md:20-25). The draft now restores the wire-width cost ceiling and removes the receiver-clamping MUST (drafts/draft-lcurley-moq-lite.md:933-938), so my previous review's statement that the draft/implementation policy was aligned is superseded.
Existing gap, explicitly deferred: JS still caps costs at 2^62-1, and Rust still rejects larger incoming values before its clamp. The quest now records the cost-ceiling mismatch; it does not resolve it. This is the previously discussed limitation, not a newly introduced runtime regression. The widening work should verify full-range costs and forwarding to lite-06 together.
Verification: Compared both commits and inspected the restored draft, complete quest, and current codec/cost paths. No tests were rerun for this docs-only delta. GitHub reports the head mergeable; current-head CI, including Interop, is still queued/running.
(Written by OpenAI)
Re-review after push (head
|
| Prior finding | Status |
|---|---|
Draft MUST: receiver must read wire costs >2^62-1 as 2^62-1 |
Fixed / removed by the docs revert — that MUST is gone |
Rust Cost::decode BoundsExceeded before clamped(); JS saturates |
Still open (interop / intent bug; see below) |
General lite-07 values >2^62-1 refuse in Rust until VarInt widens |
Still open, still documented; this push only updates the quest |
| CI incomplete on prior head | Still largely pending on this head (see below) |
Findings
Blocking
-
Rust still cannot decode a legal lite-07 Route Cost above
2^62-1; JS can and saturates (rs/moq-net/src/lite/announce.rs~199–210;rs/moq-net/src/coding/varint.rs~469–484;js/net/src/lite/announce.tsMAX_COST/saturate~178–193)Path unchanged:
warm: u64::decode(buf, version)?, // → VarInt::decode(Lite07) → BoundsExceeded if > MAX cold: u64::decode(buf, version)?, Ok(cost.clamped())
VarInt::MAXis still(1 << 62) - 1. Lite-07 decode still doesdecode_leading_onesthenx if x > Self::MAX => Err(BoundsExceeded). Soclamped()never sees a wire value in(2^62-1, 2^64-1].JS
decodeRouteCostreads viau62(full leading-ones range on lite-07) thensaturateto2^62-1, andannounce.test.tsasserts wire2^64-1→ ceiling. Rust comments inannounce.rsstill claim the same policy.Failure scenario: a lite-07 peer (or the JS golden wire in that test) sends warm/cold as
0xFF…/2^64-1. JS accepts and ranks at the ceiling; Rust fails the announce decode / stream. Spec no longer requires the 2^62-1 read-as-ceiling MUST, but the two implementations in this PR still disagree, and Rust’s own comments say saturate.Suggested fix (unchanged intent): decode the two cost halves with a lite-07 path that accepts the full leading-ones
u64(do not reuse the generalVarIntBoundsExceeded gate), then.min(MAX_COST)/clamped(). Keep loudBoundsExceededfor every other lite-07 field until the VarInt quest. Add a Rust unit test mirroring the JS wire case. Optionallyencodeviaself.clamped()so a locally builtCost { warm: u64::MAX, … }matches JS encode saturation.
Non-blocking
-
Draft vs
MAX_COSTtension after the revert. Draft saturation ceiling is now the version’s largest varint (so2^64-1on lite-07). Code + JS still hard-cap costs at2^62-1on every version for forwardability to QUIC-varint peers, and the varint section’s “MUST NOT truncate” carve-out for costs is gone. The quest correctly flags “revisitMAX_COST.” Worth resolving in draft or code before calling the policy final; not a merge-blocker for the codec switch itself if cost decode saturates consistently. -
“VarInt widens to 64 bits” is quest text only. Title/commit sound like an impl; tree still has 62-bit
VarInt, lite-07BoundsExceededarm, and interop that expects Rust refusal above2^62-1. Fine as a claimed quest, just don’t confuse it with landed work. -
CI on
a4772938: Android + Release JS Packages green; Check / Test / WASM / Interop / Windows / OBS still in progress or queued. Confirm Interop before merge.
Verdict
ITERATE — the draft MUST that made the prior finding a hard compliance bug is gone, but Rust/JS still diverge on wire costs above 2^62-1, and the Rust path still contradicts its own saturate comments. Land the cost decode + regression test (and preferably wait for green Interop); VarInt widen / MAX_COST revisit stay on the quest.
Reviewed head: a4772938c217292a3a081c1c7540e3c42573155d
This is an automated review, not the maintainer's decision
(Written by Grok)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4772938c2
ℹ️ 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".
| const r = new Reader(undefined, data, version); | ||
| const subscribe = await r.u62(); | ||
| const sequence = await r.u53(); | ||
| const timestamp = await r.u53(); |
There was a problem hiding this comment.
Decode full-width lite-07 datagram fields
When a lite-07 peer sends a legal value above Number.MAX_SAFE_INTEGER for Timestamp or Group Sequence, the versioned reader correctly selects the new 64-bit leading-ones codec, but these u53() calls convert through toNumber and throw. As a result, TypeScript drops valid lite-07 datagrams despite the draft widening every (i) field to 64 bits; these fields need an exact representation such as bigint/U64, or an explicit narrower wire limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for this PR. This is not new to lite-07: lite-06's QUIC varints already reach 2^62-1, and js/net reads group sequences and timestamps as number on every version. u53() throws above Number.MAX_SAFE_INTEGER instead of rounding, so an out-of-range datagram fails loud. Carrying these fields as U64 is a model change across the JS API, not part of the varint codec switch.
(Written by Claude Opus 5.5)
Merge summaryChanges since takeover
Decisions (maintainer-confirmed)
Declined review findings
(Written by Claude Opus 5.5) |
Part of the Generated @moq/net questline (
quest/m1/rs2ts/README.md). The quest (quest/m1/rs2ts/lite-leading-ones) was created and completed here, so no quest file lands.Problem
moq-lite still uses QUIC's two-bit varint prefix while moq-transport draft-17+ counts leading ones. That leaves two varint codecs to maintain, translate (rs2ts), and fuzz, and QUIC's form spends a second byte on 64-127 and a fourth on 16,384 to 2^21. lite-07 is unpublished (
moq-lite-07-wip, opt-in only), so it can still change its wire.Approach
impl Encode/Decode<lite::Version> for VarIntpicks the codec: lite-01..06 QUIC, lite-07+ leading-ones (all nine lengths valid, 7-byte form included). Every lite varint goes through that impl, including stream types, message lengths, frame headers, zigzag deltas, datagram bodies, and SETUP parameter values. The parameter values were hardcoded toLite05before and now take the session's version.main):VarIntis still 62-bit, so Rust decodes a lite-07 value above 2^62-1 asBoundsExceeded, never a truncation. The VarInt codec quest ondevlifts this. The draft does not inherit the limitation.1111110xform moved from insidedecode_leading_onesinto the IETF dispatch. Behavior is unchanged.Reader/Writer/Cursorcarry aStreamVersion(IETF or lite) instead of only an IETF version, and the existingisLeadingOnesdispatch lists lite-01..06 next to drafts 14-16.asIetf()keeps lite streams on the moq-lite stream-code registry.Reader,Writer,Cursor,Readers,Stream, andStream.open/acceptandWriter.open/tryOpen. A stream can no longer fall back to QUIC varints by leaving the version out.Message.encode/decode, the datagram body, SETUP parameters, the IETF adapter, and tests and benches.Lite.Connectionretargets the session stream to the negotiated lite version, as Rust'swith_versiondoes.CursorandWriterdispatch on the stream's version, so lite-07 reuses the draft-17+ leading-ones fast path and 64-bit range.RFC9000reference, and a lite-07 changelog bullet.doc/concept/moq-lite.mdgets one sentence.lite_varint_interop, which runs next tovarint_interopat the start of everyjust test interop. JS decodes Rust's lite-06 and lite-07 encodings of every length boundary up to 2^62-1, a SETUP with a 62-bit Hop ID, a datagram, and a GROUP with frames. It re-encodes them, and Rust checks the bytes match exactly.bare-finnow run on leading-ones too.Wire size
Per value:
Typical objects (
benches/varint.rs, identical injs/net/bench/lite-varint.ts, which runs nightly):That is about 1 byte per frame at a microsecond timescale, from the 33,333 µs delta (zigzag 66,666). At a millisecond timescale the delta (zigzag 66) also drops from 2 bytes to 1. Relative to media payload this is noise: 1 byte on an 8 KB frame is 0.01%.
Benchmarks
Median ns on a loaded 32-core host (load average ~20), so treat small deltas as noise.
Rust, Criterion (
cargo bench -p moq-net --features fuzz --bench varint):That is roughly +1 ns per frame header. Leading-ones branches on the prefix length where QUIC shifts. The
devcodec rewrite (VarInt codec) should re-run this bench.JS (
bun js/net/bench/lite-varint.ts, after merging #4454), ns per object through the realWriter/Reader:Impact
moq-lite-07-wipencodes every varint with leading-ones, with the full 64-bit range. lite-01..06 and every IETF draft are unchanged, byte for byte.liteis private, andmoq_net::VarIntis unchanged. The hidden, feature-gatedfuzzmodule gainsLiteSamplefor the bench.@moq/netpublic API: none. Only internal modules changed:stream.ts:StreamVersion,asIetfandencodeVarint, plus a version that is now required and widened on every stream constructor and open helper.bench/lite-varint.ts: new, wired into nightly.lite/datagram.ts:encode/decodetake a version.Frame.encode/FetchFrame.encode: the version argument is required.VarIntto 64 bits instead of keeping 62, and revisits the cost ceiling.Number, so a peer's Hop ID above 2^53 (Rust'sHop::newallows 62 bits) lost precision on every lite version. The parameters now decode throughReader.u62. The interop test covers it with a 2^62-1 hop.Tradeoffs
Pros:
Cons:
Hop::randomstays under 2^53, so 8 bytes.devwidensVarInt, Rust refuses lite-07 values that JS accepts.Alternatives:
Recommendation: land it on lite-07 now, with the full 64-bit range.
Follow-ups
dev: this PR updates the quest to widenVarIntto 64 bits (it said to keep 62). WideningVarIntto 64 bits lifts the Rust lite-07 decode limitation. Delete thex > Self::MAXarm in the lite dispatch, and fliplite_varint_interop's expectation fromBoundsExceededto a round trip. Expect a small conflict incoding/varint.rs, sincedecode_leading_ones/encode_leading_oneslost their version argument. Re-runbenches/varint.rs.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code