ToolRunner.killChildProcess() unexpectedly sends SIGTERM instead of SIGINT

[DaRosenberg]

Environment

azure-pipelines-task-lib version: 3.3.1

Issue Description

The method ToolRunner.killChildProcess() is documented as follows:

/**
 * Used to close child process by sending SIGNINT signal.
 * It allows executed script to have some additional logic on SIGINT, before exiting.
 */

However, its implementation forwards without arguments to Node's ChildProcess.kill() method (documented here) which by default sends SIGTERM to the child process unless a specific signal is specified.

This leads to unexpected behavior for scripts in downstream BashV3 pipeline task, where script tasks will receive SIGTERM instead of SIGINT.

The bug was introduced with https://github.com/microsoft/azure-pipelines-task-lib/pull/663 the intent of which was to add functionality for killing child processes.

Fixing a related incorrect behavior in BashV3 requires changes both here and in the downstream azure-pipelines-tasks library so I will create issues (and PRs) in both repos and update in both places to cross-reference.

Expected behaviour

The ToolRunner.killChildProcess() method should have a parameter allowing caller to specify which signal should be sent to the child process. If undefined, the method should default to SIGINT as per its docs and to ensure this is a non-breaking change.

Actual behaviour

The ToolRunner.killChildProcess() method implicitly sends SIGTERM to child process.

[DaRosenberg]

On second thought it's probably better to have this API mirror that of Node and have the same default, and leave it up to the caller to specify something else if desired.

[max-zaytsev]

Hi @DaRosenberg, thank you for creating an issue. We're currently working on the more prioritized tickets. We'll take a look at it once we have enough capacity.

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[DaRosenberg]

Issue is alive and well.

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[DaRosenberg]

Issue is still alive and well.

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[DaRosenberg]

Issue is alive and well.

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[DaRosenberg]

Issue is alive and well.

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[mouellet]

not stale

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[DaRosenberg]

Not stale, and PR remains available with an easy fix.

[pszypowicz]

Please someone review and approve this https://github.com/microsoft/azure-pipelines-task-lib/pull/859, without it tasks are not able to receive sigint properly. We are running terraform in bash task, and we only receive sigterm (with a proper bash trap), so terraform is not given enough time to gracefully shutdown and safe tfstate file.

Again @DaRosenberg change is NOT breaking. It is fully backward compatible, which is great!

[github-actions[bot]]

This issue has had no activity in 90 days. Please comment if it is not actually stale

[pszypowicz]

Still a problem and a PR exists with an easy fix

[KonstantinTyukalov]

Hi @pszypowicz @DaRosenberg, signal parameter was added in this PR: https://github.com/microsoft/azure-pipelines-task-lib/pull/1008 the new api is available starting from the 4.17.0 version

[KonstantinTyukalov]

I'm closing this issue as resolved. Please let me know if you have further questions