Repository navigation
feat(quic)!: report acknowledged send-stream offsets, replacing Checkpoint with offsets - #5817
Conversation
Checkpoint::acked returns the ACK-delay-corrected instant once the peer acknowledged everything before the checkpoint, and fails once it never will. StreamEvent::Acked reports the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: (Written by Opus 5.5) |
…progress # Conflicts: # rs/moq-quic/src/connection/streams/mod.rs # rs/moq-quic/src/connection/streams/send.rs # rs/moq-quic/src/connection/streams/state.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
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. |
|
Warning Review limit reachedYou'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 21 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe QUIC send-stream API replaces checkpoint-based resets with explicit byte offsets. The tokio and uring adapters now track WebTransport header boundaries as offsets. The API also adds acknowledged-prefix queries and Acked events, with tests for acknowledgment progress and stream closure. Documentation and quest descriptions are updated for the reset and acknowledgment APIs. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 88.61% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 19 files. (6 skipped: 6… |
| 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 summarizes the primary change: replacing Checkpoint with acknowledged send-stream offset APIs. |
| Description check | |
The description directly explains the motivation, API changes, behavior, compatibility impact, verification, and follow-ups for the changeset. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 88.61% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 19 files. (6 skipped: 6 unsupported.)
✨ 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.
Comment @coderabbitai help to get the list of available commands.
![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: ede4d627e9
ℹ️ 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".
An ACK that repeats its largest packet only covers reordered packets, so its delay is not theirs to subtract. A reset that leaves nothing to acknowledge fails a checkpoint taken before any write, and reports it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a1d70683d
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a867b462ac
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da616919d
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e6caef19
ℹ️ 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".
An ACK that confirms an ACK_FREQUENCY frame raising max_ack_delay was capped at the old value, so the checkpoint instant and the RTT sample looked late. Read the cap once the acknowledged packets are processed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Opus 5.5) |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…progress # Conflicts: # rs/moq-quic/src/tests/mod.rs
|
@codex review (Written by Opus 5.5) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
# Conflicts: # rs/moq-quic/MIGRATE.md
One ACK assigns the largest packet an earlier instant (less its ACK delay) than the packets before it, and a range can wait behind a gap. Keep each acknowledged range's instant and resolve each checkpoint at the latest instant among the ranges below it, so the prefix never completes before its last piece arrived. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02826ded72
ℹ️ 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".
`last = stream.checkpoint()` drops the old checkpoint after pushing the new one, so pruning the back never saw it and dead entries piled up while nothing was acknowledged. Sweep the queue whenever it doubles past the live entries the last sweep kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b54745d2ef
ℹ️ 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".
Keying an ACK's instant by its range start kept a short retransmission's instant for a longer frame from the same offset, so a checkpoint past the extension resolved before its last bytes were acknowledged. Record the instant for each newly covered sub-range instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a27124766
ℹ️ 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".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ly when polled
`Checkpoint` is back to a stream id and offset. `Checkpoint::acked(&mut Connection)`
returns whether the stream's contiguous acknowledged prefix reached it, and a `false`
stores the lowest polled offset on the stream, so the ACK that crosses it reports
`StreamEvent::Acked { id, offset }`. This replaces the per-checkpoint `Arc<OnceLock>`
slots, the `Weak` watch queue and its sweeping, and the per-range ACK instants.
The ACK-delay-corrected instant is dropped: the caller records the time when woken.
`SendStream::checkpoint` takes `&self` again.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
kixelated
changed the title
|
Reworked per maintainer review: (Written by Opus 5.5) |
`SendStream::offset()` returns how much was written, `SendStream::acked(offset)` reports whether the peer acknowledged everything before it, and `SendStream::reset_at(offset, code)` replaces `Checkpoint::reset`. An offset past the written data panics. moq-tokio's `Checkpoint` wrapper becomes `SendStream::reset_at`, and both WebTransport adapters keep their header's end as a `u64`. The 0-RTT generation guard goes with `Checkpoint`: every other stream op is already keyed by a bare id, and the adapters check 0-RTT rejection themselves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
…gress # Conflicts: # rs/moq-quic/MIGRATE.md # rs/moq-quic/README.md # rs/moq-quic/src/connection/mod.rs # rs/moq-quic/src/lib.rs # rs/moq-quic/src/tests/reset_stream_at.rs # rs/moq-tokio/src/quic/driver/mod.rs # rs/moq-tokio/src/quic/web/session.rs
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
moq/rs/moq-quic/src/connection/streams/mod.rs
Line 458 in be05f32
Notify waiters when reset invalidates their offsets
If acked(20) returned false and another caller then invokes reset_at(10, ...), this truncation makes the next acked(20) return ClosedStream, but reset_at queues no stream event and reset streams never emit Finished, so an event-driven waiter can remain asleep indefinitely. Fresh evidence beyond the prior checkpoint fix is that the new offset implementation retains Send::notify across this reset while the added reset test only registers after resetting; wake registered waiters when a reset makes their offsets impossible.
ℹ️ 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".
| if self.stop_reason.is_some() || self.notify.is_none_or(|notify| acked < notify) { | ||
| return None; | ||
| } | ||
| self.notify = None; |
There was a problem hiding this comment.
If callers register acked(10) and acked(20) before either completes, an ACK that advances the prefix only to 10 makes take_acked clear the sole notify slot. The emitted event answers the first waiter, but the second remains asleep, and a later ACK through 20 emits nothing because its registration was discarded. Preserve outstanding offsets above the acknowledged prefix rather than clearing every registration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by documenting the contract rather than keeping more offsets: the stream holds only its lowest waiting offset, and StreamEvent::Acked asks every waiter on the stream to poll again, so one past the reported offset registers anew. later_waiter_registers_again covers the 10-then-20 case (bfc9223).
(Written by Opus 5.5)
|
|
||
| Each entry is a change from `quinn-proto`. | ||
|
|
||
| ### `StreamEvent::Acked` (#5817) |
There was a problem hiding this comment.
This migration entry covers only the new StreamEvent::Acked, but the same commit removes the public Checkpoint type and SendStream::checkpoint from both moq-quic and moq-tokio and replaces their reset flow with byte offsets and reset_at; existing consumers therefore stop compiling without any documented replacement, and moq-tokio's MIGRATE.md is not updated at all. Add before/after guidance to every affected package's migration file.
AGENTS.md reference: AGENTS.md:L60-L62
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining: Checkpoint and SendStream::checkpoint came in with #5690, which no release contains yet (it is not on release), so no released consumer breaks. moq-quic's MIGRATE.md lists changes from quinn-proto, which has neither, and moq-tokio's lists changes since its last release. The StreamEvent::Acked entry is the one break that applies.
(Written by Opus 5.5)
A waiter registered by `SendStream::acked` slept forever when a reset dropped its offset or the peer sent STOP_SENDING, since only ACKs reported `StreamEvent::Acked`. Both now report it, and the docs say every waiter on the stream polls again on the event, since the stream holds only the lowest waiting offset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
|
Codex's reset finding (review on be05f32): fixed in bfc9223. A reset that drops a waiting offset, or STOP_SENDING, now reports (Written by Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfc9223d5d
ℹ️ 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".
| if acked < notify && self.stop_reason.is_none() && notify <= self.pending.offset() { | ||
| return None; |
There was a problem hiding this comment.
When callers register offsets 5 and 20 before either is acknowledged, then reset_at(10, ...) truncates the stream, the stored notify remains 5. This condition returns None because offset 5 is still achievable, so the reset emits no Acked event even though acked(20) has changed to Err; that waiter sleeps until an unrelated ACK reaches 5. Fresh evidence beyond the prior single-waiter reset case is this mixed valid-and-invalid registration: retain enough registration state to detect the dropped higher offset, or emit an event whenever reset lowers the maximum answerable offset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f58a1c5: a reset or STOP_SENDING now reports to every waiter whenever one is registered, rather than only when the stored lowest offset becomes unreachable. reset_reports_a_dropped_waiter now registers a kept lower offset beside the dropped one, and fails under the old rule.
(Written by Opus 5.5)
The stream holds only its lowest waiting offset, so a reset that keeps it but drops a higher one reported nothing until the lower one was acknowledged. A reset or STOP_SENDING now always reports to the stream's waiters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58a1c51ae
ℹ️ 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".
| match self.state.send.get(&self.id) { | ||
| Some(Some(stream)) => stream.offset(), | ||
| // Not written yet, or already closed, where the reset fails anyway | ||
| _ => 0, |
There was a problem hiding this comment.
When a stream writes 100 bytes and then calls reset_at(10, ...), Send::reset truncates the pending buffer to 10 while the send entry remains alive until the reset is acknowledged, so this method returns 10 rather than the documented number of bytes written. This makes offset() decrease and causes callers sampling it after a reset to mistake the Reliable Size for the total written offset; active reset streams should report their stored final_size instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Documented rather than changed, in c588775: after a reset, offset() reports the bytes the reset still delivers. It was never monotonic, since it is zero once the stream is gone, and after a reset no write follows, so the Reliable Size is the useful answer. reset_reports_a_dropped_waiter pins it.
(Written by Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2794e60823
ℹ️ 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".
| if let Some(offset) = stream.take_acked() { | ||
| self.events.push_back(StreamEvent::Acked { id, offset }); | ||
| } |
There was a problem hiding this comment.
When an offset retained by reset_at is awaiting acknowledgment and the reset frame has already been acknowledged, the final STREAM ACK queues this event but then immediately removes the reset stream because finished is true. The documented response to StreamEvent::Acked is to poll acked(offset) again, but that poll now returns ClosedStream, making a successfully acknowledged reliable prefix indistinguishable from a failed one. This also occurs when one packet contains both frames because on_packet_acked processes reset metadata before STREAM metadata; retain enough terminal acknowledgment state for the follow-up poll or otherwise keep the successful result queryable.
Useful? React with 👍 / 👎.
| self.notify = Some(self.notify.map_or(offset, |notify| notify.min(offset))); | ||
| Ok(false) |
There was a problem hiding this comment.
When acked(offset) returns false for a 0-RTT stream before the handshake completes, this registration can never fire if the server rejects early data: StreamsState::zero_rtt_rejected removes the entire Send, including notify, without emitting StreamEvent::Acked, while the connection remains open. A caller waiting according to this method's promise that an event is reported once the answer changes therefore sleeps indefinitely; rejection needs to report or otherwise preserve every registered acknowledgment waiter.
Useful? React with 👍 / 👎.
|
Merge summary: moq-quic reports acknowledged send-stream offsets, and byte offsets replace
Decisions, all made by the maintainer, are in the description. On review: the two reset/STOP_SENDING wake gaps are fixed, (Written by Opus 5.5) |
Problem
Quest:
quest/m2/quic-ack-progress.md.moq-quic gave callers no way to learn whether the peer has acknowledged the data written to a send stream. The ACK hook (
quest/m2/quic-ack-hook.md) and frame-granularity starvation need this. While answering that, theCheckpointhandle from #5690 turned out to add little over a byte offset, so this replaces it.Approach
SendStream::offset() -> u64: how many bytes were written so far.SendStream::acked(offset) -> Result<bool, ClosedStream>comparesoffsetagainst the stream's contiguous acknowledged prefix. Afalsestores the lowest polled offset on the stream (oneOption<u64>), so a waiter is registered only when polled.StreamEvent::Acked { id, offset }fires when the answer changes for that offset: an ACK advances the prefix past it, or STOP_SENDING or a reset makes it fail. It carries the prefix. Every waiter on the stream polls again, and one past the prefix registers anew.SendStream::reset_at(offset, code)replacesCheckpoint::reset, andreset(code)isreset_at(0, code). moq-tokio's driverCheckpointbecomesSendStream::reset_at. Both WebTransport adapters keep their header's end as au64.reset_atandacked. That keeps the guaranteeCheckpointgave, at runtime instead of in the type. On a reset stream,ackedpast its Reliable Size is an error instead, because that data was legitimately dropped.ackedfails (Err(ClosedStream)) when:StreamEvent::Finished, which already means everything was acknowledged, and a polled offset gets itsAckedbefore thatFinished.Impact
Public API (
moq-quicis unpublished;Checkpointwas never in a moq-tokio release):moq-quic:SendStream::{offset, acked, reset_at}andStreamEvent::Acked { id, offset }.StreamEventis notnon_exhaustive, so exhaustive matchers need an arm.CheckpointandSendStream::checkpoint, along with the 0-RTT generation guard they carried. Every other stream op is already keyed by a bare id. moq-tokio checks 0-RTT rejection on every stream op, and moq-uring has no 0-RTT.moq-tokio:quic::driver::CheckpointandSendStream::checkpointare replaced bySendStream::reset_at.moq-uring: internal only.Wire: none.
Cost: one
Option<u64>per send stream, and one comparison per acknowledged stream frame.Decisions
Round 2 (2026-10-17):
SendStream::acked(offset), withSendStream::reset_at(offset, code)andSendStream::offset() -> u64replacingCheckpoint.Checkpoint::acked(conn).SendStream::acked(&Checkpoint).quest/m1/quic/group-reset.md's earlier choice ofCheckpointover a byte count. That quest now says the moq-net reset takes an offset that panics past the written data.Round 1, on review of the first version (2026-10-17):
Arc<OnceLock>slots, theWeakwatch queue and its sweeping, and the per-range instant map.poll_ackedrecords theInstantwhen woken. This removes the ACK-delay plumbing inon_ack_received: the largest-packet rule, the ACK_FREQUENCY cap ordering, andpeer_ack_delay.starvation-frames:quest/m3/quic-gcc.mdalready covers a spike.Carried over from the first version:
acked(offset)answers "is X acknowledged".acked(accepted 2026-10-16), even though in-flight data may still be acknowledged, because the peer will not read it.Superseded: "every checkpoint watches", which registered eagerly so a freed stream could still report an instant. That reason went away with the instant.
Verification
moq-quictests (tests::acked):Acked, thenFinished, then is freed.ackedandreset_atpast the data written panic.tests::reset_stream_atis ported to offsets. Its 0-RTT-generation and wrong-connection tests are removed along with those guards.ResetAtandAckedon offsets clamped to the data written.cargo nextest run -p moq-quic --all-features: 647 passed.cargo clippy -p moq-quic -p moq-tokio -p moq-uring --all-features --tests --benches -D warnings: clean.just checkpasses on the merged head before the review fix (5252 Rust tests, lint, feature checks,quest check). The fix touches only moq-quic, which passes nextest and clippy on its own.Follow-ups
quest/m2/quic-ack-hook.mdis unblocked. Its plan points atSendStream::acked(offset)andStreamEvent::Acked, with the trait taking a byte offset.(Written by Opus 5.5)
🤖 Generated with Claude Code
https://claude.ai/code/session_013bQzzUiJPvi9kFCtMqbtqC