test(uring): pin down what a send staged on the way out is owed - #3153
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3720385fd2
ℹ️ 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".
| /// | ||
| /// This only stages an SQE, so the datagram reaches the kernel when the |
There was a problem hiding this comment.
When the submission queue lacks enough free slots, particularly for a non-GSO send split into multiple datagrams, Shared::push calls ring.submit() inline to make room. Thus TxBuf::send can enter the ring before returning rather than waiting for the worker's next turn, and the new single-datagram test with the default deep queue never exercises this path. Narrow the documented guarantee to SQEs that remain staged, or cover and guarantee the inline-submission case separately.
AGENTS.md reference: AGENTS.md:L109-L109
Useful? React with 👍 / 👎.
#3141 claims a datagram handed to `udp::Socket` on the last turn before `block_on` returns is lost when the worker is dropped, because `TxBuf::send` only stages an SQE and `Shared::push` only enters the ring when the submission queue fills. It is not: `Worker`'s drop submits whatever is still staged and then waits for the completions, so the packet goes out. Measured on Linux 6.19, a client that dials, closes, waits for the close to be published and then stops the runtime outright is seen closing by its peer in 26ms, application code and reason intact. What does get lost is a `close` the worker is never driven past: the driver task is what turns the recorded code into a CONNECTION_CLOSE packet, so stopping the worker on the call that asked for the close means no packet is ever built and the peer idles out 10s later. That is a layer above the socket and no completion-aware `send` reaches it. Both guarantees now have a test, and the contract is written down where each half of it lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3720385 to
3afabc2
Compare
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. |
1 participant
Closes #3141, by disproving it.
Summary
udp::Socketon the last turn beforeblock_onreturns never reaches the kernel, so dropping the worker loses it. Measured on Linux 6.19 (podman, real io_uring): it does reach the kernel.Worker's drop submits whatever SQEs are still staged (pump->submit) and then waits for their completions before freeing anything the kernel may still read, so the packet goes out. The staging is real; the loss is not.close(42, "done"), wait for the close to be published, stop the runtime outright. The peer seesApp { code: 42, reason: "done" }26ms after accepting, not the 10s idle timeout. It cannot lose the CONNECTION_CLOSE, because quiche only reportsis_closed()a draining period after that packet was sent, so the close is published many turns after the send was submitted.TxBuf::sendwould therefore buy nothing, and it would put a per-send handle on the flush hot path.closethe worker is never driven past.closeonly records the code on the connection; the driver task is what frames the CONNECTION_CLOSE and hands it to aTxBuf. Stop the worker on the call that asked for the close and no packet is ever built, so the peer idles out (measured: 10.005s,TimedOut). No socket API change reaches that, since there is nothing staged to complete. The remedy is to drive until the close is published, whichmoq_net's session machine already does.So this lands as tests and a written-down contract rather than an API change: the guarantee
Worker's drop provides was accidental and undocumented, which is what made the bug look real.tests/teardown.rspins both halves: a raw datagram staged by the last turn still reaches the wire, and a published close has already left a client that then stops.udp::TxBuf::send,Worker, andquic::Connectionnow say which half of the window each of them owns.Public API changes
None. Doc comments and a new integration test only.
Wire behavior changes
None.
Test plan
In the podman container on Linux 6.19 (real io_uring, no skips):
cargo test --locked -p moq-uring -j 1-- 44 tests, all pass, including the two new ones.cargo clippy --locked -p moq-uring --all-targets -j 1 -- -D warnings-- clean.RUSTDOCFLAGS="-D warnings" cargo doc --locked -p moq-uring --no-deps-- clean.cargo fmt -p moq-uring -- --check-- clean.On the host:
just checkandjust testboth pass (263 tests). macOS compilesmoq-uringto nothing, so the container runs above are what actually covered this crate.The two disproof runs, for the record:
Cross-package sync
No row applies: no wire format, no
moq-ffisurface, no CLI flag.(Written by Claude Opus 5)