Repository navigation
Conversation
Size the loops by what the string-count assertion needs: 1024 reads under ASAN or debug, 8192 in release, instead of 512K clones and 128K constructions per row. The budget is a fraction of the iteration count, so a getter that allocates one JSString per call overshoots it by 8x. Keep every returned string alive and run a full GC before both heap readings, so the count does not depend on when the GC runs. Also assert the returned value, that every clone reports the same method as its source, and that a lowercase `method: "post"` normalizes to "POST" on the same cached string.
|
Status: ready for review. CI is done for 29e82c2; the diff is green on every lane. Reproduced the slow run with a local debug ASAN build: Detector check: a copy of the new file that reads Build 108294 has one red job: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe request method tests now parameterize method inputs, verify uppercase normalization for cloned and newly constructed requests, retain getter results during allocation checks, and adapt thresholds for ASAN and debug builds. ChangesRequest method getter validation
Merge Risk: ⚪ Minimal · up to This test-only change makes the request-method getter checks faster and stricter while preserving the expected GET/POST behavior; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only refactor that makes the leak guard faster and deterministic.
What was reviewed:
- Retention of every returned string plus
Bun.gc(true)before bothheapStats()reads makes the growth measurement independent of mid-loop GC timing; a per-call allocation overshoots theiterations / 8budget by 8x. isASAN || isDebugbranching on iteration count follows the repo's leak-test convention; no per-test timeouts added.- The
http://localhost:3000/literal is only aRequestconstructor argument — no socket is bound, so theport: 0rule does not apply. - New value assertions (
distinctequals[method]) and the lowercase-normalization row strengthen coverage without changing what the file guards.
Extended reasoning...
Overview
This PR refactors a single test file, test/js/web/request/request-method-getter.test.ts, which guards against Request.prototype.method allocating a fresh JSString on every read (the fix landed in #18961; the getter should return a cached common-strings-table cell). The old test ran 512K/128K iterations and asserted heapStats().objectTypeCounts.string grew by fewer than a fixed 512 — a bound that depended on GC not sweeping mid-loop, and a workload that took ~124s under debug ASAN. The new version drops to 8192 iterations (1024 under isASAN || isDebug), pushes every returned string into an array so any per-call allocation is pinned live across both measurements, runs Bun.gc(true) before each heapStats() reading, and asserts growth is below iterations / 8. It also adds value assertions (every read returns exactly the expected method) and a lowercase-normalization case, and factors the two test.each blocks through a shared measure() helper.
Security risks
None. This is a test-only change with no production code touched. No network, filesystem, credentials, or user input handling is involved; the localhost:3000 URL is a string literal passed to the Request constructor and never dialed.
Level of scrutiny
Low-to-moderate. The change is small, self-contained, and confined to one test file with no CODEOWNERS coverage. The methodology deserves a careful read because leak-guard tests are easy to get subtly wrong, but the retention-then-GC approach is sounder than what it replaces: retaining samples means a regressed getter's allocations cannot be swept before the final reading, and GC'ing before both readings removes noise from unrelated dead strings. The 8x margin between budget and regressed-behavior growth matches the REVIEW.md guidance that leak-guard bounds sit well below what the unfixed leak produces. The PR description reports the negative check (reading .url instead of .method fails all 8 cases with growth ≈ iterations), which is the "prove the test fails for the right reason" evidence the review rules ask for.
Other factors
The refactor tracks repo conventions closely: isASAN/isDebug branching for iteration count, test.each for the parameter matrix, module-scope imports only, no per-test timeouts, .toEqual for the structured distinct assertion, and comments that explain the non-obvious "why" (retention makes the count GC-independent) rather than restating code. The tests remain serial, which is correct since heapStats() is process-wide and the bodies are synchronous anyway. The bug hunt exited on dry_streak with no findings and no ruled-out candidates, and the PR timeline shows no outstanding third-party objections.
Problem
test/js/web/request/request-method-getter.test.tstakes 19.3s on the debian 13 x64-asan lane (build #108217) and under 3s on release lanes. A local debug ASAN build takes 124s, and every test exceeds the 5s timeout.clone().methodreads and 128Knew Request().methodreads against a fixed budget of 512 new JSStrings. A few thousand reads detect a per-call allocation.Fix
iterations / 8, so a per-call allocation overshoots it by 8x.Bun.gc(true)before bothheapStats()readings. A leaked string stays live until the final reading, so the count does not depend on when the GC runs."GET"or"POST"), that every clone reports its source's method, and thatmethod: "post"normalizes to"POST"on the same cached string. The failure message reports the measured growth..urlinstead of.methodfails all 8 cases with growth 1022 of 1024 (debug) and 8190 of 8192 (release).Background
heapStats().objectTypeCounts.string(bun:jsc) counts JSString cells on the heap without a GC, so dead strings count until a collection sweeps them.request.methodreturns one cell from the common strings table (Bun__HTTPMethod__toJS) on every call. Make request.method getter not allocate memory #18961 made it so, and this test guards that.Notes
USE_SYSTEM_BUN=1): 486ms before, 147ms after..url). Retention is what makes the final GC safe. Without the final GC the old test counted dead strings, which works until an Eden collection runs inside the loop.test.concurrentwould not overlap them anyway.new Request(url, { method: "Post" })and{ method: "BREW" }both return"GET"in Bun. Node returns"POST"and"BREW". The new row uses the all-lowercase form, which Bun accepts.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file