Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
DaRosenberg opened this issue Aug 13, 2022 · 9 comments
Open
Labels
Area: ABTT Akvelon Build Tasks Team area of work Area: Release Task: Bash

Comments

@DaRosenberg
Copy link

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 #13573.

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

  1. Because of ToolRunner.killChildProcess() unexpectedly sends SIGTERM instead of SIGINT azure-pipelines-task-lib#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:

  • Add a parameter in upstream ToolRunner.killChildProcess() to allow specifying the signal to send
  • Listen for SIGINT and call bash.killChildProcess("SIGINT");
  • Listen for SIGTERM and call bash.killChildProcess("SIGTERM");

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
Copy link
Contributor

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
Copy link

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
Copy link
Author

@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
Copy link

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

@github-actions github-actions bot added the stale label Mar 27, 2023
@DaRosenberg
Copy link
Author

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.

@github-actions github-actions bot removed the stale label Mar 27, 2023
@OliverMKing
Copy link

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.

Copy link

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

@github-actions github-actions bot added the stale label Nov 13, 2023
@DaRosenberg
Copy link
Author

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.

@github-actions github-actions bot removed the stale label Nov 14, 2023
@natbprice
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: ABTT Akvelon Build Tasks Team area of work Area: Release Task: Bash
Projects
None yet
Development

No branches or pull requests

5 participants