Conversation
|
@robobun adopt |
|
Updated 10:07 PM PT - Jul 7th, 2026
❌ @robobun, your commit 611d473 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31505That installs a local version of the PR into your bun-31505 --bun |
|
✅ Ready for review. Reproduced: brotli Fix: matches Node's landed nodejs/node#63746. State: merged |
WalkthroughAdds codec-specific MAX_FLUSH and write-time validation, coerces out-of-range Brotli flush to Op::process, wraps native write in JS to forward sync errors to callbacks, updates macro invocations, and expands Brotli/Zstd tests and timeouts. ChangesCompression flush validation and tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 1
🤖 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 `@test/js/node/zlib/zlib.test.js`:
- Around line 356-359: The test currently asserts stderr is exactly empty
(expect(stderr).toBe("")) which fails on ASAN shards; update the assertion to
first remove known ASAN startup warnings by splitting stderr into lines,
filtering out lines that match the ASAN startup pattern (e.g. lines starting
with "WARNING: ASAN interferes" or the full ASAN warning regex), then rejoining
the remaining lines and asserting that the filtered stderr is empty; apply this
change to the assertion around the first use of proc.stderr.text() (the block
using proc, stdout, stderr, exitCode and zlib.brotliDecompressSync) and the
similar assertion later in the file so both expect(stderr).toBe("") checks use
the filtered-stderr check instead.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 512a0522-4b4c-44b4-96de-7cc22eedff1a
📒 Files selected for processing (2)
src/runtime/node/zlib/NativeBrotli.rstest/js/node/zlib/zlib.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces a process-aborting unreachable!() with a safe fallback to BROTLI_OPERATION_PROCESS, matching Node and prior Bun behavior, with subprocess regression tests.
Extended reasoning...
Overview
This PR changes a single match arm in src/runtime/node/zlib/NativeBrotli.rs's Context::set_flush: the catch-all arm for flush values outside 0..=3 now maps to Op::process instead of panicking with unreachable!(). The accompanying comment is updated to explain that the shared zlib write path validates flush against the full zlib range (0..=6), so Z_FINISH (4) and Z_BLOCK (5) can legitimately reach this function, and that both Node's static_cast and the brotli C encoder treat unknown ops with PROCESS semantics. Four regression tests are added in test/js/node/zlib/zlib.test.js that spawn a subprocess, call .flush(Z_FINISH) / .flush(Z_BLOCK) on brotli compress/decompress streams, and assert clean exit plus a correct round-trip.
Security risks
None. The change replaces an uncatchable process abort with a defined, benign operation (PROCESS — continue encoding without flushing/finishing). No new inputs are accepted, no buffer math changes, and the decoder already ignores flush entirely. The fallback is strictly less dangerous than the prior behavior.
Level of scrutiny
Low. The functional change is one line, the rationale is well-documented and matches Node.js semantics (which pass the raw int through via static_cast), and the prior behavior was a hard crash — so any regression risk is bounded by "behaves like PROCESS instead of aborting." The tests run in a subprocess specifically to verify the abort no longer occurs and that compressed output still decompresses correctly.
Other factors
No CODEOWNERS cover these files. No prior reviewer comments to address. The bug-hunting system found no issues. The test additions follow existing patterns in the file (bunExe/bunEnv/Bun.spawn with await using). The added harness imports (bunEnv, bunExe) are standard across the test suite.
There was a problem hiding this comment.
LGTM — replaces a process-aborting unreachable!() with a safe Op::process fallback that matches Node and the brotli C encoder, with subprocess round-trip tests covering both flush values.
Extended reasoning...
Overview
This PR touches two files: a one-arm change in src/runtime/node/zlib/NativeBrotli.rs:set_flush (replacing n => unreachable!(...) with _ => Op::process) and four new subprocess tests in test/js/node/zlib/zlib.test.js. The source change stops brotli streams from aborting the entire process when .flush() is called with a zlib flush constant (Z_FINISH=4, Z_BLOCK=5, Z_TREES=6) that the shared write path validates as in-range but the brotli encoder doesn't define. Out-of-range values are now coerced to BROTLI_OPERATION_PROCESS, which the PR description verifies is exactly what the brotli C encoder does with an unknown op (is_last=false/force_flush=false), and what Node and Bun 1.3.14 already do.
Security risks
None. This is compression-stream flush handling with no auth, crypto, permissions, or untrusted-input parsing involved. The change is strictly a hardening: it removes an uncatchable runtime panic reachable from user JS and replaces it with a benign no-op-equivalent operation. The decoder ignores flush entirely, so the decompress path is unaffected semantically.
Level of scrutiny
Low-to-moderate. The behavioral change is a single match arm in a small, well-understood function, and it converts a hard abort into the same fallback Node uses. The updated comment thoroughly documents why (shared write path validates 0..=6, brotli only defines 0..=3, C encoder treats unknown ops as PROCESS). The tests are subprocess-isolated so a regression would surface as a non-zero exit code rather than crashing the test runner.
Other factors
- Bug hunter found no issues.
- The one CodeRabbit review comment (filter ASAN stderr noise before asserting empty) was addressed in 73198ac and confirmed resolved; the tests now use the repo-standard
.filter(l => l && !l.startsWith("WARNING: ASAN interferes"))pattern. - Tests follow repo conventions:
bunEnv/bunExe,await usingfor the spawned process, exit code asserted last after stdout/stderr checks. - No CODEOWNERS cover these paths.
- robobun validated the gate both ways (revert → panic, fix → green).
|
Actionable comments posted: 0 |
1 similar comment
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
LGTM — replacing the unreachable!() with Op::process is a minimal, well-justified fix for the abort, and the round-trip tests cover it.
Extended reasoning...
Overview
This PR touches two files: a one-line behavioral change in src/runtime/node/zlib/NativeBrotli.rs (replacing unreachable!() with Op::process in the catch-all arm of set_flush), and test additions/adjustments in test/js/node/zlib/zlib.test.js (new subprocess-based regression tests for .flush(Z_FINISH)/.flush(Z_BLOCK) on brotli streams, plus extended timeouts for two existing 50MB streaming tests under debug/ASAN).
Security risks
None. The change makes the flush-value match arm more permissive by mapping out-of-range zlib flush constants (4/5/6) to BROTLI_OPERATION_PROCESS instead of aborting the process. No user-controlled data reaches new code paths, no auth/crypto/permissions involved, and the fallback semantics match what the brotli C encoder already does with an unrecognized op (is_last=false/force_flush=false).
Level of scrutiny
Low-to-medium. While this is native runtime code, the change is strictly a panic→fallback conversion that cannot regress previously-working inputs (values 0–3 are unchanged). The fallback choice is well-justified in the comment and PR description (matches Node's static_cast pass-through and the prior Zig @enumFromInt behavior). The fix restores parity with Bun 1.3.14 and Node.
Other factors
- The bug-hunting system found no issues.
- The single CodeRabbit review comment (filter ASAN stderr noise) was addressed in 73198ac and confirmed resolved.
- New tests run in subprocesses (so a regression to the abort would be caught as a non-zero exit rather than killing the test runner), follow repo conventions (
bunEnv/bunExe, ASAN stderr filtering, exit-code asserted last), and verify full round-trip correctness. - The timeout bumps (
isDebug || isASAN ? 60_000 : 15_000) are reasonable test-infra adjustments with no production impact. - CI shows build-rust failures on an earlier commit (2fc2459), but the actual Rust diff is a trivial match-arm change that cannot itself cause a compile error; the latest commit is an autofix.ci formatting pass.
There was a problem hiding this comment.
LGTM — minimal fix replacing a process-aborting panic with PROCESS semantics, matching Node and the brotli C encoder's own fallback; well covered by the new round-trip tests.
Extended reasoning...
Overview
Two files touched: a one-line behavioral change in src/runtime/node/zlib/NativeBrotli.rs (the catch-all arm of set_flush now maps to Op::process instead of unreachable!()), and additions to test/js/node/zlib/zlib.test.js (four new subprocess-based regression tests for .flush(Z_FINISH)/.flush(Z_BLOCK) on brotli compress/decompress, plus Debug/ASAN timeout bumps on two existing streaming tests). The large-looking test diff is mostly reindentation from moving the timeout argument.
Security risks
None. No user input parsing, auth, crypto, or filesystem changes. The fix narrows behavior from "abort the entire process" to "treat as PROCESS", which is strictly safer for callers. The flush value is already validated upstream against 0..=6, so this is just handling 4/5/6 gracefully rather than trapping. The new tests spawn bunExe() with fixed inline scripts and bunEnv, following the established harness pattern — no injection surface.
Level of scrutiny
Low. The native change is a single match-arm substitution with a clear, well-documented rationale: the shared write path validates against the zlib flush range (0..=6), brotli only defines 0..=3, and the brotli C encoder treats unknown ops as PROCESS (is_last=false/force_flush=false). The PR description confirms this was verified against vendor/brotli/c/enc/encode.c, and that Node and Bun 1.3.14 already behave this way. The decoder ignores flush entirely, so the decompress side is unaffected functionally.
Other factors
- robobun gate-verified both directions (revert → panic, fix → green).
- The one review comment (CodeRabbit's ASAN stderr filtering) was addressed in 73198ac and confirmed resolved; the final diff shows the filter applied in both test blocks per repo convention.
- Exit code is asserted last, after stderr/stdout assertions, per repo test guidelines.
- No CODEOWNERS entries match these paths.
- No bugs found by the bug-hunting system.
|
@robobun please replace the current fix with a cleaner one that eliminates the bug at the validation layer instead of papering over it in Root cause: flush validation is codec-blind. The change:
Behavior change (intended, maintainer-approved): brotli Tests (replace the current "does not abort" brotli flush tests in
When it's pushed, update the PR title/body to reflect the new approach: "reject invalid brotli flush values at validation" rather than "map out-of-range flush to PROCESS", and reference nodejs/node#63701 as the corresponding upstream bug. |
|
@robobun here's the concrete implementation of the codec-aware-validation approach — please apply this exact diff, then build/test/verify and push to this branch. I wrote it but couldn't build-verify it locally (env constraints), so the key thing is to confirm it compiles, run the tests, and pin down how the rejection surfaces (see the test note). It eliminates the bug class instead of patching the crash site: diff --git a/src/runtime/node/node_zlib_binding.rs b/src/runtime/node/node_zlib_binding.rs
@@ fn jsv_to_u32
-/// Local `std.meta.intToEnum(FlushValue, n)` shim — `bun_zlib::FlushValue` has
-/// no `TryFrom<u32>` impl upstream.
-#[inline]
-fn flush_value_is_valid(n: u32) -> bool {
- // FlushValue is `#[repr(C)]` with discriminants 0..=6.
- n <= 6
-}
-
@@ pub(crate) trait CompressionStreamImpl: Sized + Taskable + 'static {
type Stream: CompressionContext;
+ /// Largest accepted `flush` value for this codec. zlib/gzip and zstd take
+ /// the full zlib flush range (0..=6); brotli only the four
+ /// `BrotliEncoderOperation` values (0..=3), so a zlib-only mode like
+ /// Z_FINISH or Z_BLOCK is rejected at the write boundary instead of
+ /// reaching the encoder (where it would spin — see nodejs/node#63701).
+ const MAX_FLUSH: u32;
+
@@ both validation sites (in `write` and `write_sync`, ~lines 318 and 599)
- if !flush_value_is_valid(flush) {
+ if flush > T::MAX_FLUSH {
@@ macro_rules! __impl_compression_stream
- ($native:ident, $ctx:ty, $type_name:literal) => {
+ ($native:ident, $ctx:ty, $type_name:literal, $max_flush:expr) => {
@@ impl CompressionStreamImpl for $native {
type Stream = $ctx;
+ const MAX_FLUSH: u32 = $max_flush;And add the
Tests — replace the existing "flush(%s) does not abort the process" brotli tests in
Verify: |
|
@robobun one correction on the error type for the change above: throw Verified against Node v26: an out-of-range brotli flush (
Everything else in the patch (the |
robobun
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/zlib/NativeBrotli.rs (1)
420-431:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid silently coercing unexpected Brotli flush values
The Node zlib binding rejects
flush > T::MAX_FLUSHwithINVALID_ARG_VALUEbefore callingNativeBrotli::set_flush, so the_ => Op::processarm should remain unreachable; keeping it as a silent fallback can mask future boundary/parity regressions. Prefer failing fast by making the_caseunreachable!.Suggested change
self.flush = match flush { 0 => Op::process, 1 => Op::flush, 2 => Op::finish, 3 => Op::emit_metadata, - _ => Op::process, + _ => unreachable!("invalid brotli flush value: {flush}"), };🤖 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 `@src/runtime/node/zlib/NativeBrotli.rs` around lines 420 - 431, The match fallback in NativeBrotli::set_flush currently maps unexpected flush values to Op::process, which can silently hide boundary/parity regressions; change the `_ => Op::process` arm to call unreachable!() (e.g., unreachable!("invalid Brotli flush value in set_flush")) so the code fails fast if an out-of-range flush reaches set_flush, keeping the four explicit arms (0..3) intact and preserving the defensive comment.
🤖 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.
Outside diff comments:
In `@src/runtime/node/zlib/NativeBrotli.rs`:
- Around line 420-431: The match fallback in NativeBrotli::set_flush currently
maps unexpected flush values to Op::process, which can silently hide
boundary/parity regressions; change the `_ => Op::process` arm to call
unreachable!() (e.g., unreachable!("invalid Brotli flush value in set_flush"))
so the code fails fast if an out-of-range flush reaches set_flush, keeping the
four explicit arms (0..3) intact and preserving the defensive comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8356000-f2c8-410e-9535-7f2632c933ed
📒 Files selected for processing (6)
src/js/node/zlib.tssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rstest/js/node/zlib/zlib.test.js
18c5c95 to
e3d5cb0
Compare
The two 'streaming encode doesn't wait for entire input' tests push 50MB through the stream and pass comfortably on release but exceed the 15s timeout under the much slower debug/ASAN build. Bump the timeout to 60s on debug/ASAN, matching the existing convention used elsewhere.
Make flush validation codec-aware: each compression stream declares MAX_FLUSH (brotli 3, zlib/gzip/zstd 6) and the shared write path rejects anything above it with ERR_INVALID_ARG_VALUE, so a zlib-only flush value like Z_FINISH/Z_BLOCK never reaches the brotli encoder. processChunk now routes a synchronous rejection from the native write into the stream's error path (error event + errored write callback) instead of letting it escape into the stream machinery. Replaces the previous approach of coercing out-of-range brotli flush values to BROTLI_OPERATION_PROCESS.
Node v26 throws a TypeError with code ERR_INVALID_ARG_TYPE for an out-of-range flush value (both the zlib range check and brotli's), so match the code at both write-boundary validation sites. The message stays flush-specific rather than copying Node's incidental chunk-typed text.
e3d5cb0 to
062591b
Compare
|
@robobun let's make this type-safe instead of validating with a This replaces the 1.
|
Replace the MAX_FLUSH bound with a typed flush op: each codec declares FlushOp (brotli BrotliEncoderOperation, zlib FlushValue, zstd's raw c_int) and a fallible flush_op_from_u32 that is the single validation point. set_flush now takes the typed op, so an invalid flush value is unrepresentable past the write boundary and the catch-all arms in set_flush disappear. Accepted ranges are unchanged: brotli 0..=3, zlib/zstd 0..=6.
Instead of skipping the brotli/zstd 'streaming encode doesn't wait for entire input' tests under the sanitizer-instrumented build, give them a longer timeout like #31505 (with extra headroom: 50 MB through ASAN brotli measures ~64s on a slow runner, so 180s instead of 60s). Also note on the two bomb-cap edge tests that the cap-ordering is manually-verified-only: they allocate multiple GiB so they skip on CI, and the 1 GiB cap is hardcoded with no injection point.
…bort # Conflicts: # src/runtime/node/node_zlib_binding.rs # src/runtime/node/zlib/NativeBrotli.rs # src/runtime/node/zlib/NativeZlib.rs # src/runtime/node/zlib/NativeZstd.rs
|
Hit this independently while triaging a report of a JS-reachable process abort, and landed on the same root cause: the shared write entry validates Two things from that triage that may be useful here:
|
|
@robobun nodejs/node#63746 merged, so lets match its behaviour |
|
Aligned with nodejs/node#63746 in
Verified: |
…bort # Conflicts: # test/js/node/zlib/zlib.test.js
…in JS Node landed nodejs/node#63746 for nodejs/node#63701 (the upstream issue this PR opened): ZlibBase.prototype.flush(kind) now runs kind through checkRangesOrGetDefault against the codec's FLUSH_BOUND before queuing the fake flush chunk, so an out-of-range kind throws ERR_OUT_OF_RANGE synchronously (RangeError), a non-number throws ERR_INVALID_ARG_TYPE, and undefined/NaN fall through to the default. Apply the same here and store _flushBoundIdx on the instance to make FLUSH_BOUND available in .flush(). Vendor Node's test-zlib-brotli-flush-invalid-kind.js; update the subprocess regression test to assert the synchronous ERR_OUT_OF_RANGE throw. The typed FlushOp native validation stays as defense-in-depth.
The comments at the native write-boundary validation sites (and the flush_op_from_u32 doc comments) still cited the pre-nodejs/node#63746 rationale of matching Node's incidental ERR_INVALID_ARG_TYPE. Reword them to describe this check as what it is now: defense-in-depth for callers that bypass .flush()'s FLUSH_BOUND validation in zlib.ts (which throws ERR_OUT_OF_RANGE per nodejs/node#63746).
The comment said the narrowed validation is brotli-only, which was true for the earlier native-only iteration but is stale after matching nodejs/node#63746: FLUSH_BOUND validation now applies to every codec (zlib [0, Z_BLOCK], brotli [0, 3], zstd [0, 2]).
…bort # Conflicts: # src/runtime/node/node_zlib_binding.rs # src/runtime/node/zlib/NativeBrotli.rs # src/runtime/node/zlib/NativeZlib.rs # src/runtime/node/zlib/NativeZstd.rs
|
Merged
Net diff against main is 7 files: One observation for the record: Node 26.3 (before its fix) does not accept these values silently; |
This branch has not been deployed
2 participants
Brotli streams aborted the whole process (an uncatchable
unreachable!()panic, in release too) when.flush()was called with a zlib flush value the brotli encoder doesn't define, likeZ_FINISH(4) orZ_BLOCK(5). The shared write path validatedflushagainst the full zlib range for every codec, but brotli only definesBrotliEncoderOperation0..=3, so these values reached brotli'sset_flushand trapped. Node had the same codec-blind gap and spun at 100% CPU on these instead; they fixed it in nodejs/node#63746 for nodejs/node#63701.Fix (matches Node's landed behavior):
ZlibBase.prototype.flush(kind)validateskindagainst the codec'sFLUSH_BOUNDviacheckRangesOrGetDefaultbefore queuing the fake flush chunk, so an out-of-range number throwsERR_OUT_OF_RANGE(RangeError) synchronously, a non-number throwsERR_INVALID_ARG_TYPE(TypeError), andundefined/NaNfall through to the default flush op._flushBoundIdxis stored on the instance to make that lookup possible. This is exactly what zlib: validate flush kind for brotli streams nodejs/node#63746 does, and the vendored upstream test passes.FlushOp(brotliBrotliEncoderOperation, zlibFlushValue, zstd's rawc_int) with a fallibleflush_op_from_u32as the single native validation point, so an invalid flush value is unrepresentable past the write boundary andset_flushis total (no catch-all, nounreachable!).processChunkroutes a synchronous native rejection into the stream's error path so the_processChunk()backwards-compat entry never lets one escape into the stream machinery either.Tests (
test/js/node/zlib/zlib.test.js+ vendored Node parallel test):test/parallel/test-zlib-brotli-flush-invalid-kind.jsvendored verbatim: gzip/brotli/zstd × valid/out-of-range/non-number/undefined/NaN/callback-only flush kinds.flush(Z_FINISH)/.flush(Z_BLOCK)→ syncERR_OUT_OF_RANGERangeError, run in a subprocess so a regression (abort or spin) fails the test instead of killing the runner.flush()(implicit) and.flush(BROTLI_OPERATION_FLUSH)still round-tripcreateDeflate().flush(Z_FINISH)still round-tripsVerification:
bun bd test test/js/node/zlib/zlib.test.js→ 385 pass / 0 fail; all 57test/parallel/test-zlib-*.jspass; the four subprocess rejection tests and the vendored Node test both fail on the unfixed canary (abort / wrong error type).bun run rust:check-all→ 10/10 targets ok.[review] gate passed · iteration 12 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 12
evidence per changed file