Skip to content

feat(ffi): backport draining shutdown as drain() and the qmux stream limit to release - #6642

Merged
kixelated merged 4 commits into
releasefrom
fix/release-ffi-drain
Oct 24, 2026
Merged

kixelated merged 4 commits into
releasefrom
fix/release-ffi-drain

Conversation

@kixelated

@kixelated kixelated commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Backports quest/m0/ffi-shutdown-backport.md onto release. The quest's claim PR into main is moq-dev/moq#6634.

Problem

The published 0.4.x bindings have no way to drain a session: MoqSession.shutdown() is cancel(0), so a publisher that finishes its tracks and disconnects can cut off the tail it just wrote. main fixed this by making shutdown() async (7bbac32, 03fb0d1, f9de311, moq-dev/moq#5558), but that change breaks the Python, Swift, and Kotlin bindings.

qmux sessions (WebSocket, tcp://, unix://) on release also 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. main fixed 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. The tls:// and WebSocket accept() sites from main do not exist on release, so the remaining sites take the config. Its test fails at 100 stream pairs without the config.
  • feat(ffi): adds an async MoqSession.drain() that runs Session::close / Connection::close, which release's moq-net and moq-tokio already provide. It returns the drain's error, such as DeliveryTimeout after one second. shutdown() stays the synchronous cancel(0). The real-socket tests move over renamed to drain_*, including fix(ffi): test shutdown delivery with explicit Lite 07 #5558's explicit moq-lite-07-wip setup. A #[cfg(test)] version setter on MoqClient/MoqServer makes that possible, since release's FFI has no public version setter.
  • Wrappers: Python Session.drain() (with an idle-session test), Swift Session.drain(), Kotlin Moq.drain(), Go Session.Drain(ctx), Dart Moq.drain(). The generated MoqSession.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.
  • Versions: moq-rs 0.5.2, Swift 0.5.2, Kotlin 0.5.3, Dart moq 0.1.1 (with a CHANGELOG entry). go/wrapper/VERSION stays as it is because CI derives the Go patch.

Choices made without asking:

  • The Python, Kotlin, and Dart context managers, close(), and use { } stay immediate, as they are on release today. If they drained on exit, existing callers with a live track would wait a second and then get a timeout error.
  • The wrappers' moq-ffi floors (~= 0.4.9, [0.4.3,0.5), ^0.4.3) stay as they are. We can't know which patch carries drain() 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.
  • The binding docs say the drain waits for transport ACKs of the tail, not for the peer to read it, and that a moq-transport (IETF) session closes without waiting. Those are the only behaviors the bindings can reach: release's FFI never negotiates moq-lite-07-wip, and release's IETF driver reports itself drained at once. (On main, IETF also waits for transport ACKs.)
  • libmoq has no drain(). On release it is its own C API (moq_session_close), not a wrapper over moq-ffi's MoqSession, so it needs a separate design.

Impact

  • Public API (additive): MoqSession.drain() in moq-ffi, plus Session.drain() (Python, Swift), Moq.drain() (Kotlin, Dart), and Session.Drain(ctx) (Go).
  • moq-tokio: qmux sessions grant 10,000 bidi and uni streams, matching QUIC. No API change.
  • Wire: none.

Alternatives

  • Backport async shutdown() as it is: rejected by the maintainer (2026-10-20) because it breaks the 0.4.x bindings.
  • Drop moq-dev/moq#5609: rejected by the maintainer (2026-10-20). qmux 0.5.3 was published for this.

Follow-ups

  • After release-plz publishes, check that the newest moq-ffi on PyPI, the Swift package, and Maven Central contain drain(). Swift and Kotlin publish from their own tag workflows. Then merge moq-dev/moq#6634, which deletes the quest.
  • At the next breaking cut, callers replace await drain() with shutdown() followed by await closed() (the main shape).

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 match release's playwright-core 1.63 (chromium-1243), so the browser legs fail at launch out of the box. I ran them with PLAYWRIGHT_BROWSERS_PATH pointed at a local alias of chromium-1248 as 1243. The run was on the drain commit, before the doc-only and Python-test commits.
  • New tests: moq-tokio tcp::tests::sessions_grant_a_thousand_open_streams fails 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)

kixelated and others added 4 commits October 24, 2026 07:48
Backport of #5609 onto release, on qmux 0.5.3's builder session config.

(cherry picked from commit 258e3bb)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Backport of the draining shutdown (7bbac32, 03fb0d1, f9de311, #5558)
onto release as an additive drain(), so the 0.4.x bindings keep their
synchronous shutdown().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T15:39:55.537348Z be72790 Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread rs/moq-ffi/src/session.rs
/// [`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> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging with the maintainer's approval.

  • Head be72790aae23086d714ae51fe96e0c3ea33b643b: 36 checks passed, Codex reviewed this head.
  • Codex's one finding (a libmoq drain()) was declined in the thread: on release, libmoq is its own C API, so a C drain needs a separate ABI design. It is a suggested follow-up.
  • quest check --base origin/release passed. The claim PR moq-dev/moq#6634 stays a draft until release-plz publishes.
  • No changes were pushed during the merge. Lands as a merge commit, per CONTRIBUTING.md for release.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit fd1d6cc into release Oct 24, 2026
36 checks passed
@kixelated
kixelated deleted the fix/release-ffi-drain branch October 24, 2026 16:29
This was referenced Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant