Handle signal termination in @actions/exec#2328
Handle signal termination in @actions/exec#2328ueokande wants to merge 1 commit intoactions:mainfrom
Conversation
Child process termination via signal exit code is null and signal is set to the terminating signal. This change handles signal termination in @actions/exec. The error message in @actions/exec includes the signal when a process is terminated by a signal.
There was a problem hiding this comment.
Pull request overview
Improves @actions/exec’s handling of child processes that terminate via OS signals (e.g., SIGTERM), so callers receive a clearer failure reason instead of an “exit code null” error.
Changes:
- Capture
signalfrom Node’schild_processexit/closeevents and store it inExecState. - Emit a dedicated error when a process ends due to a signal.
- Add a self-terminating test script and a unit test covering signal termination.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/exec/src/toolrunner.ts | Stores termination signal and produces a signal-specific failure error message. |
| packages/exec/tests/scripts/self-terminate.cjs | New helper script that SIGTERMs itself for testing. |
| packages/exec/tests/exec.test.ts | Adds a unit test asserting the error message includes the termination signal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (this.processSignal) { | ||
| error = new Error( | ||
| `The process '${this.toolPath}' failed due to signal ${this.processSignal}` | ||
| ) | ||
| } else if (this.processExitCode !== 0 && !this.options.ignoreReturnCode) { |
There was a problem hiding this comment.
Signal-based termination currently throws regardless of options.ignoreReturnCode. Since ignoreReturnCode is documented as "will not fail leaving it up to the caller" (interfaces.ts), this changes behavior for callers that intentionally ignore failures. Consider honoring ignoreReturnCode for the signal case as well (or introducing/ documenting a separate option for ignoring signals).
| ) | ||
| } else if (this.processSignal) { | ||
| error = new Error( | ||
| `The process '${this.toolPath}' failed due to signal ${this.processSignal}` |
There was a problem hiding this comment.
The new signal-termination error message text ("failed due to signal") doesn't match the newly added test expectation ("terminated by signal"). As-is, the test will fail; consider adjusting this message to include the wording the test asserts (or update the test to match the intended message).
| `The process '${this.toolPath}' failed due to signal ${this.processSignal}` | |
| `The process '${this.toolPath}' terminated by signal ${this.processSignal}` |
This change handles signal termination in @actions/exec. The error message in @actions/exec includes the signal when a process is terminated by a signal.
Close #2313