[kixelated]

Problem

On Chromium's native WebSocketStream, the writable uses the default one-chunk count strategy, so writer.desiredSize is 0 whenever any one write is in flight, however idle the socket. WritableStreamSink.wantsMore() read that as backpressure, so SendScheduler.enqueueDatagram dropped every datagram that arrived while the previous frame was being written. moq-dev/moq#8054 fixed the same thing for the ponyfill only.

Measured in headless Chromium 156 (QMux session from the page to a Bun Session.accept, 100 one-byte datagrams on an idle localhost socket):

pattern	before	after
each writer.write awaited	50/100 arrive	100/100
all writes at once	67/100 arrive	100/100

A raw probe shows the cause: desiredSize goes 1, 0 (one write in flight), -1 (two), back to 1 once both complete.

Approach

• BacklogSink (in js/qmux/src/scheduler.ts) hands each write to the writable at once and counts bytes whose write has not completed as the send backlog. That backlog is held under the session's sendBufferSize, the same way the ponyfill holds bufferedAmount. ready(), write(), and wantsMore() all read that byte count, so only a real backlog past the mark sheds datagrams. The session uses it for every transport except the ponyfill, whose own strategy is already byte-based. That covers native WebSocketStream, a caller-passed native stream, and custom WebSocketStreamLike adapters.
• Since writes are now pipelined, SendSink gains flush(): the scheduler counts the close frame as sent only once the socket holds it, so CLOSE_TIMEOUT still bounds close and the native socket is not closed over a queued close frame.
• SendSink gains closed (the writer's closed): a write the sink already accepted can fail while the scheduler is parked, and the scheduler now fails the session from it instead of waiting for the next write. #fail is idempotent.
• setSendBufferSize and sendBufferSize now apply to native WebSocketStream too, so their "ponyfill-only" docs are gone.

Tests:

• test/interop/clients/js/qmux-datagrams.browser.ts (runs in just test harness, PR CI): headless Chromium over native WebSocketStream, both write patterns, every datagram must arrive. It fails with the session change reverted.
• BacklogSink unit tests: a write in flight is not backpressure but bytes past the mark are, a raised mark admits a parked write, a failed write fails write/ready/flush, and close waits for the close frame's write.
• Session unit tests use a one-byte send buffer so that their wedged-socket fixture (a fake native WebSocketStream) still counts one stuck write as a backlog.
• The Bun ServerWebSocket adapter moved from test/qmux/interop.test.ts to test/qmux/bun-socket.ts, so both tests share it.

Impact

• Public API: none. SendSink, WritableStreamSink, and BacklogSink are internal. SessionOptions.sendBufferSize and Session.setSendBufferSize now also take effect on native WebSocketStream (docs updated).
• Wire: none.
• Behavior: on native WebSocketStream, up to sendBufferSize (64 KiB default) of frames are now handed to the socket ahead of time instead of one frame. This matches the ponyfill, where the same amount sits in bufferedAmount. Frames past that stay in the scheduler and can still be reprioritized.

Alternatives

• Shed only when a datagram already waits behind the in-flight write. Rejected: it was measured and still lost 32/100 under the awaited pattern, because microtask interleaving decides who wins.
• Never shed on native, relying on the 1024-datagram lane cap. Rejected: under real congestion that sends seconds of stale datagrams.
• Wrap the native writable inside openWebSocketStream with a byte-sized strategy. Rejected: it misses caller-passed streams, and closing would need its own flush logic outside CLOSE_TIMEOUT.

Choices made without asking

• The regression test lives in the just test harness browser suite (PR CI), not in nightly just test qmux.
• Applied BacklogSink to custom WebSocketStreamLike adapters as well as native ones. Any count-based writable has the same bug.

Validation

• just ci origin/main: pass.
• just test harness: the new test passes. One pre-existing failure: publish-ui.browser.ts > "invalid attributes and properties refuse, retain bad markup, and recover" (invalid: "30" → threw: false, recovered: false). It reproduces unchanged on origin/main (9bf34b2) with bun test ./publish-ui.browser.ts -t "invalid attributes".
• bun test in js/qmux: 241 pass.

Follow-ups

• Fix the publish-ui.browser.ts invalid-attribute check failing on main (the moq-publish attribute value 30 is no longer refused). I found no open PR for it.

Review

Codex gpt-6.1-sol (medium), quest review round 1 of head 89fc91b against origin/main (e6f92a6): no findings.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You'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 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7bbd3238-3398-4cba-8b72-4a26eccbe7db

Commits

Reviewing files that changed from the base of the PR and between fbd1beb and 89fc91b.

Files selected for processing: 14

• js/qmux/src/scheduler.test.ts
• js/qmux/src/scheduler.ts
• js/qmux/src/session.test.ts
• js/qmux/src/session.ts
• quest/m1/README.md
• quest/m1/websocket-stream-datagrams.md
• test/interop/clients/js/qmux-datagrams.browser.ts
• test/interop/clients/js/qmux-datagrams/index.html
• test/interop/clients/js/qmux-datagrams/main.ts
• test/interop/clients/js/tsconfig.json
• test/justfile
• test/qmux/bun-socket.ts
• test/qmux/interop.test.ts
• test/qmux/tsconfig.json

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head 5bdf37e against base 9bf34b2.

No actionable findings. BacklogSink separates an outstanding write from byte-budget congestion while keeping the scheduler's single-writer ordering and bounded admission. The ponyfill retains its existing byte-based sink, and caller-provided streams use the new accounting too. Resizing wakes blocked admission; write rejection reaches waiting operations and the session even after an earlier enqueue resolved. The added flush keeps graceful transport close behind accepted frames under the existing CLOSE_TIMEOUT. Frame encoding allocates/copies payloads, so early enqueue completion does not expose queued frames to caller buffer reuse.

The browser regression explicitly requires native WebSocketStream and checks all datagram values for both awaited and burst writes, using a later ordered stream as its receive barrier. The unit cases cover congestion, resizing, errors and close-frame completion.

Direction: keep byte-backlog accounting at the sink boundary rather than disabling datagram shedding. As documented, frames already accepted into that bounded backlog cannot be reprioritized; only scheduler-queued frames can.

Verification limits: GitHub-only static review of the diff, complete scheduler, session attachment/send/teardown callers, and frame encoding. No Chromium/Bun execution, builds, CI or throughput measurements run; the reported before/after delivery counts and validation were not independently reproduced. Rechecked open/non-draft state, full head/base and existing reviews immediately before posting.

(Written by review (OpenAI))

[pullfrog]

ℹ️ No critical issues — one edge case inline, plus two smaller notes.

Reviewed changes

This run read the full PR at 5bdf37e against 9bf34b20: the new sink, the scheduler lifecycle changes, the session transport selection, the new browser regression, the moved Bun adapter, and the quest deletion.

• BacklogSink — new byte-backlog sink for every writable except the ponyfill: each write is handed over immediately and bytes whose write has not completed are counted against a resizable high-water mark.
• SendSink / SendScheduler — new flush() and closed; close now waits for accepted writes (the close frame included) to reach the socket, an accepted write failing while the loop is parked fails the session, and #fail became idempotent.
• Session — selects BacklogSink for native and caller-provided writables, makes sendBufferSize / setSendBufferSize effective there, and drops the ponyfill-only doc claims.
• Tests — BacklogSink unit tests, session fixtures lowered to a one-byte send buffer, a headless-Chromium datagram regression in just test harness, and BunSocketStream extracted to test/qmux/bun-socket.ts.
• Quest — quest/m1/websocket-stream-datagrams.md completed and deleted with its milestone entry.

Verified locally: bun test in js/qmux (241 pass) and js/net's src/connection/connect.test.ts (57 pass). The Chromium suite needs the Playwright browser, which this environment does not have, so it was not run here.

ℹ️ The new regression test is not gated on pull requests

The body records the harness placement as "(PR CI)", but just test harness only runs in .github/workflows/interop.yml, which triggers on workflow_dispatch, a daily schedule, and workflow_call from release-gate.yml — never on pull_request. A qmux regression this test catches would therefore surface on the next daily Interop run, not on the PR that introduces it.

Technical details

# Harness test gating

## Affected sites
- `test/justfile:27` — adds `qmux-datagrams.browser.ts` to `harness_browser_tests`
- `.github/workflows/interop.yml:98` — the only `just test harness` invocation

## Required outcome
- The PR body (and any later doc citing this test as a per-PR guard) must not claim
  PR coverage that does not exist.
- If per-PR gating is actually intended, name the job that would run it. Otherwise
  the daily Interop run matches this repo's existing browser-suite convention and
  only the body needs correcting.

ℹ️ The ponyfill module doc still credits the native API with byte backpressure

js/qmux/src/web-socket-stream.ts:10 still says "the native API observes the real send buffer". That is the reading this PR refutes: the native writable's desiredSize counts writes, not bytes. The new comment at session.ts:874-876 says so, so the two now disagree, and the module doc is the one a future reader is likely to trust.

Technical details

# Stale native-backpressure claim

## Affected sites
- `js/qmux/src/web-socket-stream.ts:10` — "the native API observes the real send buffer"
- `js/qmux/src/session.ts:874-876` — the new comment: the native API's `desiredSize` counts writes

## Required outcome
- The module doc must not read as "native `desiredSize` measures bytes", which is
  the assumption behind #8054/#8136. Say the native API applies backpressure per
  write, or scope the byte claim to the ponyfill's `desiredSize`.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

This run re-reviewed the delta since the prior pullfrog review at 5bdf37e: one fix commit plus an origin/main merge that changed none of the PR's files.

• sendBufferSize normalization — the Session constructor now runs the option through a shared sendBufferSize() helper (js/qmux/src/session.ts:446) that throws RangeError for a non-finite value and otherwise floors and clamps to at least one byte; setSendBufferSize reuses the same helper, so every BacklogSink, ponyfill writable strategy, and setHighWaterMark call receives a finite mark. This resolves the unclamped constructor mark raised in the previous review.
• Regression test — a new unit test (js/qmux/src/session.test.ts:279) asserts the constructor refuses NaN and Infinity instead of parking every write behind an invalid high-water mark.

Verified locally at 89fc91b: bun test src/session.test.ts in js/qmux (130 pass) and bunx tsc --noEmit -p tsconfig.json (clean).

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed head 89fc91b against base fbd1beb, incrementally since 5bdf37e, accounting for the merged main changes.

No new actionable findings. The sendBufferSize fix addresses the constructor-validation gap missed in my earlier review and identified by pullfrog. Construction now normalizes finite values to at least one byte before opening or adopting a transport. Both construction and resizing reject NaN and either infinity; the setter validates before modifying the existing budget or notifying either sink. This avoids the NaN stall that a Math.max clamp alone would preserve.

Direction: sharing validation at the Session boundary keeps native, adopted and ponyfill transports consistent without weakening byte-backlog shedding. The added regression covers constructor rejection for NaN and positive infinity; it does not directly exercise finite-value clamping or invalid resizing.

Verification limits: GitHub-only static inspection of the substantive incremental patch, current constructor/setter and test context, and the PR diff. No Bun/Chromium tests, builds or CI commands were run. Rechecked the open/non-draft state, full head/base and published reviews immediately before posting. COMMENT only, not approval.

(Written by review (OpenAI))