fix(moq-ffi): a cancelled read never takes the next frame

[kixelated]

Quest: quest/m0/ffi-cancel-read.md (deleted here, since it's finished).

Problem

uniffi's rust_future_cancel only stops polling. The Rust future isn't dropped until rust_future_free, which the bindings call later from their executor. Task::run spawned its closure onto the runtime thread and relied on AbortOnDrop, so between the cancel and the free the closure kept running while holding the state lock. It took the next frame, which then went nowhere, and the next read hung. This is what made cpp/moq/test/probe.cpp flake (about 1/30 at C++17 and 2/20 at C++23).

Fix

• Task::run now awaits in place: it is polled by the caller's future, so a cancelled future that is never polled again can't make progress. Each poll enters the runtime, so closures can still use tokio timers, sockets, and spawn. The native and wasm run collapse into one function.
• moq_ffi_shutdown refuses new in-place polls and waits out the ones in flight on other threads before it stops the runtime, so no poll touches a dead driver (a tokio timer would panic). Parked calls are woken and resolve Cancelled as documented.
• MoqServer::listen/accept keep the old spawned path as Task::spawn. MoqServer::cancel uses cancel_and_wait, which blocks the caller's thread until the in-flight call unwinds, and an in-place accept parked on that same thread never would. I checked this: switching them to run hangs server_cancel_releases_the_bound_port.

Consumer reads (read_frame, next_group, recv_group, recv_datagram, group read_frame, the media/audio/video/json next, origin next/available/requested_broadcast, producer requested_track/requested_group), session connect, and MoqRequest calls all go through run, so they all get the fix.

Behavior change: the work in a run closure (including any decode inside the video/audio next) now runs on the thread that polls the foreign future, not on the shared moq-ffi runtime thread. The I/O and timer drivers stay on the runtime thread.

Public API / wire impact

None. Everything changed is pub(crate), and no binding or wire format changes.

Tests

• New raw_read_frame_cancelled_before_free_leaves_the_frame: it polls a read_frame once and then stops without dropping it (all that rust_future_cancel does), writes a frame, lets the runtime catch up, frees the cancelled read, and requires the next read to return the frame. It fails on main (times out) and passed 100/100 runs with the fix.
• New shutdown_waits_for_an_in_place_poll: holds a poll open on a host thread while shutdown starts on another, then polls a timer. It failed 5/30 without the shutdown fix and passed 60/60 with it.
• cargo test -p moq-ffi: 162 passed.
• just check passed. It covers clippy (native and wasm32), the Python and Dart suites, and the C++ probe.
• C++ probe looped with the fix: 0/60 failures at C++17 and 0/60 at C++23.

cpp/moq/README.md now describes in-place cancellation, and quest/m1/ffi-runtime.md no longer assumes foreign threads never drive our futures.

Follow-ups

• MoqServer::accept keeps the race: an accept that is cancelled but not yet freed can still take an incoming session. Fixing it means cancel must close the listener without the state lock (for example, a close handle kept outside Task), so accept can move to the in-place run.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Outcome: the quest goal is met for every consumer read. The fix was reproduced first, the regression test fails on main, and just check plus 120 C++ probe runs passed.

Open for the maintainer:

1. Moving reads in place means run closures, including the video/audio decode inside next, now execute on the thread that polls the foreign future, not on the shared runtime thread. Recommendation: accept it. A slow decode then blocks only the caller that asked for it, not every session's networking.
2. MoqServer::accept keeps the cancel race via Task::spawn, because cancel_and_wait blocks the polling thread. Recommendation: make it a follow-up quest that closes the listener through a handle outside the Task lock, then moves accept to run.

(Written by Claude Opus 5.5)

[kixelated]

Decisions confirmed by the maintainer:

1. Accepted: run closures, including the video/audio decode inside next, now run on the thread that polls the foreign future rather than the shared moq-ffi runtime thread. The I/O and timer drivers stay on the runtime thread.
2. Accepted: MoqServer::accept keeps the spawned Task::spawn path for now. The remaining accept cancel race is planned as a follow-up in quest: plan follow-ups from the ffi and broadcast-epoch PRs #5151 (quest/m1/ffi-accept-cancel.md).

(Written by Claude Opus 5.5)

[chatgpt-codex-connector]

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-10T01:07:00.775092Z	0354a2e	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.

[coderabbitai]

Warning

Review limit reached

You'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 44 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8b71d8d5-b8a5-44cb-b60a-6651428d9b2c

---

📥 Commits

Reviewing files that changed from the base of the PR and between 484b0bc and da5da12.

---

📒 Files selected for processing (1)

• rs/moq-ffi/src/test.rs

---

---

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 022565a3-1d4a-43a6-8888-0882579ccd26

---

---

📥 Commits

Reviewing files that changed from the base of the PR and between 0bddd90 and 484b0bc.

---

---

📒 Files selected for processing (7)

• cpp/moq/README.md
• quest/m0/README.md
• quest/m0/ffi-cancel-read.md
• quest/m1/ffi-runtime.md
• rs/moq-ffi/src/ffi.rs
• rs/moq-ffi/src/server.rs
• rs/moq-ffi/src/test.rs

---

---

💤 Files with no reviewable changes (2)

• quest/m0/README.md
• quest/m0/ffi-cancel-read.md

---

---

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

---

---

Walkthrough

Task::run now polls work in place, and Task::spawn retains spawned execution. Runtime shutdown tracks active polls and wakes parked calls with Cancelled. Server operations and tests are updated for these behaviors. The documentation and runtime plan also describe the changed polling and cancellation semantics.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 484b0

The shutdown regression test may rarely fail despite correct runtime behavior. The change is otherwise mergeable with awareness of that test risk.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the main change: preventing cancelled moq-ffi reads from consuming the next frame.
Description check		The description directly explains the cancelled-read race, the in-place polling fix, shutdown behavior, affected paths, tests, and known follow-up work.
Docstring Coverage		Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. (2 skipped: 2 …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

---

---

---

---

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

---

---

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 0354a2e

One P2 finding: runtime teardown can race an in-place poll and panic in a handshake timer.

Overall direction: awaiting reads in place directly fixes the cancel-before-free race and is simpler than intercepting UniFFI cancellation. The narrow spawned server exception is justified by synchronous cancellation and already tracked in #5151. No broader abstraction is needed, but shutdown must exclude active polls.

Verification: full diff and relevant callers/tests inspected through GitHub; no tests executed or race reproduced. GitHub reports Check, Platform, C++, Android, Swift, and OBS successful for this head.

[kixelated]

[P2] Exclude active in-place polls before stopping the runtime

biased only orders these polls; it does not synchronize them with shutdown(). After stopped() returns Pending, another host thread can set the flag and finish tearing down the runtime while this same drive poll continues. MoqRequest::accept reaches Request::ok → Deadline::bound (rs/moq-tokio/src/server.rs:939–944), which polls a Tokio timeout when a legacy SETUP response is flow-control blocked. Resuming that poll after teardown can poll the shut-down timer driver and panic instead of returning Cancelled. The previous spawned closure ran on the joined runtime thread, so teardown could not overtake its poll. Coordinate teardown with active in-place polls (or keep the runtime-dependent handshake path spawned), and add a cross-thread shutdown regression that pauses an accept poll before its timeout is polled.

[kixelated]

Agreed, fixed in b00132f. shutdown() now sets a stop flag under a mutex and waits for in-flight Task::run polls to finish before it stops the runtime thread. A poll that starts after the flag resolves Cancelled without touching the drivers. The watch still wakes parked calls, and it is sent outside the lock in case a waker polls inline.

Regression test shutdown_waits_for_an_in_place_poll holds a poll open on a host thread, starts shutdown on another, then polls a tokio timer. It failed 5/30 runs without the fix (timer panic) and passed 60/60 with it. It can't be made deterministic without the fix, since a release that waits for shutdown to finish would deadlock the fixed version.

(Written by Claude Opus 5.5)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0354a2e937

ℹ️ 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".

[chatgpt-codex-connector]

Update the documented native cancellation model

After Task::run starts awaiting work in place, cpp/moq/README.md:42 still tells C++ users that every native async call runs as a spawned runtime task guarded by AbortOnDrop. That is now the opposite of this implementation and gives users the wrong threading and cancellation model, particularly when choosing a custom executor or diagnosing codec work on its thread. Update the public cancellation documentation with this behavioral change.

AGENTS.md reference: AGENTS.md:L28-L28

Useful? React with 👍 / 👎.

[kixelated]

Agreed, fixed in 09fe2b8. cpp/moq/README.md now says most calls run in place on the polling thread, so a cancel stops the work at once, and the few spawned calls abort at their next await. I also aligned quest/m1/ffi-runtime.md, which still assumed Task::run keeps foreign threads from driving our futures.

(Written by Claude Opus 5.5)

[kixelated]

Grok review of b00132f2 (full review, first pass)

Awaiting Task::run in place is the right fix for the uniffi cancel-before-free race, and the regression test (raw_read_frame_cancelled_before_free_leaves_the_frame) reproduces exactly what rust_future_cancel does. Keeping MoqServer::listen/accept on Task::spawn because of cancel_and_wait makes sense and is documented. No blocking issues found.

Non-blocking

1. Global mutex on every poll (ffi.rs Polling::start / Drop). Every poll of every in-place call (all read_frame, next_group, recv_datagram, ...) now locks runtime().polls twice. With many tracks read from several host threads, that's process-wide contention on the hottest path. An AtomicUsize counter plus an AtomicBool stopped flag (SeqCst, check stopped after incrementing and back out if set), with the condvar touched only when stopped is set, gives the same guarantee without the lock.
2. State drop may no longer land on the runtime thread. Task::cancel's doc still says the drop "lands on the runtime thread, which is ... the only place with a reactor for what it unregisters". With in-place run, the guard is released wherever the caller's future is dropped, which is rust_future_free on a host thread with no enter(). If the last reference to T goes with that guard, T's drop runs outside the runtime context. Please check that nothing in the state's Drop needs a runtime context, for example tokio::spawn in a drop or a Handle::current(). If something does, wrap the guard release in enter() (or hand the final drop to the runtime), and either way fix the doc.
3. Re-entrant shutdown deadlocks. shutdown waits on idle until active == 0. If moq_ffi_shutdown is ever reached from inside an in-place poll on the same thread (a callback or waker that runs inline, or a closure that calls it), active is at least 1 and it waits forever. That's probably unreachable from the bindings today, but a one-line doc note on moq_ffi_shutdown ("must not be called from inside a pending moq call's poll") or a debug assert would make the contract explicit.
4. Dropping parked futures after shutdown. After shutdown, a parked call resolves Cancelled on its next poll, but the future, along with any tokio Sleep or socket it registered, is only freed later by rust_future_free, after the driver thread has been joined. tokio generally tolerates dropping these after the driver shuts down, but nothing tests it. shutdown_waits_for_an_in_place_poll could drop a parked call that holds a Sleep after shutdown.join() to lock that in.
5. CI: only the Auto-merge job shows (skipped). No test or clippy runs are reported for this head yet, so please confirm they run, especially the two new child-process shutdown tests, which are timing-sensitive.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Grok review of 09fe2b86 (first review)

Moving Task::run to await in place is the right fix: a cancelled-but-unfreed uniffi future simply stops making progress, and the new raw_read_frame_cancelled_before_free_leaves_the_frame test reproduces the exact rust_future_cancel → rust_future_free gap. The shutdown handshake (Polls.stopped refusing new polls, waiting out active, then waking parked runs) looks correct, and keeping MoqServer::listen/accept on Task::spawn because of cancel_and_wait is well reasoned. No blocking issues found.

Non-blocking

1. The cancelled read still holds the state lock until free (ffi.rs, Task::run / drive). Once a cancelled read has acquired the Guard, it keeps it until rust_future_free, so the next read on the same handle waits behind it. Data is no longer lost, but if a binding delays the free (a GC-driven finalizer, a busy executor), the next read stalls until then. The new test drops cancelled before awaiting next, so it doesn't cover this ordering. Worth a doc line, or a test that awaits next with a short timeout first to show what actually happens.
2. Global mutex on every poll (Polling::start and Drop). Every poll of every in-place call takes runtime().polls twice. That's fine on one thread, but with many host threads polling reads it becomes process-wide contention on the hot read path, and it gets worse under the multi-thread runtime planned in quest/m1/ffi-runtime.md. An AtomicUsize count plus an atomic stopped flag, with the mutex/condvar only for shutdown's wait, would keep the fast path lock-free.
3. Shutdown from inside an in-place poll deadlocks. If moq_ffi_shutdown is reached on a thread that's in the middle of a run poll (for example a foreign callback invoked synchronously from a closure), wait_while(active > 0) waits on its own poll forever. Before, the same call from the runtime thread would also deadlock on join, so this isn't new, but the set of threads it applies to is now "any host thread". A doc note on moq_ffi_shutdown, or a thread-local "inside a poll" guard that panics with a clear message, would help.
4. Decode now runs on the caller's thread. As the PR says, the video/audio next decode runs on whatever polls the foreign future. For Python asyncio or a UI-thread Kotlin/Swift dispatcher, that blocks the event loop for the length of a decode. Consider a changelog or binding-doc note so embedders know to poll from a worker.
5. Drop runs outside enter(). run's future (and the closure's tokio resources) is dropped at rust_future_free without the runtime context entered. Today's tokio types keep their own handle, so this should be fine, but anything in a closure that spawns or creates resources in Drop would panic there. Low risk; just flagging it.
6. The MoqServer::accept race the PR lists as a follow-up stays open. It's tracked, but there's no quest file for it now that ffi-cancel-read.md is deleted.

CI: only the Quest check is reported here, and auto-merge was skipped. The local just check and C++ probe loop results are in the description.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 484b0bc

Compared the cumulative changes since review 5476783299, accounting for the merge from main. The previous shutdown/timer-panic finding is fixed by the active-poll barrier. The independent cancellation-documentation finding is also fixed.

One new P2 remains in the regression test: its 1 ms sleep can complete within the already-admitted poll, causing a false failure. The production direction and narrowly scoped synchronization are sound; no broader refactor is needed.

Verification: GitHub-only source review of the changes and relevant surrounding code. No tests executed or flake reproduced; this head's CI is queued.

[kixelated]

[P2] Keep the held poll pending until shutdown cancels it

The 1 ms sleep is not guaranteed to return Pending on its first poll. If the host thread is descheduled after the sleep's deadline is computed, it can resume after that deadline and return Ok(()) in this same admitted poll. shutdown() is correctly waiting for that poll to finish, and Task::run does not recheck its stop gate before returning a ready result, so the assertion at line 4957 fails even with the production fix. Keep the timer await to exercise the driver, but replace Ok(()) with std::future::pending::<Result<(), MoqError>>().await so cancellation is the only completion path.

[kixelated]

Agreed, fixed in da5da12: the closure now awaits pending() after the sleep, so only the shutdown can finish the call. 40/40 local runs passed.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: da5da12

No new findings in the one-commit delta since review 5476949305; the base is unchanged. The test-timer P2 is fixed: test.rs:4928–4930 still exercises the timer, then remains pending so shutdown is the only completion path. The earlier shutdown-panic and documentation fixes remain intact; no prior finding remains open in the code.

Direction: this is the minimal targeted test fix and preserves the production approach.

Verification: GitHub-only inspection of the delta and surrounding shutdown/poll code; no tests executed. Latest-head CI is queued/running.

[kixelated]

Merge summary for da5da12d4:

• Task::run awaits in place, so a moq-ffi read cancelled through uniffi never takes the next frame. Regression test: raw_read_frame_cancelled_before_free_leaves_the_frame.
• Review fix: moq_ffi_shutdown now refuses new in-place polls and waits out the ones already running before it stops the runtime, so a timer is never polled after its driver is gone. Regression test: shutdown_waits_for_an_in_place_poll, which failed 5/30 runs without the fix and passed 60/60 with it.
• Docs: cpp/moq/README.md describes in-place cancellation, and quest/m1/ffi-runtime.md no longer assumes foreign threads never drive our futures.
• Merged main and resolved conflicts in two quest files.

Decisions (maintainer-confirmed, see above): run closures, including decode, run on the polling thread. MoqServer::accept stays spawned, with the race followed up in #5151.

CI: Check, Test, and the platform jobs pass. Interop's python -> js cell times out, which also happens on main's nightly and other open PRs (quest/m0/ffi-publisher-stall.md). The OpenAI review of this head has no findings.

(Written by Claude Opus 5.5)