feat(moq-transport): add draft-19 stream core#41
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-16780 |
1 similar comment
|
🔗 Paperclip issue: BLO-16780 |
|
@ally Please review the draft-19 wire and state-machine boundaries, especially non-minimal vi64 decoding, SETUP option delta/parity handling, paired unidirectional control-stream closure, and SUBSCRIBE/PUBLISH FIN + RESET_STREAM/STOP_SENDING transitions. Confirm that the explicit profile cannot alter the pinned |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (Codex/OpenCode nested CLI unavailable in this k8s Job pod; adversarial pass applied directly against /tmp/pr.diff).
reviewed head: 688f731
This is a self-review (PR author resolves to my own App identity, app/allyblockcast); running the full pipeline per policy and posting via my distinct reviewer identity.
Important Issues (1)
- [native-codex]
moq-transport/src/session/draft19.rs:187-191—Draft19RequestStream::receive_first_responsepanics instead of erroring on a reachable state:if self.receiver.done().await? { self.lifecycle.receive_fin()?; unreachable!("premature FIN must fail") }
receive_fin()only errors whilereceive_required_before_finis stilltrue, which holds on the first call. After a successful first response for a non-Subscribe kind (Fetch/TrackStatus/PublishNamespace/SubscribeNamespace/SubscribeTracks), that flag is cleared tofalse. A second call to this method after the stream is done then makesreceive_fin()succeed, and execution falls through tounreachable!(), which panics. The siblingreceive_message()(lines ~226-236) handles the identical scenario correctly by returningOk(None)via?/early-return instead of assuming an invariant — this method should follow the same pattern (propagateStreamProtocolError::PrematureFinexplicitly rather than relying onunreachable!()). No test covers this async wrapper's edge case (only the underlyingRequestStreamstate machine is tested directly indraft19_wire.rs), so the gap wasn't caught by CI.
Suggestions (2)
- [pr-review-toolkit]
moq-transport/src/profile/draft19.rs:366-372—RequestKind::success_responsemapsPublish,TrackStatus,PublishNamespace,SubscribeNamespace, andSubscribeTracksall to the same response code0x07, while onlySubscribe(0x04) andFetch(0x18) get distinct codes. Worth double-checking this against the draft-19 message-type registry — if these five request kinds actually have distinct "Ok" response types, first-response validation would silently accept the wrong response for four of them. - [gstack/review]
moq-transport/src/session/draft19.rs—protocol_erroris duplicated verbatim on bothDraft19Session(lines ~55-60) andDraft19RequestStream(lines ~168-173). Could be a shared free function taking&web_transport::Sessionto avoid the copy.
Strengths
Vi64encode/decode is correct and thoroughly verified against literal test vectors, including boundary lengths (1, 2, 3, 8, 9-byte forms) and non-minimal encodings.- Legacy
moqt-16-v1fixture isolation is well done: pinned FNV-1a digest guards against silent fixture drift, and the profile-crossed negative test confirms rejection without consuming bytes. RequestStream's state machine (FIN/reset/STOP_SENDING transitions) has solid direct unit coverage across Subscribe/Publish/Fetch/TrackStatus request families.
Recommended Action
- Fix the
unreachable!()panic path inreceive_first_responsebefore this API is wired into higher-level session code. - Verify the
success_responsecode assignments against the draft-19 message-type registry. - Suggestion on
protocol_errorduplication is opportunistic — take or leave.
Summary
WireProfile::Draft19transport core0x2f00), length-bounded setup options, and generic vi64 message framingmoqt-16-v1fixture digestVerification
CARGO_HOME=/runtime-cache/tmp/opencode/moq-rs-cargo-home cargo test -p moq-transportmoqt_16_wire_vectors_are_byte_exact_and_round_trippassedCARGO_HOME=/runtime-cache/tmp/opencode/moq-rs-cargo-home cargo clippy -p moq-transport --all-targets -- -D warningscargo fmt --all -- --checkScope
This intentionally does not add
moqt-19to native QUIC ALPN or WebTransport protocol advertisement. Exact negotiation is tracked separately after this framing and stream foundation lands.Paperclip: https://paperclip.blockcast.net/BLO/issues/BLO-16780