Repository navigation
fix(net): a subscribe update racing a finished track ends it cleanly - #5833
Conversation
A publisher that finishes a track stops reading the subscribe stream, so a SUBSCRIBE_UPDATE written at that moment fails. The subscriber failed the track with the write's error instead of the publisher's finish. A failed update write now defers to the subscribe stream's responses in JS and Rust: the FIN ends the track cleanly, and a reset still fails it. The Rust Writer drops its buffer on a write error, so a later finish on the dead stream doesn't trip the unflushed-bytes assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: the quest is done in this PR, with no open decisions. It follows the rule decided on 2026-10-16: a failed update write defers to the subscribe stream's own outcome, in JS and Rust, with no retry or timeout. The race reproduced in both languages. Each regression test fails without the fix. Not run here: the browser legs of (Written by Claude Opus 5.5) |
|
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 10 minutes. View limit details
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 |
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. |
![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: 8296627fd6
ℹ️ 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".
| } catch { | ||
| return; |
There was a problem hiding this comment.
On pre-Lite-06 connections, an update whose start group is Number.MAX_SAFE_INTEGER reaches this catch as a local RangeError, not a stopped-stream error: groupBounds accepts that safe integer, but encodeStartGroup adds one for the older wire format and Writer.u53 rejects the resulting value. Returning here leaves the track open while silently ignoring its requested bound; restrict suppression to transport write failures, or reject the unencodable bound before this catch, so codec errors still surface.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 074066a. The update is now encoded into memory before the stream write, and only a failed write defers to the responses. An unencodable bound (your lite-05 MAX_SAFE_INTEGER start) fails the track again, covered by an update whose bound the wire can't carry fails the track. Rust already keeps buffer_update (encode, propagated with ?) apart from the flush, so it needed no change.
(Written by Claude Opus 5.5)
Encode SUBSCRIBE_UPDATE apart from the stream write, so only a failed write defers to the responses. A bound the encoder refuses (a lite-05 start group of MAX_SAFE_INTEGER) fails the track again instead of being dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 074066a9ce
ℹ️ 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".
| try { | ||
| for (const chunk of encoded) await stream.writer.write(chunk); | ||
| } catch { | ||
| return; |
There was a problem hiding this comment.
When the failed update lowers the start after SUBSCRIBE_START, this return defers to the publisher's END/FIN even though entry.tail.demand(...) and the producer's current subscription already reflect the update that the publisher never received. #settleTail then treats those newly requested groups as owed and waits for its grace, which can be an arbitrarily large maxDelay, before closing an otherwise finished track. Preserve the last successfully written bounds and delay, or roll back the unsent update state before deferring to the responses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but I'm keeping it out of this PR. It predates the failed-write path: the owed range comes from the current demand, not what the publisher acknowledged. So a lowered-start update that crosses SUBSCRIBE_END on the wire, with a write that succeeds, already makes the tail wait out its grace the same way. It only affects lite-05/06 (lite-07 counts streams), and the track still ends cleanly, at worst one grace late. Before this PR, the same race failed the track outright. Rolling back state only on a failed write would cover one of the two crossings. The fix belongs in how the tail bounds its debt once the publisher has ended (demand made after its END owes nothing), in both languages. I've suggested that as a follow-up.
(Written by Claude Opus 5.5)
| Poll::Ready(Err(err)) => { | ||
| // The stream is broken; drop it (the writer resets) and | ||
| // hand the track back. | ||
| serve.subscriber.remove_subscribe(active.id); | ||
| self.sub = Sub::None; | ||
| return Poll::Ready(ServeEnd::GiveBack(err)); | ||
| tracing::debug!(track = %serve.name, %err, "subscribe update dropped") | ||
| } |
There was a problem hiding this comment.
When a failed update lowered the floor after SUBSCRIBE_START, begin_subscription has already changed active.start, restarted the tail debt, and applied the new grace before this error is ignored. The subsequent END/FIN therefore enters ServeMode::Tail with groups the publisher never agreed to send and can delay a clean track end for the full, caller-controlled max-delay grace. Commit this state only after a successful flush, or restore the previously sent state on this error before processing responses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but I'm keeping it out of this PR. It predates the failed-write path: the owed range comes from the current demand, not what the publisher acknowledged. So a lowered-start update that crosses SUBSCRIBE_END on the wire, with a write that succeeds, already makes the tail wait out its grace the same way. It only affects lite-05/06 (lite-07 counts streams), and the track still ends cleanly, at worst one grace late. Before this PR, the same race failed the track outright. Rolling back state only on a failed write would cover one of the two crossings. The fix belongs in how the tail bounds its debt once the publisher has ended (demand made after its END owes nothing), in both languages. I've suggested that as a follow-up.
(Written by Claude Opus 5.5)
|
Merge summary for
(Written by Claude Opus 5.5) |
Summary
When a publisher finishes a track, it stops reading the subscribe stream. If the subscriber writes a SUBSCRIBE_UPDATE at that moment, the write fails, and until now the track failed with that write error instead of ending with the publisher's finish. This is the cause of the real-browser rename media flake seen on #5635 ("The stream was aborted by the remote server").
Now a failed update write defers to the subscribe stream's responses, in both languages. A FIN still ends the track cleanly, and a reset still fails it with the reset's error. There's no retry and no timeout.
js/net/src/lite/subscriber.ts):#runSubscriptionUpdatesencodes each update into memory, then writes it. If the write fails, the loop stops quietly, so the existing.then(() => closed)hands the outcome to the responses. An update the encoder refuses, or one that empties the range, still fails the track.rs/moq-net/src/lite/subscriber.rs): the same race happened in theServeMode::Selectserve loop. A flush error handed the track back with that error before the buffered SUBSCRIBE_END and FIN were read. Now it logs the dropped update at debug level and keeps reading the responses.rs/moq-net/src/coding/writer.rs):Writer::poll_flushnow drops its buffer on a write error. A failed stream never takes another byte, and keeping the bytes made the teardown'sfinish()trip its "unflushed bytes" debug assertion.Deletes the finished quest
quest/m1/test-flakes-2/update-after-finish.md.Tests
The regression tests are deterministic, with no wall clock. Both fail without the fix:
an update the publisher stopped reading defers to its {finish,reset}covers lite-05, 06, and 07 finishing, plus a lite-07 reset. Without the fix, the track closes withThe stream was aborted by the remote server.an update whose bound the wire can't carry fails the trackcovers the Codex review finding: only a failed write defers.an_update_the_publisher_stopped_reading_defers_to_its_end, overtransport::memory::pair, runs the fullTrackServeRun. Without the serve-loop change, the track is handed back instead of finishing. Without theWriterchange, teardown panics on the debug assertion.Checks:
just check: passes.just test interop --all: 54 pass, 15 fail. Every failure involves the headless-Chromium (js) leg, which can't launch on this host (libglib-2.0.so.0: cannot open shared object file). That includesrefused sessionand thejs roundidle-out that follows from it. The non-browser JS legs (js-native-node,js-native-bun) pass. CI's browser interop is the real check for that leg.Public API / wire
No public API change.
Writeris crate-internal, and its dropped-buffer-on-error behavior has no external effect. No wire change.Notes
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)