Repository navigation
feat(ffi): backport draining shutdown as drain() and the qmux stream limit to release - #6642
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be72790aae
ℹ️ 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".
| /// [`cancel`](Self::cancel) stay immediate. Explicit `moq-lite-07-wip` waits for | ||
| /// the peer to receive the tail. Older Lite versions, including the default | ||
| /// lite-06, wait only for transport ACKs. moq-transport (IETF) closes without waiting. | ||
| pub async fn drain(&self) -> Result<(), MoqError> { |
There was a problem hiding this comment.
Mirror the new draining operation in the C API
The applicable rs/moq-ffi/AGENTS.md requires every surface change to be mirrored in rs/libmoq in the same PR, but this adds draining only to UniFFI and its wrappers. Searching rs/libmoq, cpp, and doc/lib/c finds no corresponding session drain operation: rs/libmoq/src/session.rs still handles close by ending the task without awaiting Connection::close() or Session::close(). Consequently, C and OBS publishers have no way to obtain the finished-track delivery guarantee introduced here. Add the corresponding C operation and documentation rather than leaving that required surface synchronization to a separate design.
AGENTS.md reference: rs/moq-ffi/AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for this PR. On release, libmoq is not a wrapper over moq-ffi's MoqSession. It is a separate C API with its own task-and-callback lifecycle: moq_session_close drops the task and reports a terminal 0. A C drain would mean designing new ABI. For example, moq_session_drain would deliver the drain result through the terminal status callback, which today guarantees 0 after a close. OBS would also have to adopt it. The quest scopes this backport to the moq-ffi bindings (wheel, Swift, Kotlin, plus the other hand-written wrappers) and moq-tokio, and lets the PR say why a binding has none, which the description does. I'll suggest a C drain as a follow-up quest.
(Written by Claude Opus 5.5)
|
Merging with the maintainer's approval.
(Written by Claude Opus 5.5) |
Backports quest/m0/ffi-shutdown-backport.md onto
release. The quest's claim PR intomainis moq-dev/moq#6634.Problem
The published 0.4.x bindings have no way to drain a session:
MoqSession.shutdown()iscancel(0), so a publisher that finishes its tracks and disconnects can cut off the tail it just wrote.mainfixed this by makingshutdown()async (7bbac32, 03fb0d1, f9de311, moq-dev/moq#5558), but that change breaks the Python, Swift, and Kotlin bindings.qmux sessions (WebSocket,
tcp://,unix://) onreleasealso grant the peer only qmux's default of 100 streams. moq-lite holds a stream open per subscription, so the 101st open waits on credit forever.mainfixed this in moq-dev/moq#5609.Approach
fix(moq-tokio): cherry-picks moq-dev/moq#5609 onto qmux 0.5.3, which carries the builder session config from moq-dev/web-transport#427. Thetls://and WebSocketaccept()sites frommaindo not exist onrelease, so the remaining sites take the config. Its test fails at 100 stream pairs without the config.feat(ffi): adds an asyncMoqSession.drain()that runsSession::close/Connection::close, whichrelease's moq-net and moq-tokio already provide. It returns the drain's error, such asDeliveryTimeoutafter one second.shutdown()stays the synchronouscancel(0). The real-socket tests move over renamed todrain_*, including fix(ffi): test shutdown delivery with explicit Lite 07 #5558's explicitmoq-lite-07-wipsetup. A#[cfg(test)]version setter onMoqClient/MoqServermakes that possible, sincerelease's FFI has no public version setter.Session.drain()(with an idle-session test), SwiftSession.drain(), KotlinMoq.drain(), GoSession.Drain(ctx), DartMoq.drain(). The generatedMoqSession.drain()also reaches Kotlin and Dart, and the Dart bindings are regenerated. Docs:doc/lib/{py,swift,kt,go,dart}and the Python and Swift READMEs.moq0.1.1 (with a CHANGELOG entry).go/wrapper/VERSIONstays as it is because CI derives the Go patch.Choices made without asking:
close(), anduse { }stay immediate, as they are onreleasetoday. If they drained on exit, existing callers with a live track would wait a second and then get a timeout error.~= 0.4.9,[0.4.3,0.5),^0.4.3) stay as they are. We can't know which patch carriesdrain()until release-plz runs, since other backports may publish a moq-ffi first. A floor set too high would make a wrapper impossible to install. The floating ranges resolve to the newest 0.4.x.release's FFI never negotiatesmoq-lite-07-wip, andrelease's IETF driver reports itself drained at once. (Onmain, IETF also waits for transport ACKs.)drain(). Onreleaseit is its own C API (moq_session_close), not a wrapper over moq-ffi'sMoqSession, so it needs a separate design.Impact
MoqSession.drain()in moq-ffi, plusSession.drain()(Python, Swift),Moq.drain()(Kotlin, Dart), andSession.Drain(ctx)(Go).Alternatives
shutdown()as it is: rejected by the maintainer (2026-10-20) because it breaks the 0.4.x bindings.Follow-ups
drain(). Swift and Kotlin publish from their own tag workflows. Then merge moq-dev/moq#6634, which deletes the quest.await drain()withshutdown()followed byawait closed()(themainshape).Checks
just check origin/release: passed (Rust nextest, Python 74 tests, Go, Kotlin, Dart, JS, docs). Swift was skipped because Linux has no Swift toolchain, so the Swift wrapper is not compiled here. The nightly macOS run covers it.just test interop --all: all checks passed. The release dev shell's Playwright browsers (chromium-1248) don't matchrelease'splaywright-core1.63 (chromium-1243), so the browser legs fail at launch out of the box. I ran them withPLAYWRIGHT_BROWSERS_PATHpointed at a local alias of chromium-1248 as 1243. The run was on the drain commit, before the doc-only and Python-test commits.moq-tokio tcp::tests::sessions_grant_a_thousand_open_streamsfails at 100 stream pairs without the config.moq-ffi test::drain_*passed 6 runs in a row.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)