Skip to content

Fix ensure ordered chunks not dropping controller - #2167

Open
1egoman wants to merge 4 commits into
mainfrom
fix-ensure-ordered-chunks-not-dropping-controller
Open

1egoman wants to merge 4 commits into
mainfrom
fix-ensure-ordered-chunks-not-dropping-controller

Conversation

@1egoman

@1egoman 1egoman commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2166

#2166 describes an issue where if a data stream has packets that are delivered out of order and the stream controller is cancelled, further packets that are delivered are attempted to be enqueued on a closed controller, resulting in unexpected errors thrown by the sdk.

This pull request fixes this issue by ensuring that when a stream controller is cancelled (ie, due to out of order packets, inflate/deflate error, or some other means), the stream controller is removed from the textStreamControllers/byteStreamControllers maps, and that if any further packets are will be delivered, they will be treated as if the data stream does not exist.

@changeset-bot

changeset-bot Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 27aa5b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
dist/livekit-client.esm.mjs 115.78 KB (-0.07% 🔽)
dist/livekit-client.umd.js 125.14 KB (-0.03% 🔽)
dist/core/core.mjs 78.55 KB (0%)

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Devin Review

Comment on lines +362 to +369
cancel: () => {
// The consumer side of the pipe chain errored (e.g. `ensureOrderedChunks` saw a gap) or
// was cancelled, which cancels this source stream. Unregister it so later chunks and the
// trailer for this stream are dropped rather than hitting a closed controller.
if (
this.textStreamControllers.get(streamHeader.streamId)?.controller === streamController
) {
this.textStreamControllers.delete(streamHeader.streamId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Aborted reads keep receiving chunks

When readAll({ signal }) aborts, cancel never runs because reader cleanup only releases the lock. Both stream controllers keep accepting chunks into an unread queue until the trailer arrives.

Learn more

The manager registers an incoming stream until its source ReadableStream is canceled or a trailer arrives. The new source cancel hook unregisters it when cancellation propagates through the pipe. However, TextStreamReader and ByteStreamReader only release their reader locks when an abort or premature iteration ends. Releasing a lock does not cancel the stream, so its source remains registered and continues buffering incoming packets despite the caller having stopped reading. The same omission affects both new cancellation hooks.

Example: A client aborts reader.readAll({ signal }) during a large text transfer. The promise rejects, but later chunks continue to enter the text stream's unread queue until a trailer arrives; the byte reader behaves the same way.

Recommended fix: On abort or early iteration exit, cancel the underlying reader before releasing its lock, and test that both text and byte controllers drop subsequent packets immediately. Preserve the existing error propagation for failed reads.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@1egoman 1egoman Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is outside the scope of this fix, and I will make a follow up pull request fixing this: #2168

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incoming data stream stays registered after a missing-chunk error, so later chunks and the trailer throw TypeErrors

1 participant