fix(ext/node): implement timeout and pid for child_process spawnSync

[bartlomieju]

Summary

• Implements timeout option support for spawnSync/execSync/execFileSync by adding timeout handling to op_spawn_sync — spawns a background thread that kills the child process after the deadline, then flags killed_by_timeout in the result
• Returns pid from op_spawn_sync so spawnSync results include the pid field as Node.js expects
• Newly passing node_compat tests: test-child-process-execsync.js, test-child-process-spawnsync-timeout.js

Test plan

• ./x test-compat test-child-process-execsync.js passes
• ./x test-compat test-child-process-spawnsync-timeout.js passes
• ./x test-compat test-child-process-spawnsync-validation-errors.js still passes (no regression)
• All other test-child-process-spawnsync* tests unchanged

🤖 Generated with Claude Code

[kajukitli]

The implementation of spawnSync with timeout and killSignal has a critical race condition where the timeout thread can kill an unrelated process due to PID reuse after the child exits. Additionally, numeric killSignal values are silently mishandled, falling back to SIGTERM instead of using the intended signal.

[CRITICAL] ext/process/lib.rs:1150: Timeout thread can kill an unrelated process due to PID reuse. When the child process exits normally before the timeout expires, the spawned thread continues sleeping and then sends a signal to the now-recycled PID. On Unix, libc::kill(child_id, signal) may kill an entirely different process. On Windows, OpenProcess + TerminateProcess has the same risk.

Suggestion: Use a shared cancellation mechanism (e.g., Arc<AtomicBool> set by the main thread after wait_with_output() returns, checked by the timeout thread before killing) or replace thread::sleep with a condvar/channel recv_timeout so the main thread can signal the timeout thread to stop. This ensures the thread never attempts to kill after the child has exited.

[MEDIUM] ext/process/lib.rs:1150: The timeout thread is never joined or cleaned up. It continues sleeping for the full timeout duration even after the child has already exited. This is both a resource concern and the root cause that makes the PID reuse bug possible — there's no way to cancel it.

Suggestion: Use a condvar with a timeout or a channel with recv_timeout instead of thread::sleep, allowing the main thread to wake the timeout thread when wait_with_output() completes.

[MEDIUM] ext/process/40_process.js:505: Numeric killSignal values (e.g., 9) are converted to strings like "9" via String(), but the Rust side's signal_str_to_int() expects signal names like "SIGKILL". It will fail to parse "9" and silently fall back to SIGTERM via unwrap_or(libc::SIGTERM), ignoring the user's intended signal. Node.js supports both numeric and string kill signals.

Suggestion: Either handle numeric signal values on the Rust side (check if the string parses as an integer before calling signal_str_to_int), or convert numeric values to their corresponding signal names on the JS side before passing them through.

[kajukitli]

The PR adds timeout and killSignal support for spawnSync. The most critical issue is a race condition where the timeout thread can kill a recycled PID after the child process has already exited, since there's no cancellation mechanism for the spawned thread. Additionally, numeric kill signals passed from JS (e.g., "9") will silently fall back to SIGTERM instead of the intended signal.

[HIGH] ext/process/lib.rs:1134: Race condition: the timeout thread can kill a recycled PID. When the child exits normally before the timeout, the spawned thread continues sleeping and then calls libc::kill(child_id) (Unix) or TerminateProcess (Windows) on a PID that may have been reused by the OS for a completely different process. The JoinHandle is dropped so there's no way to cancel the thread.

Suggestion: Use a shared Arc<AtomicBool> (or condvar) to signal the timeout thread that the child has exited, so it skips the kill. Better yet, on Windows use WaitForSingleObject with a timeout on the process handle, and on Linux consider pidfd or polling waitpid with WNOHANG. At minimum, set a flag before wait_with_output() returns and check it in the timeout thread before sending the kill signal.

[MEDIUM] ext/process/lib.rs:1155: Numeric killSignal values are silently converted to SIGTERM. The JS layer converts numeric killSignal via String(killSignal) producing e.g. "9", but signal_str_to_int expects signal names like "SIGKILL" and will fail, falling back to unwrap_or(libc::SIGTERM). This means a user passing killSignal: 9 will get SIGTERM instead of SIGKILL.

