Repository navigation
Conversation
The "does not hold *Stream across user-controlled options getters" test spawned its subprocess via `bun -e`, which on a debug+ASAN build runs the 129 concurrent h2 requests in ~6.3s and overruns the default 5s per-test timeout. The other two tests in the same file spawn fixture files and finish in ~3s. Move the script to node-http2-getter-rehash.fixture.js to match, and cut the inner-loop stream count from 128 to 32 (the streams HashMap starts at capacity 8 and grows at 80% load, so 32 still forces several rehashes). Drop parent/weight from the options: they now emit DEP0194 and are deleted before the native call anyway. Before: 3/3 timeouts at 5s. After: 3/3 pass in ~3.2s.
|
Updated 9:11 PM PT - Jul 18th, 2026
❌ @robobun, your commit 136e6aa has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34676That installs a local version of the PR into your bun-34676 --bun |
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical test de-flake that moves the inline -e script to a fixture file to match its two siblings. The inline nit about the comment/title wording is non-blocking and largely pre-dates this PR.
Extended reasoning...
Overview
Test-only change to test/js/node/http2/node-http2-streams-rehash.test.ts: the second of three tests was timing out at 5s on debug+ASAN because it ran its repro via bun -e while the other two used fixture files. This PR extracts the script to node-http2-getter-rehash.fixture.js, cuts the inner loop 128→32, and drops the parent/weight options. No src/ diff.
Security risks
None. Test fixture only; spawns a local h2 server on 127.0.0.1:0, no external network, no new dependencies.
Level of scrutiny
Low. This is a mechanical refactor of an existing regression test to match the exact shape of its two neighbors in the same file. The test wrapper (spawn + drain both pipes + assert {stdout, exitCode}) is byte-identical to the sibling tests. The fixture preserves the triggered guard, so if the getter stops firing the test fails with exit 1 rather than silently passing.
Coverage-weakening check (per REVIEW.md)
Each reduction is justified in the PR description and was independently verified by the bug hunter:
- 128→32 streams: still forces ≥3 rehashes from initial cap 8 at 80% load — no coverage loss.
- Dropping
parent/weight: DEP0194 strips these in JS before the native call, so they never reached native even before this PR. -e→ fixture: pure de-flake, matches siblings.
The one inline nit notes that the fixture comment's "while the outer stream is being set up" overstates what this path guards post-#31323 — but the old -e script's comment was already stale for the same reason, so this PR doesn't introduce the inaccuracy. Non-blocking wording cleanup the author can take or leave.
Other factors
PR description shows 3/3 passing runs with the change. No outstanding reviewer comments. Approving.
The getter fires during the JS-side options spread (since #31323), not while a native *Stream is held. Make the comment say so.
|
Self-review clean (25 concerns probed, none survived). Review thread addressed in 136e6aa.
This is a test-only change (no |
There was a problem hiding this comment.
LGTM — the fixture comment in 136e6aa now accurately describes this as a JS-side re-entrancy smoke test, which addresses my earlier note. Test-only de-flake that mirrors the two sibling fixtures.
What was reviewed:
-e→ fixture file matches the pattern of the other two tests; assertion shape (toMatchObjectwith stderr captured-but-not-asserted) is unchanged.- 32 inner streams still forces ≥3 rehashes from cap 8 @ 80% load; dropped
parent/weightare DEP0194-stripped in JS before the native call anyway, so no coverage loss vs. the old script. - Fixture guards against vacuous pass via the
!triggered→ exit(1) check.
Extended reasoning...
Overview
Test-only PR touching two files under test/js/node/http2/: moves an inline bun -e script from node-http2-streams-rehash.test.ts into a new sibling fixture node-http2-getter-rehash.fixture.js, cuts the inner request loop from 128→32, and drops the deprecated parent/weight priority options. No src/ changes. The motivation is a consistent 5s timeout on debug+ASAN builds where the -e path is ~2× slower than the fixture path for this workload.
Prior review
I left one nit on the previous revision: the fixture header claimed the getter fires "while the outer stream is being set up", which was inaccurate since #31323 spreads options before any stream exists. The author pushed 136e6aa updating the header to "request() now shallow copies options in JS before any stream exists (#31323), so this is a re-entrancy/ordering smoke test rather than the original native UAF repro" — that's honest and addresses the point. They reasonably left the pre-existing test title/file header alone as out of scope for a timeout fix. Thread is resolved.
Security risks
None. Test-only; the fixture spawns a local h2 server on 127.0.0.1:0 and connects to itself, same as the old inline script and the two siblings.
Level of scrutiny
Low. This is a mechanical de-flake: the new fixture is byte-for-byte the old -e script minus (a) the loop count, (b) two options fields that are stripped in JS anyway, and (c) a stale comment. The test wrapper now matches its two neighbors exactly. The one REVIEW.md concern for test edits — "never silently weaken" — was examined in the prior round and the conclusion stands: the weakening (getter no longer reaching native) pre-dates this PR (#31323); 128→32 and dropping DEP0194 fields lose nothing further.
Other factors
- Fixture retains the
!triggeredguard so it can't vacuously pass ifrequest()stops readingpaddingStrategy. toMatchObjectincludesstderrin the actual object for diagnostics but doesn't assert it empty — correct per REVIEW.md (ASAN/debug noise).- PR description shows 3/3 passing runs at ~3.2s, comfortably under the 5s default.
- Bug-hunting system found nothing this run.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
I believe this closure was in error. Addressing each criterion: Verified the problem exists. On unmodified main at 20b4a0b, Tested that the fix works. 3/3 local passes at ~3.2s shown in the PR body. Codebase assumptions. The Completeness. The change moves the inline This is a test-only change, so the gate cannot observe a fail-before: stashing Leaving for a maintainer to reopen if they agree. |
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.