test: WebTransport browser regression harness + Firefox server-stream repro#251
Closed
kixelated wants to merge 4 commits into
Closed
test: WebTransport browser regression harness + Firefox server-stream repro#251kixelated wants to merge 4 commits into
kixelated wants to merge 4 commits into
Conversation
… repro Adds a scenario-driven test server and a browser harness for chasing browser-specific WebTransport bugs, and uses it to pin down a Firefox bug: server-initiated streams stall at 2. - rs/web-transport-quinn/examples/test-server.rs: a server that selects its behavior from the request URL path (e.g. /server-bi/5, /server-mix/2, /reject/404). Covers server-initiated bidi/uni streams, datagrams, explicit session close with a code/reason, CONNECT rejection, and a serial open-then-wait-for-close mode. Per-open timing logs make a stall obvious. - js/web-demo/test.html + test.js: a harness that runs each scenario, reports PASS/FAIL/TIMEOUT, and logs per-stream delivery. ⭐ rows reproduce the Firefox bug. - js/web-demo/TESTING.md: run instructions + findings. - js/web-demo/client.js: fix cert-hash path (../dev -> ../../dev); the certs live at repo-root dev/, so the demo did not build before. Firefox finding (confirmed via quinn frame trace): the server opens all N streams fine (Firefox grants MAX_STREAMS count=102) and writes data to each, but Firefox only surfaces 2 server-initiated streams to the page. Draining a stream fully lets the next through (server-bi-serial passes 10/10); the limit is per stream-type (server-mix 2 uni + 2 bidi passes); client-initiated streams are unaffected. Chrome passes all scenarios. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR introduces a comprehensive WebTransport regression testing harness: a browser-side test runner (HTML+JS) with embedded certificate fingerprint and helpers for streams/datagrams, a curated SCENARIOS suite, runner/UI orchestration with timeouts and deep-linking, and a Rust TLS test server that routes CONNECT requests to scenario handlers (echo, server-initiated streams/datagrams, explicit close/reject behaviors). Documentation (TESTING.md and a Firefox bug README/root-cause write-up) and a small client path fix are included for local reproduction and extending scenarios. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rs/web-transport-quinn/examples/test-server.rs (1)
329-331: 💤 Low valueConsider logging task errors for debugging visibility.
Discarding task results here silently hides any stream-open failures. While the browser harness will detect stalls, logging errors server-side could aid debugging.
📝 Optional: log task errors
for task in tasks { - let _ = task.await; + if let Err(e) = task.await { + tracing::debug!(?e, "concurrent bidi task failed"); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/web-transport-quinn/examples/test-server.rs` around lines 329 - 331, The loop currently drops each task result silently (for task in tasks { let _ = task.await; }) which hides failures; change it to inspect and log errors from each JoinHandle by awaiting and matching the result (e.g., if let Err(e) = task.await { /* log e */ } or match task.await { Ok(Err(e)) | Err(e) => log accordingly }) using the project's logger (eprintln!, tracing::error!, or process logger) so any stream-open or task failures are recorded for server-side debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/web-demo/test.js`:
- Around line 559-564: The current try/catch around awaiting t.ready swallows
the deliberate failure throw and logs it as "expected", so a resolved
transport.ready falsely passes; change the flow so only actual rejections are
treated as expected: await t.ready inside a try/catch but do not throw inside
the try and instead record success, then after the try/catch if the await
succeeded throw the failure error ("transport.ready RESOLVED but server should
have rejected"); keep the catch block (catching errors from await t.ready) to
log the expected rejection; reference t.ready (or variable t) to locate where to
implement this control-flow change.
In `@js/web-demo/TESTING.md`:
- Around line 72-76: Replace the npx runner with Bun’s runner in the serve
command: locate the line containing the shell command that starts with "npx
parcel serve client.html test.html --port 1234" and change it to use "bunx
parcel serve client.html test.html --port 1234" so the README uses Bun tooling
consistently with the project standard.
---
Nitpick comments:
In `@rs/web-transport-quinn/examples/test-server.rs`:
- Around line 329-331: The loop currently drops each task result silently (for
task in tasks { let _ = task.await; }) which hides failures; change it to
inspect and log errors from each JoinHandle by awaiting and matching the result
(e.g., if let Err(e) = task.await { /* log e */ } or match task.await {
Ok(Err(e)) | Err(e) => log accordingly }) using the project's logger (eprintln!,
tracing::error!, or process logger) so any stream-open or task failures are
recorded for server-side debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09d99261-c232-48d4-ba0e-cca3da04e404
📒 Files selected for processing (5)
js/web-demo/TESTING.mdjs/web-demo/client.jsjs/web-demo/test.htmljs/web-demo/test.jsrs/web-transport-quinn/examples/test-server.rs
Comment on lines
+559
to
+564
| try { | ||
| await t.ready; | ||
| throw new Error("transport.ready RESOLVED but server should have rejected"); | ||
| } catch (e) { | ||
| log(`ready REJECTED (expected): ${e?.name}: ${e?.message ?? e}`); | ||
| } |
There was a problem hiding this comment.
Reject scenarios can falsely PASS when transport.ready resolves.
The try/catch currently logs both paths as “expected” and never fails the scenario if t.ready resolves, which defeats this regression check.
Suggested fix
async run(t, { log }) {
+ let readyRejected = false;
try {
await t.ready;
- throw new Error("transport.ready RESOLVED but server should have rejected");
} catch (e) {
+ readyRejected = true;
log(`ready REJECTED (expected): ${e?.name}: ${e?.message ?? e}`);
}
+ assert(readyRejected, "transport.ready RESOLVED but server should have rejected");
await t.closed
.then((i) => log(`closed RESOLVED: ${JSON.stringify(i)}`))
.catch((e) => log(`closed REJECTED: ${e?.name}: ${e?.message ?? e}`));
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| await t.ready; | |
| throw new Error("transport.ready RESOLVED but server should have rejected"); | |
| } catch (e) { | |
| log(`ready REJECTED (expected): ${e?.name}: ${e?.message ?? e}`); | |
| } | |
| let readyRejected = false; | |
| try { | |
| await t.ready; | |
| } catch (e) { | |
| readyRejected = true; | |
| log(`ready REJECTED (expected): ${e?.name}: ${e?.message ?? e}`); | |
| } | |
| assert(readyRejected, "transport.ready RESOLVED but server should have rejected"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/web-demo/test.js` around lines 559 - 564, The current try/catch around
awaiting t.ready swallows the deliberate failure throw and logs it as
"expected", so a resolved transport.ready falsely passes; change the flow so
only actual rejections are treated as expected: await t.ready inside a try/catch
but do not throw inside the try and instead record success, then after the
try/catch if the await succeeded throw the failure error ("transport.ready
RESOLVED but server should have rejected"); keep the catch block (catching
errors from await t.ready) to log the expected rejection; reference t.ready (or
variable t) to locate where to implement this control-flow change.
Comment on lines
+72
to
+76
| ```bash | ||
| cd js/web-demo | ||
| bun install | ||
| npx parcel serve client.html test.html --port 1234 | ||
| ``` |
There was a problem hiding this comment.
Use Bun tooling in the serve command to match repo standards.
Switch npx parcel serve ... to Bun’s runner (bunx parcel serve ...) so setup instructions stay consistent with the project toolchain.
Suggested fix
cd js/web-demo
bun install
-npx parcel serve client.html test.html --port 1234
+bunx parcel serve client.html test.html --port 1234As per coding guidelines, “For JavaScript/TypeScript, use bun instead of pnpm, npm, or yarn.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```bash | |
| cd js/web-demo | |
| bun install | |
| npx parcel serve client.html test.html --port 1234 | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/web-demo/TESTING.md` around lines 72 - 76, Replace the npx runner with
Bun’s runner in the serve command: locate the line containing the shell command
that starts with "npx parcel serve client.html test.html --port 1234" and change
it to use "bunx parcel serve client.html test.html --port 1234" so the README
uses Bun tooling consistently with the project standard.
Source: Coding guidelines
… credit The probe scenario (accept all stream objects, then read) passes while the inline read-then-accept scenario fails on identical input. Root cause is backpressure on incomingBidirectional/UnidirectionalStreams that never resumes: pull slowly and Firefox stops delivering streams after ~2 even though it granted MAX_STREAMS count=102 and the bytes are on the wire (server retransmits the undelivered streams). Mark ⭐ = reproduces the Firefox bug; document the prompt-drain workaround. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Captured parent- and content-process WebTransport logs for the server-bi-5
repro. Every layer (neqo, HTTP-3, parent→content IPC) delivers all 5 streams;
the content-process IncomingBidirectionalStreams ReadableStream only enqueues 2
because its Pull callback fires twice and is never re-invoked from the backlog.
Adds js/web-demo/firefox-bug/{README,parent-process.log,content-process.log} and
links it from TESTING.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nsportStreams PullCallbackImpl's synchronous (length>0) path returns a pull promise that is never resolved (BuildStream omits the spec's "Resolve p with undefined"), so the incomingBidirectionalStreams ReadableStream never calls Pull again once a backlog forms — wedging delivery at 2. Adds firefox-bug/root-cause.md with the analysis, scenario mapping, and a suggested patch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a scenario-driven WebTransport test server and a browser harness for chasing browser-specific bugs, and uses it to produce a minimal, isolated reproduction of a Firefox bug: incoming server-initiated streams stall at ~2 because
incomingBidirectionalStreams/incomingUnidirectionalStreamsbackpressure never resumes.rs/web-transport-quinn/examples/test-server.rs— a server that selects its behavior from the request URL path (/server-bi/5,/server-uni/3,/server-mix/2,/server-bi-serial/10,/reject/404,/server-close/42, …). Per-open timing logs.js/web-demo/test.html+test.js— a harness that runs each scenario, reports PASS/FAIL/TIMEOUT, and logs per-stream delivery. ⭐ rows reproduce the Firefox bug.js/web-demo/TESTING.md— run instructions + findings.js/web-demo/client.js— fix the cert-hash path (../dev→../../dev); certs live at repo-rootdev/, so the existing demo didn't build.The Firefox bug
When the server opens several server-initiated streams and the page reads each stream's body before pulling the next stream from the incoming-streams reader, Firefox delivers only ~2 streams and then permanently stops — even though it granted plenty of stream credit and the bytes are on the wire. Chrome passes every scenario.
Decisive evidence: probe vs inline (same server, same 5 streams)
server-bi-probe-5read()all 5 stream objects back-to-back, then read bodiesserver-bi-5read()one, read its body, thenread()nextserver-uni-3server-bi-concurrent-3The only difference between the passing probe and the failing case is how promptly the app pulls the incoming-streams reader.
server-uni-3failing (no echo, no writable) rules out the stream's send side — it's purely the incoming-stream reader.It is NOT
MAX_STREAMS/ stream-count creditServer-side QUIC trace (
RUST_LOG=info,quinn_proto=trace) shows Firefox granting generous credit and the server opening + writing data to all 5 streams without blocking, then retransmitting the undelivered streams because Firefox never consumes them:So the bytes are on the wire for all 5 streams; Firefox simply stops surfacing streams to JS once the incoming-streams reader queue fills and isn't drained promptly, and never resumes.
Boundaries (what passes in Firefox)
server-bi-serial-10— server opens one, waits for the client to fully drain+close it, then opens the next: 10/10. Reader never backs up.server-mix-2uni-2bi— 2 uni + 2 bidi: passes. Queue limit is per stream-type.client-bi-open-*/client-uni-open-*— the client opening many streams: passes. Limited to incoming (server→client) streams.Practical impact / workaround
The idiomatic
for await (const s of transport.incomingBidirectionalStreams) { await handle(s) }is exactly the failing pattern (it pulls the next stream only afterhandlecompletes). Workaround: drain the incoming-streams reader promptly into your own queue and process bodies separately, so you neverawaitper-stream work betweenreader.read()calls.How to run
Open
http://localhost:1234/test.html?base=https://127.0.0.1:4444in Firefox and Chrome and click Run all, or deep-link a single case:…&scenario=server-bi-5. Compareserver-bi-5(fails) withserver-bi-probe-5(passes) to see the backpressure dependence directly.🤖 Generated with Claude Code