unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
denoland/deno
mid8/10
error-handlingconsistency-modelsrefactoringtesting-strategy

fix(ext/node): allow repeated child process signals

Project: denoland/deno | Language: Rust/TypeScript | Concepts: error-handling, consistency-models, refactoring, testing-strategy | Difficulty: mid
Link: https://github.com/denoland/deno/pull/36644

The problem

Deno's Node.js compatibility layer for child_process incorrectly treated the .killed property as a permanent gate: after sending any signal (even non-terminating ones like SIGSTOP), further attempts to signal the child (e.g., SIGCONT) would be blocked and return false. This behavior diverged from Node.js, where repeated signals (including stop/resume sequences) are allowed as long as the process is alive.

Why it's non-trivial

A naive fix might just remove the .killed check, but this would miss several subtle issues:

  • Some signals (like SIGSTOP) do not terminate the process, so closing stdio, IPC, or cleaning up resources immediately after sending any signal is incorrect.
  • The .killed property should indicate that a signal was sent, not that the child exited.
  • The IPC channel and stdio streams must remain alive until the process actually exits.
  • The signalCode property must reflect the actual termination signal, not just the last signal sent.
  • Windows does not provide POSIX signal semantics, requiring a fallback.
  • Disposal (Symbol.dispose) must still attempt process termination after non-terminating signals.

The approach

1. Remove the .killed gate for signals

Previously, the code would block any further kill() calls after the first successful one:

-    if (this.killed) {
-      return false;
-    }

This check is removed, allowing multiple signals to be sent as long as the process is alive. This matches Node.js behavior, especially for non-terminating signals like SIGSTOP and SIGCONT.

Annotation:
This change ensures that signals like SIGCONT can be sent after SIGSTOP, and that .kill() is not a one-shot operation.

2. Only clean up resources on actual process exit

Previously, sending any signal would close pipes and disconnect IPC:

-    this.#closePipes();
-    /* Cancel any pending IPC I/O */
-    if (this[kCanDisconnect]) {
-      this.disconnect?.();
-    }

Now, these cleanup actions are moved to the process exit handler:

+        const status = await this.#process.status;
+        // ...
+        if (this.signalCode) {
+          this.exitCode = null;
+        } else {
+          this.exitCode = status.code ?? null;
+        }
+        this.emit("exit", this.exitCode, this.signalCode);
+        await this.#_waitForChildStreamsToClose();
+        this.#closePipes();
+        // The child is gone, so the IPC channel can never carry another
+        // message. Tear it down here rather than in `kill()`: a signal does
+        // not necessarily terminate the child, but an exit always ends the
+        // channel.
+        if (this[kCanDisconnect]) {
+          this.disconnect?.();
+        }

Annotation:
This ensures that stdio streams and IPC remain alive after non-terminating signals, and are only cleaned up when the process actually exits.

3. Correct .killed and signalCode semantics

.killed is now set to true whenever a signal is successfully sent, but does not block further signals. signalCode is only set based on the final process status:

+    // `killed` records whether a signal has ever been sent successfully; it
+    // must not prevent later signals from reaching a still-running process.
+    // In particular, SIGSTOP followed by SIGCONT is a supported way to
+    // suspend and resume a child.
     let signalName = signal == null ? "SIGTERM" : toDenoSignal(signal);
     try {
       this.#process.kill(signalName);
     } catch (err) {
       // ...
     }
+    if (isWindows) {
+      // See the `#windowsSignalFallback` use in the status handler above.
+      this.#windowsSignalFallback = signalName;
+    }
+    if (signalName === "SIGKILL") {
+      this[kSigkillIssued] = true;
+    }
     this.killed = true;
-    this.signalCode = signalName;
     return true;

Annotation:
This change makes .killed an indicator that a signal was sent, not that the process has exited, and defers setting signalCode until the process actually terminates.

4. Handle Windows fallback for signals

Because Windows doesn't provide POSIX signal status, the code records the last signal sent as a fallback:

+        // Windows has no POSIX termination status, so a successfully delivered
+        // signal is recorded locally instead. Only trust that record when the
+        // child actually exited unsuccessfully: `kill()` can race a child that
+        // was already exiting on its own, and `TerminateProcess` still reports
+        // success in that window. Reporting a signal for an exit code of 0
+        // would hide the real status.
+        const windowsSignal = status.success
+          ? null
+          : this.#windowsSignalFallback;
+        this.signalCode = status.signal || windowsSignal;

Annotation:
This ensures that .signalCode is only set to the delivered signal on Windows if the process did not exit cleanly (exit code ≠ 0).

5. Always attempt termination on dispose

Previously, disposal would not attempt to kill the process if .killed was already true:

-  [SymbolDispose]() {
-    if (!this.killed) {
-      this.kill();
-    }
-  }
+  [SymbolDispose]() {
+    this.kill();
+  }

Annotation:
Now, disposal always attempts to terminate the child, even if a non-terminating signal was sent earlier (e.g., after SIGSTOP).

6. Add and update tests

New tests ensure correct behavior for repeated signals, correct IPC and stdio handling, and correct Windows fallback:

+Deno.test({
+  name: "[node/child_process] SIGSTOP and SIGCONT preserve child state",
+  ignore: Deno.build.os === "windows",
+  async fn() {
+    // ... test logic ...
+  },
+});
+
+Deno.test({
+  name: "[node/child_process] SIGSTOP and SIGCONT preserve IPC channel",
+  ignore: Deno.build.os === "windows",
+  async fn() {
+    // ... test logic ...
+  },
+});
+
+Deno.test({
+  name: "[node/child_process] windows reports the delivered signal",
+  ignore: Deno.build.os !== "windows",
+  async fn() {
+    // ... test logic ...
+  },
+});

Annotation:
These tests check that repeated signals work, that resources are only cleaned up on exit, and that Windows .signalCode fallback behaves as expected.

From the review thread

bartlomieju:
Thanks for working on this. The core repeated-signal fix looks directionally right, but could you please split the IPC write/termination machinery into a separate PR?

For this PR, I think the scope should stay focused on repeated signals:

  • remove the this.killed gate;
  • avoid closing stdio/disconnecting IPC merely because a signal was sent;
  • keep the IPC read loop alive after non-terminating signals;
  • derive signalCode from the final process status;
  • retain the focused SIGSTOP → SIGCONT coverage.

...

There is also a Symbol.dispose issue to fix here. It currently remains:

[SymbolDispose]() {
  if (!this.killed) {
    this.kill();
  }
}

After a successful non-terminating signal such as SIGSTOP, killed is already true, so disposal becomes a no-op and can leave the child stopped indefinitely. Since killed now records only that a signal was successfully sent, disposal should still call kill(). Please update this and add a regression test covering disposal after a non-terminating signal.

xz-dev:
Thanks for the review and the follow-up commit — really elegant fixes. I completely missed the unref'd-read race, and tearing the channel down on exit instead of in kill() is a much cleaner invariant. Learned a lot from this one

Takeaways

  • Resource cleanup should be tied to actual process exit, not signal delivery. Non-terminating signals must not trigger premature cleanup of stdio or IPC.
  • State flags like .killed should reflect their true semantics. Use them as indicators of actions taken (a signal was sent), not as one-way gates that block further valid operations.
  • Tests should cover platform differences and edge cases. Especially when emulating another platform's API, ensure that both POSIX and Windows behaviors are covered, and that non-terminating signals and disposal logic are tested.