fix(moq-net): send registered moq-transport request error codes - #3531
Conversation
Every rejection moq-net and js/net put on a moq-transport request stream was an HTTP status: 404 for a missing broadcast, 500 for an unsupported FETCH, 400 and 409 on the announce path. None of those are in any draft's registry, so a peer read them as unregistered and fell back to a generic failure. The registry is per draft twice over. Draft-14 gives each error message its own table and they disagree about 0x4: TRACK_DOES_NOT_EXIST on SUBSCRIBE_ERROR and FETCH_ERROR, UNINTERESTED on PUBLISH_ERROR and ANNOUNCE_ERROR. Draft-15 folded all five into REQUEST_ERROR and renumbered, so a missing broadcast moved from 0x4 to 0x10, which is draft-14's MALFORMED_AUTH_TOKEN. Sending either number without the draft in hand tells the peer the opposite of what happened. `ietf::error::request` maps a named `Condition` to the value the negotiated draft registers for that request, and back, alongside the stream reset half already there. `js/net/src/ietf/error.ts` mirrors it. Both fall back to INTERNAL_ERROR for a condition the draft does not register, and neither decodes a code the draft leaves unassigned into a meaning it never carried. A subscribe rejection now reaches the track as the reason the publisher gave rather than a bare cancel, so a subscriber can tell a broadcast that is not there from one it may not have. js/net also gained the per-draft stream reset mapping Rust has: a delivery timeout, a session close, a lag past the window and a malformed track now travel where the draft registers them instead of degrading to INTERNAL_ERROR, and a received 0x4 is only read as GOING_AWAY from draft-18 on, where draft-16 and 17 give it to UNKNOWN_OBJECT_STATUS. Public API: unchanged. `ietf::error` is crate-private and the new js/net module is not exported from the package index. Wire: moq-transport request error codes and js/net stream reset codes change to the registered values; moq-lite is untouched. Closes #3359 Co-Authored-By: Claude Opus 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. |
![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: 6cbbad12e3
ℹ️ 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 (code === undefined) return error(err); | ||
| if (options?.version === undefined && code === StreamCode.TooFarBehind) return new Lagged({ cause: err }); | ||
| if (options?.version !== undefined && !sharedStreamCode(code, options.version)) { | ||
| return new StreamError(StreamCode.Internal, { cause: err, message: `remote error: ${code}` }); |
There was a problem hiding this comment.
When an IETF peer sends a registered code that has no matching moq-lite meaning, such as UNKNOWN_OBJECT_STATUS (0x4) on draft-16/17, this branch replaces the observable StreamError.code with INTERNAL_ERROR (0). That discards the machine-readable wire value and makes the peer's condition indistinguishable from an actual internal failure, unlike Rust's from_stream_code, which retains it as Unknown(code). Keep the received code on the plain StreamError; only avoid assigning a misleading typed subclass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and taken partway.
A code moq-lite names nothing for now keeps its value: UNKNOWN_OBJECT_STATUS (0x6 from draft-18), EXPIRED_AUTH_TOKEN (0x7) and EXCESSIVE_LOAD (0x9) reach the caller as a plain StreamError carrying the wire number, since nothing can misread them.
The flattening stays only where moq-lite claims the number for something the draft does not, which is the case you named: StreamCode is one numeric space, so a draft-16/17 UNKNOWN_OBJECT_STATUS kept as 0x4 would compare equal to StreamCode.GoingAway and retire a session that is not going anywhere. Same for 0x5 and 0x12 on the drafts that predate their registration, and for the reserved 32-63 placeholders. Rust avoids this with a separate StreamError::Unknown(code) variant that cannot collide; JS has no second space, so those codes report Internal with the wire value kept in the message.
(written by claude-opus-5)
Flattening every unshared code to INTERNAL_ERROR discarded the wire value of codes that could not be misread: moq-lite names nothing for UNKNOWN_OBJECT_STATUS, EXPIRED_AUTH_TOKEN or EXCESSIVE_LOAD, so carrying their number costs nothing. Only a code moq-lite claims for something the draft does not still flattens, and the wire value goes in the message. StreamCode is one numeric space, so a draft-16 UNKNOWN_OBJECT_STATUS kept as 0x4 would test equal to StreamCode.GoingAway and retire a session that is not going anywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8980206d82
ℹ️ 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".
| Err(_) => { | ||
| return self | ||
| .reject_subscribe(stream, request_id, 404, "Broadcast not found") | ||
| .reject_subscribe( | ||
| stream, | ||
| request_id, | ||
| request::Condition::DoesNotExist, |
There was a problem hiding this comment.
When an origin::Dynamic handler rejects this broadcast request with a meaningful error such as Error::Unauthorized, Error::GoingAway, or Error::Closed, this blanket arm discards it and now sends the registered DOES_NOT_EXIST condition. The peer can therefore treat an authentication failure or draining session as missing content and take the wrong recovery action. Preserve err and convert it to request::Condition, as the track rejection branches below already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 900afba. The arm now forwards the origin's error through Condition::from(&err) and uses its Display as the reason phrase, matching the two track branches below.
Error::Unroutable still lands on DOES_NOT_EXIST for a SUBSCRIBE, which is correct: it is the one condition that does mean the broadcast is not here.
(written by claude-opus-5)
Hardcoding DOES_NOT_EXIST discarded what an origin::Dynamic handler said: an authorization refusal or a draining session reached the peer as missing content, which is a different recovery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 900afbabba
ℹ️ 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 moq-lite code for a local failure, before any draft has a say. */ | ||
| function localStreamCode(err: unknown): StreamCode { | ||
| if (err instanceof StreamError) return err.code; | ||
| if (err instanceof TimeoutError) return StreamCode.DeliveryTimeout; |
There was a problem hiding this comment.
This maps every TimeoutError to the IETF DELIVERY_TIMEOUT reset even when the timeout is unrelated to media delivery. For example, the SUBSCRIBE_OK timer in ietf/subscriber.ts and the PUBLISH_NAMESPACE response timer in ietf/publisher.ts both abort their request streams with that error, so a silent peer is told that content missed a delivery deadline although only a control response timed out. Keep generic IETF timeouts as INTERNAL_ERROR, or introduce a distinct error used only when an actual delivery deadline expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The observation is right, but the fix is not this PR's.
toStreamCode mapped every TimeoutError to DeliveryTimeout before this branch, and Rust does the same through From<&Error> for StreamError (Error::Timeout => StreamError::DeliveryTimeout). Making the IETF branch alone say INTERNAL_ERROR would leave the two languages and the two wires disagreeing about the same condition, which is exactly what this PR set out to stop.
The real question is whether "you never answered" deserves a condition separate from "your content was late", in both languages, on both wires. Filed as quest/m1/control-timeout-code.md in 104d644, including the option of dropping the code rather than adding one.
(written by claude-opus-5)
A request stream torn down because the peer never answered resets with DELIVERY_TIMEOUT, which describes content that missed its deadline. Both languages and both wires do it, so the fix is cross-cutting and out of scope here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 104d644616
ℹ️ 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 (options?.version !== undefined && !sharedStreamCode(code, options.version) && LOCAL_CODES.has(code)) { | ||
| return new StreamError(StreamCode.Internal, { cause: err, message: `remote error: ${code}` }); |
There was a problem hiding this comment.
When an IETF peer resets a stream with an unknown or future code of 64 or greater, this collision check misses it because LOCAL_CODES contains only the named properties, even though StreamCode(code) assigns every value in the 64+ range a moq-lite application meaning. The resulting StreamError can therefore satisfy an application's comparison against its own StreamCode(70), misreporting a foreign IETF condition as its application error. Fresh evidence beyond the earlier thread is the constructor at lines 62 and 72, which shows that the local code space includes the entire application range rather than only Object.values(StreamCode); include that range in the collision predicate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, fixed in 1e47491. The predicate is now code >= 64 || NAMED_CODES.has(code), so a foreign code anywhere in the application range flattens to Internal with the wire value in the message.
Rust never had this hole: from_stream_code returns StreamError::Unknown(code) for anything unregistered, which becomes Error::Remote(code) rather than App(code - 64).
(written by claude-opus-5)
moq-lite mints an application code for anything 64 and up, so an application compares against its own. moq-transport registers nothing above 0x12 and has no application range, so an IETF code landing there is never the application code it would look like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every Condition variant named an Error variant, so the enum was a second spelling of the same eight cases between the call site and the registry. to_code now takes the Error itself, mirroring the stream half, and Kind alone decides what a refused route says on each request. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Conflicts: - quest/m0/README.md: dev landed the IETF error codes quest (#3531) and deleted its entry; main added the #3558 and #3559 entries beside it. - rs/justfile: main deleted the `cargo_compile` variable (#3553), so dev's newer recipes that used it now call `cargo` directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The IETF error codes quest landed on dev as #3531 and its doc is gone, so the three links main's new quests point at now dangle. FETCH refusals already carry the registered code, and `write_fetch_ok` moved with the publisher rewrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Completes quest/m0/ietf-error-codes, deleted here. The Rust stream-reset half landed in #3450; this is the request-error half in both languages, plus the per-draft stream mappings js/net was missing.
The problem
Every rejection moq-net and js/net put on a moq-transport request stream was an HTTP status:
404for a missing broadcast,500for an unsupported FETCH,400and409on the announce path, and a function-localNOT_SUPPORTEDfor PUBLISH. None of404/500/400/409is in any draft's registry, so a conforming peer reads them as unregistered and falls back to a generic failure.The registry is per draft twice over:
0x4. It isTRACK_DOES_NOT_EXISTon SUBSCRIBE_ERROR and FETCH_ERROR, andUNINTERESTEDon PUBLISH_ERROR and ANNOUNCE_ERROR.0x4to0x10, which is draft-14'sMALFORMED_AUTH_TOKEN. Sending either number without the draft in hand tells the peer the opposite of what happened: its token is broken, rather than the broadcast simply not being here yet.GOING_AWAY(0x6) only exists from draft-17.The change
rs/moq-net/src/ietf/error.rsgains arequestmodule beside the stream-reset half already there, with the same shape:to_code(&Error, Kind, Version)picks the value the negotiated draft registers for that request, andfrom_code(u64, Kind, Version)reads one back into anError.Kindnames which request the rejection answers, which draft-14 needs to pick a registry and every draft needs to decide what a refused route says (not here to a subscriber, uninterested to a publisher).js/net/src/ietf/error.tsmirrors it with a string union in place of theErrorenum.Both directions refuse to invent meaning:
INTERNAL_ERRORrather than borrowing a number another draft assigns to something else. A duplicate is one of those: no draft registers it, and the only refusal we raise for one is a draft-14/15 implementation limit.Error::Remote(Rust) /undefined(JS), never a named condition.Every literal and function-local constant at the call sites is gone.
js/net stream resets
js/net's
toStreamCodepreviously mapped only cancellation on an IETF stream and sentINTERNAL_ERRORfor everything else, andfromTransportignored the version entirely. It now uses the same per-draft table Rust has: a delivery timeout, a session close, a lag past the window and a malformed track travel where the draft registers them, and a received0x4is read asGOING_AWAYonly from draft-18 on, where draft-16 and 17 give it toUNKNOWN_OBJECT_STATUS.A received code keeps its value unless moq-lite claims that number for something the draft does not.
StreamCodeis one numeric space, so a draft-16UNKNOWN_OBJECT_STATUSkept as0x4would compare equal toStreamCode.GoingAway, and a foreign code at 64 or above would compare equal to an application's ownStreamCode(70). Those reportInternalwith the wire value in the message;0x6,0x7and0x9, which moq-lite names nothing for, survive intact. Rust avoids all of this with aStreamError::Unknown(code)variant that cannot collide.Follow-up filed
quest/m1/control-timeout-code.md: every localTimeoutErrorresets withDELIVERY_TIMEOUT, including the SUBSCRIBE_OK and PUBLISH_NAMESPACE response timers, which never touched content. Both languages and both wires do it, so the fix is cross-cutting and does not belong here. The quest includes the option of dropping the distinction rather than adding a code.Subscribe rejections reach the track
read_subscribe_responsereturnedError::Cancelfor every rejection, discarding the code. It now decodes it, so a subscriber can tell a broadcast that is not there (Error::NotFound) from one it may not have (Error::Unauthorized).Tests
a_missing_broadcast_is_refused_with_the_draft_s_code: a SUBSCRIBE for a broadcast we do not serve, on all seven drafts, matched against the encoded reply.publish_is_rejected_without_announcingnow runs draft-14's PUBLISH_ERROR alongside draft-19's REQUEST_ERROR.403/404/409now use registered values.API and wire impact
ietf::errorispub(crate), and the newjs/net/src/ietf/error.tsis not exported from the package index.drafts/changes.moq-ffisurface is involved.Note on #3001
The
devcopy of the quest listed only #3359, having dropped #3001 as covered by #3450. #3001 is still open on GitHub. I left it alone rather than closing it here; worth a look to confirm #3450 finished it.🤖 Generated with Claude Code
(written by claude-opus-5; API reshaped by claude-fable-5-1)