async_hooks,events,http,http2,perf_hooks: port Node.js async compatibility tests and fix the gaps they surface — ALS run/disable + withScope/defaultValue, http client ALS across reused agent sockets, http2 ALS context, AsyncResource.bind, EventEmitterAsyncResource, timerify (+22 tests) - #31825
Merged
Conversation
Collaborator
|
Updated 4:17 PM PT - Jul 16th, 2026
❌ @cirospaciari, your commit 7d52ef1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31825That installs a local version of the PR into your bun-31825 --bun |
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR adds structured filesystem error handling for ChangesFilesystem copy error standardization
AsyncLocalStorage and AsyncResource enhancements
EventEmitterAsyncResource refactor
Async context preservation in streams
fs.cp path validation and fast-path optimization
Performance.timerify implementation
Comprehensive fs.cp test coverage
Stream and async iterator test coverage
Async hooks and worker regression tests
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/fs/cp.test.ts (1)
544-552:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure socket cleanup runs in all paths and is awaited.
sock.close()is not guaranteed to run if an earlier await throws, and it is not awaited. Wrap this block intry/finallyand await close completion to avoid leaking a server handle in failure paths.Proposed fix
- await using proc = Bun.spawn({ - cmd: [bunExe(), "-e", script], - env: bunEnv, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - sock.close(); - expect(stderr).toBe(""); - expect(stdout.trim()).toBe("ok"); - expect(exitCode).toBe(0); + try { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ok"); + expect(exitCode).toBe(0); + } finally { + await new Promise<void>(resolve => sock.close(() => resolve())); + }As per coding guidelines: “Track resources (servers, clients) in arrays for cleanup in afterEach().”
🤖 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/fs/cp.test.ts` around lines 544 - 552, The socket close call in the test after spawning the process (the block using Bun.spawn with variables proc, stdout/stderr/exitCode) isn't awaited and won't run if an earlier await throws; wrap the await Promise.all([...]) and the subsequent logic in a try/finally so you always await sock.close() in the finally block (e.g., await sock.close()) and also register the socket in the test-level cleanup array per the afterEach() resource tracking guideline so the server handle is cleaned up on all paths.
🤖 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/perf_hooks.ts`:
- Around line 237-242: timerify currently throws when options is null (during
destructuring) and may throw when histogram is null while checking
histogram.record; update timerify to explicitly reject null/non-object inputs:
first validate that options is not null/undefined (and if it is, throw
$ERR_INVALID_ARG_TYPE("options", "Object", options)), then extract histogram and
validate histogram !== undefined && histogram !== null && typeof histogram ===
"object" && typeof histogram.record === "function", otherwise throw
$ERR_INVALID_ARG_TYPE("options.histogram", "RecordableHistogram", histogram);
keep references to validateFunction(fn, "fn") and the $ERR_INVALID_ARG_TYPE
helper when implementing these checks.
- Around line 199-224: The observe method can add this to functionObservers
prematurely and never remove it if validation or super.observe throws or if
later calls stop requesting "function" entries; change the logic in observe (the
observe method and its use of functionObservers) so that registration to
functionObservers happens only after super.observe succeeds and ensure any prior
registration is removed when options do not include "function" (including when
entryTypes is present but contains no "function"), and also wrap the
super.observe call in a try/catch to remove the registration if an error is
thrown (i.e., delay functionObservers.add(this) until after successful
super.observe and call functionObservers.delete(this) when switching modes or on
error).
- Around line 244-255: The timerified wrapper in function timerified skips
calling processTimerifyComplete when fn throws synchronously; modify timerified
so the invocation of fn (via fn.$apply for normal calls or Reflect.construct for
constructor calls) is executed inside a try block and ensure
processTimerifyComplete(fn.name, start, args, histogram) runs in a finally block
so timing is recorded for both success and synchronous exceptions; for
promise-returning results keep the existing result.finally behavior but make
sure synchronous exceptions are rethrown after the finally so the original error
propagation is preserved (update code around timerified, fn.$apply,
Reflect.construct, start, args, histogram, and processTimerifyComplete
accordingly).
In `@test/js/node/test/common/fs.js`:
- Around line 20-23: In assertDirEquivalent, the current matching uses
entry.name which breaks for nested trees; change the matching logic that
iterates dir1Entries/dir2Entries to find pairs by comparing the computed relPath
(relative path from the root) rather than entry.name, and after pairing verify
the file type (e.g. directory vs file) on the matched entry (the existing checks
that use entry.type or entry.isDirectory()/isFile()) so nested items like
"a/file.txt" vs "b/file.txt" won't be conflated; update the two loops that
reference dir1Entries/dir2Entries (the loop starting at the snippet with
entry1/entry2 and the similar block at lines ~35-43) to locate entries by
relPath and then run the same equivalence assertions on the paired entries.
- Line 24: The assertion dereferences entry2 in the message which will throw if
entry2 is undefined; change the assert in the test so the failure message does
not access entry2.name (e.g., use a static message or reference the expected
name from the original entry variable) — update the assert call that mentions
entry2 to avoid entry2.name and instead use a safe expression or known
expectedName in the message.
In
`@test/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs`:
- Around line 18-20: The test is not exercising the "force: false" path because
the options passed to cp(...) only include dereference and recursive; update the
options object in the cp(...) call (the cp function invocation) to include
force: false (e.g., cp(src, dest, { dereference: true, recursive: true, force:
false })) so the test actually validates the force-false behavior described by
the filename.
---
Outside diff comments:
In `@test/js/node/fs/cp.test.ts`:
- Around line 544-552: The socket close call in the test after spawning the
process (the block using Bun.spawn with variables proc, stdout/stderr/exitCode)
isn't awaited and won't run if an earlier await throws; wrap the await
Promise.all([...]) and the subsequent logic in a try/finally so you always await
sock.close() in the finally block (e.g., await sock.close()) and also register
the socket in the test-level cleanup array per the afterEach() resource tracking
guideline so the server handle is cleaned up on all paths.
🪄 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: 44f86698-4476-443a-85bc-612f8b5cf035
📒 Files selected for processing (69)
src/js/internal/fs/cp-sync.tssrc/js/internal/fs/cp.tssrc/js/node/_http_client.tssrc/js/node/async_hooks.tssrc/js/node/events.tssrc/js/node/fs.promises.tssrc/js/node/fs.tssrc/js/node/http2.tssrc/js/node/perf_hooks.tstest/js/node/fs/cp.test.tstest/js/node/test/common/fs.jstest/js/node/test/common/index.mjstest/js/node/test/common/repl.jstest/js/node/test/parallel/test-async-hooks-stack-overflow-nested-async.jstest/js/node/test/parallel/test-async-hooks-stack-overflow-try-catch.jstest/js/node/test/parallel/test-async-hooks-stack-overflow.jstest/js/node/test/parallel/test-async-local-storage-http-agent.jstest/js/node/test/parallel/test-async-local-storage-http-parser-leak.jstest/js/node/test/parallel/test-async-local-storage-isolation.jstest/js/node/test/parallel/test-async-local-storage-run-scope.jstest/js/node/test/parallel/test-async-local-storage-weak-asyncwrap-leak.jstest/js/node/test/parallel/test-asyncresource-bind.jstest/js/node/test/parallel/test-eventemitter-asyncresource.jstest/js/node/test/parallel/test-fs-cp-async-async-filter-function.mjstest/js/node/test/parallel/test-fs-cp-async-copy-non-directory-symlink.mjstest/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjstest/js/node/test/parallel/test-fs-cp-async-dereference-symlink.mjstest/js/node/test/parallel/test-fs-cp-async-dest-symlink-points-to-src-error.mjstest/js/node/test/parallel/test-fs-cp-async-dir-exists-error-on-exist.mjstest/js/node/test/parallel/test-fs-cp-async-dir-to-file.mjstest/js/node/test/parallel/test-fs-cp-async-error-on-exist.mjstest/js/node/test/parallel/test-fs-cp-async-file-to-dir.mjstest/js/node/test/parallel/test-fs-cp-async-file-to-file.mjstest/js/node/test/parallel/test-fs-cp-async-file-url.mjstest/js/node/test/parallel/test-fs-cp-async-filter-child-folder.mjstest/js/node/test/parallel/test-fs-cp-async-filter-function.mjstest/js/node/test/parallel/test-fs-cp-async-identical-src-dest.mjstest/js/node/test/parallel/test-fs-cp-async-invalid-mode-range.mjstest/js/node/test/parallel/test-fs-cp-async-invalid-options-type.mjstest/js/node/test/parallel/test-fs-cp-async-nested-files-folders.mjstest/js/node/test/parallel/test-fs-cp-async-no-errors-force-false.mjstest/js/node/test/parallel/test-fs-cp-async-no-recursive.mjstest/js/node/test/parallel/test-fs-cp-async-overwrites-force-true.mjstest/js/node/test/parallel/test-fs-cp-async-preserve-timestamps-readonly-file.mjstest/js/node/test/parallel/test-fs-cp-async-preserve-timestamps.mjstest/js/node/test/parallel/test-fs-cp-async-same-dir-twice.mjstest/js/node/test/parallel/test-fs-cp-async-skip-validation-when-filtered.mjstest/js/node/test/parallel/test-fs-cp-async-socket.mjstest/js/node/test/parallel/test-fs-cp-async-subdirectory-of-self.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-dest-points-to-src.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-over-file.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjstest/js/node/test/parallel/test-fs-cp-async-with-mode-flags.mjstest/js/node/test/parallel/test-fs-cp-promises-async-error.mjstest/js/node/test/parallel/test-fs-cp-sync-async-filter-error.mjstest/js/node/test/parallel/test-http2-async-local-storage.jstest/js/node/test/parallel/test-perf-hooks-timerify-histogram-async.mjstest/js/node/test/parallel/test-performance-function-async.jstest/js/node/test/parallel/test-quic-callback-error-ondatagram-async.mjstest/js/node/test/parallel/test-quic-callback-error-onstream-async.mjstest/js/node/test/parallel/test-quic-callback-error-suppressed-async.mjstest/js/node/test/parallel/test-quic-endpoint-async-dispose.mjstest/js/node/test/parallel/test-quic-stream-body-async-error.mjstest/js/node/test/parallel/test-quic-stream-body-async-iterable.mjstest/js/node/test/parallel/test-quic-writer-async-dispose-ended.mjstest/js/node/test/parallel/test-stream-finished-async-local-storage.jstest/js/node/test/parallel/test-stream-readable-async-iterators.jstest/js/node/test/parallel/test-webcrypto-methods-not-async.jstest/js/node/test/parallel/test-worker-process-exit-async-module.js
cirospaciari
force-pushed
the
claude/node-v26-async-tests
branch
from
June 5, 2026 01:56
719e212 to
30adb17
Compare
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/node/async_hooks.ts`:
- Around line 81-93: RunScope.dispose currently always calls
this.#storage.enterWith(this.#previousStore), which creates a stale store entry
when there was no prior store; change dispose to check if this.#previousStore is
undefined and, if so, call this.#storage.disable() (or the ALS instance's method
to clear/remove the store) instead of enterWith; otherwise restore with
enterWith(this.#previousStore). Apply the same check-and-disable fix to the
other similar disposal site where enterWith(previousStore) is used (the block
referenced around the other dispose-like code).
In `@src/js/node/events.ts`:
- Around line 825-827: The constructor for EventEmitterAsyncResource calls
validateString(options?.name, "options.name") even when options is omitted,
causing new EventEmitterAsyncResource() to throw; change the check so
validateString is only invoked when options is provided and options.name is
non-null/defined (e.g., guard with options != null && options.name != null)
before calling validateString("options.name"), leaving behavior unchanged when a
caller supplies an options.name; reference EventEmitterAsyncResource and
validateString to locate the constructor and adjust the conditional.
In `@src/js/node/http2.ts`:
- Around line 2338-2349: Add an override of _destroy(err, callback) on
ClientHttp2Stream that calls this.#asyncResource.emitDestroy() exactly once
(guarding if necessary) before delegating to super._destroy(err, callback); this
ensures the per-instance AsyncResource created in the constructor
(this.#asyncResource) is properly destroyed when the stream is torn down and
keeps emit(event, ...) runInAsyncScope behavior correct.
🪄 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: de7adb4d-3c57-4ef1-8a01-53cf5cdc61e1
📒 Files selected for processing (70)
src/js/internal/fs/cp-sync.tssrc/js/internal/fs/cp.tssrc/js/node/_http_client.tssrc/js/node/async_hooks.tssrc/js/node/events.tssrc/js/node/fs.promises.tssrc/js/node/fs.tssrc/js/node/http2.tssrc/js/node/perf_hooks.tssrc/jsc/bindings/NodeValidator.cpptest/js/node/fs/cp.test.tstest/js/node/test/common/fs.jstest/js/node/test/common/index.mjstest/js/node/test/common/repl.jstest/js/node/test/parallel/test-async-hooks-stack-overflow-nested-async.jstest/js/node/test/parallel/test-async-hooks-stack-overflow-try-catch.jstest/js/node/test/parallel/test-async-hooks-stack-overflow.jstest/js/node/test/parallel/test-async-local-storage-http-agent.jstest/js/node/test/parallel/test-async-local-storage-http-parser-leak.jstest/js/node/test/parallel/test-async-local-storage-isolation.jstest/js/node/test/parallel/test-async-local-storage-run-scope.jstest/js/node/test/parallel/test-async-local-storage-weak-asyncwrap-leak.jstest/js/node/test/parallel/test-asyncresource-bind.jstest/js/node/test/parallel/test-eventemitter-asyncresource.jstest/js/node/test/parallel/test-fs-cp-async-async-filter-function.mjstest/js/node/test/parallel/test-fs-cp-async-copy-non-directory-symlink.mjstest/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjstest/js/node/test/parallel/test-fs-cp-async-dereference-symlink.mjstest/js/node/test/parallel/test-fs-cp-async-dest-symlink-points-to-src-error.mjstest/js/node/test/parallel/test-fs-cp-async-dir-exists-error-on-exist.mjstest/js/node/test/parallel/test-fs-cp-async-dir-to-file.mjstest/js/node/test/parallel/test-fs-cp-async-error-on-exist.mjstest/js/node/test/parallel/test-fs-cp-async-file-to-dir.mjstest/js/node/test/parallel/test-fs-cp-async-file-to-file.mjstest/js/node/test/parallel/test-fs-cp-async-file-url.mjstest/js/node/test/parallel/test-fs-cp-async-filter-child-folder.mjstest/js/node/test/parallel/test-fs-cp-async-filter-function.mjstest/js/node/test/parallel/test-fs-cp-async-identical-src-dest.mjstest/js/node/test/parallel/test-fs-cp-async-invalid-mode-range.mjstest/js/node/test/parallel/test-fs-cp-async-invalid-options-type.mjstest/js/node/test/parallel/test-fs-cp-async-nested-files-folders.mjstest/js/node/test/parallel/test-fs-cp-async-no-errors-force-false.mjstest/js/node/test/parallel/test-fs-cp-async-no-recursive.mjstest/js/node/test/parallel/test-fs-cp-async-overwrites-force-true.mjstest/js/node/test/parallel/test-fs-cp-async-preserve-timestamps-readonly-file.mjstest/js/node/test/parallel/test-fs-cp-async-preserve-timestamps.mjstest/js/node/test/parallel/test-fs-cp-async-same-dir-twice.mjstest/js/node/test/parallel/test-fs-cp-async-skip-validation-when-filtered.mjstest/js/node/test/parallel/test-fs-cp-async-socket.mjstest/js/node/test/parallel/test-fs-cp-async-subdirectory-of-self.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-dest-points-to-src.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-over-file.mjstest/js/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjstest/js/node/test/parallel/test-fs-cp-async-with-mode-flags.mjstest/js/node/test/parallel/test-fs-cp-promises-async-error.mjstest/js/node/test/parallel/test-fs-cp-sync-async-filter-error.mjstest/js/node/test/parallel/test-http2-async-local-storage.jstest/js/node/test/parallel/test-perf-hooks-timerify-histogram-async.mjstest/js/node/test/parallel/test-performance-function-async.jstest/js/node/test/parallel/test-quic-callback-error-ondatagram-async.mjstest/js/node/test/parallel/test-quic-callback-error-onstream-async.mjstest/js/node/test/parallel/test-quic-callback-error-suppressed-async.mjstest/js/node/test/parallel/test-quic-endpoint-async-dispose.mjstest/js/node/test/parallel/test-quic-stream-body-async-error.mjstest/js/node/test/parallel/test-quic-stream-body-async-iterable.mjstest/js/node/test/parallel/test-quic-writer-async-dispose-ended.mjstest/js/node/test/parallel/test-stream-finished-async-local-storage.jstest/js/node/test/parallel/test-stream-readable-async-iterators.jstest/js/node/test/parallel/test-webcrypto-methods-not-async.jstest/js/node/test/parallel/test-worker-process-exit-async-module.js
Vendors the async-related tests from node v26.3.0 test/parallel that Bun can
support (57 files: fs.cp, AsyncLocalStorage, AsyncResource, stack-overflow,
stream iterators, quic skip-guards, webcrypto, perf_hooks) and fixes the
runtime bugs they exposed. All 56 vendored async tests pass, plus the existing
unit suites for every touched module.
Runtime fixes:
- AsyncLocalStorage.run restored its context slot by the index captured before
the callback; disable() during the callback splices the array and corrupts
the restore. Now restores by identity, matching node's enterWith(prior)
semantics (including re-adding and re-enabling a previous value).
- AsyncLocalStorage: defaultValue + name constructor options, withScope()
disposable RunScope (new in node 26).
- AsyncResource.bind lost the wrapped function's arity and forced thisArg to
the resource; now preserves fn.length and forwards call-time this.
- EventEmitterAsyncResource was missing the asyncId/triggerAsyncId/
asyncResource getters and the asyncResource.eventEmitter back-reference;
rewritten to node's class shape (EventEmitterReferencingAsyncResource).
- fs.cp callback got undefined instead of null on success.
- fs.cp/cpSync/promises.cp now run node's validateCpOptions and route through
the JS port (with node's ERR_FS_CP_* SystemError shapes restored in
internal/fs/cp{,-sync}) whenever node error semantics can fire; the native
clonefile fast path is kept for fresh-destination copies of plain files and
directory trees. Also ports node 26's "dest dir exists + errorOnExist +
!force => ERR_FS_CP_EEXIST" rule and URL src/dest normalization.
- ClientRequest.destroy(err) never emitted 'error' on the request (node relays
it via the socket error listener).
- http2 client streams lost the AsyncLocalStorage context active at request()
time; events from native parser callbacks now run in it via an AsyncResource.
- perf_hooks: implement performance.timerify + top-level timerify export, the
'function' PerformanceObserver entry type (JS-side dispatch wrapping the
WebCore observer), and PerformanceNodeEntry.
- test/common: vendor common/fs.js + common/repl.js, export hasQuic from
index.mjs, add test-runner fixtures.
Test adaptations (commented in-file): stack-overflow tests use non-tail
recursion (JSC has proper tail calls), weak-asyncwrap-leak uses a
FinalizationRegistry instead of v8.queryObjects, webcrypto skips the
unimplemented ML-KEM/getPublicKey methods, readable-async-iterators forces
multi-chunk reads where Bun's eager EOF made a close non-premature.
cp.test.ts updates: EISDIR/EEXIST expectations become node's ERR_FS_EISDIR/
ERR_FS_CP_EEXIST, and the AsyncCpTask use-after-free regression test now
triggers via a unix socket in a fresh destination on Linux (the per-file
native path is no longer reachable on macOS, where fresh trees are a single
clonefile and existing destinations go through the JS port).
- PerformanceObserver.observe: the entryTypes form replaces the observed set, so drop a prior 'function' registration when the new list lacks it, and only register after super.observe() succeeds - timerify: validate options with validateObject and reject null histograms with ERR_INVALID_ARG_TYPE instead of crashing on property access - cpSync: give the identical-src-dest error the same SystemError shape (errno, syscall, path, info) as the async path - fs.cp/cpSync: stop resolving src/dest before passing them to the JS implementation; node hands the caller's strings to filter callbacks verbatim, and resolving broke mixed-separator paths on Windows - NodeValidator: check the exception scope after JSC::isArray in validateObject; isArray can throw through the proxy slow path - cp.test.ts: same-file copy on Windows now expects ERR_FS_CP_EINVAL, matching node (EINVAL fires before any file op, so EBUSY cannot occur)
node's getValidatedPath only converts file URLs and validates the string; it never resolves or normalizes. The path field on ERR_FS_EISDIR and ERR_FS_CP_EEXIST therefore echoes the string the caller passed in. The test built its inputs with string concatenation but asserted against join(), which only agrees on posix; on Windows join() produces backslashes while the error carries the original forward-slash concatenation.
…servers on function-only re-observe
- getValidatedCpPath: accept Uint8Array/Buffer paths like node's
getValidatedPath does; decode to a string and skip file: URL sniffing
since node treats byte paths literally
- PerformanceObserver.supportedEntryTypes now includes 'function', which
this module supports via timerify, so feature detection works
- observe({entryTypes: ['function']}) after a web-type subscription now
disconnects the underlying web observer; the entryTypes form replaces
the whole observed set in node
cirospaciari
force-pushed
the
claude/node-v26-async-tests
branch
from
June 5, 2026 20:11
65099eb to
190edd3
Compare
node's fs.cp validates src and dest before starting the async work, so an invalid path type throws ERR_INVALID_ARG_TYPE synchronously and the callback is never invoked. We already did this for options; hoist the path validation too instead of letting it reject inside promises.cp.
The debug-only assert at the end of run()'s finally compared getStore() against the raw previous value. With the new defaultValue option, getStore() falls through to the default once the entry is removed from the context, so any run() on a storage with a non-undefined defaultValue and no prior entry threw an AssertionError in debug builds (masking the callback's result). Compare against the value getStore() actually reports: the previous value when one existed, undefined when a disable() during the callback left the storage disabled, and the defaultValue otherwise.
…promises.cp options - PerformanceNodeEntry's prototype chain now links to PerformanceEntry so observer entries pass instanceof checks like node's. extends can't work (the WebCore constructor throws) and $toClass would swap the prototype object out, dropping the class's own toJSON; setPrototypeOf keeps the class body intact, and the instances' own data fields shadow the inherited brand-checked accessors. - fsPromises.cp is now async like node's, so option-validation errors (e.g. mode out of range) surface as rejections instead of synchronous throws. The callback form keeps its intentional sync throw.
Accessing an ES private field on a wrong receiver throws the engine's brand-check TypeError before any === undefined comparison runs, and the constructor always assigns the field, so these ERR_INVALID_THIS throws could never fire. node v26's lib/events.js uses guard-free private-field getters with the same observable behavior, so delete the dead checks rather than reintroducing Symbol-keyed storage.
Contributor
There was a problem hiding this comment.
All prior review feedback has been addressed and the latest pass found nothing new, but this is a large change spanning six core Node-compat modules (ALS run/restore semantics, fs.cp native↔JS-port routing with an acknowledged perf trade-off, http2 async context, timerify, EventEmitterAsyncResource rewrite, plus a NodeValidator.cpp tweak) — worth a human pass before merge.
Extended reasoning...
Overview
This PR ports ~56 upstream Node.js async-tagged tests and fixes the runtime gaps they expose across async_hooks (ALS run() restore-by-identity, defaultValue/name/withScope, AsyncResource.bind arity/this), events (EventEmitterAsyncResource rewritten to Node's class shape), fs.cp/cpSync/promises.cp (Node's validateCpOptions, ERR_FS_CP_* SystemError shapes, native-fast-path gating), _http_client (destroy(err) now emits 'error'), http2 (per-stream AsyncResource so events run in the request()-time ALS context), and perf_hooks (timerify, PerformanceNodeEntry, NodePerformanceObserver wrapping the WebCore observer for the 'function' entry type). It also touches src/jsc/bindings/NodeValidator.cpp to add an exception check around JSC::isArray. ~60 of the 71 files are vendored upstream tests or test-common helpers.
Security risks
None identified. No auth/crypto/permissions surfaces are touched; the C++ change is a defensive RETURN_IF_EXCEPTION reordering. The fs.cp changes affect error-shape and routing only — no new path-traversal or symlink-following behavior beyond what the Node port already had.
Level of scrutiny
High. While the bulk of the diff is vendored tests, the runtime changes are non-trivial and cross-cutting:
- The
fs.cpfamily now gates the native fast path onlstat(dest)returning ENOENT and routes existing-destination merges through the JS port. The PR description explicitly calls this out as a perf trade-off with a follow-up to teach the native task Node's error shapes. This is a design decision a maintainer should sign off on. AsyncLocalStorage.run()'sfinallyrestoration logic was rewritten (restore-by-identity instead of by captured index) and interacts withdisable(),#defaultValue, and a debug-only$assert. This area went through multiple review iterations.http2.ClientHttp2Streamnow wraps everyemit()inrunInAsyncScope— a hot-path change.perf_hooksexports a subclassedPerformanceObserverinstead of the WebCore one; the observe/disconnect replace-vs-additive semantics took several rounds to get right.
Other factors
The author has been responsive — every inline comment I and CodeRabbit raised across four review rounds (observe() replace semantics, timerify null guards, cp-sync areIdentical SystemError shape, Buffer cp paths, supportedEntryTypes, sync-vs-async cp path validation, the defaultValue debug-assert, PerformanceNodeEntry prototype chain, fsPromises.cp async, dead ERR_INVALID_THIS guards) was either fixed or correctly rebutted as upstream-verbatim/Node-matching behavior. The latest bug-hunting pass found nothing. CI build #61022 is referenced. Given the breadth and the explicitly acknowledged follow-up, human review is appropriate; I'm not approving.
cirospaciari
force-pushed
the
claude/node-v26-async-tests
branch
from
June 8, 2026 18:59
4aa666f to
a34c136
Compare
- _http_client.ts: take main. #31825's destroy(err) error-emit workaround targeted the now-removed fetch-backed client; #31587's rewrite routes destroy(err) through the real socket. - perf_hooks.ts: main generalized node-only entry routing (PerformanceObserverForNodeTypes + NodeEntryObserver in internal/shared, kNodeEntryTypes={net,dns,http}); #31825 built a parallel single-purpose observer for the 'function' type (timerify). Resolved by porting timerify + PerformanceNodeEntry onto main's framework: 'function' added to kNodeEntryTypes, a new enqueueNodeEntry() exported from internal/shared, and processTimerifyComplete() routes through it instead of the now- dropped function-only Set/dispatch.
…-tests # Conflicts: # src/js/internal/fs/cp-sync.ts # src/js/internal/fs/cp.ts # src/js/node/fs.promises.ts # src/js/node/fs.ts # src/jsc/bindings/NodeValidator.cpp # test/js/node/fs/cp.test.ts # test/js/node/test/parallel/test-fs-cp-async-file-url.mjs # test/js/node/test/parallel/test-stream-finished-async-local-storage.js # test/js/node/test/parallel/test-stream-readable-async-iterators.js
…ient parser callbacks
A keep-alive socket dispatched parser callbacks (and the response/data/end
chain) in the ALS context of whichever request first connected it, because
Bun's native HTTPParser ignores the async-resource argument. tickOnSocket
now snapshots the active $asyncContext frame on the request and the socket
data/end listeners restore it; the snapshot is cleared at request close so
retaining the request object does not pin the stores. The raw frame is used
directly so http.request() does not flip on async-context tracking when no
ALS is in use.
Also fixes the run() finally to scan only key (even) slots when re-locating
the storage — Array.prototype.indexOf could match an odd value slot when
another ALS stored this storage as its value — and gates timerify entry
emission on hasObserver('function').
… store getStore() reports #defaultValue while the storage is disabled, but the active frame can still hold a real value for it — a snapshot() taken before disable() restores exactly that. run()'s unchanged-value short-circuit compared against getStore(), so inside such a frame `exit()` / `run(undefined, ...)` matched the mask, returned without installing the store, and the callback then read the frame's unmasked value: 'X' where Node gives undefined. Skip the short-circuit while disabled and take the full install/restore path, which re-enables the storage as before.
…ocess The destroy(err) fixture was the only one of the four still exiting abruptly. It happens to be safe today -- destroy(err) tears the session down first -- but the sibling that did the same leaked its session and aborted the x64-asan shard, so align it rather than leave the one exception.
Contributor
There was a problem hiding this comment.
No new findings on fafa64d — the last nit (subprocess process.exit(0) alignment) is addressed and all prior threads are resolved. Deferring to a human for sign-off given the scope: this reworks AsyncLocalStorage.run() restore semantics and threads async-context frames through the http/http2 client hot paths.
What was reviewed:
run()'s new sameValue short-circuit + identity-based restore against disable()/snapshot()/enterWith() interleavings — the disabled-storage masking case is now guarded.- Frame-clear ordering vs throwing listeners across every
runInFrameemit site in http2 (destroy,emitSessionCloseNT,_destroy) and_http_client(upgrade/connect,closeRequest). - The four subprocess frame-cleanup tests for timer-free conditions and ASAN-safe teardown.
Extended reasoning...
Overview
33 files across five Node-compat modules. The runtime changes are: AsyncLocalStorage (defaultValue/name/withScope, SameValue comparison, identity-based restore in run()'s finally, unconditional restoration on disabled storages, !#disabled guard on the short-circuit); AsyncResource.bind (arity + call-time this); EventEmitterAsyncResource (rewritten to Node's EventEmitterReferencingAsyncResource shape with own-property emit deletion); _http_client.ts (per-request kClientAsyncContext snapshot, seven socket listeners split into runInFrame wrappers, cleared in closeRequest); http2.ts (per-stream and per-session frame capture, withStreamFrame around client #Handlers, read-and-clear before 'error'/'close' emits in both destroy() paths); perf_hooks (timerify, PerformanceNodeEntry, 'function' observer type, kEmptyObject defaults + validateObject for createHistogram); and internal/async_context_frame (current/exchange/run). The rest is 22 vendored Node parallel tests, harness additions (hasQuic, common/repl.js), and unit-test coverage in AsyncLocalStorage.test.ts / EventEmitterAsyncResource.test.ts / perf_hooks.test.ts.
Security risks
None identified. The pollution-safety changes (kEmptyObject defaults, __proto__: null descriptors in bind/timerify) are hardening. runInFrame uses $apply intrinsics; sameValue is pure operators so a userland Object.is patch cannot influence run(). No auth/crypto/permissions surface.
Level of scrutiny
High. AsyncLocalStorage.run() is on the hot path of essentially every request-scoped-context workload, and its restore logic now branches on four cases (context2===context, idx>-1 && hasPrevious, idx>-1 && !hasPrevious, idx===-1 && hasPrevious, idx===-1 && !hasPrevious) each verified against Node v26. Wrapping http/http2 socket listeners changes when user callbacks observe which store on every request. The review history bears this out — roughly a dozen real defects were surfaced and fixed over six weeks (index-based restore corruption, emitSessionCloseNT throw-past-clear, sibling destroy() error-emit throw-past-clear, _destroy 'aborted' throw-past-clear, upgrade-handler throw-past-clear, disabled-storage short-circuit unmasking, setTimeout-in-test flakiness, ASAN subprocess exits). That churn is exactly why a maintainer should sign off on the final shape rather than an automated pass.
Other factors
All 89 review threads are resolved. The author mutation-checked each fix and cross-referenced Node v26.3.0 behaviour explicitly. Test coverage is thorough (each new run() branch and each frame-clear site has a dedicated test that fails without its fix), and the PR description records a release A/B showing no perf regression. A human reviewer (alii) has been tracking outstanding items and last commented 2026-07-09; the seven commits since then address that round plus the subsequent bot findings, so a fresh human pass over the settled diff is the right next step.
| }, | ||
| streamError(self: ClientHttp2Session, stream: ClientHttp2Stream, error: number) { | ||
| }), | ||
| streamError: withStreamFrame((self: ClientHttp2Session, stream: ClientHttp2Stream, error: number) => { |
Contributor
There was a problem hiding this comment.
Wrapping streamError also changes the session-teardown fan-out: session.destroy(err) (and GOAWAY-with-error) calls emitErrorToAllStreams, which now runs each open client stream's 'error'/'close' inside that stream's request()-time frame. Node runs them in the destroy() caller's context (or the session's context for a peer-driven teardown) — never the request()-time one — and Bun before this PR matched Node, so this specific case is a divergence introduced here.
Repro (connect in ctx CONNECT, request() in ctx REQUEST, then als.run("DESTROY", () => client.destroy(err))):
- Node v25.6.1: stream
'error'/'close'→DESTROY, session'error'/'close'→CONNECT - this branch: stream
'error'/'close'→REQUEST, session'error'/'close'→CONNECT
Single-stream RST (aborted/streamError for one stream) matches Node, so it's only the session-destroy fan-out. It also contradicts the "user-initiated destroy() keeps the caller's context" principle stated in the description. Could emitErrorToAllStreams bypass the stream-frame swap (i.e. keep the ambient/caller frame on the teardown path)?
Member
Author
There was a problem hiding this comment.
Good catch — fixed in 1f2e4a6e57. The destroy()/emitErrorToAllStreams fan-out now runs each open stream's 'error'/'close' in the destroy() caller's captured context instead of re-entering the stream's request()-time frame (session 'error'/'close' still use the connect-time frame; the native single-stream RST swap is unchanged), verified against v26.3.0. One aside from my repro: v26.3.0's peer-driven teardown isn't self-consistent (one stream's 'close' ran in the request context, the other's in the session context), so I keyed off the user-initiated case here.
| return this.#asyncResource.asyncId(); | ||
| } | ||
|
|
||
| get triggerAsyncId() { |
Contributor
There was a problem hiding this comment.
This regresses the returned value: #asyncResource is a plain AsyncResource, whose triggerAsyncId() is Bun's stub that always returns 0, so the triggerAsyncId option passed to the constructor is now dropped.
new EventEmitterAsyncResource({ name: "x", triggerAsyncId: 7 }).triggerAsyncId
// Node: 7 · bun main: 7 · this branch: 0
The pre-PR code echoed options.triggerAsyncId ?? 0. Since Bun's AsyncResource already receives triggerAsyncId in its options, having it store the value and return it from triggerAsyncId() would restore parity (and fix the same getter for plain AsyncResource too).
Member
Author
There was a problem hiding this comment.
Fixed in 1f2e4a6e57 — AsyncResource now stores the resolved triggerAsyncId and returns it from triggerAsyncId(), so EventEmitterAsyncResource#triggerAsyncId echoes the option again (7 → 7) and the plain AsyncResource getter is fixed too; the default stays 0 (Bun's execution async id). Added a test pinning both.
…w covered by withStreamFrame The merge with main brought in main's enterStreamAsyncContext/exitStreamAsyncContext helpers alongside this branch's withStreamFrame wrapper, so streamEnd swapped the async-context frame twice. withStreamFrame already wraps every client #Handlers callback, so the inner swap and its supporting symbol/field/constructor are dead. Also: the 'aborted' frame-clear test used a default GET request, which since the merge ends the writable side at request() time, so destroy() no longer emits 'aborted' and the subprocess waited forever. Use POST so the writable side stays open and the 'aborted' path is actually exercised.
…m:false, keep the destroy() caller's context on session teardown AsyncResource stores the resolved triggerAsyncId and returns it from triggerAsyncId() instead of a hardcoded 0, so EventEmitterAsyncResource's triggerAsyncId getter echoes the constructor option like Node instead of always reporting 0. The default stays 0 (Bun's execution async id). ClientHttp2Session.destroy() fans stream teardown out through the native emitErrorToAllStreams, whose dispatch re-enters JS under the session's captured frame. Capture the destroy() caller's frame on the session and run each open stream's error handler under it, so those streams' 'error'/'close' observe the destroy() caller's async context (matching Node) while the session's own 'error'/'close' keep the connect-time context. The frame lives on the session, so a coincident dispatch on another session is unaffected. request() only defaults endStream to true for GET/HEAD/DELETE when the caller did not specify it: an explicit endStream:false now keeps the writable side open like Node (destroy() emits 'aborted' and a body can follow), and the no-payload content-length rejection keys on the resolved endStream rather than the method alone. The aborted-listener async-context test now keeps the writable side open with endStream:false: a plain GET ends it up front and never emits 'aborted' (in Node either), which the merged transport rewrite correctly exposed.
…uit tests leave behind Match the sibling NaN test's cleanup: enterWith() is not scoped, so splice the entries back out in a finally so later tests start from an empty async-context frame. No-Verification-Needed: test-only cleanup, no runtime surface
sosukesuzuki
approved these changes
Jul 17, 2026
robobun
added a commit
that referenced
this pull request
Jul 17, 2026
…annel-node26 Take main's RunScope/withScope from #31825; drop this PR's setStoreInCurrentContext workaround since main's enterWith-based implementation passes the vendored tests.
robobun
added a commit
that referenced
this pull request
Jul 17, 2026
The branch that runs when m_nextTickQueue is set cleared the microtask-tick hook without draining the queue, so als.enterWith(x); process.nextTick(cb) on an otherwise-idle tick never ran cb. With #31825 on main, RunScope uses enterWith and diagnostics_channel's RunStoresScope reports transformer errors via process.nextTick, so test-diagnostics-channel-bind-store.js needs this drain to fire uncaughtException. Drops the setStoreInCurrentContext workaround this PR previously carried in async_hooks.ts (now identical to main), and the Known divergence in the PR description no longer applies.
robobun
added a commit
that referenced
this pull request
Jul 17, 2026
…type emit #31825's EventEmitterAsyncResource picked between emitWithRejectionCapture and super.emit based on this[kCapture], and deleted the own-property emit the base constructor stamped. This PR removed both of those: there is one prototype emit now, it gates rejection capture on kCapture internally via addCatch's early return, and the constructor never stamps an own emit. Always route through super.emit (which is what node's lib/events.js does), and drop the now-dead own-property delete. Verified against node v26.3.0: listener and rejection handler both observe the resource's creation-time store with captureRejections on.
5 tasks
cirospaciari
added a commit
that referenced
this pull request
Jul 17, 2026
Only src/js/node/worker_threads.ts conflicted, in three hunks where #34338 ("don't hang when captured stdout/stderr is never consumed") and this branch touch the same lines. #34338 removed the #stdoutAutoPipe/#stderrAutoPipe fields and moved the stdio port ref/unref out of ref()/unref() — ports now manage their own ref via makePortReadable's incrementsPortRef. This branch only added #hasRef bookkeeping there, so main's structure is taken wholesale and only the two `if (!this.#exited) this.#hasRef = ...` lines and the field are kept. async_hooks.ts (#31825) and VirtualMachine.rs (#34293, #32498) auto-merged. `git diff origin/main -- src/js/node/worker_threads.ts` is a pure addition: zero deleted lines, so nothing from #34338 or #31825 is reverted. Verified on the merge result: test-worker-hasref, test-worker-error-stack- getter-throws, test-perf-hooks-worker-timeorigin, test-diagnostics-channel- worker-threads and the new "online fires before the entry point finishes" all pass; #34338's own repro still exits 0 like node; BroadcastChannel ref()/unref() and the 'online' timing fix both still match node v26.3.0.
This was referenced Jul 18, 2026
This was referenced Aug 12, 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
4 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.
Brings the async-tagged portion of the Node test suite in line with Node v26.3.0 by porting upstream tests verbatim and fixing the runtime gaps they expose across
async_hooks,events,http,http2, andperf_hooks.22 upstream tests added (the rest of the original 56 have since landed via #31826 / #31830; coverage 32% → 65%; every vendored test passes).
Behavior changes
AsyncLocalStorage.runcontext corruption: the context slot was restored by an index captured before the callback, so adisable()during the callback spliced the array and the restore landed on the wrong store (or resurrected a disabled one). Restore is now by identity, matching Node'senterWith(prior)semantics, including re-adding and re-enabling a previous value.AsyncLocalStoragev26 API surface: thedefaultValueandnameconstructor options, andwithScope()returning a disposableRunScope.AsyncResource.bindpreserved neither the wrapped function's arity (fn.length) nor call-timethis(it forced the resource asthisArg); both now match Node.EventEmitterAsyncResourcewas missing theasyncId/triggerAsyncId/asyncResourcegetters and theasyncResource.eventEmitterback-reference; rewritten to Node's class shape (EventEmitterReferencingAsyncResource).AsyncLocalStorageacross reused agent sockets: the llhttpHTTPParserbinding ignores the async-resource argument, so a keep-alive socket reused by a second request dispatched parser callbacks (and the'response'/'data'/'end'chain) in the first request's ALS context.tickOnSocketnow snapshots the active async-context frame on the request and each socket listener (data/end/error/close/drain/timeout) runs inside it viainternal/async_context_frame.run(test-async-local-storage-http-agent). The frame is captured inonSocket()as well astickOnSocket:onSocketattaches the socket'serrorlistener synchronously buttickOnSocketonly runs a tick later, andrunInFrameinstalls the frame it is given rather than leaving the ambient one alone — so an error arriving in that window used to run the user's'error'handler in the root context and lose the store, where Node keeps it via the socket's own AsyncWrap.AsyncLocalStoragecontext active atrequest()time —Http2Streamcaptures the frame at construction and native#Handlersare wrapped viawithStreamFrame; the session captures its own frame so'close'doesn't inherit the last stream's. Both frames are released once their last read is done (stream in_destroy, session after the emit inemitSessionCloseNT) so a stream or session retained past its terminal event does not pin the store — the http1 counterpart ofcloseRequest()'s cleanup.perf_hooks:performance.timerify(and the top-leveltimerifyexport), the'function'PerformanceObserverentry type (JS-side dispatch wrapping the WebCore observer), andPerformanceNodeEntry.perf_hooksexport surface now matches v26.3.0's 13 keys:PerformanceNodeEntryis no longer exported (Node names the class but deliberately does not export it — an earlier revision of this PR did), and the top-leveleventLoopUtilizationNode exports was missing and is added. A test pins the surface against the real v26.3.0 key list.PerformanceNodeTimingis knowingly left as a bun-only extra: it predates this branch and removing a public export is a breaking change that belongs in its own PR.run()on a disabled storage now restores on the way out. The restore block was gated on!wasDisabled, soals.disable(); als.run('Y', cb)left'Y'installed afterrun()returned where Node yields thedefaultValue(Node's finally is an unconditionalenterWith(prior)). The gate dates to fix(asyncLocalStorage) store can be disabled multiple times #7015, which guarded adisable()during the callback by reading the flag at exit; snapshotting it at entry silently widened it to skip restoration for storages disabled before the call. The was-absent case is already handled by the identity-relocating branches, so the gate is gone.perf_hooksoption defaults are pollution-safe.timerify()andcreateHistogram()defaulted options to a plain{}, so a pollutedObject.prototype.histogram/figuresmade them throw where Node — which defaults both tokEmptyObject— succeeds.createHistogramadditionally tookoptions || {}, silently acceptingnull/0/'x'/[]that Node rejects withERR_INVALID_ARG_TYPE; it now validates liketimerifyalready did.AsyncLocalStoragestore comparison is SameValue, matching Node's primordialObjectIs. Stores were compared with===, soNaNwas unusable as a store value (run(NaN),enterWith(NaN)and{ defaultValue: NaN }all tripped the debug restore assertions Node has no trouble with).run()'s unchanged-value short-circuit also readObject.isoff the mutable global: userland patchingObject.ismaderun()return the wrong store in release builds, where Node is immune. Both now use a pure-operator SameValue helper — a load-timeObject.iscapture would not fix it, since builtins load lazily and would inherit a patch applied before the firstrequire.validateObject: defer theisArrayprobe (which can throw on a Proxy) until after the cheapnull/callable rejections, keeping the existingRETURN_IF_EXCEPTION.common/repl.js, exporthasQuicfromindex.mjs, add test-runner fixtures.Test adaptations (commented in-file)
test-async-local-storage-weak-asyncwrap-leakuses aFinalizationRegistryinstead ofv8.queryObjects.getPublicKeymethods.Known limitations / follow-ups
createHookinit/before/after/destroylifecycle, which Bun intentionally does not implement; the rest needinternalBinding/--expose-internals, inspector async stack traces,--trace-events,vm.Module.hasAsyncGraph, TLS 1.2 legacy session resumption, the experimentalstream/itermodule, or Node's python harness.repl.start()lands (separate workstream).fs.cpandClientRequest.destroy(err)work originally in this PR has landed separately via fs: port Node.js v26.3.0 fs tests and fix the gaps they surface — cp error semantics, watcher event delivery, watch ignore+AbortSignal, FileHandle pull/writer, glob port, opendir/Dir, mkdtempDisposable, rmdir-recursive end-of-life, mock.fn (+119 tests) #31830 and node:http: rewrite the client on net/tls + llhttp (Node http suite: ~55% → 82.3%, +182 vendored tests, client proxy support) #31587 and is dropped from this diff.Testing
No performance regression: a release A/B (interleaved across fresh processes, same native objects, only the JS swapped) puts
AsyncLocalStorage.run()at 9.12ns vs 9.75ns (-6.5%; the pure-operator SameValue inlines where theObject.ishost call did not) andhttp.request()over a keep-alive agent at 52.8µs vs 53.1µs (-0.55%, within noise).All 111 vendored async tests pass with the debug build, plus the unit suites for every touched module (
async_hooks/,events/,perf_hooks/,http2/,http/); remaining failures in those suites reproduce on a clean-main binary (pre-existing).