Upgrade reported Node.js version to 26.3.0#31818
Open
Jarred-Sumner wants to merge 29 commits into
Open
Conversation
Bump the compat target from Node 24.3.0 to 26.3.0: - nodejs-headers 26.3.0, NODE_MODULE_VERSION 147, bootstrap/flake pins - process.versions.v8 = 14.6.202.34-node.20, modules = 147 - V8 shim updated for 14.6: Isolate roots layout, flattened FunctionCallbackInfo exit frame, String Write/WriteOneByte/WriteUtf8 V2 + Utf8LengthV2 (legacy exports kept), External::New pointer-tag overload, Number::NewFromInt32/NewFromUint32, HandleScope::Extend/DeleteExtensions - napi_get_value_string_* no longer forms a slice from a null buffer pointer when only querying the encoded length - v8/napi test fixtures migrated to the V2 string APIs
Per https://streams.spec.whatwg.org/#readable-stream-cancel step 5, a BYOB reader's pending read requests must be closed with {value: undefined, done: true} when the stream is cancelled. readableStreamClose only settles default-reader read requests, so a pending byob read() hung forever after reader.cancel().
Port the observable behavior changes between Node v24.3.0 and v26.3.0:
streams:
- read() with no size returns one buffered chunk at a time in paused mode
- pause()/resume() are no-ops on destroyed streams; compose() moved to the
prototype and returns the composed Duplex directly
- Duplex.from(async generator) destroy aborts with the error and unblocks a
parked iterator; duplexPair destroy propagates to the other side
- eos()/finished(): conditional AsyncLocalStorage binding, immediate-result
precompute for already-settled streams, real errors override AbortError in
pipeline()
- webstreams adapters: Writable.toWeb no longer hangs on synchronous drain,
fromWeb writev failures error the stream instead of an unhandled rejection,
Readable.toWeb supports type 'bytes' (BYOB), Duplex.toWeb readableType
(DEP0201 for the old alias), ArrayBuffer/SharedArrayBuffer chunk handling,
brotli decoder errors surface as TypeError with the original code
- write(string, 'buffer') throws ERR_UNKNOWN_ENCODING
http:
- ServerResponse.writeHeader removed (DEP0063 end-of-life)
- upgrade requests with no 'upgrade' listener no longer vanish
- getHeader('set-cookie') returns undefined when absent, and
setHeader('set-cookie', []) round-trips as a present-but-empty array
- remove dead half-ported proxy symbols in Agent, https.Agent no longer
shadows createConnection, drain is only emitted when needed
- http2: respond() rejects raw-header arrays instead of sending garbage
frames, request() on closed sessions uses the right error codes, option
range errors use the options. prefix, initialWindowSize is capped
Vendored the matching upstream tests and updated the v24-era ones whose
expectations changed.
Collaborator
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 Node/V8 to 26.3.0 and updates build/tooling; rewrites WebStreams adapters and compression builtins; refactors stream EOS, BYOB, and compose semantics; tightens HTTP/HTTPS/HTTP2 header/session handling and outgoing buffering; updates V8 ABI/bindings and exported symbols; and adds/updates tests to match Node v26 behavior. ChangesVersion & build
WebStreams adapters & Compression
Streams core and EOS
HTTP / HTTPS / HTTP2
V8 binding & ABI changes
N-API and safety
Tests
Possibly related PRs
Suggested reviewers
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/builtins/CompressionStream.ts (1)
8-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
CompressionStreamchunk validation to reject non-BufferSourcewrites, not onlySharedArrayBuffer-backed ones.
src/js/builtins/CompressionStream.ts’s[kValidateChunk]only throws when the chunk’s backing store is aSharedArrayBuffer; it still allows other spec-invalid chunk types (e.g., strings/plain objects) to reach the underlying zlib stream. The Compression Streams spec (and Node’s enforcement) requiresCompressionStream.writablechunks to beBufferSource(ArrayBufferorArrayBufferView) and disallowsSharedArrayBuffer-backed sources up front.🤖 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/builtins/CompressionStream.ts` around lines 8 - 30, The kValidateChunk implementation in CompressionStream currently only rejects SharedArrayBuffer-backed sources; update the [kValidateChunk] (validateBufferSourceChunk) used in newReadableWritablePairFromDuplex to first ensure the chunk is a BufferSource — i.e., either an ArrayBuffer (instanceof ArrayBuffer) or an ArrayBufferView (use isArrayBufferView) — and if not, throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk); then, for ArrayBuffer or ArrayBufferView, continue to reject cases whose backing store is a SharedArrayBuffer by using isSharedArrayBuffer on the appropriate .buffer. Keep the error type/message consistent with existing throws in this file and update only the validateBufferSourceChunk function referenced by CompressionStream/kValidateChunk.
🤖 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/internal/streams/end-of-stream.ts`:
- Around line 165-172: The synchronous-callback path can call the callback
before eos() returns so the later "cleanup" isn't initialized; to fix, declare
and initialize cleanup to a no-op before the immediateResult branch (e.g. let
cleanup = () => {};), then later overwrite cleanup with the real return value
from eos(...). Update the code paths in finished() that reference
options[kEosNodeSynchronousCallback], immediateResult, callback.$call and the
eventual assignment from eos(...) so the early return safely returns the
initialized no-op cleanup.
In `@src/js/node/_http_outgoing.ts`:
- Line 33: The sentinel kEmptySetCookie used to mark an explicit empty
"set-cookie" must be cleared/recomputed whenever the entire header bag
(this[headersSymbol]) is replaced; update code paths that reassign the header
store (e.g., where msg.headers = new Headers(...), or any direct assignment to
this[headersSymbol]) to reset or recompute kEmptySetCookie so subsequent calls
to getHeader("set-cookie"), hasHeader(), and getHeaderNames() reflect the new
Headers instance; specifically, in the header-replacement code clear the
kEmptySetCookie flag (or recompute from the new this[headersSymbol]) so
setHeader, getHeader and related methods behave correctly.
In `@src/js/node/http2.ts`:
- Around line 384-389: The GOAWAY handling currently destroys the
ClientHttp2Session immediately (making this.destroyed true) so request() never
hits the new ERR_HTTP2_GOAWAY_SESSION branch; instead add and use a separate
"gotGoaway" flag on ClientHttp2Session (set inside the `#Handlers.goaway` handler
before any destroy) and update request() to check that flag and throw the
ERR_HTTP2_GOAWAY_SESSION error (created by goawaySessionError) when appropriate;
defer or conditionally call destroy() so destroy does not preempt the GOAWAY
check, and update any logic that relied on destroyed to also consider gotGoaway
where necessary.
In `@test/js/node/http/node-http-parser.test.ts`:
- Around line 254-255: Hoist the require of "node:_http_common" to module scope
instead of inside the test: move the const { parsers } =
require("node:_http_common") out of the test body so the test only calls
parsers.alloc() (creating parser via parser = parsers.alloc()) at runtime;
update references to parsers and parser to use the module-scoped parsers
variable and keep the test focused on behavior, not dynamic loading.
In `@test/js/node/stream/node-stream.test.js`:
- Around line 798-1004: The file contains an exact duplicate of the describe
block "node v26 stream semantics" (the second occurrence repeats all tests such
as the it("read() with no size returns one buffered chunk at a time in paused
mode"), it("read() with no size still concatenates when setEncoding is active"),
the pause/resume destroyed test, compose tests, and write(string,'buffer')
test). Remove the entire second duplicate describe("node v26 stream semantics",
...) block (the duplicate that repeats the same it(...) tests) so only a single
describe block and its matching closing brace remain.
---
Outside diff comments:
In `@src/js/builtins/CompressionStream.ts`:
- Around line 8-30: The kValidateChunk implementation in CompressionStream
currently only rejects SharedArrayBuffer-backed sources; update the
[kValidateChunk] (validateBufferSourceChunk) used in
newReadableWritablePairFromDuplex to first ensure the chunk is a BufferSource —
i.e., either an ArrayBuffer (instanceof ArrayBuffer) or an ArrayBufferView (use
isArrayBufferView) — and if not, throw $ERR_INVALID_ARG_TYPE("chunk",
["ArrayBuffer", "Buffer", "TypedArray", "DataView"], chunk); then, for
ArrayBuffer or ArrayBufferView, continue to reject cases whose backing store is
a SharedArrayBuffer by using isSharedArrayBuffer on the appropriate .buffer.
Keep the error type/message consistent with existing throws in this file and
update only the validateBufferSourceChunk function referenced by
CompressionStream/kValidateChunk.
🪄 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: dcaf62e9-ee50-4b2e-8c62-7c2c241fa415
📒 Files selected for processing (48)
scripts/bootstrap.ps1scripts/bootstrap.shsrc/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/bindings/NodeHTTP.cppsrc/jsc/bindings/v8/v8_handle_scope_data.htest/js/node/http/node-http-parser.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.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/web/streams/compression.test.tstest/js/web/streams/streams.test.js
Comment on lines
+165
to
+172
| if (immediateResult !== undefined && options[kEosNodeSynchronousCallback]) { | ||
| if (immediateResult === null) { | ||
| callback.$call(stream); | ||
| } else { | ||
| callback.$call(stream, immediateResult); | ||
| } | ||
| return cleanup; | ||
| } |
Contributor
There was a problem hiding this comment.
Make finished() safe before exposing synchronous EOS callbacks.
This path can invoke the callback before eos() returns. finished() below still does const cleanup = eos(...), so finished(stream, { cleanup: true, [eos.kEosNodeSynchronousCallback]: true }) on an already-settled stream will hit cleanup before initialization and throw.
💡 One way to harden `finished()`
return new Promise<void>((resolve, reject) => {
- const cleanup = eos(stream, opts, err => {
+ let cleanup = nop;
+ let cleanupAfterAssign = false;
+ cleanup = eos(stream, opts, err => {
if (autoCleanup) {
- cleanup();
+ if (cleanup === nop) cleanupAfterAssign = true;
+ else cleanup();
}
if (err) {
reject(err);
} else {
resolve();
}
});
+ if (cleanupAfterAssign) cleanup();
});Also applies to: 371-371
🤖 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 165 - 172, The
synchronous-callback path can call the callback before eos() returns so the
later "cleanup" isn't initialized; to fix, declare and initialize cleanup to a
no-op before the immediateResult branch (e.g. let cleanup = () => {};), then
later overwrite cleanup with the real return value from eos(...). Update the
code paths in finished() that reference options[kEosNodeSynchronousCallback],
immediateResult, callback.$call and the eventual assignment from eos(...) so the
early return safely returns the initialized no-op cleanup.
Collaborator
Author
There was a problem hiding this comment.
Not applicable to the current code: cleanup is initialized (let cleanup = () => { callback = nop; }) before the immediateResult branch, so the synchronous-callback path always returns an initialized cleanup.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, you're right — I missed that let cleanup = () => { callback = nop; } is declared and initialized before the immediateResult branch, so the synchronous-callback path always returns a valid cleanup function. My concern was incorrect. Sorry for the noise!
🧠 Learnings used
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31830
File: src/js/internal/streams/iter/transform.ts:0-0
Timestamp: 2026-06-05T21:35:42.657Z
Learning: In `src/js/internal/streams/iter/transform.ts` (oven-sh/bun PR `#31830`, commit 7088bada7), the `*Sync` zlib transform variants (e.g., `makeZlibTransformSync`) are NOT buffering implementations — they stream incrementally chunk-by-chunk using node:zlib's native handle `writeSync` method plus the shared `Uint32Array` write-state, which Bun's runtime exposes identically to Node's `internalBinding('zlib')` surface. This is a direct port of Node's `makeZlibTransformSync` loop including no-copy buffer handoff rules; output is yielded as the engine produces it, not accumulated until EOF. Do NOT flag these sync transforms as O(total input) buffering implementations.
Learnt from: robobun
Repo: oven-sh/bun PR: 28430
File: src/js/internal/fs/glob.ts:67-80
Timestamp: 2026-03-22T15:00:33.251Z
Learning: For oven-sh/bun files under `src/js/internal/`, use `new Promise(...)` as the established way to construct promises. Treat `$Promise` as an intrinsic used only for prototype method access (e.g. `$Promise.prototype.$then`), not as a replacement constructor. In this directory, don’t recommend changing `new Promise(...)` to any `$`-prefixed constructor or `$Promise(...)`-style alternative.
Learnt from: robobun
Repo: oven-sh/bun PR: 28480
File: src/js/node/repl.ts:441-489
Timestamp: 2026-03-23T20:24:12.665Z
Learning: In src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts, the review rule about “builtin functions must include `this` parameter typing” should be applied only to functions that are directly bound/called from native code (e.g., via mechanisms like `$newZigFunction` or similar C++/Zig interop). Do not require typed `this` for unrelated JavaScript functions such as prototype methods on function constructors (e.g., `REPLServer.prototype.defineCommand`, `Interface.prototype.question`), which should follow the existing untyped-`this` conventions used in files like `readline.ts`.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Learnt from: robobun
Repo: oven-sh/bun PR: 29215
File: src/js/node/worker_threads.ts:245-305
Timestamp: 2026-04-12T08:49:31.996Z
Learning: In the JS builtin/workers area (e.g., `src/js/node/worker_threads.ts` and other `src/js` builtin context code), remember that `WeakMap` does not have intrinsic `$`-prefixed method variants (do not expect or require `.$get`, `.$set`, `.$has`, `.$delete`). Only plain `Map` supports these intrinsic `$`-prefixed variants. When reviewing `src/js` files, do not flag usage of the normal WeakMap prototype methods `.get`, `.set`, `.has`, or `.delete` as a guideline violation.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30936
File: test/bundler/transpiler/runtime-transpiler.test.ts:225-225
Timestamp: 2026-05-17T19:03:05.577Z
Learning: This repo (oven-sh/bun) does not enforce Biome lint rules in CI because there is no root Biome config (`biome.json` or `.biome*`). Therefore, during code review do not suggest adding `// biome-ignore` (or similar) suppression comments for Biome rule violations.
Additionally, in test files under `test/bundler/transpiler/`, do not “fix” switch-case code by wrapping intentionally-bare (unwrapped) `const` declarations in `{}` blocks when the test is specifically asserting TDZ/const-inlining behavior across sibling cases (e.g., regression tests like issue `#30932`). Adding a `{}` block can interfere with the const-prefix inliner and the single-use substitution pass, causing the test to miss the intended failure mode.
Learnt from: robobun
Repo: oven-sh/bun PR: 31201
File: scripts/strip-long-rs-comments.ts:72-74
Timestamp: 2026-05-22T05:32:25.972Z
Learning: In this repo (oven-sh/bun), .gitattributes enforces LF line endings for tracked files, so CR characters from CRLF inputs should not be present. When reviewing TypeScript code that reads text and splits lines (e.g., using `split("\n")`), don’t flag CRLF/"trailing `\r`" concerns as issues, since tracked inputs are expected to contain only `\n` line endings.
… images] The previous commit's message described the full compat bump but the tracked file changes were lost to a stray hard reset before committing — only the new header made it in. This restores them: - nodejs-headers 26.3.0 / NODE_MODULE_VERSION 147, bootstrap + flake pins, process.versions.v8 = 14.6.202.34-node.20 - V8 shim for 14.6: Isolate roots layout, flattened FunctionCallbackInfo exit frame, String Write/WriteOneByte/WriteUtf8 V2 + Utf8LengthV2 (legacy exports kept), External::New pointer-tag overload, Number::NewFromInt32/NewFromUint32, HandleScope::Extend/DeleteExtensions; export lists updated for all platforms - napi_get_value_string_* no longer forms a slice from a null buffer pointer when only querying the encoded length - v8/napi test fixtures migrated to the V2 string APIs On top of the restoration, review fixes to the shim: - DeleteExtensions now actually reclaims the slots Extend granted inside a closing scope (per-iteration v8::HandleScopes in a long native call no longer accumulate handles until the callback returns), with a debug assert against out-of-LIFO scope destruction - WriteUtf8V2 encodes UTF-16 in chunks so the 32-bit counts from the encoder can't wrap for >4GB outputs, and uses view() instead of flattening ropes - Utf8LengthV2 skips the scalar surrogate scan for valid UTF-16 (SIMD validate first); WriteV2/WriteOneByteV2 use vectorized copies - FunctionCallbackInfo exit frame stays on the stack for up to 16 arguments - version strings come from a single source of truth: REPORTED_NODEJS_V8_VERSION is defined from scripts/build/deps/nodejs-headers.ts and used by both process-report objects, alongside the existing REPORTED_NODEJS_ABI_VERSION
- _http_outgoing: drop the lazy outputData prototype accessor — reading it with `this` set to the prototype (spread, Object.assign, inspection) defineProperty'd a single shared array onto the prototype, reinstating the cross-instance leak it was meant to prevent, and reads on frozen instances threw. Like Node, the prototype now has no outputData property; methods lazily init for subclasses that don't chain the constructor. - _http_outgoing: getRawHeaderNames() now reports a present-but-empty set-cookie under its original casing (kEmptySetCookie stores the name); setHeader skips the per-call toLowerCase for names that can't be set-cookie. - ERR_HTTP2_GOAWAY_SESSION moved into the ErrorCode table instead of a hand-rolled Error factory. Appended at the end of the list: the table's discriminants are index-aligned with the checked-in Rust mirror (src/jsc/ErrorCode.rs). Also declared ErrorCode.ts as an input of the bundle-modules codegen step — error indices are baked into the bundled JS by replacements.ts, and an ErrorCode.ts edit previously left stale numbers in the bundles (errors thrown from builtins came out as the wrong code). - CompressionStream/DecompressionStream share one buffer-source validator via newBufferSourceTransformPairFromDuplex instead of two verbatim copies. - primordials: SafePromiseAllReturnVoid/ReturnArrayLike share one scheduler. - end-of-stream: deduplicate the immediate-result callback invocation. - node-stream.test.js: remove a byte-identical duplicate of the "node v26 stream semantics" describe block. - flake.nix: fix stale Node 24 comment.
…tput [build images]
xwin's splat phase moves unpacked SDK files into place with rename(2). With
the cache under the download dir on tmpfs and /opt/winsysroot on the root
filesystem, every move fails with EXDEV ("Cross-device link"), killing the
linux image bakes. Cache next to the output instead and clean it up after.
The previous head commit came from the formatting bot, whose message lacks the [build images] tag — the pipeline then asked for published v35 images that don't exist yet.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/internal/webstreams_adapters.ts`:
- Around line 869-872: The validateBufferSourceChunk implementation keyed by
kValidateChunk currently only rejects SharedArrayBuffer-backed inputs but
doesn't enforce that non-SAB values are actually BufferSource types; update
validateBufferSourceChunk (kValidateChunk) to first verify the chunk is a
BufferSource by checking isArrayBufferView(chunk) or isArrayBuffer(chunk) and if
not throw $ERR_INVALID_ARG_TYPE("chunk", ["ArrayBuffer", "Buffer", "TypedArray",
"DataView"], chunk); retain the existing
isSharedArrayBuffer(isArrayBufferView(chunk) ? chunk.buffer : chunk) check and
keep throwing on SAB-backed buffers using $ERR_INVALID_ARG_TYPE. Ensure you
reference and use isArrayBufferView, isArrayBuffer, isSharedArrayBuffer and
$ERR_INVALID_ARG_TYPE in the validation.
In `@src/jsc/bindings/v8/shim/HandleScopeBuffer.cpp`:
- Around line 72-81: createRawHandleSlot currently calls createEmptyHandle()
outside the m_gcLock and then re-acquires the lock to append to m_rawGrants,
leaving a tiny race window; change it so both operations happen under one lock
by moving the createEmptyHandle() call inside the WTF::Locker scope (or provide
a createEmptyHandleWithLock helper that assumes the lock is held) and then
append { slot, m_storage.size() - 1 } before releasing m_gcLock in
HandleScopeBuffer::createRawHandleSlot to ensure atomicity; keep function
signature and return the same TaggedPointer* slot.
In `@test/js/node/http/node-http.test.ts`:
- Around line 2435-2443: The test spreads OutgoingMessage.prototype which
triggers enumerable getters (like socket) and mutates the prototype, creating
shared state; remove the unsafe spread expression (the line with "void {
...OutgoingMessage.prototype }") and either omit it entirely or replace it with
a no-op that doesn't enumerate prototype properties (e.g., a plain void 0),
leaving the checks around OutgoingMessage.prototype and the instances c/d and
their outputData assertions intact.
🪄 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: 3f439a28-c630-41e7-b702-de5723734229
📒 Files selected for processing (52)
flake.nixscripts/bootstrap.ps1scripts/bootstrap.shscripts/build/codegen.tsscripts/build/config.tsscripts/build/deps/nodejs-headers.tsscripts/build/flags.tssrc/js/builtins/CompressionStream.tssrc/js/builtins/DecompressionStream.tssrc/js/internal/primordials.jssrc/js/internal/streams/end-of-stream.tssrc/js/internal/webstreams_adapters.tssrc/js/node/_http_outgoing.tssrc/js/node/http2.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcessReportObjectWindows.cppsrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/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/shim/FunctionTemplate.cppsrc/jsc/bindings/v8/shim/Handle.hsrc/jsc/bindings/v8/shim/HandleScopeBuffer.cppsrc/jsc/bindings/v8/shim/HandleScopeBuffer.hsrc/runtime/napi/napi_body.rssrc/symbols.defsrc/symbols.dynsrc/symbols.txttest/js/node/http/node-http.test.tstest/js/node/process/process.test.jstest/js/node/stream/node-stream.test.jstest/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
💤 Files with no reviewable changes (1)
- test/js/node/stream/node-stream.test.js
…ild images] The private S3 mirror has no v26.3.0 musl tarballs, which 403'd the Alpine image bakes. nodejs/unofficial-builds publishes both linux-x64-musl and linux-arm64-musl for current releases (the mirror predates arm64-musl being available there), so pull straight from it and skip the manual re-upload on every Node bump.
…ild images] - process.config.variables: add enable_thin_lto=false and lto_jobs="" to match Node 26. Its common.gypi evaluates these in MSVS settings conditions and gyp hard-fails on undefined variables, breaking every node-gyp build on Windows (v8/napi/dlopen suites). - The cipher streaming tests assumed read() with no size returns the whole buffered ciphertext; since Node 26 it returns one chunk at a time, so the fixtures truncated the ciphertext and OpenSSL reported BAD_DECRYPT — real Node 26 fails the old fixtures identically. Drain read() in a loop and add the second cipher data event Node 26 emits (output verified against Node 26.3.0 byte-for-byte modulo quote style); drop the stale TODO claiming the readable/prefinish order was a bug.
…tions [build images] Deliberate divergence from Node 26 (nodejs/node#62557 made pause()/resume() early-return on destroyed streams). Legacy Readable subclasses like fd-slicer assign `this.destroyed = true` — which hits the prototype setter on modern streams — right before push(null). With the upstream guard, a piped destination's 'drain' can no longer resume the source, so the last buffered chunk is silently dropped and the pipeline never finishes. In practice that breaks yauzl → extract-zip → puppeteer browser installs and other zip/tar tooling, and the same hang reproduces on Node 26.3.0 itself. Keep the Node 24 behavior of letting such streams flush their buffer, and replace the no-op assertion test with a regression test for the fd-slicer pattern (verified: extract-zip now extracts a full chrome-headless-shell archive instead of stopping after the first entries).
…ed [build images]
The dev-server-puppeteer launcher already prefers a system Chromium when one
is installed (CI bootstraps one on every Linux flavor), and several CI
platforms have no Chrome for Testing build at all (linux-arm64,
windows-arm64). The unconditional download during bun install wasted ~150MB
per run and, worse, a half-finished download left in the shared agent cache
by an earlier failed run makes @puppeteer/browsers refuse every later
install ("browser folder exists but the executable is missing"). Set
PUPPETEER_SKIP_DOWNLOAD for the install step when a system browser exists or
the platform can't run the downloaded one.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/next-pages/test/dev-server.test.ts (1)
8-8: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove unused imports.
isCIandisWindowsare imported but never used. The code checksprocess.env.CIandprocess.platform === "win32"directly instead. Remove these from the import list.Cleanup
-import { bunEnv, bunExe, isCI, isWindows, tmpdirSync, toMatchNodeModulesAt } from "../../../harness"; +import { bunEnv, bunExe, tmpdirSync, toMatchNodeModulesAt } from "../../../harness";🤖 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/integration/next-pages/test/dev-server.test.ts` at line 8, The import list includes unused symbols isCI and isWindows; remove them from the import statement that currently imports bunEnv, bunExe, isCI, isWindows, tmpdirSync, toMatchNodeModulesAt so only the actually used imports remain (bunEnv, bunExe, tmpdirSync, toMatchNodeModulesAt) and run tests/lint to ensure no other references to isCI/isWindows exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/next-pages/test/next-build.test.ts`:
- Around line 13-25: The duplicated environment-detection logic for skipping
Puppeteer browser downloads (the hasSystemChromium, skipBrowserDownload and
puppeteerInstallEnv logic) should be extracted into a shared helper to avoid
drift; add a new exported function getPuppeteerInstallEnv in the test harness
(e.g., harness.ts) that encapsulates the checks and returns the env dict, then
replace the in-file variables (hasSystemChromium, skipBrowserDownload,
puppeteerInstallEnv) in next-build.test.ts (and the corresponding blocks in
dev-server.test.ts and dev-server-ssr-100.test.ts) with a single call to
getPuppeteerInstallEnv().
---
Outside diff comments:
In `@test/integration/next-pages/test/dev-server.test.ts`:
- Line 8: The import list includes unused symbols isCI and isWindows; remove
them from the import statement that currently imports bunEnv, bunExe, isCI,
isWindows, tmpdirSync, toMatchNodeModulesAt so only the actually used imports
remain (bunEnv, bunExe, tmpdirSync, toMatchNodeModulesAt) and run tests/lint to
ensure no other references to isCI/isWindows exist.
🪄 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: db7f406d-96b8-472b-9b7c-b8282ef63d02
📒 Files selected for processing (3)
test/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.ts
…, goaway semantics [build images] - v8: reserve the EscapableHandleScope escape slot at construction (like real V8) instead of allocating it inside Escape(). Allocated at Escape() time it sits above any in-scope inline handle grants, so the inline ~HandleScope's DeleteExtensions swept the just-escaped handle together with the scope. The reservation lives in a side registry keyed by the scope's address because the V8 ABI leaves exactly one usable word in the object; entries are purged when their buffer clears, and a reused stack address overwrites its stale entry. New fixture test creates inline Local copies inside the scope before escaping. Also: createRawHandleSlot now does its two bookkeeping steps under one lock. - _http_outgoing: the headers getter/setter, appendHeader and getRawHeaderNames now agree with the kEmptySetCookie marker — the getter mirrors getHeaders(), replacing the header bag or appending a cookie drops the marker, and getRawHeaderNames can't report set-cookie twice. - FetchHeaders.getRawKeys: the HTTPHeaderMap iterator only walks the common and uncommon segments while size() also counts set-cookie values, so the returned array had trailing holes whenever set-cookie was present; size for unique names and append "Set-Cookie" explicitly. - http2: receiving GOAWAY now mirrors Node — NGHTTP2_NO_ERROR begins a graceful close() (request() then throws ERR_HTTP2_GOAWAY_SESSION), any other code destroys the session with ERR_HTTP2_SESSION_ERROR. The rejected-stream test asserted the old behavior; its expectation now matches what a real Node 26 client reports for the same goaway. - tests: hoist the _http_common require to module scope, drop a redundant in-test require, probe the prototype's outputData directly instead of spreading the prototype (which invoked unrelated deprecated getters), and share the puppeteer skip-download env via a harness helper.
…uest [build images] Two fallouts from mirroring Node's GOAWAY handling surfaced by CI: - After a graceful close(), the socket dying left active streams hanging forever: #onClose ran stream.close(NGHTTP2_CANCEL) and then detached the parser, so the writable side could never finish and 'close' never fired (test-http2-server-shutdown-options-errors timed out). Mirror Node's closeSession: hard-destroy every stream that survives the cancel pass, on both client and server sessions. - A request created with an already-aborted signal went through the native parser, which RST'd a stream the peer never saw — a connection error that makes conforming servers reply GOAWAY(INTERNAL_ERROR). The old code swallowed that session error; now that goaway destroys the session with ERR_HTTP2_SESSION_ERROR like Node, the noise became visible. Like Node, never touch the wire: create an id-less stream and destroy it with an AbortError on the next tick (the stream reports aborted=true and rstCode CANCEL but does not emit 'aborted' since the request never started). Also for CI on Windows: pass -Denable_lto=false -Denable_thin_lto=false -Dlto_jobs= to node-gyp in the v8/napi/dlopen test builds. A clang-cl-built Node 26 reports thin-LTO build flags in process.config, node-gyp copies them into addon builds, and MSVC's link.exe hard-fails on /opt:lldltojobs. The defines are no-ops elsewhere. And when puppeteer's browser download can't be skipped, install into a fresh per-run PUPPETEER_CACHE_DIR (forwarded to the launcher) so a half-extracted download left in the shared agent cache by an earlier failed run can't block the install.
The exported ~HandleScope popped the Bun scope stack and cleared its buffer but left the isolate's HandleScopeData alone. If Extend had granted slots from that buffer while the scope was current (an addon's inline Local::New inside e.g. an Array::Iterate callback), next/limit kept pointing into the cleared buffer; the next inline v8::HandleScope then snapshotted that stale limit as prev_limit_, and its DeleteExtensions ran deleteGrantsBack against the enclosing buffer where no grant matches a foreign address — popping every grant, including handles of still-open outer scopes. Snapshot next/limit at scope push (stored in the scope's buffer cell — the 3-word V8 ABI leaves no room in the frame itself) and write them back on pop. New fixture test drives exactly this through Array::Iterate. Also gate the duckdb smoke test on NODE_MODULE_VERSION <= 137: the deprecated duckdb package publishes no prebuilts past ABI 137 (checked npm.duckdb.org for 1.3.1-1.4.1), and node-pre-gyp's fallback source build is not viable in CI. The gate can go away if the test migrates to the N-API based @duckdb/node-api.
Matches node's ordering and semantics (verified against v26.3.0):
validateAbortSignal accepts any object with an 'aborted' property, so a
duck-typed { aborted: true } takes the abort fast path — but objects
without 'aborted' and non-objects throw ERR_INVALID_ARG_TYPE
synchronously, before the fast path can read .aborted.
Azure repeatedly failed to allocate Standard_D4ds_v6 in-region (AllocationFailed) during image bakes; D4as_v7 is one of the alternatives Azure's allocation guidance suggests. Build-only VM — CI runner sizes are unchanged in ci.mjs.
A build without the [build images] tag fell back to published v35 images, and stale v35 alpine images exist from before the Node 26.3.0 install landed (bootstrap already said Version: 35 while still installing 24.3.0) — agents came up with Node 24, failing the version check and building ABI-137 addons that an ABI-147 runtime rightly refuses to load. Move to v36/v21 so the poisoned tags can never be selected again.
…r bun [build images] - Http2Session.close() (client and server) now sends GOAWAY immediately like Node (core.js marks the session closed and calls goaway() right away), so peers stop routing new work to a draining session; the client goaway() gained Node's default arguments. The grpc-js unbind test additionally accepts CANCELLED: a call that wins the race onto the draining session before the GOAWAY/socket-close is processed is torn down with NGHTTP2_CANCEL — the same in-flight teardown Node performs at socket close; which of the three statuses you get is pick-timing. - Run every test-suite node-gyp build under bun (--bun): the gyp -D defines could not neutralize the thin-LTO flags a clang-cl-built Node 26 carries in process.config.target_defaults (node-gyp copies them into config.gypi and MSVC's link.exe fails on /opt:lldltojobs), and the system Node's ABI may not match ours at all. Bun reports the same ABI (147) with clean target_defaults, so the produced modules load in node 26 unchanged. The dlopen fixtures invoke the bun under test explicitly and their beforeAll hooks get a real timeout — the builds legitimately exceed the 5s default under a debug/ASAN binary.
Same rationale as the previous commit's dlopen/napi change — this invocation was missed: gyp -D defines can't override the thin-LTO flags a clang-cl-built Node carries in process.config.target_defaults, so build the node-side module with bun (--bun) as well; it targets the same ABI (147) and loads in node 26 unchanged. Also drop the now-redundant defines from the bun-side invocation.
…ness [build images] common.gypi only applies CLANG_CXX_LANGUAGE_STANDARD (gnu++20) to addon builds when the clang variable is 1, which real Node reports on macOS. With bun running node-gyp and reporting clang=0, Apple clang compiled addons at its ancient default standard and every node-gyp build on darwin failed on <type_traits> (std::is_enum_v) in the Node 26 headers. The v8 harness now includes the child's exit code in crash messages — the Windows fixture crashes currently report nothing, and the code distinguishes an access violation from a DLL load failure.
sharp has no win32-arm64 prebuilt, so its install script falls back to a node-gyp source build under the system Node — which the clang-cl-built Node 26 breaks (process.config leaks thin-LTO flags that MSVC's link.exe rejects with LNK1117). The test exercises package-lock.json migration, not lifecycle scripts, so pass --ignore-scripts there.
…[build images] The puppeteer-based next-pages tests still hit flaky per-run Chrome for Testing downloads on ubuntu/debian x64 (the only CI platforms with neither a system browser nor a skip gate). Bake google-chrome-stable into those images and teach the browser lookups about its binary names — with a system browser present the tests skip the ~300MB download entirely and launch the system binary, like they already do on alpine.
These were in symbols.txt/.dyn (so Linux and macOS resolved them) but never in the Windows .def. Node addons delay-load the host's exports, so the gap only surfaces at the first call: the v8 fixture tests that exercise these type checks died with the delay-load "procedure not found" SEH status (0xC06D007F, reported as exit code 127) after printing their first lines. Manglings verified against clang-cl and the remaining 83 v8 exports audited for further gaps (none).
…[build images] The exit-127 crashes on windows-aarch64 print nothing: stdout shows the test started and stderr is empty. The exe exports every referenced v8 symbol (checked both pre- and post-61197 artifacts against the addon's full import set), so the delay-load theory needs narrowing. Markers go to stderr, which checkSameOutput never compares but does include in crash messages, so the next CI run names the exact call that dies.
Root cause of the Windows --debug fixture crashes (exit 127), found by reproducing under wine with the CI binary and a clang-cl-built debug addon: V8_INLINE members whose bodies live at namespace scope in the headers — HandleScope::CreateHandle(v8::Isolate*, Address), HandleScope::Initialize, and Value::QuickIsUndefined/QuickIsNull/QuickIsNullOrUndefined/QuickIsString — are imported rather than emitted by MSVC debug builds of a dllimport class. Real Node exports its entire V8 surface so this works there; Bun's curated export list didn't include them, and the delay-load helper's "procedure not found" SEH status (0xC06D007F) surfaces as exit code 127 at the first call. That also explains the seemingly random crash points across tests: each died at its own first imported-inline call. Implementations are thin: the CreateHandle overload forwards to the internal::Isolate* variant (same object), Initialize performs V8's inline frame setup (snapshot next/limit, level++ — never pushing a Bun scope, like EscapableHandleScopeBase), and the QuickIs* checks are the corresponding JSC value checks. Exported on all platforms for symmetry; ELF builds emit inline copies locally so only Windows strictly needs them. Verified under wine: with the missing import resolved, test_v8_global, test_v8_escapable_handle_scope_inline_grants and test_v8_locals_survive_nested_call all pass with a debug-CRT fixture.
…d images]
A graceful close() sends GOAWAY through the JS default lastStreamId = 0, and
the native parser only fell back to the session's real last stream id for
undefined/null — a literal 0 went on the wire, telling the peer per RFC 9113
§6.8 that no streams were processed and everything is retriable. Node's JS
layer has the same 0 default and relies on its native correction
(lastStreamID <= 0 → nghttp2_session_get_last_proc_stream_id); mirror that
in the parser so every caller (close(), destroy(), user goaway) is covered.
Also: the server session's goaway handler now matches the client's (and
Node's onGoawayData) — NO_ERROR begins a graceful close, any other code
destroys with ERR_HTTP2_SESSION_ERROR; corrected a comment that claimed
duck-typed { aborted } signals throw (validateAbortSignal accepts any object
with an 'aborted' property); and stripped the v8 fixture's temporary stderr
step markers now that the Windows exit-127 is root-caused.
Member
|
Superseded by #31991, which merges current main (resolving the bootstrap.ps1 conflict and picking up the baked-in Intel SDE that fixes verify-baseline) and fixes the remaining red jobs: ABI-matching node resolution + toolchain capability skip for the macOS boxes, and a deterministic-finalizer fix for the napi abort on windows-11-aarch64. |
cirospaciari
added a commit
that referenced
this pull request
Jun 16, 2026
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_VERSION` 147), and syncs `node:stream`/`node:http` with the behavioral changes upstream made between v24.x and v26.x. ## Native / addon ABI - Headers, bootstrap pins, flake, `process.versions` updated - V8 shim updated for 14.6: Isolate roots layout, flattened `FunctionCallbackInfo` exit frame, `String` `Write*V2`/`Utf8LengthV2` (legacy exports kept for older binaries), `External::New` pointer-tag overload, `Number::NewFromInt32/NewFromUint32`, `HandleScope::Extend/DeleteExtensions` — with Itanium + MSVC symbol exports - Fixed a `napi_get_value_string_*` panic when querying length with a null buffer - v8/napi fixtures migrated to the new header APIs ## stream/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.toWeb` sync-drain hang fix; `fromWeb` writev rejection no longer crashes the error path - `read()` one-chunk semantics in paused mode (semver-major in 26), `compose()` on the prototype - `Duplex.from(async gen)` destroy-during-idle hang fix; BYOB `Readable.toWeb({type:'bytes'})`; DEP0201 - `writeHeader` removed (DEP0063 EOL); upgrade requests without a listener no longer disappear; `set-cookie` getHeader/setHeader edge cases; http2 `respond()` raw-array rejection and session error codes - Also fixes a pending BYOB `reader.read()` never settling after `reader.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-baseline` failure: main now bakes Intel SDE into the Windows image (#31893) instead of downloading it at job time, which the Intel mirror blocks. - **`test/harness.ts`: `nodeExeMatchingAbi()`** — returns the system `node` when its `process.versions.modules` matches 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.ts` and `test/napi/napi.test.ts` resolve `node` through it, so the bun-vs-node comparison no longer fails with `ERR_DLOPEN_FAILED` on machines that still have Node 24 installed (the three darwin `v8.test.ts` failures on #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 two `process.dlopen` tests) skip on machines whose toolchain cannot compile the headers at all (the `macOS-13` Intel box failures on #31818). - **napi fixture: pin the `test_deferred_exceptions` wrapped object** with a strong ref so its `napi_throw`-ing finalizer runs at env teardown instead of from GC. The fixture builds with `NAPI_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-aarch64 `napi.test.ts` failure on #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`). The `respond()`/`request()` header path set the stream to HALF_CLOSED_LOCAL unconditionally — regressing it backwards when the peer had already half-closed — and never dispatched `onStreamEnd`, unlike the `send_data`/`send_trailers` paths. A `res.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, and `server.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 the `test-http2-compat-serverrequest-pipe.js` timeout — 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's `socketOnClose` and 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 with `node --expose-gc`) - `test/v8/v8.test.ts`: 52 pass / 1 skip / 0 fail - both `process.dlopen` suites: 5 pass / 0 fail - `node-http2`, `node-stream`, `node-stream-uint8array`, `web/streams/compression`: 362 pass / 1 fail — the failure is the `http2 maxSessionMemory` timeout that #31818 documents as debug-build-only and reproduced at its base - `test-http2-compat-serverrequest-pipe.js` (timed out once on darwin in #31818): passes 10/10 consecutive local runs - `nodeExeMatchingAbi()` download path exercised end-to-end by hiding the system node from `PATH` ## Known-unrelated failures Same as #31818: ASAN-debug only, reproduced identically at base — `BIO_ctrl` SEGV on aggregate http test runs, http2 `maxSessionMemory` timeout, 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-body `UpgradeStream`, flag-gated `stream/iter` family.
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
3 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.
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 buffertest/v8: 50 pass / 1 skip ·test/napi: 99 pass / 0 failstream/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.
Known-unrelated failures
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.