microsoft / azure-pipelines-tasks

Tasks for Azure Pipelines
https://aka.ms/tfbuild
MIT License
3.5k stars 2.61k forks source link

BashV3 task incorrectly sends SIGTERM to child process instead of SIGINT #16731

Open DaRosenberg opened 2 years ago

DaRosenberg commented 2 years ago

Required Information

Type: Bug Task Name: BashV3

Environment

Server: Azure Pipelines Agent: Hosted

Issue Description

BashV3 contains code to listen for SIGINT from the agent and forward the signal to the child process (via the ToolRunner.killChildProcess() method). This logic was initially introduced in https://github.com/microsoft/azure-pipelines-tasks/pull/13573.

The implementation does not work as expected, for two reasons:

  1. Because of https://github.com/microsoft/azure-pipelines-task-lib/issues/858 the POSIX signals get mixed up; the agent sends SIGINT to the task, but the task ends up sending SIGTERM to the child process.

  2. The task listens only for SIGINT but it should also listen for SIGTERM because as stated here the agent apparently sends both, at different times to indicate different levels of urgency.

A more correct implementation would be:

Small changes are necessary in both libraries to address this, so I will create issues and PRs in both repos and update both with links to cross-reference.

Task logs

Here's a simple repro pipeline:

pool:
  vmImage: ubuntu-latest

trigger: none

jobs:

  - job: signals
    displayName: Test POSIX signals
    # Have the job cancel after 1 minute:
    timeoutInMinutes: 1
    steps:
      - checkout: none
      - bash: |
          trap "echo 'SIGHUP'" SIGHUP
          trap "echo 'SIGINT'" SIGINT
          trap "echo 'SIGQUIT'" SIGQUIT
          trap "echo 'SIGTERM'" SIGTERM
          trap "echo 'EXIT'" EXIT
          while true; do sleep 1; echo .; done
        displayName: Wait for signals

Running it will show the following:

image

As show in the screenshot, the script task receives SIGTERM and nothing else.

kirill-ivlev commented 2 years ago

Hi @DaRosenberg! Thanks for your request, we've added this feature request to our backlog. We'll take a look at it once we have enough capacity.

snathanail commented 2 years ago

Note, this messes up docker deployments using bash shells, as it shuts down the container after exiting the shell. I had been banging my head against the wall for the past couple of days...

DaRosenberg commented 2 years ago

@kirill-ivlev Thanks for the notice that you've seen this — however, this is really not a feature request but rather a bug report, and regarding capacity and your backlog, perhaps you did not notice that I have submitted the PRs to fix this issue? All you need to do is accept them.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 180 days with no activity. Remove the stale label or comment on the issue otherwise this will be closed in 5 days

DaRosenberg commented 1 year ago

This issue is still alive and well, so please remove stale label. Non-breaking PRs have been open in both repos for 6 months. All MS has to do is merge them.

OliverMKing commented 1 year ago

Note, this messes up docker deployments using bash shells, as it shuts down the container after exiting the shell. I had been banging my head against the wall for the past couple of days...

I've been impacted by this as well. Wondering if anyone has any workarounds.

github-actions[bot] commented 12 months ago

This issue is stale because it has been open for 180 days with no activity. Remove the stale label or comment on the issue otherwise this will be closed in 5 days

DaRosenberg commented 12 months ago

This issue is still alive and well, so please remove stale label. Non-breaking PRs have been open in both repos for 12 months. All MS has to do is merge them.

natbprice commented 11 months ago

When running a bash task inside a Docker container I don't get any signals on timeout (SIGINT or SIGTERM). My workaround is to manually signal the process (pkill) in an always() cleanup step. Not sure if that should be tracked as a separate issue related to Docker or fixed as part of bash task.

trigger: none

pool:
  vmImage: 'ubuntu-latest'

container:
  image: ubuntu:22.04
  options: --init

steps:
- checkout: none
- bash: |
    trap "echo 'SIGHUP'" SIGHUP
    trap "echo 'SIGINT'" SIGINT
    trap "echo 'SIGQUIT'" SIGQUIT
    trap "echo 'SIGTERM'" SIGTERM
    trap "echo 'EXIT'" EXIT
    while true; do sleep 1; echo .; done
  condition: always()
  timeoutInMinutes: 1
  displayName: DockerTimeout
DaRosenberg commented 1 month ago

The necessary signal parameter has now finally been added in the upstream library: https://github.com/microsoft/azure-pipelines-task-lib/pull/1008

Which means this bug can now be fixed.