feat(moq-net): specify the error code registries and split SessionError from StreamError - #2620
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e443ff5b4
ℹ️ 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".
| if let Some((code, _reason)) = err.session_error() { | ||
| return Self::from_code(code); |
There was a problem hiding this comment.
In the moq-wasm adapter, Error::session_error() and Error::stream_error() both return the same code (rs/moq-wasm/src/transport.rs:41-46), regardless of whether the browser reported a stream or session error. Because this new branch runs first, a browser-side stream reset with code 6, 13, or 64 is now misclassified as Unauthorized, NotFound, or App(0) instead of taking the existing stream path and becoming Remote(code). Distinguish the WebTransport error source in the adapter, or use a session-specific decoder only when awaiting Session::closed().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5388a2338c
ℹ️ 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".
|
|
||
| // An auth rejection is terminal however it arrived (a connect failure or a | ||
| // session close): redialing with the same credentials cannot succeed. | ||
| if (cause instanceof RemoteError && cause.code === CloseCode.Unauthorized) { |
There was a problem hiding this comment.
When an unauthorized peer closes while connect() is still exchanging SETUP, as on the IETF ALPNs and compatibility/WebSocket path, there is no Established.closed promise yet to apply fromClose; the setup read instead throws a session-scoped transport error, which fromTransport deliberately leaves unwrapped. Consequently cause here is not a RemoteError, this guard does not fire, and Reload retries until the timeout or forever when retries are unlimited. The added test negotiates lite-06, which returns an Established object before the close, so it does not cover this path. Preserve the session close information at the setup boundary so this guard receives the decoded code. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa95c89299
ℹ️ 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".
| /** The peer rejected the credentials or the requested path. Terminal: retrying with the same credentials fails again. */ | ||
| Unauthorized: 6, |
There was a problem hiding this comment.
On any moqt-* ALPN, the IETF session-error table assigns UNAUTHORIZED to 0x2 and TOO_MANY_REQUESTS to 0x6, while this table is the moq-lite/Rust mapping. Because ietf/connection.ts also passes closes through fromClose, Reload.#retry now treats a standards-compliant TOO_MANY_REQUESTS close as an auth rejection and stops reconnecting, while a real IETF auth rejection is treated as Old and keeps retrying. Keep the close-code namespace protocol-specific before applying terminal retry policy. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L28-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ecce9b87f
ℹ️ 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".
| Both spaces are shared with {{moqt}} below 1024. | ||
| A code below 1024 has the meaning {{moqt}} assigns it, and MUST NOT be assigned a moq-lite specific meaning. |
There was a problem hiding this comment.
With this new code-space rule, the draft renumbers moq-lite error codes, but the implementations in the same commit still use the old single application-specific table: rs/moq-net/src/error.rs::to_code sends Unauthorized as 6 and Cancel stream resets as 0, while the JS close decoder still treats nonzero session codes as peer-private. A peer updated from this draft would read an auth close from this repo as KEY_VALUE_FORMATTING_ERROR and a cancel reset as INTERNAL_ERROR, so the spec and Rust/JS need to move together or this draft change should be deferred.
AGENTS.md reference: AGENTS.md:L168-L168
Useful? React with 👍 / 👎.
kixelated
changed the title
6f7bb92 to
ecf7797
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecf779744c
ℹ️ 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".
| pub fn abort(mut self, err: &Error) { | ||
| if let Some(mut stream) = self.stream.take() { | ||
| stream.reset(err.to_code()); | ||
| stream.reset(StreamError::from(err).to_code()); |
There was a problem hiding this comment.
This updates RESET_STREAM to the new StreamError registry, but the receive-side abort path still calls Reader::abort -> self.stream.stop(err.to_code()), so any STOP_SENDING we send still carries the old implementation-specific Error::to_code() values. For example, rejecting an early non-SETUP uni stream in accept_setup sends UnexpectedStream as 10 instead of the new stream/session-derived code, leaving peers seeing different registries depending on whether we reset our send side or refuse their send side. Please route Reader::abort through StreamError::from(err).to_code() as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8059a0126b
ℹ️ 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".
| StreamError::App(app) => Self::App(app), | ||
| StreamError::Session(inner) => inner.into(), |
There was a problem hiding this comment.
When a peer resets a stream with SESSION_CLOSED (0x3), StreamError::from_code creates Session(SessionError::Internal) and this arm converts it through the session table, so Error::from_transport reports Remote(1) instead of the stream code the peer actually sent. Even when the transport error reaches the stream decoder correctly, this also turns peer application reset codes such as 64 into local App(0), despite Error::App being documented as a code this side chooses to send. Consumers that log or match remote reset codes now see the wrong value; keep received stream codes as Remote(err.to_code()) (or otherwise carry the original code) when there is no precise local error.
Useful? React with 👍 / 👎.
…or from StreamError A server that accepts the transport and then rejects at the MoQ layer (Request::close mapping 401/403) was unclassifiable at the client, in both implementations. Decoding the peer's close code, the obvious fix, was itself unsound: moq-net's to_code table was implementation-specific and unspecified, so reading a peer's 26 as our Lagged assumed a shared meaning the wire never carried. The draft said only "an application-specific error code", with 0 the sole defined value. Two real bugs fell out of the same gap: - The IETF paths sent moq-lite codes on a moq-transport session, so our Unauthorized arrived at a spec-compliant peer as KEY_VALUE_FORMATTING_ERROR. - from_transport read a stream reset of 0 as a routine cancellation, where moq-transport defines 0 as INTERNAL_ERROR and puts CANCELED at 1. Both trace to one to_code table serving two registries that moq-transport assigns independently. Spec first (draft-lcurley-moq-lite, new Error Codes section). Two spaces, mirroring moq-transport, each split into three ranges: 0-31 moq-transport's with moq-transport's meaning, 32-63 moq-lite's, 64+ the application's. Keeping the app base at 64 means the existing App(n) => n + 64 offset needs no migration. Then the code. SessionError and StreamError carry their own to_code/from_code. StreamError::Session flattens to SESSION_CLOSED on the wire, since the spaces are disjoint and the specific reason travels on the session close instead. Error stays the crate's local error type and maps into whichever registry the call site encodes for, so the ~2300 error construction sites are untouched: only the wire boundary had to learn which space applies. js/net mirrors the tables as SessionCode/StreamCode. Decoding is sound now that both registries are specified, so the reconnect loop stops on a session-level UNAUTHORIZED rather than retrying credentials that cannot work. This is a wire break: every code an existing implementation sends is renumbered, and a stream reset of 0 changes from "cancelled" to "internal error". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The codes above the shared ones were registered as if moq-lite had settled them. It hasn't. Reserve the range instead: implementations still emit values there (something has to go on the wire for a condition moq-transport has no code for), but the draft assigns them no meaning, so a receiver MUST treat one as an unspecified error. Accordingly from_code stops decoding 32-63: SessionError::from_code and StreamError::from_code return Unknown(code) there rather than reading a peer's 0x22 back as our own Old. to_code is deliberately not injective as a result, which the round-trip tests now assert directly instead of covering the placeholders. The draft also now lists the moq-transport codes moq-lite uses in tables of their own rather than citing moq-transport for them, so a reader gets the values without a second document. Spelling follows moq-transport-19: CANCELLED, not CANCELED. js/net's SessionCode/StreamCode drop the placeholder entries for the same reason: exporting them invites an app to compare against a code the wire doesn't define. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ries Review follow-ups on the registry split. Each is the same class of bug the split exists to catch: a code sent or re-sent against the wrong table. - `Reader::abort` still reset with `Error::to_code()`, so every STOP_SENDING carried a local-table value while RESET_STREAM carried a stream code. A peer saw two different registries depending on which side refused. - `moq-wasm`'s adapter answered both `session_error()` and `stream_error()` with the same code, and callers ask about the session first, so a browser stream reset decoded against the session table. The upstream error already distinguishes the two; we just weren't reading the variant. - A peer's `SESSION_CLOSED` (0x3) decoded through the *session* table into `Error::Remote(1)`, which re-encoded onto a stream as 0x1 = CANCELLED: a relay turned an upstream session teardown into a routine cancel. Decode it to the new `Error::SessionClosed` instead, which re-encodes as 0x3. - More generally, `Error::Remote` carries no registry, so forwarding one into the other space could land on a value that IS registered there (a session 0x4 becomes the stream's GOING_AWAY). Send the unspecified-error code rather than mistranslating; the draft already says an unrecognized code is an unspecified error. `MALFORMED_TRACK` gets a real `Error` variant so it keeps round-tripping instead of relying on `Remote` to carry it. - `is_protocol_violation` names `WrongSize` and `Encode`, but neither reached `SessionError::ProtocolViolation`, so we closed with INTERNAL_ERROR after deciding the peer broke the protocol. - A `client.rs` assertion compared a close code against the local table, so it could no longer fail and had stopped guarding its regression. js/net sent no codes at all: every `abort`/`cancel` took a plain `Error`, which every transport puts on the wire as 0. That was harmless while 0 meant "cancelled" and is not anymore, so a routine browser unsubscribe now reads as INTERNAL_ERROR at the publisher. `toTransport` builds the shape the transports actually read the code from, and a relayed reset keeps the peer's code instead of being flattened. The `WebTransportError` test stub took its arguments in a different order than the real constructor, which is why nothing caught this; it now mirrors the DOM signature. Finally, `Reload` stops on an UNAUTHORIZED close like moq-native already does. The test that pinned the opposite behavior argued the library had no basis to call an arbitrary code terminal, which was true before this code was specified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebase onto dev picked up #2667, whose test pinned the local table's value for a malformed NAMESPACE close. A session close carries a session code, so the peer is told PROTOCOL_VIOLATION (0x3) rather than whichever decode error we happened to hit. The two `close(0, "")` / `close(1, "")` literals alongside it already matched SessionError::Cancel and Internal, so name them: the whole point of the split is that a wire code says which table it came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8059a01 to
f33932b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f33932b82a
ℹ️ 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".
| // specified code rather than one we guessed at, so this is the peer saying these | ||
| // credentials will never work; retrying them just burns the window. Matches | ||
| // moq-native's reconnect loop, which stops on the same close. | ||
| if (cause instanceof RemoteError && cause.code === SessionCode.Unauthorized) { |
There was a problem hiding this comment.
When connect() fails while exchanging the compat/control SETUP stream, Reader or Writer wraps a peer reset in RemoteError; in that context code 0x2 is StreamCode.DeliveryTimeout, not SessionCode.Unauthorized. This numeric-only check therefore rejects Reload.closed and schedules no retry after a stream delivery timeout. Carry the registry/source in RemoteError, or limit this terminal auth check to errors decoded from a session close.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
1 participant
Rebased off the #2618 / #2614 reconnect stack and onto
devdirectly, so this no longer waits on either. Started as "surface the peer's session close code" and turned into specifying the codes properly, since decoding one was the thing that wasn't sound.Root cause
A server that accepts the transport and then rejects at the MoQ layer (
Request::closemapping 401/403) was unclassifiable at the client, in both implementations. The obvious fix, decoding the peer's close code, was itself unsound: moq-net'sto_codetable was implementation-specific and unspecified, so reading a peer's 26 as ourLaggedassumed a shared meaning the wire never carried. The draft said only "an application-specific error code", with 0 the sole defined value.Two real bugs fell out of the same gap:
ietf/session.rs,ietf/subscriber.rs), so ourUnauthorizedarrived at a spec-compliant peer asKEY_VALUE_FORMATTING_ERROR.from_transportread a stream reset of 0 as a routine cancellation, where moq-transport defines 0 asINTERNAL_ERRORand putsCANCELEDat 1.Both trace to one
to_codetable serving two registries that moq-transport assigns independently.Fix
Spec first (
drafts/draft-lcurley-moq-lite.md, new Error Codes section). Two spaces, mirroring moq-transport, each split into three ranges: 0-31 moq-transport's with moq-transport's meaning, 32-63 moq-lite's, 64+ the application's. Keeping the app base at 64 means the existingApp(n) => n + 64offset needs no migration.Then the code.
SessionErrorandStreamErrorcarry their ownto_code/from_code.StreamError::Sessionflattens toSESSION_CLOSEDon the wire, since the spaces are disjoint and the specific reason travels on the session close instead.Errorstays the crate's local error type and maps into whichever registry the call site encodes for, so the ~2300 error construction sites are untouched: only the wire boundary (session closes, stream resets,from_transport) had to learn which space applies.js/netmirrors the tables asSessionCode/StreamCode.Decoding is sound now that both registries are specified, so the reconnect loop stops on a session-level
UNAUTHORIZEDrather than retrying credentials that cannot work. That is the behavior the PR originally wanted, arrived at legitimately.What the rebase changed
The original branch sat on top of #2618, whose
Connectionhandle (optimistic connect, one-shot mode,with_reconnect) does not exist ondev. Retargeted onto whatdevactually has:Connection::runintodev'sReconnect::run, at the same point in the loop.devto adjust. Both come back for free when feat(moq-ffi): sessions reconnect automatically #2618 rebases on this.dev's API.dev'sClient::connectis a one-shot dial that completes the transport handshake before the MoQ-layer rejection arrives, so the dial succeeds and the rejection lands on the session's close.session_close_surfaces_a_rejection_codeasserts that close decodes toError::Unauthorized;reconnect_stops_on_a_session_level_rejectioncovers the loop viaClient::reconnect. They sit next todev's existing WebSocket-401 pair, which makes the same claim for the transport-level rejection.Also folded in three
session.close(Error::…to_code())sites that landed ondevafter this branch forked (two inietf/session.rs's SETUP decode, one inietf/subscriber.rs's PUBLISH_NAMESPACE handler). They are the same IETF-sends-lite-codes bug; leaving them would have defeated the fix.This is a wire break
Every code an existing implementation sends is renumbered. Sharpest edge: a stream reset of
0changes from "cancelled" to "internal error". Old and new peers disagree about every code, which is why this targetsdev.Tests
session_codes_round_trip/stream_codes_round_trip: every variant survives encode/decode, the reused moq-transport values are pinned, ours are asserted inside 32-63, and the disjointness is pinned directly (0decodes differently per registry).from_transport_selects_the_matching_registry: same integer, both spaces, asserting they diverge.session_close_surfaces_a_rejection_codeandreconnect_stops_on_a_session_level_rejection, per above. The latter proves the loop stops rather than burning the 5m backoff window.the code tables match the specpins the TS tables against the same values, since they are a contract with the Rust side rather than a local convenience.Verification
Rust: 1243 tests across moq-net/moq-native/moq-relay/moq-ffi, all passing;
just rs checkclean (clippy-D warnings, fmt, rustdoc-D warnings, shear, sort).JS: 355 tests in js/net,
tscand biome clean.Drafts:
just drafts checkclean.Not run on this rebase: the cross-language interop matrix (
just test smoke-full). It passed on the pre-rebase branch, and the wire encoding is unchanged by the rebase, but the two implementations are validated against each other after renumbering, not against an older peer, which by design no longer interoperates.Cross-package sync:
js/netanddoc/lib/js/@moq/net.mdupdated. Nomoq-ffisurface change, so no binding wrappers.Review round
A review pass found seven more instances of the same class of bug, all fixed here:
Reader::abortstill reset withError::to_code(), so STOP_SENDING and RESET_STREAM carried values from different tables depending on which side refused.moq-wasm's adapter answered bothsession_error()andstream_error()with the same code, and callers ask about the session first, so a browser stream reset decoded against the session table. The upstream error already distinguishes the two.SESSION_CLOSED(0x3) decoded through the session table intoError::Remote(1), which re-encoded onto a stream as 0x1 = CANCELLED: a relay turned an upstream session teardown into a routine cancel. NewError::SessionClosedround-trips it.Error::Remotecarries no registry, so forwarding one into the other space could land on a value that is registered there (a session 0x4 becomes the stream's GOING_AWAY). It now sends the unspecified-error code instead of mistranslating, which is what the draft already requires of an unrecognized code.MALFORMED_TRACKgained a realErrorvariant so it keeps round-tripping rather than relying onRemote.is_protocol_violationnamesWrongSizeandEncode, but neither reachedSessionError::ProtocolViolation, so we closed with INTERNAL_ERROR right after deciding the peer broke the protocol.client.rsassertion compared a close code against the local table, so it could no longer fail and had stopped guarding its regression.abort/canceltook a plainError, which every transport puts on the wire as 0. Harmless while 0 meant "cancelled", not anymore, so a routine browser unsubscribe read as INTERNAL_ERROR at the publisher.toTransportbuilds the shape the transports read the code from, and a relayed reset keeps the peer's code. TheWebTransportErrortest stub took its arguments in a different order than the real constructor, which is why nothing caught this; it now mirrors the DOM signature.Reloadalso stops on an UNAUTHORIZED close, matching moq-native. The test that pinned the opposite argued the library had no basis to call an arbitrary code terminal, which was true before this code was specified.Deliberately left as a follow-up:
Session::closed()no longer surfaces the peer's close reason string, which the oldcode=N: reasonformatting preserved. js/net keeps it, so the two disagree. For an application code the reason is the only place the meaning lives.Follow-ups, not in this PR
Reload.#retryrejectsclosedon the retry timeout while leaving its effect and url/enabled subscriptions live, so a later url change dials again behind an already-rejected promise. Predates this branch.rs/moq-wasmdoes not compile for wasm32 ondeveither, for two reasons unrelated to this PR (lib.rs:91and:177); verified against a cleanorigin/devworktree. Filed separately.(Written by Opus 5)
🤖 Generated with Claude Code