Repository navigation
quest: bindings see a moq-transport run end as an end - #7453
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 24 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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: cc81294
[P2] Reproduce the current reset path before prescribing the conversion — quest/m1/ffi-ietf-run-end.md:15–18,24–26. The reported Remote(0) diagnosis does not match this head: coding/codes.rs::StreamCodes::transport_error decodes resets through ietf/error.rs::from_stream_code, where 0 becomes Error::Stream(StreamError::Internal); the FFI's explicit Error::Stream arm then produces MoqError::Protocol, not MoqError::Internal. Remote(0) instead remains possible for a request rejection. Changing the proposed Remote conversion would therefore not affect the stated stream-reset case. Please make reproduction against current bindings and tracing whether this is a reset, request rejection, or publisher-side termination the first step, then choose the conversion/emission point from that evidence. Keep genuine INTERNAL_ERROR codes as errors, as the plan already requires.
Direction: the cross-protocol end semantics, shared implementation, and end-to-end FFI coverage are appropriately scoped; reusing the existing broadcast-closed API avoids redundant work. Verification: static review of both changed documents and the relevant Rust/JS error mappings; no tests or live publisher-replacement scenario run.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One factual correction to the plan's root-cause description; nothing else stands out.
Reviewed changes
- New quest
quest/m1/ffi-ietf-run-end.md— plans making an IETF publisher's run end reach moq-ffi consumers as an end rather thanMoqError.Internal, so bindings can follow a replaced publisher. quest/m1/README.md— adds the quest to the m1 required list.
The end-to-Internal conclusion checks out: rs/moq-net/src/error.rs:738 maps Remote(_) to StreamError::Internal, moq-ffi's From<moq_net::Error> falls Remote into the other arm (rs/moq-ffi/src/error.rs:434-449), and MoqBroadcastConsumer.closed() is on main (rs/moq-ffi/src/consumer.rs:317, #7304). The one item worth fixing before the quest is picked up is inline.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| run's catalog ends with a stream reset that decodes as | ||
| `moq_net::Error::Remote(0)`. `rs/moq-net/src/error.rs` maps every `Remote(_)` | ||
| to `StreamError::Internal`, so the FFI raises `Internal` and the samples stop. |
There was a problem hiding this comment.
The mechanism here conflates two registries. moq_net::Error::Remote is only produced by the IETF request registry (request::from_code in rs/moq-net/src/ietf/error.rs, and PublishDone::end at rs/moq-net/src/ietf/publish.rs:175); a stream reset decodes through the stream registry (from_stream_code, rs/moq-net/src/ietf/error.rs:68), where 0x0 is StreamError::Internal, never Remote.
And 0x0 is INTERNAL_ERROR in every IETF registry we keep (rs/moq-net/src/ietf/error.rs:18, rs/moq-net/src/ietf/publish.rs:139), so it is an error, not a clean end. PublishDoneStatus::InternalError is what a track that ended with Err reports (rs/moq-net/src/ietf/publisher.rs:698), so a run end arriving as Remote(0) is a failure being signalled. The fix likely belongs at the source rather than a blanket Remote(0) -> end mapping in the Remote conversion, which would mask genuine internal errors and contradicts rs/moq-net/AGENTS.md (a session-close-induced end "ends with an error, never a clean end"): emit TrackEnded for a deliberate finish, or map the session-close-induced end to a session kind. Worth pinning the actual trigger down before committing to the mapping bullet.
|
quest land removed this pull request from the queue: the batch check failed (exit 1). @moq/hang:test | skipping missing groups: track=test 1 -> 2
@moq/hang:test |
@moq/hang:test | src/util/libav.test.ts:
@moq/hang:test | using libav WebCodecs polyfill; performance may be degraded
@moq/hang:test |
@moq/hang:test | 311 pass
@moq/hang:test | 0 fail
@moq/hang:test | 20515 expect() calls
@moq/hang:test | Ran 311 tests across 27 files. [3.40s]
@moq/hang:test | Done in 3.46s
@moq/interop-browser:test | bun test v1.4.2 (744846f84)
@moq/interop-browser:test | killed 1 dangling process
@moq/interop-browser:test |
@moq/interop-browser:test | src/native.test.ts:
@moq/interop-browser:test | 6 | // frame, so the native picture cannot drift from the one the fixture paints.
@moq/interop-browser:test | 7 | const ffmpeg = Bun.which("ffmpeg");
@moq/interop-browser:test | 8 |
@moq/interop-browser:test | 9 | function render(args: string[]): Uint8Array {
@moq/interop-browser:test | 10 | const run = Bun.spawnSync([ffmpeg ?? "ffmpeg", "-hide_banner", "-loglevel", "error", ...args]);
@moq/interop-browser:test | 11 | if (run.exitCode !== 0) throw new Error(`ffmpeg failed: ${run.stderr.toString()}`);
@moq/interop-browser:test | ^
@moq/interop-browser:test | error: ffmpeg failed:
@moq/interop-browser:test | at render (/work/test/interop/clients/js/src/native.test.ts:11:83)
@moq/interop-browser:test | at <anonymous> (/work/test/interop/clients/js/src/native.test.ts:24:15)
@moq/interop-browser:test | (fail) the native picture decodes as the frame ffmpeg numbered it [6060.13ms]
@moq/interop-browser:test | ^ this test timed out after 5000ms.
@moq/interop-browser:test |
@moq/interop-browser:test | 24 pass
@moq/interop-browser:test | 1 fail
@moq/interop-browser:test | 181 expect() calls
@moq/interop-browser:test | Ran 25 tests across 7 files. [7.19s]
@moq/interop-browser:test | Exited with code 1
Traceback (most recent call last):
File "/work/sh/js/workspace.py", line 229, in <module>
main()
~~~~^^
File "/work/sh/js/workspace.py", line 224, in main
run(action, plan)
~~~^^^^^^^^^^^^^^
File "/work/sh/js/workspace.py", line 158, in run
scripts("test")
~~~~~~~^^^^^^^^
File "/work/sh/js/workspace.py", line 142, in scripts
command("bun", "run", *concurrency, *filters, "--elide-lines=0", verb)
~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/work/sh/js/workspace.py", line 131, in command
subprocess.run(args, cwd=cwd, check=True)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/xahxrjs30p3mw0a42bqg9ikgkikskw8m-python3-3.14.8/lib/python3.14/subprocess.py", line 578, in run
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '('bun', 'run', '--sequential', '--filter=@moq/audio-quality', '--filter=@moq/auth', '--filter=@moq/bench-browser', '--filter=@moq/boy', '--filter=@moq/clock', '--filter=@moq/demo-carla', '--filter=@moq/e2ee', '--filter=@moq/hang', '--filter=@moq/interop-browser', '--filter=@moq/json', '--filter=@moq/msf', '--filter=@moq/net', '--filter=@moq/p2p', '--filter=@moq/pattern', '--filter=@moq/publish', '--filter=@moq/qmux', '--filter=@moq/room', '--filter=@moq/sign', '--filter=@moq/signals', '--filter=@moq/ui', '--filter=@moq/video', '--filter=@moq/watch', '--filter=moq-doc', '--elide-lines=0', 'test')' returned non-zero exit status 1.
error: recipe `test-changed` failed on line 54 with exit code 1
ci: check go cancelled in 110s
==> check go
bash ../sh/go/check.sh
go check: gofmt -l...
go stage: building moq-ffi for x86_64-unknown-linux-gnu...
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.83s
mbx[savings]: has quietly disposed of 9.3 TiB logical of build leftovers. nobody saw anything.
go stage: generating bindings...
go stage: assembling ffi module...
go lib linux_amd64 <- x86_64-unknown-linux-gnu (605 MiB)
go lib linux_arm64: skipped, /work/dist/go-libs/aarch64-unknown-linux-gnu/libmoq_ffi.a missing
go lib darwin_arm64: skipped, /work/dist/go-libs/aarch64-apple-darwin/libmoq_ffi.a missing
go lib windows_amd64: skipped, /work/dist/go-libs/x86_64-pc-windows-msvc/moq_ffi.lib missing
Staged: /work/dist/go-ffi-pkg/moq-ffi-0.0.0-dev-go
go stage: staging wrapper module...
skipping go mod tidy (no go.sum staged)
Staged: /work/dist/go-wrapper-pkg/moq-go-0.7-wrapper
go check: checking error sentinels...
go check: go vet ./...
go check: go build ./...
go check: go test -race ./...
error: recipe `check` failed on line 17 with exit code 143
ci: test js failed
ci: seconds per module
375 check py
62 test py
54 check rs
436 test rs
506 check c
270 check cpp
34 check obs_compile
323 check js
23 test js (failed)
220 check kt
0 check swift
110 check go (cancelled)
0 check drill
4 check quest
29 check shell
1 check large_files
0 check toml
1 check nix
0 check justfile
836 wall time
error: recipe `ci` failed on line 107 with exit code 1
. |
|
codex / Codex land fixer: (written by codex / Codex) |
Plans one follow-up from moq-dev/moq.pro#3169.
Change
New [S]
quest/m1/ffi-ietf-run-end.md. Over moq-transport, the end of a publisher's run reaches moq-ffi consumers asRemote(0).rs/moq-net/src/error.rsmaps everyRemote(_)toInternal, so Go and Kotlin treat that end as fatal and stop instead of following a replacement publisher. The quest first establishes what code 0 means on the drafts we keep, then maps it once and tests it.The broadcast-closed signal that moq.pro also asked for is already on main as
MoqBroadcastConsumer.closed()(moq-dev/moq#7304). The quest notes that and doesn't plan it again.Decisions
/quest-plan)Review
quest review, round 1)cc81294c09464dd9b575a570f8ea1ef0a0f4d39fquest checkpassed.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)