Upgrade reported Node.js version to 26.3.0 - #31991
Merged
Merged
Conversation
Collaborator
|
Updated 1:33 PM PT - Jun 16th, 2026
✅ @cirospaciari, your commit 5c81608e6b9eaf6f79a578d6a6ac9cd12d03d398 passed in 🧪 To try this PR locally: bunx bun-pr 31991That installs a local version of the PR into your bun-31991 --bun |
Contributor
|
Found 3 issues this PR may fix:
Also advances tracking issues #4290 and #3110 (V8 C++ API coverage) with new shim symbols:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
cirospaciari
force-pushed
the
claude/upgrade-nodejs-26-v2
branch
2 times, most recently
from
June 8, 2026 19:09
a832c84 to
3693c99
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPins and build flags updated to Node 26.3.0 / V8 14; streams, webstreams adapters, HTTP/HTTP2/HTTPS internals, V8 ABI shims/exports, and tests/harness adjusted to match Node 26 semantics. ChangesNode 26 alignment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/v8/v8.test.ts (1)
32-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid mutating shared
bunEnvat module scope.This mutates the shared harness env object directly, so changes can bleed into other tests and parallel workers. Build a per-file/per-spawn env via spread-copy and mutate that copy instead.
Suggested fix
-delete bunEnv.CC; -delete bunEnv.CXX; - -// Node.js 26.3.0 requires C++20 -bunEnv.CXXFLAGS ??= ""; -if (process.platform == "darwin") { - bunEnv.CXXFLAGS += " -std=gnu++20"; -} else { - bunEnv.CXXFLAGS += " -std=c++20"; -} +const addonEnv = { ...bunEnv }; +delete addonEnv.CC; +delete addonEnv.CXX; + +// Node.js 26.3.0 requires C++20 +addonEnv.CXXFLAGS ??= ""; +if (process.platform == "darwin") { + addonEnv.CXXFLAGS += " -std=gnu++20"; +} else { + addonEnv.CXXFLAGS += " -std=c++20"; +} ... if (process.platform == "win32") { - bunEnv.__FAKE_PLATFORM__ = "linux"; + addonEnv.__FAKE_PLATFORM__ = "linux"; }Then use
env: addonEnvfor the subprocesses in this file.As per coding guidelines, "Use
bunEnvwith spread operator when modifying environment variables in tests - never mutate the shared object directly."🤖 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 `@test/v8/v8.test.ts` around lines 32 - 46, The code mutates the shared test harness env object (bunEnv) at module scope (deleting bunEnv.CC/CXX, appending to bunEnv.CXXFLAGS, setting bunEnv.__FAKE_PLATFORM__), which can leak state across tests; instead create a per-file/env copy (e.g., const addonEnv = { ...bunEnv }) and perform deletions/assignments on addonEnv (remove addonEnv.CC/CXX, append to addonEnv.CXXFLAGS, set addonEnv.__FAKE_PLATFORM__ for win32) and then pass that copy to subprocess calls via env: addonEnv so the shared bunEnv is never mutated.Source: Coding guidelines
🤖 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 `@src/js/node/_http_outgoing.ts`:
- Around line 337-352: The loop handling headers treats "set-cookie"
case-sensitively and can drop cookies when keys like "Set-Cookie" are present;
update the check in the headers iteration inside setHeaders (the block that
currently checks if (key === "set-cookie")) to perform a case-insensitive
comparison (e.g., compare String(key).toLowerCase() to "set-cookie") so it
matches any case variant, keep the existing cookies collection logic (cookies
??= []; cookies.push(...)) and the final this.setHeader("set-cookie", cookies)
behavior, and ensure non-string keys are stringified before lowercasing to match
the same rule used by setHeader().
- Around line 308-315: The code replaces a caller-supplied empty Set-Cookie
array with a marker string via this[kEmptySetCookie], breaking live-array
semantics; instead preserve and store the original rawName and the actual array
value when setHeader("set-cookie", []) is called and thread that stored
rawName/value through getHeader, getHeaders, getRawHeaderNames, hasHeader and
the headers getter (and any logic in setHeader/removeHeader) so subsequent
mutations to the original array are reflected; locate the handling around
kEmptySetCookie and change it to retain the array reference and raw header name
(rather than a marker string) and update the mentioned accessors (getHeader,
getHeaders, getRawHeaderNames, hasHeader, headers getter) to consult this
preserved rawName/value path.
In `@src/js/node/http2.ts`:
- Around line 2481-2485: Several send paths still convert array headers into
numeric-key objects; update the same header validation used in the respond*()
paths across Http2Stream.sendTrailers and ServerHttp2Stream.additionalHeaders
(and the other occurrences around the mentioned spots) so arrays are rejected
instead of spread. Replace the existing "$isObject(headers) ||
$isArray(headers)"/spread logic with the same guard used in respond: if headers
=== undefined set to {}; else if (!$isObject(headers) || $isArray(headers))
throw $ERR_INVALID_ARG_TYPE("headers", "object", headers); else proceed to use
headers as an object. Ensure you apply this same pattern in sendTrailers,
additionalHeaders, and the sibling sites flagged (the other occurrences with the
$isObject/$isArray + spread pattern).
- Around line 3318-3324: The close() method is registering callbacks after it
sets this.#closed and returns early if already closed, which can drop callbacks;
move the registration of the provided callback (the this.once("close", callback)
call) to before the closed/destroyed guard and before setting this.#closed so
callbacks are reliably queued even if close() has already been initiated; update
both occurrences of close() (the one referencing this.#closed/this.destroyed and
the other at the similar block around 3902-3908) to follow the same order: if
callback is a function register it with this.once("close", callback) first, then
check/handle closed/destroyed and set this.#closed. Ensure unique symbols
mentioned (close, this.#closed, this.destroyed, this.once("close", callback))
are updated accordingly.
- Around line 4029-4035: The current guard "if ($isObject(options) &&
options.signal)" treats falsy values (null, 0, false, "") as absent and skips
validation; change it to check presence of the property instead. Update the
condition around validateAbortSignal/options.signal (the block starting with "if
($isObject(options) && options.signal)" and the validateAbortSignal call) to use
a presence check (e.g., "signal" in options or
Object.prototype.hasOwnProperty.call(options, "signal")) so validateAbortSignal
is invoked even when options.signal is a falsy value, then continue to read
options.signal.aborted as before.
In `@src/js/node/https.ts`:
- Around line 43-69: The new HTTPS Agent stores this.maxCachedSessions but never
wires up TLS session caching; implement the same session-cache plumbing here by
adding an internal _sessionCache map and helper methods _getSession,
_cacheSession, and _evictSession on Agent, then in
Agent.prototype.createConnection use _getSession to supply a session when
calling tls.connect(options) and attach socket 'session' and 'close' hooks to
call _cacheSession(socket.session) and evict entries on close; ensure
_cacheSession respects this.maxCachedSessions and _evictSession removes oldest
entries so https.Agent({ maxCachedSessions }) behaves like the original
implementation.
In `@src/jsc/bindings/v8/shim/FunctionTemplate.cpp`:
- Around line 84-118: Add a compile-time guard that ties the hard-coded
constexpr viewOffset to V8's frame index so future changes don't break the slot
math: after declaring constexpr size_t viewOffset = 1, add a static_assert that
enforces viewOffset == -Info::kNewTargetIndex (or equivalently viewOffset +
Info::kNewTargetIndex == 0) so the slot lambda and uses of Info::kNewTargetIndex
/ slot(...) remain in-bounds; reference the symbols viewOffset, Info
(FunctionCallbackInfo<Value>), slot, and Info::kNewTargetIndex when adding the
assertion.
In `@src/jsc/bindings/v8/V8EscapableHandleScopeBase.cpp`:
- Around line 12-46: The constructor for V8EscapableHandleScopeBase currently
avoids pushing a Bun handle scope and thus prevents per-scope reclamation of
non-escaped Locals; restore per-scope reclamation by creating and owning a
short-lived Bun-side handle-scope or buffer that is pushed/popped locally while
still initializing the V8-visible words for ABI neutrality: in the
V8EscapableHandleScopeBase constructor (symbols: shim::getHandleScopeData,
m_isolate, m_previousHandleScope, m_buffer, data->level++), allocate or push a
Bun handle scope/buffer tied to this object (so non-escaped handles and the
reserved escape slot are reclaimed at scope destruction) and ensure Escape()
still uses current->m_buffer and escapeReservations().set(this, ...) as before;
make sure the destructor/pop restores HandleScopeData exactly like existing
paths so ABI behavior is unchanged.
In `@src/jsc/bindings/v8/V8String.cpp`:
- Around line 228-288: String::WriteUtf8V2 always uses
TextEncoder__encodeInto8/16 which replaces unpaired UTF-16 surrogates with
U+FFFD; update the function to honor WriteFlags::kReplaceInvalidUtf8 by
branching on that flag: when kReplaceInvalidUtf8 is set keep the current
TextEncoder__encodeInto* path, but when it is not set use the WTF-8-preserving
encoder (the variant that emits surrogate code units as WTF-8 rather than
calling utf16CodepointWithFFFD*/strings::copyUTF16IntoUTF8*). Modify both the
8-bit and 16-bit branches in String::WriteUtf8V2 to select the appropriate
encoder (instead of always calling
TextEncoder__encodeInto8/TextEncoder__encodeInto16) so the code path without the
flag preserves unpaired surrogates.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 5147-5158: The code currently casts id to u32 before checking
MAX_STREAM_ID which causes negative ids to overflow and be rejected before the
sentinel check; change the order and logic in the h2 goaway handling so you
first test for the sentinel (id <= 0) and only for positive ids perform the
u32/MAX_STREAM_ID validation and assignment to last_stream_id; specifically, in
the block handling lastStreamId use the signed id comparison (id <= 0) to
implement the sentinel behavior, then for id > 0 validate that id as u32 <=
MAX_STREAM_ID and call global_object.throw with the existing message if out of
range, and only then set last_stream_id = id as u32.
In `@test/cli/install/migration/complex-workspace.test.ts`:
- Around line 55-61: The current change sets installArgs to include
"--ignore-scripts" on Windows, which disables all lifecycle scripts (removing
coverage of packages/with-postinstall); instead, limit the workaround to avoid
the sharp/node-gyp failure while preserving other lifecycle script coverage:
keep the normal installArgs (bunExe(), "install") and, for Windows only, add a
targeted workaround that disables sharp's build (e.g., set an env var or install
a no-op for sharp) or, if simpler, run the global install without
--ignore-scripts and then explicitly run the postinstall for the fixture: after
subprocess = Bun.spawn(installArgs, ...) complete, spawn a second process to
execute the postinstall in packages/with-postinstall (e.g., Bun.spawn([bunExe(),
"run", "postinstall"], {cwd: "packages/with-postinstall"})) so sharp is handled
separately but the fixture's postinstall still runs and test coverage remains
intact.
In `@test/harness.ts`:
- Around line 2158-2162: The current branch creates a per-run Puppeteer cache
dir with tmpdirSync("puppeteer-cache") but never exposes a way to remove it;
change the return value so callers can clean it up — e.g., return both the cache
path under the PUPPETEER_CACHE_DIR key and a cleanup hook (or disposable object)
that removes the temp directory (use the tmpdirSync return's removeCallback or
equivalent). Update the returned object from the harness code that uses
tmpdirSync to include { PUPPETEER_CACHE_DIR: <path>, cleanup: <function> }
(reference tmpdirSync and the PUPPETEER_CACHE_DIR key so callers can call
cleanup in using/afterAll).
- Around line 232-239: canBuildNodeAddons() currently returns true
unconditionally on non-macOS which misleads callers; change it to actually probe
the system toolchain: when canBuildNodeAddonsCached is undefined, detect
platform and attempt to locate a C++ compiler (on POSIX check for
clang++/g++/c++ via which/command -v; on Windows check for cl.exe or MSVC
toolchain availability), run a harmless version check or --version spawn to
confirm it runs, and set canBuildNodeAddonsCached to true only on success (false
on failure); update the function (and use of canBuildNodeAddonsCached) to cache
this boolean and ensure callers of canBuildNodeAddons() rely on the real probe
result rather than assuming true.
- Around line 168-185: The test helper nodeExeMatchingAbi currently fetches Node
builds from nodejs.org (calls to fetch, shasumsUrl, response, etc.), which
violates hermetic test rules; change it to only use locally-provisioned
artifacts: remove the online fetch and checksum download logic and instead look
up a pre-fetched archive or extracted Node binary under a configurable local
cache/staging location (e.g., the existing stagingDir logic), failing fast with
a clear error if the expected version/archive (name + archiveExt) or checksum
manifest is not present; keep the write()/extraction/atomic-rename behavior but
source the archive from the local cache path and validate against a local
SHASUMS256.txt provided by CI/bootstrap.
In `@test/integration/next-pages/test/dev-server-puppeteer.ts`:
- Around line 33-43: The current code builds shell strings with the cachePath
variable and calls execSync (in dev-server-puppeteer.ts) which risks shell
injection; replace those execSync calls with child_process.execFileSync (or
spawnSync/execFileSync) using argv arrays instead of interpolated shell
commands: call execFileSync('xattr', ['-rd', 'com.apple.quarantine', cachePath],
{ stdio: 'ignore' }) for the quarantine removal and call execFileSync('find',
[cachePath, '-type', 'f', '-name', 'Google Chrome for Testing', '-exec',
'chmod', '+x', '{}', '+'], { stdio: 'ignore' }) and similarly for
'chrome-headless-shell' and 'chrome'; keep stdio: 'ignore' and preserve existing
error handling but do not enable shell parsing.
In `@test/js/node/stream/node-stream.test.js`:
- Around line 575-606: The spawned-process tests using Bun.spawn (variable proc)
only await Promise.all([proc.stdout.text(), proc.exited]) and must also drain
stderr to avoid deadlocks; update each affected Promise.all to include
proc.stderr.text() (i.e. Promise.all([proc.stdout.text(), proc.stderr.text(),
proc.exited])) for the block that runs the Writable/fromWeb test (the proc
created with bunExe() and the inline "-e" script) and do the same for the two
other subprocess blocks referenced (the ones around the other Bun.spawn calls),
ensuring the rest of the assertions still read stdout and exitCode as before.
- Around line 866-898: Add a test that verifies Readable.prototype.compose
respects an already-aborted options.signal: create an AbortController, call
controller.abort(), then call Readable.from(["a"]).compose(new PassThrough(), {
signal: controller.signal }) and assert that the call fails on the abort path
(check the thrown error is the expected abort error, e.g., has code
"ERR_ABORTED" or is an AbortError). Place the test alongside the existing
compose tests (referencing Readable.from, Readable.prototype.compose, and
AbortController) to prevent regressions of the signal-handling behavior.
---
Outside diff comments:
In `@test/v8/v8.test.ts`:
- Around line 32-46: The code mutates the shared test harness env object
(bunEnv) at module scope (deleting bunEnv.CC/CXX, appending to bunEnv.CXXFLAGS,
setting bunEnv.__FAKE_PLATFORM__), which can leak state across tests; instead
create a per-file/env copy (e.g., const addonEnv = { ...bunEnv }) and perform
deletions/assignments on addonEnv (remove addonEnv.CC/CXX, append to
addonEnv.CXXFLAGS, set addonEnv.__FAKE_PLATFORM__ for win32) and then pass that
copy to subprocess calls via env: addonEnv so the shared bunEnv is never
mutated.
🪄 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: 99e8555c-8c99-4c65-bbb6-977af11a75af
⛔ Files ignored due to path filters (1)
test/napi/napi-app/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (109)
flake.nixscripts/bootstrap.ps1scripts/bootstrap.shscripts/build/codegen.tsscripts/build/config.tsscripts/build/deps/nodejs-headers.tsscripts/build/flags.tsscripts/packer/windows-x64.pkr.hclsrc/js/builtins/CompressionStream.tssrc/js/builtins/DecompressionStream.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal/http.tssrc/js/internal/primordials.jssrc/js/internal/streams/duplex.tssrc/js/internal/streams/duplexify.tssrc/js/internal/streams/end-of-stream.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/writable.tssrc/js/internal/webstreams_adapters.tssrc/js/node/_http_common.tssrc/js/node/_http_outgoing.tssrc/js/node/_http_server.tssrc/js/node/http2.tssrc/js/node/https.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcessReportObjectWindows.cppsrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeHTTP.cppsrc/jsc/bindings/napi.cppsrc/jsc/bindings/v8/V8Array.cppsrc/jsc/bindings/v8/V8EscapableHandleScopeBase.cppsrc/jsc/bindings/v8/V8EscapableHandleScopeBase.hsrc/jsc/bindings/v8/V8External.cppsrc/jsc/bindings/v8/V8External.hsrc/jsc/bindings/v8/V8FunctionCallbackInfo.cppsrc/jsc/bindings/v8/V8FunctionCallbackInfo.hsrc/jsc/bindings/v8/V8HandleScope.cppsrc/jsc/bindings/v8/V8HandleScope.hsrc/jsc/bindings/v8/V8Isolate.cppsrc/jsc/bindings/v8/V8Isolate.hsrc/jsc/bindings/v8/V8Number.cppsrc/jsc/bindings/v8/V8Number.hsrc/jsc/bindings/v8/V8String.cppsrc/jsc/bindings/v8/V8String.hsrc/jsc/bindings/v8/V8Value.cppsrc/jsc/bindings/v8/V8Value.hsrc/jsc/bindings/v8/shim/FunctionTemplate.cppsrc/jsc/bindings/v8/shim/GlobalInternals.hsrc/jsc/bindings/v8/shim/Handle.hsrc/jsc/bindings/v8/shim/HandleScopeBuffer.cppsrc/jsc/bindings/v8/shim/HandleScopeBuffer.hsrc/jsc/bindings/v8/v8_handle_scope_data.hsrc/jsc/bindings/webcore/JSFetchHeaders.cppsrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/napi/napi_body.rssrc/symbols.defsrc/symbols.dynsrc/symbols.txttest/cli/install/migration/complex-workspace.test.tstest/harness.tstest/integration/next-pages/test/dev-server-puppeteer.tstest/integration/next-pages/test/dev-server-ssr-100.test.tstest/integration/next-pages/test/dev-server.test.tstest/integration/next-pages/test/next-build.test.tstest/js/bun/crypto/cipheriv-decipheriv.test.tstest/js/node/crypto/crypto.test.tstest/js/node/http/node-http-parser.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.test.jstest/js/node/process/dlopen-duplicate-load.test.tstest/js/node/process/dlopen-non-object-exports.test.tstest/js/node/process/process.test.jstest/js/node/stream/node-stream-uint8array.test.tstest/js/node/stream/node-stream.test.jstest/js/node/test/parallel/test-crypto-cipheriv-decipheriv.jstest/js/node/test/parallel/test-http2-getpackedsettings.jstest/js/node/test/parallel/test-stream-compose.jstest/js/node/test/parallel/test-stream-push-strings.jstest/js/node/test/parallel/test-stream-readable-emittedReadable.jstest/js/node/test/parallel/test-stream-readable-infinite-read.jstest/js/node/test/parallel/test-stream-readable-needReadable.jstest/js/node/test/parallel/test-stream-readable-to-web-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination.jstest/js/node/test/parallel/test-stream-typedarray.jstest/js/node/test/parallel/test-stream-uint8array.jstest/js/node/test/parallel/test-stream2-transform.jstest/js/node/test/parallel/test-webstreams-adapters-writable-buffer-sources.jstest/js/node/test/parallel/test-webstreams-compression-bad-chunks.jstest/js/node/test/parallel/test-webstreams-compression-buffer-source.jstest/js/node/test/parallel/test-webstreams-duplex-fromweb-writev-unhandled-rejection.jstest/js/node/test/parallel/test-whatwg-webstreams-compression.jstest/js/node/test/parallel/test-zlib-flush-write-sync-interleaved.jstest/js/third_party/duckdb/duckdb-basic-usage.test.tstest/js/third_party/grpc-js/test-server.test.tstest/js/web/streams/compression.test.tstest/js/web/streams/streams.test.jstest/napi/napi-app/package.jsontest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.tstest/napi/node-napi-tests/harness.tstest/v8/bad-modules/mismatched_abi_version.cpptest/v8/bad-modules/no_entrypoint.cpptest/v8/v8-module/main.cpptest/v8/v8.test.ts
Comment on lines
337
to
352
| for (const { 0: key, 1: value } of headers) { | ||
| if (key === "set-cookie") { | ||
| if ($isArray(value)) { | ||
| cookies ??= []; | ||
| cookies.push(...value); | ||
| } else { | ||
| cookies ??= []; | ||
| cookies.push(value); | ||
| } | ||
| continue; | ||
| } | ||
| this.setHeader(key, value); | ||
| } | ||
| if (cookies.length) { | ||
| if (cookies != null) { | ||
| this.setHeader("set-cookie", cookies); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle set-cookie case-insensitively in setHeaders().
setHeaders() accepts Map, but this branch only catches the exact lowercase key. A Map containing "Set-Cookie" and "set-cookie" entries will still take the overwrite path and lose one of the cookies. Reuse the same case-insensitive check used in setHeader() here.
As per coding guidelines, "Treat 'empty', 'zero', and 'unset' as three distinct states" and "Enumerate the input space deliberately."
🤖 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/js/node/_http_outgoing.ts` around lines 337 - 352, The loop handling
headers treats "set-cookie" case-sensitively and can drop cookies when keys like
"Set-Cookie" are present; update the check in the headers iteration inside
setHeaders (the block that currently checks if (key === "set-cookie")) to
perform a case-insensitive comparison (e.g., compare String(key).toLowerCase()
to "set-cookie") so it matches any case variant, keep the existing cookies
collection logic (cookies ??= []; cookies.push(...)) and the final
this.setHeader("set-cookie", cookies) behavior, and ensure non-string keys are
stringified before lowercasing to match the same rule used by setHeader().
Source: Coding guidelines
Comment on lines
3318
to
3324
| close(callback?: Function) { | ||
| if (this.closed || this.destroyed) return; | ||
| this.#closed = true; | ||
|
|
||
| if (typeof callback === "function") { | ||
| this.on("close", callback); | ||
| this.once("close", callback); | ||
| } |
Contributor
There was a problem hiding this comment.
Register close() callbacks before the closed-state guard.
Once close() sets #closed = true, a later session.close(cb) before the actual 'close' event fires now returns early and drops cb. That makes repeated close calls racey for callers attaching the callback after shutdown has already started.
Suggested fix
close(callback?: Function) {
- if (this.closed || this.destroyed) return;
- this.#closed = true;
-
if (typeof callback === "function") {
this.once("close", callback);
}
+ if (this.closed || this.destroyed) return;
+ this.#closed = true;Also applies to: 3902-3908
🤖 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/js/node/http2.ts` around lines 3318 - 3324, The close() method is
registering callbacks after it sets this.#closed and returns early if already
closed, which can drop callbacks; move the registration of the provided callback
(the this.once("close", callback) call) to before the closed/destroyed guard and
before setting this.#closed so callbacks are reliably queued even if close() has
already been initiated; update both occurrences of close() (the one referencing
this.#closed/this.destroyed and the other at the similar block around 3902-3908)
to follow the same order: if callback is a function register it with
this.once("close", callback) first, then check/handle closed/destroyed and set
this.#closed. Ensure unique symbols mentioned (close, this.#closed,
this.destroyed, this.once("close", callback)) are updated accordingly.
Comment on lines
+4029
to
+4035
| if ($isObject(options) && options.signal) { | ||
| // Node validates the signal before reading .aborted: any object with an | ||
| // 'aborted' property passes (so a duck-typed { aborted: true } takes | ||
| // the abort fast path), while objects without one and non-objects | ||
| // throw ERR_INVALID_ARG_TYPE synchronously. | ||
| validateAbortSignal(options.signal, "options.signal"); | ||
| if (options.signal.aborted) { |
Contributor
There was a problem hiding this comment.
Validate options.signal on presence, not truthiness.
if ($isObject(options) && options.signal) lets signal: null, 0, false, or "" skip validateAbortSignal() entirely and proceed as if no signal was supplied. This branch needs to gate on property presence before reading .aborted.
Suggested fix
- if ($isObject(options) && options.signal) {
+ if ($isObject(options) && ObjectPrototypeHasOwnProperty.$call(options, "signal")) {
// Node validates the signal before reading .aborted: any object with an
// 'aborted' property passes (so a duck-typed { aborted: true } takes
// the abort fast path), while objects without one and non-objects
// throw ERR_INVALID_ARG_TYPE synchronously.
validateAbortSignal(options.signal, "options.signal");As per coding guidelines, Treat "empty", "zero", and "unset" as three distinct states: gate on presence (...) not truthiness.
🤖 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/js/node/http2.ts` around lines 4029 - 4035, The current guard "if
($isObject(options) && options.signal)" treats falsy values (null, 0, false, "")
as absent and skips validation; change it to check presence of the property
instead. Update the condition around validateAbortSignal/options.signal (the
block starting with "if ($isObject(options) && options.signal)" and the
validateAbortSignal call) to use a presence check (e.g., "signal" in options or
Object.prototype.hasOwnProperty.call(options, "signal")) so validateAbortSignal
is invoked even when options.signal is a falsy value, then continue to read
options.signal.aborted as before.
Source: Coding guidelines
Comment on lines
+232
to
+239
| export function canBuildNodeAddons(): boolean { | ||
| if (canBuildNodeAddonsCached === undefined) { | ||
| if (!isMacOS) { | ||
| // Linux and Windows CI toolchains are provisioned by the bootstrap | ||
| // scripts in lockstep with the reported Node version; only macOS test | ||
| // boxes have independently-managed Xcode installs. | ||
| canBuildNodeAddonsCached = true; | ||
| } else { |
Contributor
There was a problem hiding this comment.
canBuildNodeAddons() reports success on Linux/Windows without checking.
The docstring says this answers whether the system toolchain can compile addons, but non-macOS returns true unconditionally. On a machine without c++/MSVC, the gated suites will still hard-fail instead of skip. Either probe those platforms too, or narrow the function contract so callers do not treat it as a real capability check.
🤖 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 `@test/harness.ts` around lines 232 - 239, canBuildNodeAddons() currently
returns true unconditionally on non-macOS which misleads callers; change it to
actually probe the system toolchain: when canBuildNodeAddonsCached is undefined,
detect platform and attempt to locate a C++ compiler (on POSIX check for
clang++/g++/c++ via which/command -v; on Windows check for cl.exe or MSVC
toolchain availability), run a harmless version check or --version spawn to
confirm it runs, and set canBuildNodeAddonsCached to true only on success (false
on failure); update the function (and use of canBuildNodeAddonsCached) to cache
this boolean and ensure callers of canBuildNodeAddons() rely on the real probe
result rather than assuming true.
Comment on lines
+2158
to
+2162
| // No system browser: download into a fresh per-run cache instead of the | ||
| // shared agent-global one — a half-extracted download left there by an | ||
| // earlier failed run otherwise blocks every later install. Pass the same | ||
| // env to whatever later launches puppeteer so it finds the browser. | ||
| return { PUPPETEER_CACHE_DIR: tmpdirSync("puppeteer-cache") }; |
Contributor
There was a problem hiding this comment.
Clean up the per-run Puppeteer cache.
When this path is taken, every caller gets a fresh temp cache directory with no disposal path. Repeated runs will accumulate full browser downloads under the OS temp dir. Return a caller-owned disposable path (or a cleanup hook) here so tests can remove the cache in using/afterAll.
As per coding guidelines, tests must be hermetic and leave no resources behind.
🤖 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 `@test/harness.ts` around lines 2158 - 2162, The current branch creates a
per-run Puppeteer cache dir with tmpdirSync("puppeteer-cache") but never exposes
a way to remove it; change the return value so callers can clean it up — e.g.,
return both the cache path under the PUPPETEER_CACHE_DIR key and a cleanup hook
(or disposable object) that removes the temp directory (use the tmpdirSync
return's removeCallback or equivalent). Update the returned object from the
harness code that uses tmpdirSync to include { PUPPETEER_CACHE_DIR: <path>,
cleanup: <function> } (reference tmpdirSync and the PUPPETEER_CACHE_DIR key so
callers can call cleanup in using/afterAll).
Source: Coding guidelines
Comment on lines
+575
to
+606
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const { Writable } = require("node:stream"); | ||
| const theError = new Error("boom"); | ||
| const ws = new WritableStream({ | ||
| write() { | ||
| return Promise.reject(theError); | ||
| }, | ||
| }); | ||
| const w = Writable.fromWeb(ws); | ||
| process.on("unhandledRejection", () => { | ||
| console.log("UNHANDLED"); | ||
| process.exit(2); | ||
| }); | ||
| w.on("error", e => { | ||
| console.log("error-is-original:" + (e === theError)); | ||
| }); | ||
| w.cork(); | ||
| w.write("a"); | ||
| w.write("b"); | ||
| process.nextTick(() => w.uncork()); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("error-is-original:true"); | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
Drain stderr in these spawned-process tests.
Each case only awaits stdout and exited. If the child writes warnings or failure output to stderr, the test can hang instead of failing cleanly.
Patch pattern
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`...`,
],
env: bunEnv,
+ stderr: "pipe",
});
-const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+const [stdout, stderr, exitCode] = await Promise.all([
+ proc.stdout.text(),
+ proc.stderr.text(),
+ proc.exited,
+]);
+expect(stderr).toBe("");
expect(stdout.trim()).toBe("...");
expect(exitCode).toBe(0);Apply the same shape to the other two subprocess blocks.
As per coding guidelines, "Subprocess tests must drain pipes concurrently using Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) to avoid deadlock."
Also applies to: 725-746, 749-772
🤖 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 `@test/js/node/stream/node-stream.test.js` around lines 575 - 606, The
spawned-process tests using Bun.spawn (variable proc) only await
Promise.all([proc.stdout.text(), proc.exited]) and must also drain stderr to
avoid deadlocks; update each affected Promise.all to include proc.stderr.text()
(i.e. Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])) for
the block that runs the Writable/fromWeb test (the proc created with bunExe()
and the inline "-e" script) and do the same for the two other subprocess blocks
referenced (the ones around the other Bun.spawn calls), ensuring the rest of the
assertions still read stdout and exitCode as before.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/streams/end-of-stream.ts (1)
278-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispose the abort listener from
cleanup().Lines 300-306 create a disposable abort subscription, but Lines 278-291 never release it when callers invoke the returned cleanup early. That leaves the signal holding the stream/callback closure until it aborts, which breaks the cleanup contract and can leak listeners on long-lived signals.
🛠️ Suggested fix
+ let abortDisposable; + cleanup = () => { callback = nop; + abortDisposable?.[SymbolDispose](); + abortDisposable = undefined; stream.removeListener("aborted", onclose); stream.removeListener("complete", onfinish); stream.removeListener("abort", onclose); @@ - const disposable = addAbortListener(options.signal, abort); + abortDisposable = addAbortListener(options.signal, abort); const originalCallback = callback; callback = once((...args) => { - disposable[SymbolDispose](); + abortDisposable?.[SymbolDispose](); + abortDisposable = undefined; originalCallback.$apply(stream, args); }); }🤖 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/js/internal/streams/end-of-stream.ts` around lines 278 - 306, The cleanup function never disposes the abort listener (created via addAbortListener and stored in disposable), causing a listener/capture leak; update cleanup (the cleanup closure in end-of-stream.ts) to call disposable[SymbolDispose]() (guarded for existence) before nulling callback so the abort subscription is released when cleanup runs, and ensure disposable is declared in the outer scope where callback and abort use it so SymbolDispose is available to call.
♻️ Duplicate comments (8)
test/harness.ts (3)
232-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProbe Linux/Windows toolchains before returning
true.The docstring says this reflects whether the system compiler can build addons, but Lines 234-238 return
trueon every non-macOS host without checking anything. On machines missingc++/MSVC, the gated suites will hard-fail later instead of skipping.🤖 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 `@test/harness.ts` around lines 232 - 239, The function canBuildNodeAddons currently returns true for all non-macOS hosts without probing the compiler; update canBuildNodeAddons (and respect canBuildNodeAddonsCached and isMacOS) to actually detect a usable native toolchain before caching true: run a lightweight probe (e.g., invoke "c++ --version" or MSVC equivalent like "cl" on Windows) and treat non-zero exit or missing executable as false, cache the result in canBuildNodeAddonsCached, and ensure the probe is only performed once per process so callers relying on canBuildNodeAddons get an accurate skip decision.
168-185:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep ABI-matching Node provisioning offline.
nodeExeMatchingAbi()fetches Node andSHASUMS256.txtfrom nodejs.org during test execution, so the addon/V8 suites become non-hermetic and fail on offline or restricted machines. Please source the matching archive from a pre-provisioned local artifact/cache instead of the public network. As per coding guidelines, tests must be hermetic and must never contact external network hosts or live registries.🤖 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 `@test/harness.ts` around lines 168 - 185, nodeExeMatchingAbi() currently fetches Node binaries and SHASUMS256.txt from nodejs.org (the url, response, shasumsUrl and write usage) which makes tests non-hermetic; change it to prefer a pre-provisioned local artifact/cache: add logic in nodeExeMatchingAbi() to check a configured local cache path or environment variable (e.g., BUN_TEST_NODE_CACHE) and read the archive and checksum file from there instead of calling fetch(url) and fetch(shasumsUrl); if the local artifact is missing fail fast with a clear message instructing the test harness to provision the artifact (do not fall back to network), and reuse the existing stagingDir, archive, and checksum verification flow once files are read from disk.Source: Coding guidelines
2158-2162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the per-run Puppeteer cache disposable.
Line 2162 creates a fresh temp cache directory, but the API gives callers no way to remove it. Repeated runs will accumulate full browser downloads under the OS temp dir. As per coding guidelines, tests must be hermetic and leave no resources behind.
Source: Coding guidelines
src/js/node/http2.ts (3)
4029-4035:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
options.signalon presence, not truthiness.
if ($isObject(options) && options.signal)skipsvalidateAbortSignal()for explicitsignal: null,0,false, or"", so those invalid values are treated as “no signal” instead of throwing synchronously like Node.Suggested fix
- if ($isObject(options) && options.signal) { + if ($isObject(options) && ObjectPrototypeHasOwnProperty.$call(options, "signal")) { // Node validates the signal before reading .aborted: any object with an // 'aborted' property passes (so a duck-typed { aborted: true } takes // the abort fast path), while objects without one and non-objects // throw ERR_INVALID_ARG_TYPE synchronously. validateAbortSignal(options.signal, "options.signal");As per coding guidelines,
Treat "empty", "zero", and "unset" as three distinct states: gate on presence (...) not truthiness.🤖 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/js/node/http2.ts` around lines 4029 - 4035, The conditional currently gates validateAbortSignal on truthiness (if ($isObject(options) && options.signal)), which skips validation for explicit invalid values like null/0/false/""; change the check to gate on presence instead of truthiness by testing for the existence of the signal property (e.g., $isObject(options) && "signal" in options) before calling validateAbortSignal(options.signal, "options.signal") so that validateAbortSignal runs for explicit but invalid signals and options.signal.aborted is only read after validation.Source: Coding guidelines
3318-3324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQueue
close()callbacks before the closed-state guard.Once graceful shutdown has started, a later
session.close(cb)now returns at Line 3319 / Line 3903 and dropscbeven though'close'has not fired yet. That makes repeated close calls racey for callers attaching the callback after shutdown has begun.Suggested fix
close(callback?: Function) { - if (this.closed || this.destroyed) return; - this.#closed = true; - if (typeof callback === "function") { this.once("close", callback); } + if (this.closed || this.destroyed) return; + this.#closed = true;Also applies to: 3902-3908
🤖 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/js/node/http2.ts` around lines 3318 - 3324, The close(callback?: Function) method currently returns early on the closed/destroyed guard and can drop a passed callback; move the callback queuing ahead of that guard so callers attaching a callback after shutdown still have it registered. Concretely, in the close method (referenced symbols: close(), this.once("close", callback), this.closed, this.destroyed, and the private `#closed` flag) register the callback with this.once("close", callback) before checking/returning for closed/destroyed, then proceed to set `#closed` and continue existing shutdown logic; apply the same change to the second occurrence of this logic around the other close implementation (the 3902–3908 block) so both paths queue callbacks before returning.
2641-2647:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish the array-header rejection across the remaining send paths.
This hardens
respond*(), butHttp2Stream.sendTrailers()at Line 2038 andServerHttp2Stream.additionalHeaders()at Line 2572 still accept arrays because they only gate on$isObject(headers).["x", "y"]still spreads into{ "0": "x", "1": "y" }there and emits invalid trailer/info-header names on the wire.Suggested follow-up
// Http2Stream.sendTrailers() - } else if (!$isObject(headers)) { + } else if (!$isObject(headers) || $isArray(headers)) { throw $ERR_INVALID_ARG_TYPE("headers", "object", headers); } else { headers = { ...headers }; } // ServerHttp2Stream.additionalHeaders() - } else if (!$isObject(headers)) { + } else if (!$isObject(headers) || $isArray(headers)) { throw $ERR_INVALID_ARG_TYPE("headers", "object", headers); } else { headers = { ...headers }; }As per coding guidelines, "Fix the whole class in the same PR - grep for every sibling site sharing the pattern."
🤖 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/js/node/http2.ts` around lines 2641 - 2647, Several send paths still accept array headers because they only check $isObject(headers); update Http2Stream.sendTrailers() and ServerHttp2Stream.additionalHeaders() to reject arrays the same way respond*() does: if headers is undefined set {}, else if not $isObject(headers) or $isArray(headers) throw $ERR_INVALID_ARG_TYPE("headers", "object", headers); locate those methods (sendTrailers and additionalHeaders) and mirror the array-rejection logic and comment from the respond* implementation so arrays no longer spread into numeric keys and emit invalid header frames.Source: Coding guidelines
src/jsc/bindings/v8/shim/FunctionTemplate.cpp (1)
85-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a
static_assertto guard theviewOffset/kNewTargetIndexrelationship.The hard-coded
constexpr size_t viewOffset = 1assumesInfo::kNewTargetIndex == -1. If V8 changes that constant, the frame indexing silently breaks. Add a compile-time assertion:static_assert(viewOffset == static_cast<size_t>(-Info::kNewTargetIndex), "viewOffset must offset kNewTargetIndex to index 0");🤖 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/jsc/bindings/v8/shim/FunctionTemplate.cpp` around lines 85 - 90, Add a compile-time guard to ensure the hard-coded constexpr size_t viewOffset = 1 stays correct if V8's Info::kNewTargetIndex changes: insert a static_assert that compares viewOffset to static_cast<size_t>(-Info::kNewTargetIndex) with a clear message (e.g., "viewOffset must offset kNewTargetIndex to index 0") near the viewOffset declaration so frame indexing (frame, slot, argc) is validated at compile time.src/runtime/api/bun/h2_frame_parser.rs (1)
5147-5158:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
lastStreamId <= 0sentinel is still bypassed by unsigned cast order.On Line 5147,
id as u32 > MAX_STREAM_IDrejects all negative values before the sentinel logic on Lines 5152-5158 can run, so the documented “<= 0means use current last processed id” behavior is not honored for negatives.Suggested fix
- let id = last_stream_arg.to_int32(); - if id as u32 > MAX_STREAM_ID { + let id = last_stream_arg.to_int32(); + if id > 0 && (id as u32) > MAX_STREAM_ID { return Err(global_object.throw(format_args!( "Expected lastStreamId to be a number between 1 and 2147483647" ))); } // Like Node's native goaway, a lastStreamId <= 0 means "use the // actual last processed stream id" — Node's JS layer defaults the // argument to 0 and relies on this correction, and sending a // literal 0 would tell the peer no streams were processed. if id > 0 { last_stream_id = id as u32; }🤖 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/api/bun/h2_frame_parser.rs` around lines 5147 - 5158, The code casts `id` to u32 before handling the sentinel <= 0 case, causing negative values to be rejected by the MAX_STREAM_ID check and bypassing the intended sentinel logic; reorder the checks in the block that sets last_stream_id so you first test the signed `id` for <= 0 and handle the sentinel (leaving last_stream_id unchanged), then validate positive ids by casting to u32 and comparing against MAX_STREAM_ID and calling `global_object.throw` on overflow; reference the local variables `id`, `last_stream_id`, `MAX_STREAM_ID` and the error path using `global_object.throw` when making the change.
🤖 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 `@flake.nix`:
- Around line 34-35: Update the stale inline comment that says "version pinned
to 24" to reflect Node.js 26: find the comment near the Node pin and the symbol
nodejs = pkgs.nodejs_26 and change the text to mention Node 26 (e.g., "Node.js
pinned to 26" or similar) so the package-list comment matches the actual pin.
In `@scripts/bootstrap.sh`:
- Around line 1787-1789: The current use of execute_sudo around only apt-get
causes the helper to abort on a non-zero exit so the dpkg fallback and || true
never run; instead run both install attempts inside a single sudo'd shell so the
internal OR chain can succeed and return 0 to execute_sudo. Replace the two-line
sequence that uses chrome_deb and execute_sudo apt-get install -y ... followed
by || execute_sudo dpkg -i ... || true with one execute_sudo invocation that
runs a shell command like: execute_sudo sh -c 'apt-get install -y "$chrome_deb"
|| dpkg -i "$chrome_deb" || true', ensuring chrome_deb, download_file and
execute_sudo are referenced as in the diff.
In `@src/js/internal/streams/pipeline.ts`:
- Around line 209-212: In finishImpl, the conditional allows later teardown
errors to overwrite an earlier AbortError because it checks error.name ===
"AbortError" instead of inspecting the incoming err; change the logic so you
only assign error = err when there is no existing error OR the existing error is
ERR_STREAM_PREMATURE_CLOSE OR the incoming err is itself an AbortError — and
ensure you never overwrite an already-recorded AbortError. Update the if
condition in finishImpl to reference err.name (and/or explicitly guard against
existing error.name === "AbortError") so that an original AbortError in the
stored variable is preserved.
In `@src/js/internal/webstreams_adapters.ts`:
- Around line 869-873: The validateBufferSourceChunk ([kValidateChunk] /
function validateBufferSourceChunk) currently only rejects
SharedArrayBuffer-backed inputs but doesn't enforce that chunk is a
BufferSource; update it to first check that chunk is either an ArrayBuffer
(isArrayBuffer(chunk)) or an ArrayBufferView (isArrayBufferView(chunk)), and if
not, throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer",
"TypedArray", "DataView"], chunk); then perform the existing SharedArrayBuffer
check (use isSharedArrayBuffer on chunk or chunk.buffer for views) and throw the
same ERR_INVALID_ARG_TYPE on that case. Ensure you reference the same error
helper ($ERR_INVALID_ARG_TYPE) and keep both rejection reasons consistent.
In `@src/js/node/http2.ts`:
- Around line 3944-3949: The throws for pre-stream failures (checks of
this.destroyed and this.closed in request()) occur before a stream ID/connection
is allocated but the shared catch always does this.#connections-- which can
drive the counter negative and prematurely tear down the session; change the
logic so the connection counter is decremented only if a stream/connection was
actually allocated. Concretely, in request() introduce a local flag (e.g.,
streamAllocated) or set a temporary marker on the instance when the stream
ID/connection allocation executes, set it to true after allocation, and in the
shared catch decrement this.#connections only if that flag/marker is true (or
clear the marker after decrement). Apply the same guard where similar
pre-allocation throws exist (the other request() paths around the checks
currently at the start and the catch paths referenced) and ensure
emitErrorNT(...) still uses destroy=true only when the counter truly reaches
zero.
In `@src/runtime/napi/napi_body.rs`:
- Around line 3106-3109: The Windows/MSVC symbol aliases are missing for
HandleScope::Initialize and the Value::QuickIs* variants; add corresponding
#[cfg(windows)] pub(super) extern declarations in the v8_api module using the
MSVC link_name mangled names (e.g.
?Initialize@HandleScope@v8@@IEAAXPEAVIsolate@v8@@@Z and the
?QuickIsUndefined@Value@v8@@AEBA_NXZ / ?QuickIsNull@Value@v8@@AEBA_NXZ /
?QuickIsNullOrUndefined@Value@v8@@AEBA_NXZ / ?QuickIsString@Value@v8@@AEBA_NXZ
variants) and then include those exact symbol strings in the Windows
keep_symbols! list so the C++ shims can link correctly for MSVC debug builds;
locate the existing non-windows declarations (functions around
v8_Number_NewFromInt32/v8_Number_NewFromUint32) and mirror them for
#[cfg(windows)] and update keep_symbols! accordingly.
In `@test/cli/install/migration/complex-workspace.test.ts`:
- Around line 55-60: The current installArgs assignment applies --ignore-scripts
for any win32 run; restrict this to CI only by making the condition include
detection of a CI environment (e.g. process.env.CI or
process.env.GITHUB_ACTIONS) so that installArgs uses [bunExe(), "install",
"--ignore-scripts"] only when process.platform === "win32" && (process.env.CI ||
process.env.GITHUB_ACTIONS) is truthy, otherwise use [bunExe(), "install"];
update the expression that sets installArgs (the variable named installArgs and
the platform check using process.platform and bunExe()) accordingly.
In `@test/harness.ts`:
- Around line 2151-2156: The skipBrowserDownload condition is too broad and
skips Puppeteer downloads for all Windows CI runs; update the
skipBrowserDownload boolean expression (the const skipBrowserDownload) so the
win32 branch only triggers for windows-arm64 CI lanes by adding process.arch ===
"arm64" to that clause (i.e., change (process.platform === "win32" &&
(!!process.env.CI || !!process.env.BUILDKITE)) to (process.platform === "win32"
&& process.arch === "arm64" && (!!process.env.CI || !!process.env.BUILDKITE))).
Ensure the subsequent return { PUPPETEER_SKIP_DOWNLOAD: "1" } remains guarded by
the revised skipBrowserDownload.
In `@test/js/node/http/node-http-parser.test.ts`:
- Around line 269-272: Add a negative maxHeaderPairs test to verify the "<= 0
means unlimited" branch: after the existing case that sets parser.maxHeaderPairs
= 0 and calls onHeaders(...), add the same steps but set parser.maxHeaderPairs =
-1, call onHeaders.call(parser, ["c", "4"], ""), and assert parser._headers
equals ["x", "1", "a", "2", "c", "4"]. This ensures the maxHeaderPairs <= 0
branch (checked in the parser logic referenced by parser.maxHeaderPairs and
onHeaders) is fully covered.
In `@test/js/node/http/node-http.test.ts`:
- Around line 2284-2298: The test uses Bun-only OutgoingMessage.headers
access/mutation (msg3.headers and (msg4 as any).headers = ...) which is
non-portable; replace those with Node-supported APIs: for the first assertion
drop expect(msg3.headers)... and assert via msg3.getHeaders()["set-cookie"] (or
msg3.getHeader("set-cookie")) instead; for the mutation that replaces the whole
header bag, emulate it with removeHeader("set-cookie") and
setHeader("x-test","1") (or clear all existing headers then set the new ones) on
the OutgoingMessage instance; keep references to OutgoingMessage, msg3/msg4,
getHeader/getHeaders/getRawHeaderNames/setHeader/removeHeader to locate the
code.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 2809-2818: The test attaches 'data'/'end' listeners after awaiting
the 'response' promise, which can race and miss 'end' if the stream finishes
quickly; fix by registering the 'data' and 'end' listeners (and an 'error'
listener that rejects) on the request stream (the variable req returned by
client.request) before awaiting the promise that resolves on the 'response'
event, and ensure the promise wiring rejects on 'error' so test failures surface
instead of hanging; update the code paths around client.request / req, the
response promise, and the body accumulation to install listeners first and then
await the response.
In `@test/v8/bad-modules/no_entrypoint.cpp`:
- Line 5: Replace the hardcoded ABI integer 147 used for nm_version in
test/v8/bad-modules/no_entrypoint.cpp with the NODE_MODULE_VERSION macro; find
the array/initializer where "147, // nm_version (Node.js 26.3.0)" is set (look
for nm_version or the ABI/version entry) and change that entry to
NODE_MODULE_VERSION, keeping any descriptive comment if desired so the fixture
remains correct across Node/V8 version bumps.
---
Outside diff comments:
In `@src/js/internal/streams/end-of-stream.ts`:
- Around line 278-306: The cleanup function never disposes the abort listener
(created via addAbortListener and stored in disposable), causing a
listener/capture leak; update cleanup (the cleanup closure in end-of-stream.ts)
to call disposable[SymbolDispose]() (guarded for existence) before nulling
callback so the abort subscription is released when cleanup runs, and ensure
disposable is declared in the outer scope where callback and abort use it so
SymbolDispose is available to call.
---
Duplicate comments:
In `@src/js/node/http2.ts`:
- Around line 4029-4035: The conditional currently gates validateAbortSignal on
truthiness (if ($isObject(options) && options.signal)), which skips validation
for explicit invalid values like null/0/false/""; change the check to gate on
presence instead of truthiness by testing for the existence of the signal
property (e.g., $isObject(options) && "signal" in options) before calling
validateAbortSignal(options.signal, "options.signal") so that
validateAbortSignal runs for explicit but invalid signals and
options.signal.aborted is only read after validation.
- Around line 3318-3324: The close(callback?: Function) method currently returns
early on the closed/destroyed guard and can drop a passed callback; move the
callback queuing ahead of that guard so callers attaching a callback after
shutdown still have it registered. Concretely, in the close method (referenced
symbols: close(), this.once("close", callback), this.closed, this.destroyed, and
the private `#closed` flag) register the callback with this.once("close",
callback) before checking/returning for closed/destroyed, then proceed to set
`#closed` and continue existing shutdown logic; apply the same change to the
second occurrence of this logic around the other close implementation (the
3902–3908 block) so both paths queue callbacks before returning.
- Around line 2641-2647: Several send paths still accept array headers because
they only check $isObject(headers); update Http2Stream.sendTrailers() and
ServerHttp2Stream.additionalHeaders() to reject arrays the same way respond*()
does: if headers is undefined set {}, else if not $isObject(headers) or
$isArray(headers) throw $ERR_INVALID_ARG_TYPE("headers", "object", headers);
locate those methods (sendTrailers and additionalHeaders) and mirror the
array-rejection logic and comment from the respond* implementation so arrays no
longer spread into numeric keys and emit invalid header frames.
In `@src/jsc/bindings/v8/shim/FunctionTemplate.cpp`:
- Around line 85-90: Add a compile-time guard to ensure the hard-coded constexpr
size_t viewOffset = 1 stays correct if V8's Info::kNewTargetIndex changes:
insert a static_assert that compares viewOffset to
static_cast<size_t>(-Info::kNewTargetIndex) with a clear message (e.g.,
"viewOffset must offset kNewTargetIndex to index 0") near the viewOffset
declaration so frame indexing (frame, slot, argc) is validated at compile time.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 5147-5158: The code casts `id` to u32 before handling the sentinel
<= 0 case, causing negative values to be rejected by the MAX_STREAM_ID check and
bypassing the intended sentinel logic; reorder the checks in the block that sets
last_stream_id so you first test the signed `id` for <= 0 and handle the
sentinel (leaving last_stream_id unchanged), then validate positive ids by
casting to u32 and comparing against MAX_STREAM_ID and calling
`global_object.throw` on overflow; reference the local variables `id`,
`last_stream_id`, `MAX_STREAM_ID` and the error path using `global_object.throw`
when making the change.
In `@test/harness.ts`:
- Around line 232-239: The function canBuildNodeAddons currently returns true
for all non-macOS hosts without probing the compiler; update canBuildNodeAddons
(and respect canBuildNodeAddonsCached and isMacOS) to actually detect a usable
native toolchain before caching true: run a lightweight probe (e.g., invoke "c++
--version" or MSVC equivalent like "cl" on Windows) and treat non-zero exit or
missing executable as false, cache the result in canBuildNodeAddonsCached, and
ensure the probe is only performed once per process so callers relying on
canBuildNodeAddons get an accurate skip decision.
- Around line 168-185: nodeExeMatchingAbi() currently fetches Node binaries and
SHASUMS256.txt from nodejs.org (the url, response, shasumsUrl and write usage)
which makes tests non-hermetic; change it to prefer a pre-provisioned local
artifact/cache: add logic in nodeExeMatchingAbi() to check a configured local
cache path or environment variable (e.g., BUN_TEST_NODE_CACHE) and read the
archive and checksum file from there instead of calling fetch(url) and
fetch(shasumsUrl); if the local artifact is missing fail fast with a clear
message instructing the test harness to provision the artifact (do not fall back
to network), and reuse the existing stagingDir, archive, and checksum
verification flow once files are read from disk.
🪄 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: f0804fa1-d3a5-458f-a5b5-a72c13c80210
⛔ Files ignored due to path filters (1)
test/napi/napi-app/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (109)
flake.nixscripts/bootstrap.ps1scripts/bootstrap.shscripts/build/codegen.tsscripts/build/config.tsscripts/build/deps/nodejs-headers.tsscripts/build/flags.tsscripts/packer/windows-x64.pkr.hclsrc/js/builtins/CompressionStream.tssrc/js/builtins/DecompressionStream.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal/http.tssrc/js/internal/primordials.jssrc/js/internal/streams/duplex.tssrc/js/internal/streams/duplexify.tssrc/js/internal/streams/end-of-stream.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/writable.tssrc/js/internal/webstreams_adapters.tssrc/js/node/_http_common.tssrc/js/node/_http_outgoing.tssrc/js/node/_http_server.tssrc/js/node/http2.tssrc/js/node/https.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcessReportObjectWindows.cppsrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeHTTP.cppsrc/jsc/bindings/napi.cppsrc/jsc/bindings/v8/V8Array.cppsrc/jsc/bindings/v8/V8EscapableHandleScopeBase.cppsrc/jsc/bindings/v8/V8EscapableHandleScopeBase.hsrc/jsc/bindings/v8/V8External.cppsrc/jsc/bindings/v8/V8External.hsrc/jsc/bindings/v8/V8FunctionCallbackInfo.cppsrc/jsc/bindings/v8/V8FunctionCallbackInfo.hsrc/jsc/bindings/v8/V8HandleScope.cppsrc/jsc/bindings/v8/V8HandleScope.hsrc/jsc/bindings/v8/V8Isolate.cppsrc/jsc/bindings/v8/V8Isolate.hsrc/jsc/bindings/v8/V8Number.cppsrc/jsc/bindings/v8/V8Number.hsrc/jsc/bindings/v8/V8String.cppsrc/jsc/bindings/v8/V8String.hsrc/jsc/bindings/v8/V8Value.cppsrc/jsc/bindings/v8/V8Value.hsrc/jsc/bindings/v8/shim/FunctionTemplate.cppsrc/jsc/bindings/v8/shim/GlobalInternals.hsrc/jsc/bindings/v8/shim/Handle.hsrc/jsc/bindings/v8/shim/HandleScopeBuffer.cppsrc/jsc/bindings/v8/shim/HandleScopeBuffer.hsrc/jsc/bindings/v8/v8_handle_scope_data.hsrc/jsc/bindings/webcore/JSFetchHeaders.cppsrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/napi/napi_body.rssrc/symbols.defsrc/symbols.dynsrc/symbols.txttest/cli/install/migration/complex-workspace.test.tstest/harness.tstest/integration/next-pages/test/dev-server-puppeteer.tstest/integration/next-pages/test/dev-server-ssr-100.test.tstest/integration/next-pages/test/dev-server.test.tstest/integration/next-pages/test/next-build.test.tstest/js/bun/crypto/cipheriv-decipheriv.test.tstest/js/node/crypto/crypto.test.tstest/js/node/http/node-http-parser.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.test.jstest/js/node/process/dlopen-duplicate-load.test.tstest/js/node/process/dlopen-non-object-exports.test.tstest/js/node/process/process.test.jstest/js/node/stream/node-stream-uint8array.test.tstest/js/node/stream/node-stream.test.jstest/js/node/test/parallel/test-crypto-cipheriv-decipheriv.jstest/js/node/test/parallel/test-http2-getpackedsettings.jstest/js/node/test/parallel/test-stream-compose.jstest/js/node/test/parallel/test-stream-push-strings.jstest/js/node/test/parallel/test-stream-readable-emittedReadable.jstest/js/node/test/parallel/test-stream-readable-infinite-read.jstest/js/node/test/parallel/test-stream-readable-needReadable.jstest/js/node/test/parallel/test-stream-readable-to-web-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination.jstest/js/node/test/parallel/test-stream-typedarray.jstest/js/node/test/parallel/test-stream-uint8array.jstest/js/node/test/parallel/test-stream2-transform.jstest/js/node/test/parallel/test-webstreams-adapters-writable-buffer-sources.jstest/js/node/test/parallel/test-webstreams-compression-bad-chunks.jstest/js/node/test/parallel/test-webstreams-compression-buffer-source.jstest/js/node/test/parallel/test-webstreams-duplex-fromweb-writev-unhandled-rejection.jstest/js/node/test/parallel/test-whatwg-webstreams-compression.jstest/js/node/test/parallel/test-zlib-flush-write-sync-interleaved.jstest/js/third_party/duckdb/duckdb-basic-usage.test.tstest/js/third_party/grpc-js/test-server.test.tstest/js/web/streams/compression.test.tstest/js/web/streams/streams.test.jstest/napi/napi-app/package.jsontest/napi/napi-app/standalone_tests.cpptest/napi/napi.test.tstest/napi/node-napi-tests/harness.tstest/v8/bad-modules/mismatched_abi_version.cpptest/v8/bad-modules/no_entrypoint.cpptest/v8/v8-module/main.cpptest/v8/v8.test.ts
Comment on lines
209
to
212
| function finishImpl(err, final?) { | ||
| if (err && (!error || error.code === "ERR_STREAM_PREMATURE_CLOSE")) { | ||
| if (err && (!error || error.code === "ERR_STREAM_PREMATURE_CLOSE" || error.name === "AbortError")) { | ||
| error = err; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't let teardown errors overwrite the original AbortError.
Line 210 checks error.name === "AbortError" on the stored error, so once abort is recorded, any later teardown error—including ERR_STREAM_PREMATURE_CLOSE from destruction—replaces it. That flips the precedence in abort flows and can surface the wrong final error to the pipeline callback.
🛠️ Suggested fix
- if (err && (!error || error.code === "ERR_STREAM_PREMATURE_CLOSE" || error.name === "AbortError")) {
+ if (
+ err &&
+ (!error ||
+ error.code === "ERR_STREAM_PREMATURE_CLOSE" ||
+ (error.name === "AbortError" &&
+ err.name !== "AbortError" &&
+ err.code !== "ERR_STREAM_PREMATURE_CLOSE"))
+ ) {
error = err;
}🤖 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/js/internal/streams/pipeline.ts` around lines 209 - 212, In finishImpl,
the conditional allows later teardown errors to overwrite an earlier AbortError
because it checks error.name === "AbortError" instead of inspecting the incoming
err; change the logic so you only assign error = err when there is no existing
error OR the existing error is ERR_STREAM_PREMATURE_CLOSE OR the incoming err is
itself an AbortError — and ensure you never overwrite an already-recorded
AbortError. Update the if condition in finishImpl to reference err.name (and/or
explicitly guard against existing error.name === "AbortError") so that an
original AbortError in the stored variable is preserved.
Comment on lines
+869
to
+873
| [kValidateChunk]: function validateBufferSourceChunk(chunk) { | ||
| if (isSharedArrayBuffer(isArrayBufferView(chunk) ? chunk.buffer : chunk)) { | ||
| throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk); | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
validateBufferSourceChunk misses non-BufferSource rejection.
This validator only blocks SharedArrayBuffer-backed chunks, so non-BufferSource values can pass through even though this path is documented as BufferSource-only.
Proposed fix
function newBufferSourceTransformPairFromDuplex(duplex) {
const { isArrayBufferView, isSharedArrayBuffer } = require("node:util/types");
return newReadableWritablePairFromDuplex(duplex, {
[kValidateChunk]: function validateBufferSourceChunk(chunk) {
- if (isSharedArrayBuffer(isArrayBufferView(chunk) ? chunk.buffer : chunk)) {
+ const isView = isArrayBufferView(chunk);
+ const isBuffer = isAnyArrayBuffer(chunk);
+ if (!isView && !isBuffer) {
+ throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk);
+ }
+ if (isSharedArrayBuffer(isView ? chunk.buffer : chunk)) {
throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk);
}
},
[kDestroyOnSyncError]: true,
});
}🤖 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/js/internal/webstreams_adapters.ts` around lines 869 - 873, The
validateBufferSourceChunk ([kValidateChunk] / function
validateBufferSourceChunk) currently only rejects SharedArrayBuffer-backed
inputs but doesn't enforce that chunk is a BufferSource; update it to first
check that chunk is either an ArrayBuffer (isArrayBuffer(chunk)) or an
ArrayBufferView (isArrayBufferView(chunk)), and if not, throw
$ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray",
"DataView"], chunk); then perform the existing SharedArrayBuffer check (use
isSharedArrayBuffer on chunk or chunk.buffer for views) and throw the same
ERR_INVALID_ARG_TYPE on that case. Ensure you reference the same error helper
($ERR_INVALID_ARG_TYPE) and keep both rejection reasons consistent.
Comment on lines
+2151
to
+2156
| const skipBrowserDownload = | ||
| hasSystemChromium || | ||
| (process.platform === "linux" && process.arch === "arm64") || | ||
| (process.platform === "win32" && (!!process.env.CI || !!process.env.BUILDKITE)); | ||
| if (skipBrowserDownload) { | ||
| return { PUPPETEER_SKIP_DOWNLOAD: "1" }; |
Contributor
There was a problem hiding this comment.
Don't skip Puppeteer downloads on every Windows CI lane.
The comment narrows the unsupported case to windows-arm64, but Lines 2153-2154 set PUPPETEER_SKIP_DOWNLOAD=1 for all win32 CI runs. If a Windows x64 lane has no preinstalled Chromium, install will now skip the only browser source and the Puppeteer tests will fail later.
🤖 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 `@test/harness.ts` around lines 2151 - 2156, The skipBrowserDownload condition
is too broad and skips Puppeteer downloads for all Windows CI runs; update the
skipBrowserDownload boolean expression (the const skipBrowserDownload) so the
win32 branch only triggers for windows-arm64 CI lanes by adding process.arch ===
"arm64" to that clause (i.e., change (process.platform === "win32" &&
(!!process.env.CI || !!process.env.BUILDKITE)) to (process.platform === "win32"
&& process.arch === "arm64" && (!!process.env.CI || !!process.env.BUILDKITE))).
Ensure the subsequent return { PUPPETEER_SKIP_DOWNLOAD: "1" } remains guarded by
the revised skipBrowserDownload.
Comment on lines
+2284
to
+2298
| // The Bun-specific headers accessor agrees with getHeaders(). | ||
| expect(msg3.headers).toEqual({ "set-cookie": [] }); | ||
|
|
||
| // Appending a cookie supersedes the present-but-empty marker (no duplicate | ||
| // name in getRawHeaderNames, value visible everywhere). | ||
| msg3.appendHeader("Set-Cookie", "a=1"); | ||
| expect(msg3.getHeader("set-cookie")).toEqual(["a=1"]); | ||
| expect(msg3.getRawHeaderNames().filter(n => n.toLowerCase() === "set-cookie")).toHaveLength(1); | ||
| expect(msg3.getHeaders()["set-cookie"]).toEqual(["a=1"]); | ||
|
|
||
| // Replacing the whole header bag drops the marker. | ||
| const msg4 = new OutgoingMessage(); | ||
| msg4.setHeader("set-cookie", []); | ||
| (msg4 as any).headers = { "x-test": "1" }; | ||
| expect(msg4.getHeader("set-cookie")).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Node compatibility for the Bun-specific surface used here.
node -e '
const { OutgoingMessage } = require("node:http");
const msg = new OutgoingMessage();
console.log("headers" in msg, Object.getOwnPropertyDescriptor(Object.getPrototypeOf(msg), "headers"));
'Repository: oven-sh/bun
Length of output: 71
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/js/node/http/node-http.test.ts"
echo "== Context around flagged lines =="
sed -n '2260,2325p' "$file" | cat -n
echo
echo "== Search for OutgoingMessage.headers usage in this file =="
rg -n "OutgoingMessage\)|\.headers\b|setHeader\(|getHeaders\(|appendHeader\(" "$file" | head -n 80Repository: oven-sh/bun
Length of output: 5799
🏁 Script executed:
node -e '
const { OutgoingMessage } = require("node:http");
const msg = new OutgoingMessage();
console.log("headers" in msg, Object.getOwnPropertyDescriptor(Object.getPrototypeOf(msg), "headers"));
'
# => false undefinedRepository: oven-sh/bun
Length of output: 71
Remove Bun-only OutgoingMessage.headers accessor/mutation from node-http compat test
Node’s node:http OutgoingMessage does not have a headers property ('headers' in msg is false), so expect(msg3.headers)... and (msg4 as any).headers = ... are non-portable. Replace them with Node-supported APIs (getHeaders() / setHeaders() / setHeader() / removeHeader()) to assert the intended header-bag behavior.
🤖 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 `@test/js/node/http/node-http.test.ts` around lines 2284 - 2298, The test uses
Bun-only OutgoingMessage.headers access/mutation (msg3.headers and (msg4 as
any).headers = ...) which is non-portable; replace those with Node-supported
APIs: for the first assertion drop expect(msg3.headers)... and assert via
msg3.getHeaders()["set-cookie"] (or msg3.getHeader("set-cookie")) instead; for
the mutation that replaces the whole header bag, emulate it with
removeHeader("set-cookie") and setHeader("x-test","1") (or clear all existing
headers then set the new ones) on the OutgoingMessage instance; keep references
to OutgoingMessage, msg3/msg4,
getHeader/getHeaders/getRawHeaderNames/setHeader/removeHeader to locate the
code.
Comment on lines
+2809
to
+2818
| const req = client.request({ ":path": "/" }); | ||
| const response = await new Promise((resolve, reject) => { | ||
| req.on("error", reject); | ||
| req.on("response", resolve); | ||
| req.end(); | ||
| }); | ||
| let body = ""; | ||
| req.on("data", chunk => (body += chunk)); | ||
| await new Promise(resolve => req.on("end", resolve)); | ||
|
|
Contributor
There was a problem hiding this comment.
Attach body listeners before awaiting response to avoid a race/hang.
response resolves before data/end listeners are installed. If the stream ends quickly, end can be missed and this test can hang intermittently.
💡 Suggested fix
- const req = client.request({ ":path": "/" });
- const response = await new Promise((resolve, reject) => {
- req.on("error", reject);
- req.on("response", resolve);
- req.end();
- });
- let body = "";
- req.on("data", chunk => (body += chunk));
- await new Promise(resolve => req.on("end", resolve));
+ const req = client.request({ ":path": "/" });
+ let body = "";
+ const responsePromise = new Promise((resolve, reject) => {
+ req.on("error", reject);
+ req.on("response", resolve);
+ });
+ const bodyDonePromise = new Promise((resolve, reject) => {
+ req.on("error", reject);
+ req.on("data", chunk => (body += chunk));
+ req.on("end", resolve);
+ });
+ req.end();
+ const response = await responsePromise;
+ await bodyDonePromise;As per coding guidelines, tests should avoid flaky timing/event-order behavior and should await the actual condition with failure paths wired to rejection.
🤖 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 `@test/js/node/http2/node-http2.test.js` around lines 2809 - 2818, The test
attaches 'data'/'end' listeners after awaiting the 'response' promise, which can
race and miss 'end' if the stream finishes quickly; fix by registering the
'data' and 'end' listeners (and an 'error' listener that rejects) on the request
stream (the variable req returned by client.request) before awaiting the promise
that resolves on the 'response' event, and ensure the promise wiring rejects on
'error' so test failures surface instead of hanging; update the code paths
around client.request / req, the response promise, and the body accumulation to
install listeners first and then await the response.
Source: Coding guidelines
Member
Author
|
Addressed the CodeRabbit review in ce7aa4b. Disposition of each finding: Fixed
Matches Node 26 verbatim (intentionally unchanged)
Not applicable / intentional
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/bootstrap.sh (1)
1787-1790:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
chrome_debas a positional argument tosh -c.
chrome_debcomes fromdownload_file()→create_tmp_directory()→mktemp, so it can inherit aTMPDIRpath with apostrophes or other shell-significant bytes. Interpolating it into thesh -cstring breaks quoting and can execute unintended commands undersudo. Pass it via$1instead.Safer invocation
- execute_sudo sh -c "apt-get install -y '$chrome_deb' || dpkg -i '$chrome_deb' || true" + execute_sudo sh -c 'apt-get install -y "$1" || dpkg -i "$1" || true' sh "$chrome_deb"As per coding guidelines, validate untrusted input before side effects.
🤖 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 `@scripts/bootstrap.sh` around lines 1787 - 1790, The current invocation interpolates chrome_deb into the sh -c string (chrome_deb comes from download_file→create_tmp_directory→mktemp), which can break quoting and allow shell injection; modify the execute_sudo call to pass chrome_deb as a positional parameter to the shell (use "$1" inside the sh -c command and supply chrome_deb as an extra argument), and add a lightweight validation of chrome_deb (e.g., ensure it is a regular file path without newlines or embedded single/double quotes) before calling execute_sudo; reference symbols: chrome_deb, execute_sudo, download_file, create_tmp_directory, mktemp, and the sh -c invocation.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@scripts/bootstrap.sh`:
- Around line 1787-1790: The current invocation interpolates chrome_deb into the
sh -c string (chrome_deb comes from download_file→create_tmp_directory→mktemp),
which can break quoting and allow shell injection; modify the execute_sudo call
to pass chrome_deb as a positional parameter to the shell (use "$1" inside the
sh -c command and supply chrome_deb as an extra argument), and add a lightweight
validation of chrome_deb (e.g., ensure it is a regular file path without
newlines or embedded single/double quotes) before calling execute_sudo;
reference symbols: chrome_deb, execute_sudo, download_file,
create_tmp_directory, mktemp, and the sh -c invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c84f3d7e-ce0a-427f-9e56-4c38750829d1
📒 Files selected for processing (9)
flake.nixscripts/bootstrap.shsrc/js/node/http2.tssrc/jsc/bindings/v8/shim/FunctionTemplate.cppsrc/runtime/api/bun/h2_frame_parser.rstest/integration/next-pages/test/dev-server-puppeteer.tstest/js/node/http/node-http-parser.test.tstest/js/node/stream/node-stream.test.jstest/v8/bad-modules/no_entrypoint.cpp
cirospaciari
force-pushed
the
claude/upgrade-nodejs-26-v2
branch
from
June 9, 2026 19:21
b99a5c6 to
67640d1
Compare
cirospaciari
force-pushed
the
claude/upgrade-nodejs-26-v2
branch
from
June 10, 2026 21:37
8e939c6 to
a54090a
Compare
Bump the Node.js compatibility target from 24.3.0 to 26.3.0 (V8 14.6.202.34, NODE_MODULE_VERSION 147) and sync node:stream and node:http with the behavioral changes upstream made between v24.x and v26.x. - Headers, bootstrap pins, flake, process.versions updated; CI images rebuilt for the Node 26 toolchain - V8 shim updated for 14.6 (Isolate roots layout, flattened FunctionCallbackInfo exit frame, String Write*V2/Utf8LengthV2, External::New pointer-tag overload, HandleScope::Extend) with Itanium + MSVC symbol exports - napi_get_value_string_* no longer panics when querying length with a null buffer; v8/napi fixtures migrated to the new header APIs - stream/http v26 sync: Writable.toWeb sync-drain hang, read() one-chunk semantics, Duplex.from destroy-during-idle hang, BYOB Readable.toWeb, writeHeader removal (DEP0063 EOL), upgrade-listener fallthrough, set-cookie header edge cases, http2 respond() raw-array rejection and session error codes - Vendored the matching upstream tests and verified changed expectations against real Node 26.3.0
cirospaciari
force-pushed
the
claude/upgrade-nodejs-26-v2
branch
from
June 10, 2026 22:45
a54090a to
3917481
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cirospaciari
force-pushed
the
claude/upgrade-nodejs-26-v2
branch
from
June 15, 2026 19:35
224f103 to
d1f2bc0
Compare
… [build images]
Node 26 throws ERR_INVALID_ARG_TYPE for an array passed to either method
(verified on v26.3.0); without the guard an array spreads to {'0':..} and
emits invalid trailer/info-header frames. Completes the $isArray guard added
to respond()/respondWithFD()/respondWithFile().
… [build images] When session.goaway() is called without a lastStreamID (or with the JS-default 0), the auto-filled value was the highest stream id seen in either direction — for a client that is its own (odd) request id. RFC 9113 §6.8 says GOAWAY's last-stream-id refers to streams the RECEIVER initiated; nghttp2 servers reject a wrong-parity id with NGHTTP2_ERR_PROTO (-505) and tear the connection down. Track the highest peer-initiated id separately (odd for a server, even for a client) and use it for the auto value — node's last_proc_stream_id semantics. Verified: with the fix, the spawned-node fixture in node-http2.test.js no longer hits the -505 (10/10 clean; was 5/10).
cirospaciari
enabled auto-merge (squash)
June 16, 2026 19:45
cirospaciari
disabled auto-merge
June 16, 2026 19:45
cirospaciari
enabled auto-merge (squash)
June 16, 2026 19:46
cirospaciari
disabled auto-merge
June 16, 2026 20:37
This was referenced Jun 16, 2026
robobun
added a commit
that referenced
this pull request
Jun 16, 2026
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This was referenced Jun 18, 2026
alii
added a commit
that referenced
this pull request
Jun 22, 2026
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
alii
added a commit
that referenced
this pull request
Jun 23, 2026
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
alii
added a commit
that referenced
this pull request
Jun 23, 2026
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
alii
added a commit
that referenced
this pull request
Jun 23, 2026
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This was referenced Jul 5, 2026
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 11, 2026
## What | | | |---|---| | Removes | `test/cli/install/bunx.test.ts` from `test/expectations.txt` | | Changes | `@angular/cli@latest` → `@angular/cli@20` in the node-24 test | | Restores | 28 tests that have not run in CI since 2026-06-09 | ## Why | | | |---|---| | Skipped by | #32042 — "`@angular/cli@latest` needs a newer Node.js than Bun reports (24.3.0), unskip after the Node.js version bump" | | Bump landed | #31991, 2026-06-16 (`NODEJS_VERSION = "26.3.0"`) — one week later | | Entry removed | never | An `expectations.txt` entry removes the **whole file** from the run — `getRelevantTests` filters on the platform modifier and never reads the `[ FAIL ]` / `[ SKIP ]` kind. So quarantining one case took all 28 tests with it, and the file stayed dark for ~3.5 weeks after its own precondition was met. The angular test passes on main today. ## Pin `@latest` is what made this a recurring failure. Current engines: | spec | resolves to | `engines.node` | Bun (26.3.0) | |---|---|---|---| | `@angular/cli@latest` | 22.0.6 | `^22.22.3 \|\| ^24.15.0 \|\| >=26.0.0` | passes, breaks on next major | | `@angular/cli@20` | 20.x | `^20.19.0 \|\| ^22.12.0 \|\| >=24.0.0` | passes, open-ended | `@20` keeps expressing the test's stated intent ("requires node 24") regardless of Bun's reported version. ## Verification `bun bd test test/cli/install/bunx.test.ts` → 30 pass, 1 skip, 3 fail. All 3 failures are artifacts of the local machine, not the code: | failure | cause | |---|---| | `npm_config_user_agent` | debug build reports `1.4.0-debug`; UA embeds `1.4.0` | | symlinked bunx postinstall | local security policy SIGKILLs a binary copied to `/tmp` | | `bunx claude` squatter | a `claude` binary on PATH shadows the registry lookup |
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #31818 — same change set, rebased onto current main with conflicts resolved and the remaining CI failures fixed.
What
Bumps the Node.js compatibility target from 24.3.0 to 26.3.0 (V8
14.6.202.34-node.20,NODE_MODULE_VERSION147), and syncsnode:stream/node:httpwith the behavioral changes upstream made between v24.x and v26.x.Native / addon ABI
process.versionsupdatedFunctionCallbackInfoexit frame,StringWrite*V2/Utf8LengthV2(legacy exports kept for older binaries),External::Newpointer-tag overload,Number::NewFromInt32/NewFromUint32,HandleScope::Extend/DeleteExtensions— with Itanium + MSVC symbol exportsnapi_get_value_string_*panic when querying length with a null bufferstream/http sync
Audited every behavioral diff in upstream
lib/internal/streams/*,lib/_http_*, webstreams adapters, and http2 between v24.3.0 and v26.3.0, and ported the ones we were carrying v24 (or divergent) behavior for — highlights:Writable.toWebsync-drain hang fix;fromWebwritev rejection no longer crashes the error pathread()one-chunk semantics in paused mode (semver-major in 26),compose()on the prototypeDuplex.from(async gen)destroy-during-idle hang fix; BYOBReadable.toWeb({type:'bytes'}); DEP0201writeHeaderremoved (DEP0063 EOL); upgrade requests without a listener no longer disappear;set-cookiegetHeader/setHeader edge cases; http2respond()raw-array rejection and session error codesreader.read()never settling afterreader.cancel()(spec step 5)Vendored the matching upstream tests; updated v24-era vendored tests whose expectations legitimately changed and verified each new expectation against real Node 26.3.0.
New in this PR (vs #31818)
The remaining red CI jobs on #31818 all came from test machines whose environment lags the new ABI, plus one GC-timing-dependent fixture. Fixed here:
Merged current main (only conflict: the bootstrap.ps1 version counter). This alone fixes the
windows-x64-baseline-verify-baselinefailure: main now bakes Intel SDE into the Windows image (ci: bake Intel SDE into the Windows image for verify-baseline #31893) instead of downloading it at job time, which the Intel mirror blocks.test/harness.ts:nodeExeMatchingAbi()— returns the systemnodewhen itsprocess.versions.modulesmatches what Bun reports; otherwise downloads the matching official Node build once into a cached temp directory (SHA-256-verified against the release SHASUMS).test/v8/v8.test.tsandtest/napi/napi.test.tsresolvenodethrough it, so the bun-vs-node comparison no longer fails withERR_DLOPEN_FAILEDon machines that still have Node 24 installed (the three darwinv8.test.tsfailures on Upgrade reported Node.js version to 26.3.0 #31818).test/harness.ts:canBuildNodeAddons()— macOS-only probe for a libc++ that ships<source_location>, which Node 26 headers include unconditionally (real Node 26 has the same minimum-Xcode requirement). Addon-compiling suites (v8,napi, the twoprocess.dlopentests) skip on machines whose toolchain cannot compile the headers at all (themacOS-13Intel box failures on Upgrade reported Node.js version to 26.3.0 #31818).napi fixture: pin the
test_deferred_exceptionswrapped object with a strong ref so itsnapi_throw-ing finalizer runs at env teardown instead of from GC. The fixture builds withNAPI_EXPERIMENTAL, and Node 26 aborts (Finalizer is calling a function that may affect GC state) whenever GC happens to collect the object mid-process — that's the windows-11-aarch64napi.test.tsfailure on Upgrade reported Node.js version to 26.3.0 #31818, reproducible on Linux with--expose-gc. Pinning makes the finalizer timing deterministic on both runtimes with unchanged expected output.http2: close the stream when a headers-only END_STREAM response completes (
h2_frame_parser.rs). Therespond()/request()header path set the stream to HALF_CLOSED_LOCAL unconditionally — regressing it backwards when the peer had already half-closed — and never dispatchedonStreamEnd, unlike thesend_data/send_trailerspaths. Ares.end()with no body therefore leaked the stream and the session's connection count; a session marked closed by a received GOAWAY then waited forever for a destroy that couldn't come, andserver.close()hung. Linux masked the leak (the peer's FIN tears the socket down); the macOS fleet doesn't redeliver that FIN once the session has written from inside the read callback, which is why only darwin hung. This was thetest-http2-compat-serverrequest-pipe.jstimeout — diagnosed with instrumented CI builds (request/response completed,streamEnd(7)never dispatched, server connection count stuck at 1), and verified fixed on the same darwin agents that failed 4/4 before.http2: destroy the server session when its socket closes (
http2.ts), mirroring Node'ssocketOnCloseand the client session's own#onClose—close()alone early-returns once a received GOAWAY has marked the session closed.There is also a latent macOS kqueue FIN-redelivery quirk in uSockets (observed with kernel-event tracing: after the h2 session writes from inside the socket's data callback, the pending EOF event for that socket is never redelivered). The stream-state fix makes it unobservable here; filing it as a follow-up issue.
Verification
Local (Linux x64 debug build, system Node 26.3.0):
test/napi/napi.test.ts: 99 pass / 0 fail (including the previously aborting fixture under forced GC, verified directly withnode --expose-gc)test/v8/v8.test.ts: 52 pass / 1 skip / 0 failprocess.dlopensuites: 5 pass / 0 failnode-http2,node-stream,node-stream-uint8array,web/streams/compression: 362 pass / 1 fail — the failure is thehttp2 maxSessionMemorytimeout that Upgrade reported Node.js version to 26.3.0 #31818 documents as debug-build-only and reproduced at its basetest-http2-compat-serverrequest-pipe.js(timed out once on darwin in Upgrade reported Node.js version to 26.3.0 #31818): passes 10/10 consecutive local runsnodeExeMatchingAbi()download path exercised end-to-end by hiding the system node fromPATHKnown-unrelated failures
Same as #31818: ASAN-debug only, reproduced identically at base —
BIO_ctrlSEGV on aggregate http test runs, http2maxSessionMemorytimeout, streams-leak absolute-RSS bound.Not in scope (follow-ups)
New Node 26 APIs (
fs.Utf8Stream,req.signal,writeInformation, http2 options), full env-proxy support, upgrade-with-bodyUpgradeStream, flag-gatedstream/iterfamily.