Skip to content

fix streams byob request handling after close for spec compliance#5769

Merged
anonrig merged 1 commit intomainfrom
yagiz/fix-byob-stream-handler
Feb 23, 2026
Merged

fix streams byob request handling after close for spec compliance#5769
anonrig merged 1 commit intomainfrom
yagiz/fix-byob-stream-handler

Conversation

@anonrig
Copy link
Copy Markdown
Member

@anonrig anonrig commented Dec 24, 2025

Fix ReadableByteStreamController to properly handle byob requests after close(), making respondWithNewView() throw correct errors per the WHATWG Streams spec.

Removed premature byob request invalidation because spec required byob requests to remain accessible after close so that respondWithNewView throw correct errors. Had to store original buffer byte length and byte offset as well in order to make it available for validation even after the underlying ByobRequest is invalidated during close.

Had to update several test contexts due to invalid configuration.

Fixes WPT readable-byte-streams/bad-buffers-and-views.any.js

@anonrig anonrig requested a review from jasnell December 24, 2025 18:38
@anonrig anonrig requested review from a team as code owners December 24, 2025 18:38
@anonrig anonrig force-pushed the yagiz/fix-byob-stream-handler branch from 7d10d2a to 8546b50 Compare December 24, 2025 18:40
@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Dec 24, 2025

I might merge this into the state-machine update for this class. Please do not merge this yet.

@anonrig anonrig force-pushed the yagiz/fix-byob-stream-handler branch from 8546b50 to e60c1c6 Compare December 29, 2025 16:55
Copy link
Copy Markdown

@Hardanish-Singh Hardanish-Singh left a comment

Choose a reason for hiding this comment

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

@anonrig
Copy link
Copy Markdown
Member Author

anonrig commented Feb 13, 2026

I might merge this into the state-machine update for this class. Please do not merge this yet.

@jasnell ping

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Feb 13, 2026

No updates yet, the remaining state machine improvements are pending completion of rolling out a couple autogates. One of those just finished rolling out this morning the remaining one will roll out monday morning. I'll update when there's additional progress.

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Feb 21, 2026

Ok, the state machine changes have landed. This should be revisited now.

@anonrig anonrig force-pushed the yagiz/fix-byob-stream-handler branch from e60c1c6 to a721cfe Compare February 23, 2026 16:02
@anonrig
Copy link
Copy Markdown
Member Author

anonrig commented Feb 23, 2026

@jasnell you can re-review this. i've fixed the conflicts.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.67347% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.55%. Comparing base (3af1e97) to head (a721cfe).

Files with missing lines Patch % Lines
src/workerd/api/streams/queue.c++ 70.00% 2 Missing and 4 partials ⚠️
src/workerd/api/streams/standard.c++ 93.10% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5769      +/-   ##
==========================================
- Coverage   70.56%   70.55%   -0.02%     
==========================================
  Files         409      409              
  Lines      109532   109575      +43     
  Branches    18038    18053      +15     
==========================================
+ Hits        77291    77310      +19     
- Misses      21436    21454      +18     
- Partials    10805    10811       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Collaborator

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Nice, well-scoped fix! Two optional nits below — neither is blocking. 🤖 This review was generated by an AI assistant.

@anonrig anonrig merged commit d0eaa7e into main Feb 23, 2026
22 of 23 checks passed
@anonrig anonrig deleted the yagiz/fix-byob-stream-handler branch February 23, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants