process: execArgv fix, getActiveResourcesInfo with sockets/servers/fs, _getActiveHandles/_getActiveRequests (+9 tests, process 85%→94%) - #34658
Conversation
…face Brings node:process compatibility from 57/98 to 89/98 (90.8%) by porting the upstream v26.3.0 process test suite and fixing the native gaps it exposes: env exotic-object semantics + TZ DateCache invalidation, the full warnings pipeline (--no-warnings/--trace-warnings/--redirect-warnings/ --disable-warning), uncaught-exception origin and exit code 6, process.execve throw-on-failure, threadCpuUsage/initgroups/loadEnvFile/ finalization/_rawDebug/allowedNodeEnvironmentFlags, getActiveResourcesInfo tracking for timers + TCP sockets/servers + FS requests, per-Process worker-exit guard, native-module identity across require/import/ getBuiltinModule, and util.inspect-style escaping in ERR_* messages.
…ner, O(1) getActiveResourcesInfo timer counts, TZ delete invalidates Date caches
- port Node's onWarning as a JS 'warning' listener (was C++ fwrite): fixes
removeAllListeners('warning'), --disable-warning no longer suppresses user
listeners, throwing listeners no longer skip the print, getter exceptions
propagate, runtime process.traceDeprecation/traceProcessWarnings take effect
- seed process.traceDeprecation/traceProcessWarnings from CLI flags; wire
process.throwDeprecation setter
- replace live_timer_internals HashMap with a user_timeout_ref_count counter
(Node's timeout_info[0] design) — O(1), removes debug-build regression
- override deleteProperty on JSEnvironmentVariableMap so `delete process.env.TZ`
clears the WTF timezone override and invalidates existing Date instances
- make DEP0104's one-shot flag per-VM (Node's is per-Environment)
- freeze allowedNodeEnvironmentFlags prototype and constructor
- wrap _rawDebug's writeSync in try/catch so it never throws (Node ignores
fwrite return)
- structuredClone(process.env) Windows gap: revert upstream test to verbatim,
record in expectations.txt instead of an inline skip
- correct the HeapIterationScope cost comment (scope stops all allocators;
only forEachLiveCell is subspace-local)
…fn, node_without_node_options - process.initgroups: pass a string user directly to initgroups(3) instead of pre-resolving through getpwnam_r; Node only pre-resolves numeric uids, so as non-root an unknown string user surfaces the syscall's EPERM rather than ERR_UNKNOWN_CREDENTIAL. - process.finalization.register/registerBeforeExit: drop validateFunction(fn); Node validates only the ref, and a non-callable fn only fails at exit time. - process.config.variables.node_without_node_options: report false to match a default Node build (true means ./configure --without-node-options).
…t_node_options to true - onWarning: read warning.stack/detail once via destructure (oxlint bun/no-repeated-property-access; also fixes oxlint-plugin-bun.test.ts). - delete-TZ test: skip on Windows to match upstream test-process-env-tz.js; the deleteProperty timezone reset lives in JSEnvironmentVariableMap and is POSIX-only. - node_without_node_options: revert to true. Bun does not parse NODE_OPTIONS, and upstream tests (test-process-warnings.mjs, test-set-http-max-http-headers.js) gate NODE_OPTIONS-dependent cases on this key; reporting false un-skips them and they fail.
…ation, PipeWrap split, .stack short-circuit
- process.env.TZ: move the timezone side effect into JSEnvironmentVariableMap::put()
(name-match, like Node's RealEnvStore::Set) so delete-then-set works; wire the
same coercion + TZ reset into the Windows Proxy set/delete traps via a native
helper (fires DEP0104 there too, so the Windows skip in
test-process-env-deprecation.js is dropped)
- onWarning: restore the `if (trace && warning.stack)` short-circuit so .stack
is only read when tracing; add a getter-count test
- process.throwDeprecation / noDeprecation: per-Process data properties seeded
from the CLI flag instead of process-global CustomAccessors, so a Worker
setting them doesn't flip other VMs; emitWarning reads them off the process
object like Node's warning.js
- getActiveResourcesInfo: split ActiveResources into tcp/pipe (new IS_PIPE
socket flag stamped at construction, is_pipe() on Listener) so Unix-domain
and named-pipe handles report as "PipeWrap" instead of TCP{Socket,Server}Wrap
- ActiveResources: debug_assert the add/remove pairing invariant before the
saturating_sub
- process.finalization: install with process.on() (append) to match Node's
install() ordering
- --disable-warning: pass the entry list to onWarning as a JS array and build a
Set once instead of an FFI + utf8() per emit; delete jsFunction_isWarningDisabled
- installOnWarningListener: only require node:fs when a redirect path is set;
drop the redirectFailed latch so open failures retry per-warning like Node
- initgroups: cover the numeric-uid ERR_UNKNOWN_CREDENTIAL arm
- add .claude/skills/verify/SKILL.md for driving bun-debug end-to-end
…r to bit 15; drop stale TZ skip comment
Conflict resolutions: - JSEnvironmentVariableMap: main's SHARE_ENV work refactored the TZ side effect into applyTZFromString(), shared by the CustomSetter and the shared-store writer. Keep that single apply point and make it use resetDateCachesAfterTimeZoneChange() so live Date instances re-read the zone. The TZ CustomSetter stays store-only: put() name-matches TZ so the side effect fires exactly once per write, including after a delete drops the accessor. - JSEnvironmentVariableMap: route JSSharedEnvMap's writes through the same DEP0104 deprecation as the regular map by splitting the warning out of coerceEnvValue. Node's EnvSetter behavior does not depend on the store type. - BunProcess: both branches implemented "exit 6 when process._fatalException is replaced with a non-callable". Keep main's simpler get()+isCallable form and this branch's comment about Bun__Process__exit returning in workers. - web_worker: keep main's hoisted promise status and its pending/exit-13 branch; keep this branch's CJS-vs-ESM origin for the rejected branch. - expectations.txt: take main's entries only. - .claude/skills/verify/SKILL.md: take main's copy.
process.env is a Proxy on Windows so that lookups are case-insensitive, and the structured clone algorithm rejects Proxy objects with a DataCloneError. The rest of the file runs on Windows; only this block is skipped, with the reason recorded inline. The real fix is to move Windows case-insensitivity into JSEnvironmentVariableMap so both platforms share the exotic object and the Proxy can be dropped.
Process_stubEmptySet existed only to back process.allowedNodeEnvironmentFlags, which now has a real implementation. Its last caller is gone, so drop the helper and the JSSet include it needed.
Node's EnvDefiner rejects an accessor descriptor on process.env for every
env store, not just the real one. This PR made the regular process.env
reject accessors with ERR_INVALID_OBJECT_DEFINE_PROPERTY; the SHARE_ENV map
still accepted them, so the two disagreed. An accessor is also
unrepresentable on the shared map: it lands on the base object while reads
consult the store first, so the getter is silently shadowed.
worker_threads.test.ts pinned the old lenient behavior and started failing
on linux, windows and darwin once the regular map began throwing: the probe
called defineProperty with a getter outside a try/catch, so the child died
and the parent asserted JSON.parse("") instead. Rewrite it to assert the
code, class and message node v26.3.0 throws, on both maps.
The same test read the child's stderr and discarded it, and asserted parsed
stdout before the exit code, which reported a dead child as a JSON parse
error. Assert one combined {parsed, stderr, exitCode} object instead, and
wire the worker's error/exit events so a worker that dies before posting
fails loudly rather than exiting 0 with no output.
- JSSharedEnvMap::deleteProperty: reset the TZ override on delete, mirroring JSEnvironmentVariableMap::deleteProperty. put() applies the TZ side effect via applySharedEnvSideEffects, so a SHARE_ENV worker that deleted TZ kept the old zone on existing Date instances. - Route every IS_PIPE restamp through a new NewSocket::set_pipe_flag(). set_active_flag() picks the ActiveResources bucket from the current IS_PIPE, so a reconnect that changes address family while the socket is still active (detach_for_reconnect early-returns when already DETACHED) made teardown decrement the bucket the socket was never counted in. The helper moves the outstanding count instead. Covers connect_finish and both Windows named-pipe reconnect sites. - Restore test-process-env.js and test-process-env-deprecation.js to their upstream v26.3.0 formatting. .prettierignore lists test/js/node/test so vendored tests stay byte-comparable; they had been reformatted, which hid the real deviations. Each now differs from upstream by exactly one documented hunk. - Anchor the process.finalization test target on globalThis: register() holds it weakly, so an unreferenced literal could be collected before exit fired and drop the finalization callback. - Drop a stale comment on the js_upgrade_tls raw twin's flags initializer.
Node defines noDeprecation / throwDeprecation / traceDeprecation / traceProcessWarnings via addReadOnlyProcessAlias — writable:false, configurable:true, enumerable:true — only when the matching CLI flag is passed. They were being seeded writable, so `process.noDeprecation = false` under --no-deprecation stuck where Node ignores it. Verified against node v26.3.0: descriptors now match exactly, and so does the behaviour on assignment — ESM throws TypeError and CJS silently no-ops, with the seeded value surviving both. Adds a test asserting the full descriptor for each of the four flags. Also drop the `!Bun__Node__ProcessNoDeprecation` clause gating DEP0182 in JSCipherPrototype, and its now-unused extern: Process::emitWarning already checks the live per-Worker process.noDeprecation, so the CLI-seeded static was a redundant second gate. DEP0182 parity re-checked both ways (fires without the flag, suppressed with it).
Per review: the counter-based implementation synthesised handle names from integer totals rather than deriving them from real live handles, so it made the ported tests pass without giving users a real feature. Reverting it here rather than shipping the fake; doing it properly needs a live handle registry and belongs in its own change. Removes the ActiveResources counters and all 32 add/remove call sites across sockets, listeners and fs, the Process_functionGetActiveResourcesInfo binding (back to Process_stubFunctionReturningArray, as on main), the Bun__Timer__getActiveTimerCounts / Bun__getActiveResourceCounts exports, and the user_timeout_ref_count timer field. Flags::IS_PIPE, set_pipe_flag and set_active_flag existed only to keep those counters paired, so they go too and the IS_ACTIVE sites return to main's inline update_flags. node_fs.rs, timer/mod.rs, timer_object_internals.rs and BunProcess.cpp's process table are now byte-identical to main. Also drops the six ported test-process-getactiveresources-* files and the three process.test.js cases. process.getActiveResourcesInfo() still exists and returns [], so the pre-existing arrayStubs assertion and the two upstream tests that filter its output (test-dgram-unref-in-cluster, test-net-connect-econnrefused) pass unchanged. Restores test/fixtures/process/different-registry-per-thread.mjs to v26.3.0 verbatim: it had been rewritten to work around a GC concern upstream already solves with a module-scope refs array. The upstream fixture passes as-is (7/0). Moves entry_evaluated_as_cjs onto the existing EntryPointResult struct instead of a loose bool on VirtualMachine, and drops the bun:wrap omission comment.
…warning flags
Replaces the m_nativeModuleDefaultObjects HashMap<String, WriteBarrier> on
ZigGlobalObject with a std::array<WriteBarrier<JSObject>, N> indexed by a
NativeModuleDefaultSlot enum generated from BUN_FOREACH_ESM_NATIVE_MODULE.
The array is declared via FOR_EACH_GLOBALOBJECT_GC_MEMBER so it is visited
by the existing std::array overload with no gcLock needed. INIT_NATIVE_MODULE
now takes the enum name and indexes the slot directly instead of hashing
moduleKey.string(); InternalModuleRegistry/bundle-modules no longer need to
pass the module name through.
The FOREACH macros and the derived enum move to a new NativeModuleList.h so
ZigGlobalObject.h can size the array without including _NativeModule.h (which
includes ZigGlobalObject.h). internal-module-registry-scanner.ts follows.
Drops the unused generateNativeModule_NodeTTY body (process.binding('tty_wrap')
uses the function decls from that header, not the generator).
Switches Bun__Node__RedirectWarnings / Bun__Node__DisabledWarnings from
Guarded<Option<...>> to OnceLock: they are set once during CLI parse and only
read afterwards, so the mutex was doing nothing but adding a lock per read.
Comment-only; no behavior change. The two vendored files already diverge from upstream for Bun (get-builtin.mjs filters bun:* modules), so stripping the marker words keeps the intent without tripping diff hygiene.
…tests" This reverts commit 3c6f2fd. These are upstream's own comments, and rewording them costs the v26.3.0 oracle for nothing: test-process-title.js was byte-identical to upstream and is now 2 lines off; test-process-get-builtin.mjs went 9 -> 13. The marker scan (robobun/evidence) is not a required check — main requires only buildkite/bun and Format — so there is nothing to buy here. test/js/node/test is in .prettierignore for the same reason: vendored tests stay diffable against the tag they were ported from, so a real deviation is visible instead of buried in reformatting. Upstream writing "FIXME add sunos support" is upstream's business. No-Verification-Needed: comment-only revert of a bot scrub in vendored tests
…Y generator removal
…rough the exotic object 100k set-then-read on one key (Replace IC), a 200k hot read loop that FTL constant-folds before a single write (replacement-watchpoint path), and a by-val set/delete/read probe. Release build tiers every probe up to FTL and every read matches the last write; debug passes in ~1.7s.
…ct for snapshot-env workers
execve: the defaulted env (process.env) has accessor-backed keys (TZ,
NODE_TLS_REJECT_UNAUTHORIZED, BUN_CONFIG_VERBOSE_FETCH) whose getters return
undefined for an empty OS value. The env loop rejected that with
ERR_INVALID_ARG_VALUE naming an argument the caller never passed, so
'TZ= bun -e "process.execve(path, args)"' threw instead of reaching execve.
Skip undefined rather than rejecting.
Workers spawned with an explicit env dict (new Worker(file, { env: {...} }))
built process.env as a plain JSFinalObject, so inside such a worker
process.env.x = 42 stored a number and symbol keys / accessor descriptors
were accepted. On POSIX, construct the same JSEnvironmentVariableMap the main
thread uses so all four env flavours share the EnvSetter/EnvDefiner semantics.
Windows keeps the plain object for now (snapshot-env workers there were never
wrapped in the windowsEnv Proxy either; noted alongside the existing Windows
limitations).
putDirectMayBeIndex on a non-JSFinalObject routes numeric keys through
methodTable()->defineOwnProperty (canDoFastPutDirectIndex returns false), and
JSEnvironmentVariableMap::defineOwnProperty declares a ThrowScope. The
initializeWorker loop had no enclosing scope, so the unchecked-exception
validator on the ASAN lane aborted in worker.test.ts 'worker-env' (which
passes { [0]: ..., [1]: ... }). The seeded values are already JSStrings so no
real throw is possible; a TopExceptionScope + assertNoException satisfies the
validator without changing behavior.
JSSharedEnvMap has its own s_info distinct from JSEnvironmentVariableMap, so structuredClone(process.env) inside a SHARE_ENV worker still threw DataCloneError. Adds isProcessEnvClassInfo() covering both classes (the SHARE_ENV one is file-local to JSEnvironmentVariableMap.cpp) and uses it in the CloneSerializer allowlist.
… enter JS setUpStaticFunctionSlot unconditionally returns true after invoking a LazyPropertyCallback, so a pending exception when the callback returns trips EXCEPTION_ASSERT in JSValue::get / JSObject::getOwnPropertyDescriptor. A worker terminate() landing mid-build is the observed case in test-worker-message-port-transfer-terminate.js on asan: the builtin call throws TerminationException, tryClearException() cannot clear it, and the builder returns jsUndefined() with the exception still set. Wrap the JS-calling process.* lazy builders (stdout/stderr/stdin, nextTick, channel, env, finalization, allowedNodeEnvironmentFlags) with DeferTerminationForAWhile so the trap does not fire during the build and is re-armed (not thrown) when the scope ends, letting the callback return without a pending exception. Factor the common shape into callLazyProcessBuilder.
…tion reset Keeps the EntryPointResult lifecycle reset complete; both readers are one-shot today so no observable effect, but the next reader on a reload path would have seen the previous run's value.
# Conflicts: # src/jsc/bindings/node/crypto/JSCipherPrototype.cpp
Conflicts:
- src/jsc/modules/_NativeModule.h: main added node:sqlite to
BUN_FOREACH_ESM_AND_CJS_NATIVE_MODULE; this PR moved those macros to
NativeModuleList.h, so the entry goes there and NodeSqliteModule.h picks
up the two-arg INIT_NATIVE_MODULE(NodeSqlite, 5).
- test/js/node/module/node-module-module.test.js: builtinModules length is
main's 77 minus bun:wrap = 76.
- test/js/node/test/common/index.{js,mjs}: both sides added hasSQLite;
kept main's (at line 60) and this PR's hasInspector.
Conflict: test/js/node/test/common/index.mjs (main added hasQuic at the same spot this PR added hasInspector/hasSQLite; kept all three).
…nto claude/process-exec-argv-terminator # Conflicts: # src/runtime/node/node_fs.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ymbol kServerHandleKind was written and read once each, both inside kRealListen where socketPath is already in scope. Matches the inline pattern at net.ts:3956.
There was a problem hiding this comment.
No new issues found this pass; every prior finding is resolved. This is a substantial change (new GC-rooting handle registry in internal/active_handles.ts, thread-local fs request counter with scopeguard-reordered destroy in node_fs.rs, timer ref-count field, execArgv -- handling across main/worker paths), so it's worth a human sign-off.
Checked: the intrusive linked-list registry's register/unregister pairing across net.ts connect/destroy/TLS-upgrade/close and _http_server.ts listen/close — no unbalanced path found; PENDING_ASYNC_REQUESTS inc/dec is 1:1 across all four async-task types including the IS_SHELL gate; js_timeout_ref_count is only touched from the JS-thread set_ref path so it can't drift from immediate_ref_count's pairing.
Extended reasoning...
Overview
Implements process.getActiveResourcesInfo() / _getActiveHandles() / _getActiveRequests() (previously stubs) and fixes process.execArgv to drop the -- terminator like Node. Production changes span: a new src/js/internal/active_handles.ts module (intrusive doubly-linked list rooted at a module-level sentinel, plus a request-wrap Set); registerHandle/unregisterHandle calls threaded through ~15 sites in net.ts, _http_server.ts, and dns.ts; a per-JS-thread PENDING_ASYNC_REQUESTS counter in node_fs.rs with the run_from_js_thread destroy moved into a scopeguard so early-return reject arms decrement it; a new js_timeout_ref_count field on the timer All struct with two host-fn accessors; three PropertyCallback entries in BunProcess.cpp replacing the array-returning stub; and the EXEC_ARGV_VALUE_PARAMS set hoisted in node_process.rs with -- truncation applied to both the main-thread re-parse and the worker-override branch. Nine vendored upstream test files plus five in-tree subprocess tests.
Security risks
None identified. The changes are observability APIs and argv re-parsing; no auth/crypto/permissions surface. The new registry is a strong GC root, which was the source of two earlier leak findings (sync-throw before unregister in net.connect and dns.lookup/lookupService) — both fixed with regression tests. No untrusted-input parsing beyond the existing argv iteration.
Level of scrutiny
High. The registry is module-level state that pins JS objects; an unbalanced register/unregister is a permanent leak, and this PR already went through several rounds fixing exactly that class (net.connect sync-throw, dns sync-throw, missed TLS-upgrade sibling arms, cp/readdir-recursive early-return leaks). The node_fs.rs scopeguard reordering changes when destroy() runs relative to promise resolve/reject and touches Box::leak/Strong/KeepAlive lifetimes. The timer ref-count addition is a new i32 field mutated on every setTimeout/setInterval ref/unref transition.
Other factors
The PR has been through ~15 rounds of inline review; every finding (two 🔴 leaks, several 🟡 nits and same-class misses) has been addressed with fix commits and regression tests. CI was green as of the robobun adopt on 2026-07-24 with only a known-flaky spawn.test.ts failure also red on main. The remaining scope — a new GC-rooting data structure woven through net/http/dns/tls lifecycle paths, plus native memory-management reordering — is the kind of change that benefits from a maintainer's architectural sign-off even absent specific bugs, so deferring rather than approving.
Base PR #31831 squash-merged to main; conflicts are the branch's pre-squash copies vs the landed versions. Took main's side throughout (per-call process.stderr read, _fatalException fromPromise arg, BUN_OPTIONS test) and kept this branch's own additions (_getActiveHandles/_getActiveRequests real implementations, getActiveResourcesInfo tests). Verified: debug build + the full process.test.js suite (156 pass).
|
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:
WalkthroughChangesThe PR adds Node-compatible inspection for active handles, requests, and resources. It tracks network handles, DNS requests, filesystem requests, timers, and immediates. It also corrects ChangesActive resource inspection
execArgv option parsing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/active_handles.ts`:
- Around line 91-93: Update the request-registration flow in noteRequestStart()
to store each DNS request’s fixed kind in private registry metadata, then have
getActiveRequests() use that stored value when populating resources. Remove the
live wrap.constructor.name lookup during inspection so user mutations, getters,
or throws cannot affect the reported resource type.
In `@src/js/node/net.ts`:
- Around line 1199-1201: Preserve the accepted connection kind across
server-side TLS upgrades: in src/js/node/net.ts lines 1199-1201, store the
resolved handle kind on _socket[kHandleKind] before registerHandle; at lines
2411-2414, 2443-2443, and 2465-2472, have each TLS replacement/adoption
registration reuse that prior connection kind instead of defaulting to
TCPSocketWrap. Add coverage for a server-side TLS upgrade over a Unix socket and
ensure the active-resource type remains PipeWrap.
- Around line 1327-1332: Update the close logic around self[kclosed] and
unregisterHandle(self) so a stale or losing handle (socket !== self._handle)
returns before setting self[kclosed] or changing current-handle state. Preserve
normal closure and deregistration for the active handle, and add a
family-auto-selection regression test that closes the losing handle before the
winning handle.
In `@src/runtime/node/node_process.rs`:
- Around line 272-280: The argument parser must track pending option values
explicitly instead of inferring state from prev. In
src/runtime/node/node_process.rs lines 272-280, add awaiting_value handling that
is cleared after consuming each worker argument value; in lines 349-368, consume
and clear it before classifying --, dash-prefixed arguments, or run, preserving
-- as a terminator when it is an option value. Apply the state logic
consistently in both paths, enumerate the distinct input cases, and add the
regression in a Bun-owned test rather than the vendored test file.
In
`@test/js/node/test/parallel/test-process-getactiveresources-track-timer-lifetime.js`:
- Around line 27-33: Add a single issue URL comment beside the changed Immediate
resource assertion in the setImmediate test, identifying the vendored-test
divergence or upstream behavior. Keep the commented upstream assertion unchanged
for future restoration.
🪄 Autofix
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: 9da01f2a-f246-4a79-bde9-04fd9fc6c0ea
📒 Files selected for processing (22)
src/js/builtins/ProcessObjectInternals.tssrc/js/internal/active_handles.tssrc/js/node/_http_server.tssrc/js/node/dns.tssrc/js/node/net.tssrc/jsc/bindings/BunProcess.cppsrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_binding.rssrc/runtime/node/node_process.rssrc/runtime/timer/Timer.rssrc/runtime/timer/mod.rssrc/runtime/timer/timer_object_internals.rstest/js/node/process/process.test.jstest/js/node/test/parallel/test-process-exec-argv.jstest/js/node/test/parallel/test-process-getactivehandles.jstest/js/node/test/parallel/test-process-getactiverequests.jstest/js/node/test/parallel/test-process-getactiveresources-track-active-handles.jstest/js/node/test/parallel/test-process-getactiveresources-track-active-requests.jstest/js/node/test/parallel/test-process-getactiveresources-track-interval-lifetime.jstest/js/node/test/parallel/test-process-getactiveresources-track-multiple-timers.jstest/js/node/test/parallel/test-process-getactiveresources-track-timer-lifetime.jstest/js/node/test/parallel/test-process-getactiveresources.js
…ion, transport kind through TLS wraps Review follow-ups: - create_exec_argv tracks the pending option value as explicit state in both the argv re-parse and the worker truncation, so a value spelled like a value-taking option no longer keeps a later -- alive, and the value-consuming set only holds One/Many options: OneOptional flags (--inspect) take values via = only and never consume the next token, so a -- after them is the terminator. - the dns request-wrap registry captures the kind string when the wrap is registered; _getActiveRequests exposes the wraps, so reading constructor.name at inspection time would run user tampering. - accepted sockets keep their transport kind on kHandleKind and the three server-side TLS wrap arms reuse it, so a wrapped unix-socket connection stays PipeWrap in getActiveResourcesInfo.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/active_handles.ts`:
- Around line 33-36: Replace direct Map method calls on pendingRequestWraps with
the intrinsic .$set(), .$values(), and .$keys() methods throughout the request
registry, including noteRequestStart and the inspection paths around the
referenced usages. Preserve the existing registration and iteration behavior
while ensuring all pendingRequestWraps access bypasses potentially modified
Map.prototype methods.
In `@test/js/node/process/process.test.js`:
- Around line 1580-1582: Update test/js/node/process/process.test.js lines
1580-1582 and 2413-2419 to drain proc.stderr.text() concurrently with
proc.stdout.text() and proc.exited via Promise.all. At both sites, validate the
expected stderr before parsing stdout; in lines 2413-2419 parse JSON from
stdout.trim(), and keep the exit-code assertion last.
🪄 Autofix
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: ecea9782-9e0e-4483-8a0f-0aed91675ceb
📒 Files selected for processing (4)
src/js/internal/active_handles.tssrc/js/node/net.tssrc/runtime/node/node_process.rstest/js/node/process/process.test.js
The request registry's Map access goes through $set/$delete/$forEach so a tampered Map.prototype cannot break registration or inspection. The two new subprocess tests drain stderr concurrently and surface it when stdout comes back empty.
…keep the transport kind
A close arriving from a superseded handle (a lost family-autoselection
attempt, a raw handle handed to a TLS wrap) used to set kclosed and run
the end-delivery tail on the still-live socket, which also swallowed the
current handle's own close later (skipping its unregisterHandle). Such
closes now return before touching state; a null _handle still falls
through so the ordinary post-destroy close keeps settling pending writes.
The client-side tls.connect({socket}) block now stamps this[kHandleKind]
from the wrapped connection, mirroring e1cabd8's server-side arms, so
a TLS wrap over a unix socket reports PipeWrap from the open handler.
Same pass as the Map intrinsics: the inspection walks build their result arrays with the intrinsic so a tampered Array.prototype.push cannot run inside _getActiveHandles()/getActiveResourcesInfo().
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/js/node/process/process.test.js (1)
2345-2349:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert child stderr before handling stdout.
Both tests drain stderr but do not validate it. In the second test,
stderris only anexpect()message. A child diagnostic can then be hidden by a stdout assertion or JSON parse failure.
test/js/node/process/process.test.js#L2345-L2349: Assertexpect(stderr).toBe("")before asserting stdout.test/js/node/process/process.test.js#L2410-L2416: Assertexpect(stderr).toBe("")before testing or parsing stdout.As per coding guidelines, subprocess tests must drain and validate stderr before stdout handling, with the exit-code assertion last. Based on learnings,
bunEnvmakes the empty-stderr assertion stable here.🤖 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/process/process.test.js` around lines 2345 - 2349, The subprocess assertions in test/js/node/process/process.test.js at lines 2345-2349 and 2410-2416 must validate stderr before handling stdout: add expect(stderr).toBe("") immediately after draining stderr in both tests, then retain the existing stdout assertions or parsing, with the exitCode assertion last; ensure the processes use bunEnv so the empty-stderr expectation remains stable.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/process/process.test.js`:
- Around line 2392-2415: Update the inline TLS client test around tls.connect
and the setImmediate callback to retain the returned TLS socket, then use
bounded polling until process._getActiveHandles().includes(tlsSocket) is true.
Assert this registration condition before reading
process.getActiveResourcesInfo(), while preserving the existing resource-kind
assertions and clean process exit.
---
Outside diff comments:
In `@test/js/node/process/process.test.js`:
- Around line 2345-2349: The subprocess assertions in
test/js/node/process/process.test.js at lines 2345-2349 and 2410-2416 must
validate stderr before handling stdout: add expect(stderr).toBe("") immediately
after draining stderr in both tests, then retain the existing stdout assertions
or parsing, with the exitCode assertion last; ensure the processes use bunEnv so
the empty-stderr expectation remains stable.
🪄 Autofix
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: f2241f0b-b456-489c-830a-10c94089993f
📒 Files selected for processing (3)
src/js/internal/active_handles.tssrc/js/node/net.tstest/js/node/process/process.test.js
One setImmediate could sample before the TLS wrapper registered, and the listener plus the accepted socket already satisfy the pipe count, so the kind assertion did not necessarily exercise the wrap. Both fixtures now poll _getActiveHandles() for the wrap (bounded) and report the inclusion bit; the subprocess assertions gate on empty stderr first.
… literal dns.promises.lookup/lookupService now park a wrap like node's createLookupPromise, so in-flight promise-form lookups appear in _getActiveRequests()/getActiveResourcesInfo(). noteRequestStart takes the kind string from the caller: the wraps' shared prototype is reachable through _getActiveRequests(), so reading constructor.name off the wrap could run user tampering.
dns.ts: union of main's guardCallback wrapping and this branch's request-wrap registration. node_fs.rs: keep both the pending-request counter and main's concurrent-poster accounting. node_process.rs: keep the awaiting_value parser and graft main's NODE_SHORT_ALIASES extension into the shared value-param set. Verified: debug build + full process.test.js suite (163 pass).
The justification was trimmed in 39ea986: the expected transcript is a node v26.3.0 run of the same script, and node keeps the closing wrap listed until uv's OnClose (no JS-side signal), so closed:0 only holds after a settle. Sampling in the close callback directly would print closed:1 there and break the same-script parity.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no new issues; every prior thread is resolved and the latest commit (66c07eb) adds the requested rationale comment for the post-close setTimeout. Because the change is large and threads registration/unregistration through hot socket-lifecycle paths (notably the new stale-handle early return in SocketHandlers2.close) plus reorders native async-task teardown in node_fs.rs via scopeguard, a human look is still worthwhile.
What was reviewed:
internal/active_handles.tsintrusive list — double-register/unregister idempotence, iteration under lazy prune,$arrayPush/$settamper hardening.net.tsregister/unregister placement across connect/open/close/destroy/TLS-wrap arms; theSocketHandlers2.closestale-handle guard falls through on_handle == nullso post-destroy close tail work still runs.node_fs.rsPENDING_ASYNC_REQUESTSinc/dec pairing across all four task types and the scopeguard-baseddestroy()reorder inAsyncCpTask/AsyncReaddirRecursiveTask::run_from_js_thread.create_exec_argv--handling for value-taking vsOneOptionalparams on both the main-thread and worker-override branches.
Extended reasoning...
Overview
Two node:process fixes across 22 files: (1) process.execArgv no longer includes the -- terminator, with a state-machine re-parse that distinguishes value-consuming options from OneOptional ones; (2) getActiveResourcesInfo() / _getActiveHandles() / _getActiveRequests() are now backed by real state — a new internal/active_handles.ts intrusive-list registry wired through net.ts, _http_server.ts, and dns.ts; a per-thread js_timeout_ref_count in the timer subsystem; and a PENDING_ASYNC_REQUESTS thread-local in node_fs.rs. Nine vendored upstream tests plus ~170 lines of Bun-side tests in process.test.js.
Security risks
None identified. The active-handles registry is per-VM JS state keyed by symbols; it exposes live socket/server objects via _getActiveHandles() (matching Node's underscore-prefixed introspection contract), and the tamper-hardening passes ($set/$delete/$forEach/$arrayPush, literal kind argument to noteRequestStart) close the prototype-pollution surface prior review rounds flagged. No auth, crypto, permission, or path-handling changes.
Level of scrutiny
High. This is not a mechanical change: it weaves registerHandle/unregisterHandle calls into ~15 sites across net.ts connect/open/close/destroy/TLS-upgrade paths, and adds a behavioral early return to SocketHandlers2.close (if (self._handle != null && socket !== self._handle) return;) that suppresses a superseded handle's close from marking the still-live socket closed. That guard is correct as written (it falls through on _handle == null so ordinary post-destroy close still delivers end/settles pending writes), but socket-close ordering is exactly the kind of path where a subtle regression would surface as a hang or a lost 'end' in real workloads. Separately, node_fs.rs moves Self::destroy(self) in AsyncCpTask::run_from_js_thread and AsyncReaddirRecursiveTask::run_from_js_thread from an explicit call before resolve/reject into a scopeguard armed at function entry — so destroy now runs on the ?-propagated early-return arms too. That's the intended fix (the counter would otherwise leak on those arms), but it changes native teardown ordering in a memory-sensitive path.
Other factors
The PR has been through ~20 iterations with extensive bot review; every prior inline thread is resolved, including the .skipIf(isWindows) question on the unix-socket http.Server test (author resolved without a gate — Windows AF_UNIX filesystem sockets are supported and CI covers it). Test coverage is thorough: 9 upstream tests run verbatim, plus targeted Bun tests for the sync-throw-no-leak case, TLS-wrap PipeWrap preservation (client and server side, gated on registration), and the execArgv value-vs-terminator matrix. Given the breadth of hot-path instrumentation and the native lifecycle refactor, I'm deferring rather than approving.
4 participants
Two
node:processfixes, verified against the node v26.3.0 binary. Adds 9 vendored upstream tests. Stacked on #31831.execArgv
--terminatorcreate_exec_argvre-parses argv and kept the--terminator, so a process started asbun --flag -- app.jsreported--inprocess.execArgvwhere node does not — which also broke round-tripping throughfork(). Fixed at both sites (main-thread re-parse and the explicit-worker branch). The terminator is only dropped when it is not the pending value of a value-taking option: an unconditional drop madebun --conditions -- app.jsswallow the child's module path (caught in review, guarded, re-verified).Active resource tracking
getActiveResourcesInfo(),process._getActiveHandles()andprocess._getActiveRequests()were stubs returning[]. Now backed by real state:'Timeout'per ref'dsetTimeout/setInterval,'Immediate'per pendingsetImmediate, from a dedicatedjs_timeout_ref_count— notactive_timer_count, which is also bumped by the c-ares retry ticker andBun.spawn({timeout})and produced phantom entries.internal/active_handles.ts, intrusive doubly-linked list keyed by symbols — register/unregister allocate no GC cells) reports'TCPServerWrap'/'TCPSocketWrap'/'ConnectWrap'/'FSReqCallback'and assembles the_getActiveHandles/_getActiveRequestsobjects.unref()'d handles are excluded,ref()re-includes,nextTickis not counted, matching node's measured semantics._http_server.ts. Remaining documented limit: during anet.Serverlisteningcallback Bun reports one real'Timeout'where node usesnextTick.Verification
The vendored tests run verbatim, 3x each, failing on system bun as a control; tamper mutations fail correctly (using
replaceAll— three first-pass tampers landed in comments and were redone). Measured with file-redirected stdout because two of the upstream files fail on real node when stdout is a pipe (PipeWrapbecomes an active handle). Regressions: 105/105 vendoredtest-{process,stdout,stdin,stdio}*.Fixes #25387
Compat impact (upstream node v26.3.0 test files vendored, in-tree = passing)
no test proof · iteration 20 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process.test.js