node:zlib: don't abort when the native handle is driven outside the zlib.ts lifecycle - #32762
Merged
Merged
Claude / Claude Code Review
completed
Jun 26, 2026 in 33m 40s
Code review found 2 important issues
Found 3 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/node/zlib/NativeZlib.rs:133-135 |
init() lacks write_in_progress/pending_close guard → data race with worker-thread do_work() |
| 🔴 Important | src/runtime/node/zlib/NativeBrotli.rs:435-439 |
throw_if_closed misses mode==NONE state from brotli/zstd init() param-fail path |
| 🟡 Nit | test/js/node/zlib/zlib-handle-bounds-check.test.ts:160-167 |
Test run() helper pipes stderr but never drains it |
Annotations
Check failure on line 135 in src/runtime/node/zlib/NativeZlib.rs
claude / Claude Code Review
init() lacks write_in_progress/pending_close guard → data race with worker-thread do_work()
`init()` now checks `closed` but still doesn't check `write_in_progress` / `pending_close`, so `h.write(...); h.init(...)` runs `stream.with_mut(|s| s.init(...))` on the JS thread while the worker thread is inside `stream.with_mut(|s| s.do_work())` — two concurrent `&mut Context` on the same `JsCell` (no runtime borrow check), i.e. `deflateInit2_` racing `deflate()` on the same `z_stream`. This is pre-existing, but it's the same out-of-lifecycle abort class this PR is closing and the new `throw_
Check failure on line 439 in src/runtime/node/zlib/NativeBrotli.rs
claude / Claude Code Review
throw_if_closed misses mode==NONE state from brotli/zstd init() param-fail path
The `set_params`-failure branch here calls `s.close()` (which sets `mode = NONE` and frees the encoder) but never sets `self.closed`, so `throw_if_closed` still passes afterward. A subsequent `writeSync()` then survives the new `state.is_none()` guard in `do_work()` only to abort in `get_error_info()`'s `_ => unreachable!()` arm (mode is `NONE`); a second `init()` likewise hits `Context::init`'s `_ => unreachable!()`. `NativeZstd::init` has the same gap. Adding `self.closed.set(true)` next to `s
Check warning on line 167 in test/js/node/zlib/zlib-handle-bounds-check.test.ts
claude / Claude Code Review
Test run() helper pipes stderr but never drains it
The `run()` helper sets `stderr: "pipe"` but never reads it — only `stdout` and `exited` are awaited. Per CLAUDE.md ("Subprocess tests: drain pipes concurrently"), an unread pipe can fill the OS buffer and deadlock the child, and on a regression the Rust panic backtrace (which goes to stderr) is silently discarded so the failure shows only `{ stdout: "", exitCode: 134 }`. Add `proc.stderr.text()` to the `Promise.all` and include it in the returned object, matching the pre-existing subprocess tes
Loading