Skip to content

Keep a patched then out of TypeScript body consumption - #8060

Merged
jasnell merged 1 commit into
mainfrom
jasnell/ts-streams-consume-broken-then
Oct 28, 2026
Merged

jasnell merged 1 commit into
mainfrom
jasnell/ts-streams-consume-broken-then

Conversation

@jasnell

@jasnell jasnell commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Fixup patched then handling in consumer methods

@jasnell
jasnell requested review from a team and guybedford October 28, 2026 02:42
@jasnell
jasnell added this pull request to stack #8061 October 28, 2026 02:42
@jasnell
jasnell requested review from a team as code owners October 28, 2026 02:42
@clanky-the-clanker

clanky-the-clanker Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

💬 Approved with comments — The code change is correct and well tested. The only finding is a documentation WARNING: the new AGENTS.md invariant covers more than the code guarantees.

Findings: 1 warning

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #8060.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Documentation] The invariant says every promise the C++ bridge awaits resolves with a primitive, `undefined` or a null-prototype record. But the `ReadableStream.from()` bridge (`js-readable-stream.c++:672-681`) awaits promises yielded by the user's generator, which can resolve to ordinary objects. Limit the rule to body consumption.
- **File:** src/per_isolate/webstreams/AGENTS.md:166

📝 Documentation: ⚠️ 1 warning

The new promise-settlement invariant covers more than the code guarantees.

Recommendation: 📝 Minor Comments

  • [WARNING] The invariant says every promise the C++ bridge awaits resolves with a primitive, undefined or a null-prototype record. But the ReadableStream.from() bridge (js-readable-stream.c++:672-681) awaits promises yielded by the user's generator, which can resolve to ordinary objects. Limit the rule to body consumption. (src/per_isolate/webstreams/AGENTS.md:166)
View full review

src/per_isolate/webstreams/AGENTS.md:166-170 says that every promise the C++ bridge awaits resolves with a primitive, undefined or a null-prototype record. That is not true for the whole bridge. The C++ ReadableStream.from() path (src/workerd/api/js-readable-stream.c++:672-681) awaits promises yielded by a user generator through js.toPromise(...).then(...), and those can resolve to ordinary objects. Limit the statement to the body-consumption bridge, so readers don't conclude that yielded values also avoid the then lookup.


✅ All other checks passed

🔒 Security: ✅ LGTM

No security concerns. Taking user-patched then lookups out of the internal consumption steps makes it harder for user code to interfere with the runtime's internals.


📖 AGENTS.md: ✅ LGTM

The AGENTS.md updates match the new consumption path and its test guards.


📋 Code Quality: ✅ LGTM

All body consumers now go through consumeReadableStreamAsArrayBufferRecord, and C++ builds each result (ArrayBuffer, Uint8Array, Blob, text, parsed JSON) from the { buffer } record. BOM handling and UTF-8 decoding live in getReadableStreamText, which json() reuses. The unused TypeScript consumers are removed, and the C++ side no longer dispatches to them. The C++ and JS tests cover all five consumers.


⚡ Performance: ✅ LGTM

No performance concerns.


Reviewed commit: a518a391 — same changes as 8180d414, so this is that commit's review (no new review was run; line numbers are as of 8180d414)

@ask-bonk

ask-bonk Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Since last review: 0 resolved, 0 still open, 0 new.
LGTM!

Not re-run: tests, docs, api-compat, compat-flags, design-simplicity, jsg-gc, kj-style, memory-safety (no author changes in their files since the last review)


Reviewed commit: a518a391 · github run

Comment thread src/per_isolate/webstreams/readable.ts
Base automatically changed from jasnell/ts-streams-formdata to main October 28, 2026 12:16
@jasnell
jasnell force-pushed the jasnell/ts-streams-consume-broken-then branch from 8180d41 to e90de05 Compare October 28, 2026 12:16
Under typescript_implemented_streams, the C++ bridge consumed a body
by awaiting a TypeScript async function that resolved with the
result: an ArrayBuffer, a Uint8Array or a parsed JSON value.
Resolving a promise with an object looks up its then, so a patched
Object.prototype.then ran at that internal step. arrayBuffer() and
blob() then failed with an internal error (the bridge's ArrayBuffer
check), and bytes() resolved with an empty Uint8Array, because its
TypeScript side awaited the ArrayBuffer itself. The same paths serve
R2's GetResult and container stdout/stderr capture. With the C++
implementation the patch is consulted only when the caller's promise
resolves.

Every body consumption now goes through the one export text()
already uses, consumeReadableStreamAsArrayBufferRecord, which
resolves with a null-prototype { buffer } record. C++ builds each
result from it, as the legacy implementation builds them from its
own reads: bytes() wraps the buffer in a Uint8Array, blob() makes a
Blob of it, and json() parses text(), as Body::json() does. The
ArrayBuffer, Uint8Array and JSON consume functions are gone, and
with them the TypeScript-side text decoding that only json() used.

Tests: the readable suite checks that a self-removing patched then
intercepts only the result of each consumption method, for native
and queued bodies, under both implementations. A bridge unit test
installs a persistent patch and checks that arrayBuffer(), bytes(),
text(), json() and blob() still give C++ the stream's contents
without consulting it.

typescript_implemented_streams is still experimental, with no enable
date, so the change needs no compatibility flag.

Fixes: #8043
@jasnell
jasnell force-pushed the jasnell/ts-streams-consume-broken-then branch from e90de05 to a518a39 Compare October 28, 2026 14:19
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@jasnell
jasnell merged commit 13b798a into main Oct 28, 2026
36 of 40 checks passed
@jasnell
jasnell deleted the jasnell/ts-streams-consume-broken-then branch October 28, 2026 15:13
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.

2 participants