Suggestion: First try kill_signal.parse::<i32>() to handle numeric strings, then fall back to signal_str_to_int for named signals. This matches Node.js behavior where killSignal accepts both forms.

[MEDIUM] ext/process/lib.rs:1134: The timeout thread is detached (JoinHandle is dropped). If spawnSync is called many times with large timeouts, threads accumulate sleeping in the background even after the child processes have exited. This is a resource leak.

Suggestion: Use a condvar or cancellation token shared between the main thread and timeout thread so the timeout thread wakes up and exits promptly when the child process completes.

[LOW] ext/process/40_process.js:499: Negative timeout values are silently ignored without validation. Node.js validates that timeout is a non-negative integer and throws otherwise.

Suggestion: Add validation: if timeout is provided, ensure it's a non-negative integer, throwing a TypeError/RangeError to match Node.js behavior.

[Copilot]

Pull request overview

Implements Node.js-compatible spawnSync/execSync/execFileSync behavior by adding timeout-based termination in the Rust op_spawn_sync and by plumbing the child PID back into Node’s child_process.spawnSync() result handling.

Changes:

• Added timeout/killSignal options to op_spawn_sync and returned pid + timeout-kill status in its output.
• Extended Deno.Command internals and the Node child_process polyfill to pass timeout options through and map timeout-kills to ETIMEDOUT.
• Enabled additional Node compatibility tests in the node_compat runner config.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/node_compat/config.jsonc	Enables the relevant Node compatibility tests for execSync and spawnSync timeout behavior.
ext/process/lib.rs	Adds timeout handling + returns pid/timeout-kill flag from op_spawn_sync.
ext/process/40_process.js	Wires private timeout/killSignal options through to op_spawn_sync and attaches internal output fields.
ext/node/polyfills/internal/child_process.ts	Forwards timeout/killSignal into Deno.Command and maps internal timeout-kill output to ETIMEDOUT + pid.

[Copilot]

The timeout thread uses Condvar::wait_timeout without checking the returned WaitTimeoutResult or looping on a predicate. This can wake spuriously and kill the child before the timeout expires, and it can also sleep the full timeout if the main thread sets cancelled = true before the timer thread starts waiting (missed notify). Use wait_timeout_while (or an explicit loop) and only kill when the wait actually timed out and the cancelled predicate is still false; also check the predicate before the first wait.

Suggested change

	let (guard, _) = cvar.wait_timeout(guard, timeout).unwrap();
	// If cancelled, the child already exited — don't kill.
	if *guard {
	let (guard, wait_result) =
	cvar
	.wait_timeout_while(guard, timeout, |cancelled| !*cancelled)
	.unwrap();
	// If cancelled or woken before the timeout, the child already exited — don't kill.
	if *guard || !wait_result.timed_out() {

[Copilot]

Command.outputSync() deletes _pid and _killedByTimeout from the object returned by spawnSyncInner(), but the Node polyfill (ext/node/polyfills/internal/child_process.ts) relies on these properties to set result.pid and to translate timeouts into an ETIMEDOUT error. As written, those fields will always be missing for callers of Deno.Command().outputSync(). Consider keeping these fields (eg make them non-enumerable), or exposing them via exported Symbols, or making the stripping conditional so the Node polyfill can still access the values.

[Copilot]

killSignal numeric values from Node are converted to strings (eg 9 -> "9") and then passed through deno_signals::signal_str_to_int, which won't recognize numeric strings. This means numeric killSignal values won't work on Unix and will silently fall back to SIGTERM. Consider supporting numeric signals (eg parse integers or accept a number in SpawnArgs.kill_signal).

Suggested change

	let signal =
	deno_signals::signal_str_to_int(kill_signal).unwrap_or(libc::SIGTERM);
	let signal = deno_signals::signal_str_to_int(kill_signal)
	.or_else(|| kill_signal.parse::<i32>().ok())
	.unwrap_or(libc::SIGTERM);

[Copilot]

Now that spawnSync forwards timeout/killSignal into Deno.Command and interprets _killedByTimeout, the SpawnSyncOptions.killSignal typing/comment earlier in this file is out of date (it currently says the option is not implemented and only types it as string). Consider updating the type to string | number (Node accepts both) and removing/updating the "not yet implemented" note to match the new behavior.