improve spec compliance on WritableStreamDefaultWriterRelease

[anonrig]

No description provided.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR fixes WritableStreamDefaultWriterRelease to reject the ready promise (not just closed) when a writer is released, matching the WHATWG Streams spec.

Issue: Missing WPT expected-failure cleanup (medium severity)

The writable-streams/general.any.js entry in src/wpt/streams-test.ts (lines 938-943) still lists two expected failures:

• 'closed and ready on a released writer'
• 'ready promise should fire before closed on releaseLock'

Both test the exact behavior this PR adds — rejecting the ready promise on writer release, in the correct spec order (ready before closed). With this fix, these WPTs should now pass. Please verify and remove them from expectedFailures (changing the entry to just 'writable-streams/general.any.js': {}). Leaving stale expected failures masks future regressions and hides that this fix actually works.

The C++ change itself looks correct: maybeRejectPromise safely no-ops when the fulfiller is already consumed (closed/errored states), and the rejection order (ready then closed) matches the spec.

[ask-bonk]

Review posted successfully. Here's what I found:

The C++ change in standard.c++ is correct — it adds rejection of the ready promise before the closed promise in releaseWriter, matching the WHATWG Streams spec for WritableStreamDefaultWriterRelease. The maybeRejectPromise helper safely no-ops when the fulfiller is already consumed (closed/errored states), so there are no safety concerns.

The one actionable issue I flagged: the writable-streams/general.any.js WPT expected failures ('closed and ready on a released writer' and 'ready promise should fire before closed on releaseLock') test exactly the behavior this PR fixes and should now pass. They should be removed from expectedFailures to avoid masking regressions.

github run

[jasnell]

LGTM modulo consideration about whether this needs a compat flag or not. I'd like to see an argument about why a compat flag is not needed.

[jasnell]

/bigbonk roast this pr

[ask-bonk]

Alright, I've now reviewed the entire PR thoroughly. Time to deliver.

---

The Roast

Yagiz, my brother. You titled this PR "improve spec compliance on WritableStreamDefaultWriterRelease" and then proceeded to fix four different spec compliance issues scattered across writer setup, writer release, write-after-release, and write algorithm microtask ordering. The title is technically correct in the same way that "some weather happened" is a correct description of a hurricane.

Commit hygiene: Two commits - "improve spec compliance on WritableStreamDefaultWriterRelease" and "improve spec compliance on streams." I see you ran out of thesaurus entries for "improve spec compliance" after exactly two commits. The first commit adds expected failures for reentrant-strategy.any.js, and the second commit immediately removes them and just marks it as {}. Bold of you to leave the evidence of your "wait, actually those all pass now" moment in the git history.

The compat flag tango: James asked "does this need a compat flag?" You said "nah, it just fixes a hanging promise." James pushed back with "but what about await writer.ready in other cases?" You caved with "I'd just put it behind a compat flag." The entire conversation reads like a man slowly realizing he should've added the compat flag from the start. At least the flag exists now, so good on you for listening. The 2026-03-23 enable date gives three whole weeks - living dangerously.

The pedanticWpt cameo at standard.c++:1415: You're using the pedanticWpt flag to gate the sync-to-async write algorithm change instead of your shiny new writableStreamSpecCompliantWriter flag. So now this PR's behavior depends on two different compat flags, and the comment says "preserving correct microtask ordering (e.g., ready rejects before closed on releaseLock)" which is literally the behavior your own flag is supposed to control. Pick a lane.

The releaseReason consolidation: The old code created the TypeError string twice (once for ready, once for closed). You refactored it to auto releaseReason = js.v8TypeError(...) and reuse it. Nice. But then in the "fulfiller already consumed" branch at line 438-442, you create a brand new promise-resolver pair, mark it handled, reject it, and call setReadyFulfiller. This is spec-correct but you've turned a two-liner into a twelve-liner with a branching structure that requires understanding the fulfiller lifecycle. A comment saying "the fulfiller was already consumed" is doing a lot of heavy lifting for readers who don't have the WHATWG spec open in the next tab.

The tryGetOwner() check at line 1716: You're checking !owner.isLockedToWriter() after the size algorithm runs, because strategy.size() could call releaseLock(). This is correct per spec step 5, but the comment says "This check must occur before any state checks, as the stream state may still appear writable even after the writer was released." If the stream "may still appear writable," doesn't that mean someone reading the code below will see isWritable() assertions and wonder why they don't catch this? A breadcrumb in that lower code would help.

WPT cleanup: Bonk flagged it, you did it. The reentrant-strategy.any.js going from disabledTests: true to {} is chef's kiss. That test file was entirely disabled with the comment "A hanging Promise was canceled." and you just... turned it all on. The confidence. The audacity. I hope the CI agrees with you.

What's actually good: The core logic is correct. Rejecting ready before closed on release matches the spec. The backpressure-based ready resolution in lockWriter instead of the old isStarted() check is the right fix. The hasBackpressure() accessor is clean. And removing 13 lines of expected WPT failures while only adding 2 is always a W.

TL;DR: Solid spec compliance work hiding behind a misleading title, with a compat flag that was added under gentle peer pressure, and a sneaky dependency on pedanticWpt that makes this a two-flag feature instead of one. The code is correct; the organization could use a second pass.

github run