Conversation
…trengthen assertions The two GC-guard tests each spawned a bun child; in contended parallel CI batches on Windows the per-process overhead (spawn, startup, zlib module load) stacked up past the per-test timeout while the file passes alone. Compression volume is nearly irrelevant on the repetitive buffer, so the fix is structural: one child interleaves 8 brotli + 8 zstd streams with the same 34 full-GC sample count as the old two processes combined, with samples timed at write-queued, mid drive loop, and after close. Also lowers compression quality/level (the guarded race is marking-thread vs work-pool timing, not compression effort) and tightens assertions: native class names, footprint stability while live, a footprint ceiling, _handle nulled on destroy, per-stream output required, exact fixture stdout.
|
Warning Review limit reached
Next review available in: 9 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 (1)
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 de-flake with strengthened assertions; the inline nit about the two identical test.concurrent bodies is a fair cleanup but non-blocking.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/zlib/zlib-estimated-size-gc.test.ts, which guards the #31865 regression (concurrent-GC estimatedSize on NativeBrotli/NativeZstd). It merges two spawned child processes into one to cut the per-process fixed cost that was causing timeouts in Windows parallel-batch CI, and strengthens the in-process assertions (native class name, stable-while-live read, _handle nulled after destroy, upper bound on the estimate) and the fixture assertions (every stream must reach end with non-empty output, exact sorted stdout, empty stderr, exit 0).
Security risks
None. Test-only change; no production code, no untrusted input handling, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The key risk in a de-flake is silently weakening the guard, so I checked that specifically: the GC sample count is preserved (2 per stream × 16 streams + 2 finals = 34, same as before), the GC timing is arguably better (one sample with the write queued, one mid drive-loop on first data, plus a final after close), and both native classes are now live in the same heap for every GC. The fixture wires error → reject, and an unhandled rejection surfaces via non-zero exit / non-empty stderr, which the test asserts against — so failure paths still fail the test loudly. The new max ceilings (60k / 1M) are generous enough to survive dep bumps but tight enough to catch a garbage read.
Other factors
One nit was flagged: the two test.concurrent bodies are now byte-identical since both await the same memoized gcRun() and assert the same sorted array. That's redundant per REVIEW.md ("if your fix makes two functions byte-identical, delete one"), and -t zstd will still assert the brotli line — but it doesn't weaken coverage or introduce flakiness, so it's a cosmetic cleanup rather than a blocker. The PR description includes concrete before/after timings on the affected Windows lane and a 32-way contention simulation showing the slowest fixture halves, which is exactly the metric that was tripping the timeout.
…ncident description The two GC-guard tests asserted identical things on one shared child and could never diverge, so one honestly-named test replaces the memoized pair; -t brotli and -t zstd still match it. Fixture errors now carry their group name so the stderr of a rejection identifies the failing codec. Also drop the timeout claim from the comment: the CI incident this file was flagged for was a parallel-batch worker crash (exit code 9) that passed on solo retry, not a timeout.
There was a problem hiding this comment.
LGTM — the duplicate-test feedback from the earlier pass is addressed in 7ad49f4, and the merged fixture + strengthened assertions look correct.
What was reviewed:
- Fixture rejection path: no
.catch()on the twoPromise.allchains, but a rejected stream surfaces as an unhandled rejection with the group-prefixed message on stderr and a non-zero exit, which the assertions catch — so failures self-identify as intended. toBeGreaterThan(855)for brotli decode is fine even though 855 is the exact cached footprint:estimateShallowMemoryUsageOfaddssizeof(cell), so the total is strictly greater. This bound was unchanged from the original anyway.- GC sample count and coverage: 16 streams × 2 GCs + 2 finals = 34, same as before; both native classes now live in one heap so every GC marks both. Compression effort is lowered but the guarded race is marking-thread timing, not codec effort.
Extended reasoning...
Overview
Test-only refactor of test/js/node/zlib/zlib-estimated-size-gc.test.ts, the guard for #31865 (NativeBrotli/NativeZstd estimatedSize on the concurrent GC marking thread). Two changes: (1) the four in-process checkFootprintStableAcrossClose tests gain assertions for the native class name, a second live read for stability, _handle nulled after destroy(), and a generous max ceiling; (2) the two per-codec spawned fixtures are merged into a single child that interleaves 8 brotli + 8 zstd streams, with GCs fired after write() and again on the first data event, and per-group Promise.all gates that print "<group> OK" only after every stream in the group reaches end with non-zero output. The parent asserts empty stderr, exact sorted stdout ["brotli OK", "zstd OK"], and exit 0.
Prior feedback
My earlier pass flagged that an intermediate revision had two byte-identical test.concurrent bodies awaiting the same memoized spawn. The author took the collapse-into-one option in 7ad49f4: single test named brotli+zstd: … (still matches both -t brotli and -t zstd), memoized helper removed, and fixture errors now prefix the group name so the stderr path also identifies the failing codec. That thread is resolved and the fix is exactly what was suggested.
Security risks
None. No production code is touched; the change is confined to a test file that spawns bunExe() with bunEnv and a fixed inline script.
Level of scrutiny
Low-to-medium. Test-only, one file, no new harness patterns. The main risk with a change like this is silently weakening a regression guard, so I checked coverage explicitly: GC count is preserved at 34, both classes are now marked on every GC (previously each class only saw its own 17), the fixture now requires the full stream lifecycle (end with output) rather than just the write callback, and the in-process tests add four new assertions without removing any. Every assertion is at least as strong as before.
Other factors
Harness conventions are followed: await using for the subprocess, concurrent pipe draining via Promise.all, stderr asserted before stdout before exit code, bunEnv/bunExe(), no sleeps or hardcoded ports. The lowered brotli quality / zstd level is justified — the guarded bug is a marking-thread vs work-pool race, not compression volume, and chunkSize 32KB against 128KB input still yields multiple native rounds per write. The PR body reports verification on debug+ASAN Linux and Windows aarch64 (both idle and 32-way parallel), all green.
There was a problem hiding this comment.
LGTM — the duplicate-test feedback is addressed (single brotli+zstd fixture test), and every assertion is now equal to or stronger than before.
What was reviewed
- Confirmed no coverage loss: the fixture now requires each stream to reach
endwith output (old fixture only waited on the write callback), and the in-process tests add class-name, upper-bound, stability, and_handle-nulled checks on top of the existing floor andafter === beforecontract. - Checked the rejection path:
drive()wireserrorto reject with a group-prefixed message; the unhandled rejection surfaces on stderr, which is asserted empty before the stdout/exit checks — so a codec-specific failure still self-identifies. - GC sample count preserved (16×2 + 2 finals = 34, same as the two old fixtures combined); lowered compression effort doesn't affect the marking-thread race being guarded.
Extended reasoning...
Overview
Test-only refactor of test/js/node/zlib/zlib-estimated-size-gc.test.ts, the guard for #31865 (NativeBrotli/NativeZstd estimatedSize on the concurrent GC marking thread). Two changes: (1) the four in-process checkFootprintStableAcrossClose tests gain extra assertions — native class name, a second live read to prove stability, engine._handle nulled after destroy(), and a generous upper bound to catch garbage/uninitialized reads; (2) the two per-codec spawned GC fixtures are merged into one child process that interleaves 8 brotli + 8 zstd streams, with GC samples now placed after write() and on first data (mid drive loop) plus one per-group final after close.
Prior feedback resolved
My earlier review noted the intermediate revision had two byte-identical test.concurrent bodies awaiting the same memoized spawn. Commit 7ad49f4 collapsed them into a single brotli+zstd: … test and removed the memoized helper, exactly as suggested; the exact sorted-stdout assertion is retained, and fixture rejections now carry a <group>: prefix so the stderr assertion identifies the failing codec even when no stdout diff is available. Nothing outstanding remains from that thread.
Security risks
None. This touches only a test file: no runtime code, no auth/crypto/network paths, no external hosts contacted (the fixture is a local bun -e child using bunEnv/bunExe).
Level of scrutiny
Low-to-moderate. The change is self-contained in one test file with no production code impact. The main risk for a GC-guard test refactor is silently weakening what it protects — I checked each assertion against the old version and every one is preserved or strengthened. The fixture's completion criterion moved from "all write callbacks fired" to "every stream produced output and reached end", which is strictly more; stdout is now asserted exactly (sorted ["brotli OK", "zstd OK"]) rather than a single trimmed OK. Compression effort was reduced (brotli quality 2, zstd level 1, 128KB buffers) but the guarded condition is marking-thread vs work-pool timing, which only requires work to be in flight when GC fires — the GC placement makes that at least as likely as before.
Other factors
- Verified test run in the PR's evidence block: 5 pass / 0 fail, 27 expect() calls, 5.86s under debug+ASAN.
- No CODEOWNERS on this path.
- The
Promise.all(...).then(...)in the fixture intentionally has no.catch(): a rejected group becomes an unhandled rejection on stderr, andexpect(stderr).toBe("")is asserted first — this is by design per the author's reply and is an improvement over the old fixture'sthrowinside an event callback. - The lone remaining
test.concurrentno longer has a peer to run alongside, but it's harmless.
|
The worker crash this PR's batch failures trace back to is root-caused in #37140: worker_threads argv/execArgv StringImpls were shared by pointer with the worker thread, worker-side atomization poisoned them, and the parent's GC sweep of the Worker wrapper aborted in AtomStringImpl::remove (silent 0xC0000409 on Windows release). zlib-estimated-size-gc.test.ts was just the file on CPU when the batch worker died. |
This branch has not been deployed
1 participant
Problem
In build 89973,
test/js/node/zlib/zlib-estimated-size-gc.test.ts(the guard for #31865, added in #31867) failed its parallel batch on both Windows lanes and passed when retried alone. The job logs show the batch failure was the worker process dying while on this file (worker crashed: exit code 9, the u8-truncated form of NTSTATUS 0xC0000409), with the solo retry passing 6/6 in about 200ms; there are no test timeouts in those logs. A test-only PR cannot fix a native worker crash, and parallel workers run many files before the one they die on, so the crash is not even proven to be this file's doing.What a test-only PR can do is shrink the file's footprint in contended parallel batches, where it was flagged as expensive, and strengthen what it asserts. Measured under the debug+ASAN build on Linux: child startup 0.38s,
require("zlib")about 0.6s, the 17 full GCs about 0.3s, while compression volume is nearly irrelevant (the repetitive buffer compresses in about the same time at brotli quality 11 as at quality 2, and shrinking it 4x did not move the numbers). The dominant fixed cost was paid twice because each codec's fixture spawned its own child.Change
write(); now one fires with the write queued on the work pool and one on the stream's firstdataevent, i.e. mid drive loop, and each group's final GC marks the already-closed handles (the after-closeestimatedSizepath on the marking thread).chunkSize32KB keeping 4 native chunk rounds per write as before. The guarded race is marking-thread vs work-pool timing, not compression effort, so the work only needs to be in flight when the GC fires, which is unchanged.-t brotliand-t zstd, and a failing group identifies itself either in stderr (fixture errors are prefixed with their group name) or as the missing"<group> OK"line in the exact stdout assertion.Strengthened assertions:
NativeBrotli/NativeZstd), footprint stable across two live reads,engine._handlenulled bydestroy(), and a generous ceiling that catches a garbage estimate, on top of the existing floor and theafter === beforecontract;end(full lifecycle, not just the write callback) before its group prints OK, stdout is asserted exactly (sorted["brotli OK", "zstd OK"]), stderr must be empty, exit code 0.Test count goes from 6 to 5: the four per-mode value tests are untouched, no coverage is removed, and every remaining assertion is strictly stronger than before.
Verification
bun bd test test/js/node/zlib/zlib-estimated-size-gc.test.ts(debug+ASAN, Linux): 7.66s before; 5.9 to 7.9s across repeated runs after, all pass.[stamp-90s] gate passed · iteration 2 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file