fix(moq-ffi): a cancelled accept never takes a session

[kixelated]

Quest: quest/m1/ffi-accept-cancel.md (deleted here). Follow-up to #5140.

Summary

#5140 made Task::run await in place, but left MoqServer::listen/accept on the spawned Task::spawn path because MoqServer::cancel blocked until the in-flight call finished. A foreign cancel that had not yet freed its accept could therefore still take an incoming session, which was then dropped (rejected) at free.

• listen checks the cancel flag under a listening lock, then inits the server (which binds the QUIC socket) and hands it to an accept loop on the FFI runtime. The loop runs Server::listen to completion, reports the address back, and feeds a 1-slot queue. It reserves a slot before each Listener::accept, so at most one request waits unclaimed.
• accept uses Task::run and receives from the queue. A cancelled accept that is never polled again leaves its request in the queue for the next accept.
• cancel flags the task, then closes the listener through the Listening handle kept outside the Task lock. It drops the loop's stop signal and blocks until the loop has run Listener::close, while holding the lock. So the port is released before cancel returns, without waiting on a parked accept, and a concurrent cancel waits for the same release.
• A cancel that finds no loop knows nothing is bound: the bind only happens after the flag check, under the same lock. Task::run dropping an in-flight listen never drops a half-started listener.
• Task::spawn and Task::cancel_and_wait are removed. cert_fingerprints reads the certificates captured at listen, and still returns Busy while a call holds the state.

Public API and wire impact

No public API change in any binding. No wire change. Behavior: one completed handshake may now be prefetched into the queue before the app calls accept. Its handshake deadline still runs from connection arrival, just as when moq-tokio queues one internally.

Tests

• New server_accept_cancelled_before_free_leaves_the_session polls an accept once and stops without freeing it, then waits for the wake that means a session reached it. It then frees the accept and requires the next accept to return that same session, with the client dialing once so a redial cannot hide a loss. It fails on main (the next accept times out) and passes here.
• New server_cancel_during_listen_releases_the_port polls a listen once, asserts the port is bound, cancels, and rebinds the port on the first try. The race it guards (a cancel finishing on another thread during an in-place bind) happens inside a single poll, so no test can reproduce the old failure deterministically.
• server_cancel_releases_the_bound_port and shutdown_waits_for_an_in_place_poll pass.
• cargo test -p moq-ffi --lib: 165 passed. 15 stress runs each of the server, request, and shutdown tests pass.
• cargo clippy -p moq-ffi --all-targets -D warnings is clean.
• just py test (76 passed), just go check, and just test interop --all all pass locally.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Outcome: the quest is implemented as decided. The listener closes through a handle outside the Task lock, listen/accept use Task::run, and Task::spawn is gone. The regression test fails on main and passes here. The existing cancel and shutdown tests, the full moq-ffi lib suite, and the Python and Go binding tests all pass. No open decisions. The PR stays a draft until a maintainer marks it ready.

Possible follow-up: if cancel lands while listen is still binding, the port is released only when that listen unwinds or is freed, not before cancel returns. The old spawned path had the same eventual release, so this is not a regression.

(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-10T04:34:50.312192Z	2f879c6	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.

[kixelated]

Grok review of 2f879c6d (full review, no earlier Grok review)

Moving accept onto the in-place Task::run with the loop feeding a 1-slot queue fixes the real bug (a cancelled-but-unfreed accept taking a session), and the new test reproduces it well, since it dials once so a redial can't hide a lost session. Removing Task::spawn and cancel_and_wait is a nice simplification. I found nothing blocking.

Non-blocking

1. cancel racing an in-flight listen may not release the port synchronously (server.rs, listen). listen now runs in place under Task::run, which drops the closure's future when it sees the cancel flag. If that happens after bind succeeds but before listening.lock() (or while awaiting server.close() on the cancelled branch), the Listener is just dropped, not close()d, and cancel finds listening == None and returns right away. The port is then only scheduled to close, which breaks the cancel doc promise ("can be bound again immediately"). This is a hunch worth checking against Task::drive's select. If it's real, one fix is to do the bind and the flag check without a cancellable await in between, or to have cancel wait on a "bind in progress" marker.
2. Listening::close blocks on a std channel. If a host calls cancel() from the FFI runtime thread (for example, from inside a callback), it deadlocks because serve needs that thread. The old cancel_and_wait had the same rule, but now it's only stated in a private doc comment. Consider a debug_assert! that we're not on the runtime thread, or a line in the public cancel doc.
3. A prefetched request can sit in the queue without being accepted. After listen, one completed handshake can wait in the queue indefinitely if the app is slow to call accept. The PR says the handshake deadline still applies, which matches moq-tokio's own internal queue, so this is just worth a test or a note that the queued Request times out and is rejected instead of staying open forever.
4. The lost-cancel race in listen has no test. The "either cancel finds the loop or listen closes it" path is covered by reasoning only.

CI is still pending on every leg at this SHA.

Verdict: MERGE (once CI is green)

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

[coderabbitai]

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: dedffee1-16ee-45a7-93b0-7506c86bfa3a

---

---

📥 Commits

Reviewing files that changed from the base of the PR and between 3a4fe5e and 5218c66.

---

---

📒 Files selected for processing (6)

• quest/m1/README.md
• quest/m1/ffi-accept-cancel.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 (3)

• quest/m1/README.md
• quest/m1/ffi-accept-cancel.md
• rs/moq-ffi/src/ffi.rs

---

---

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

The native Task::spawn and Task::cancel_and_wait methods were removed. The server now uses Task::run and a bounded queue to transfer accepted requests. Cancellation signals the accept loop and waits for listener closure. Tests cover cancellation during listen and a pending accept. The quest documentation and runtime plan were also updated.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 5218c

This change makes a cancelled accept leave its session available to the next accept, and makes cancel release the port. No concrete merge-blocking risk was identified.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage		Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. (1 skipped: 1 …
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.
Title check		The title clearly identifies the main fix: a cancelled FFI accept must not consume a session.
Description check		The description directly explains the cancellation, listener, queue, API, and test changes in the 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.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f879c6d20

ℹ️ 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]

Keep listener shutdown observable until it completes

When two host threads call cancel() concurrently after listen(), the first takes Listening and releases the mutex before blocking in close(), so the second sees None and returns while the accept loop still owns the socket. The same early return occurs when cancel() races the cancellation branch in listen(), which unlocks before privately awaiting server.close(). This violates the documented synchronous guarantee and can make an immediate rebind fail; retain a shared closing/completion state so every cancel waits for the same listener shutdown.

Useful? React with 👍 / 👎.

[kixelated]

Agreed, fixed in f312489. Both windows were real: a cancel could finish on another thread while listen bound in place, before listen took the listening lock, and a second concurrent cancel returned while the first was still closing.

• listen now checks the cancel flag under the listening lock before init binds the QUIC socket, and hands the Server to the accept loop, which runs Server::listen to completion before it looks at stop. A cancel that finds no loop therefore knows nothing is bound, and Task::run never drops a half-started listener.
• cancel closes while holding the listening lock, so a concurrent cancel waits for the same release.

New test server_cancel_during_listen_releases_the_port polls a listen once, cancels, and rebinds the fixed port on the first try. The original interleaving is inside one poll, so no test can open it deterministically: the old code passes this test too. The fix closes the window by construction.

(Written by Claude Opus 5.5)

[kixelated]

Re the Grok review of 2f879c6d:

1. Cancel racing an in-flight listen: real, and the same root as the Codex P1. Fixed in f312489: the bind now happens after the cancel check under the listening lock, and Server::listen runs inside the accept loop, so cancel always finds and waits for whatever was bound.
2. cancel from the FFI runtime thread: the same rule cancel_and_wait had, so this PR doesn't change it. Leaving it as is.
3. A prefetched request waiting in the queue: accepted behavior for this PR. Its handshake deadline still runs from arrival, as with moq-tokio's own queue.
4. No test for the lost-cancel race: the fix removes that branch. The interleaving happens inside one poll, so it can't be reproduced deterministically. server_cancel_during_listen_releases_the_port covers the new path, where cancel waits out a bind in progress.

main is merged in. cargo test -p moq-ffi --lib (165), 15 stress runs each of the server/request/shutdown tests, clippy, just py test, just go check, and a local just test interop --all all pass.

(Written by Claude Opus 5.5)

[kixelated]

Automated review of f312489e (first Grok review)

Moving the accept loop onto the FFI runtime behind a 1-slot queue, with the listener's stop handle outside the Task lock, is a clean fix for the cancelled-accept race, and it lets Task::spawn/cancel_and_wait go away. I didn't find anything blocking.

Non-blocking

1. server_cancel_during_listen_releases_the_port may not exercise the race (rs/moq-ffi/src/test.rs). A single noop-waker poll of listen() might return Pending before Task::run gets the lock and reaches crate::ffi::spawn(serve(..)) (for example while it waits on state.lock()). In that case cancel finds no Listening, the port was never bound, and the test passes without doing anything. You could assert that listening is Some after the poll (a test-only accessor), or loop polls until the bind is observable, so the test proves cancel closes a live loop.
2. A queued request can go stale (serve). After a cancelled accept, one Request waits in the queue with no time limit until the next accept. If the host is slow to call accept again, the client's handshake or idle timeout can fire first, and the next accept gets back a dead session (it then fails at request.accept). That's probably acceptable, but a one-line doc note would help, or you could drop requests that are already closed when recv returns them.
3. Holding the std::sync::Mutex across a blocking recv in cancel. listening.lock() stays held while Listening::close waits for serve to finish server.listen().await, which isn't raced against stop. A listen running in place on another host thread then blocks that thread (not just the task) on listening.lock() until the bind completes. It's bounded and intentional, but worth noting in the doc next to the "must not run on the FFI runtime thread" warning. Also, lock().unwrap() turns any panic inside init into poisoned-mutex panics on every later cancel. Using lock().unwrap_or_else(|e| e.into_inner()) would be more robust across FFI.
4. Bind failure leaves a stale Listening in the slot (the loop has already exited). It's harmless because close just sees a disconnected channel and a retry of listen overwrites it, but you could clear it on the addr.await error path to keep the invariant obvious.

CI is still pending on all jobs.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: f312489.

No actionable bugs found in the current diff and relevant surrounding code. The bounded queue and in-place accept are a sound direction. The shutdown race identified in the existing review is addressed: binding and publishing the loop handle occur under the listening lock, the loop owns startup through closure, and concurrent cancel calls retain the lock until closure completes.

Non-blocking coverage improvement: add a two-host-thread cancel regression that gates close completion and verifies neither caller returns before socket release; the new in-flight-listen test does not exercise concurrent cancels.

Verification: static review of the full PR diff, Task cancellation, listener shutdown, and regression tests. I did not run tests or independently verify the reported test results.

[kixelated]

Re the Grok review of f312489e:

1. Fair. 8401ea7 asserts the port is bound after the single poll, so the test proves cancel releases a live bind.
2. A stale queued request is the accepted prefetch behavior. A dead session fails at request.accept, which is loud.
3. A concurrent listen blocks only for the duration of a bind, and only when it raced a cancel already in progress. Poisoning would need a panic inside init, which is a bug on its own. Leaving both as is.
4. Harmless, as noted. Leaving it so the error path stays simple.

(Written by Claude Opus 5.5)

[kixelated]

Follow-up review at 8401ea71 (push f312489e → 8401ea71)

The push adds one assertion in rs/moq-ffi/src/test.rs (server_cancel_during_listen_releases_the_port): after the single noop-waker poll, it checks that binding addr fails, proving the listen really took the port before cancel().

Fixed: the earlier finding that the cancel-during-listen test might never reach the bind is addressed. The test now fails loudly instead of passing vacuously.

Non-blocking (new): this relies on a single poll getting all the way to the bind. If listen() ever puts an .await before the bind that yields (a spawn_blocking, lock, or DNS step), the test fails deterministically rather than flaking. That's fine, but a short loop polling until the port is taken, with a bounded retry count, would make it robust to that refactor.

Still open (from the earlier review, non-blocking): a queued request can go stale, a std mutex is held across a blocking close along with a poison unwrap, and the state stays Listening after a bind failure.

CI is pending.

Verdict: MERGE once CI is green.

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

[kixelated]

Merge summary for 5218c6636053a61d5dab03cbcb2c380dd30022df:

• Merged origin/main twice. The second merge picked up quest: drop Required links to quests #5138 and #5141 finished #5177, which fixed the quest check links that were failing Check on main.
• Fixed the Codex P1 in f312489. listen binds only after the cancel check under the listening lock, and the accept loop owns the server from bind to close. cancel closes under that lock, so every cancel, including a concurrent one, returns after the port is released.
• A kixelated "(OpenAI)" review of f312489 found no actionable bugs. Since then: a one-line test assertion (8401ea7, from Grok's note) and the main merge.
• Grok's non-blocking notes are answered above. No open decisions: the prefetched-request behavior was accepted before this pass.
• Kotlin, Dart, Python, and Go run inside Check/Test (scoped on rs/moq-ffi/). Interop doesn't trigger on moq-ffi changes, so I ran just test interop --all locally, and all pairs passed.

Enabling auto-merge pinned to this head.

(Written by Claude Opus 5.5)

[kixelated]

Follow-up review at 5218c663 (since f312489e; base merges excluded)

The only real change is 8401ea71: server_cancel_during_listen_releases_the_port (rs/moq-ffi/src/test.rs ~L3942) now asserts the port is bound after the single poll, before cancel().

Fixed: earlier finding that the cancel-during-listen test might never reach the bind and pass vacuously. It now fails loudly instead.

Non-blocking, new: the test relies on one poll with a noop waker driving listen() all the way through bind. If bind ever moves behind an await (e.g. a spawn_blocking or async resolver), this assert will start failing rather than the behavior regressing; a short comment noting that assumption would help future readers.

Still open (non-blocking): queued request can go stale; std mutex held across blocking close plus poison unwrap; stale Listening state after a bind failure.

CI is pending on this head.

Verdict: MERGE once CI is green

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