Cancelling a job doesn't run post hooks or upload artifacts

[keithpitt]

When we cancel a job, we send a KILL signal to the bootstrap.sh process. This is troublesome because it doesn't allow us to upload artifacts, or run any post-script hooks. So, the idea I have is to send the kill signals to the underlying script process, not the bootstrap one.

To do this, first we need to find out the PID, I did some bash and came up with this:

#bootstrap.sh

./script.sh &
PID=$!
echo "(PID is: $PID) Waiting for it..."
wait $PID
echo "Run the hooks here"

#script.sh

function say-goodbye {
  echo "Goodbye!"
}

trap say-goodbye EXIT
SLEEP=15

echo "This is my current PID: $$"
echo "And now I'm sleeping for $SLEEP seconds"
sleep $SLEEP

Now the problem is communicating the PID back to the agent, but I think this can be done with buildkite-agent meta-data or something. Once we have that PID, the canceller just kills the process PID, not the boostrap one. And all the after script tasks + cleanup should all "just work"...I think!

[graemej]

fyi: SIGTERM might be a better choice of signal so that it's catchable. In our usecase we're launching Docker containers as we have a few custom volume mounts and a SIGKILL doesn't reliably disconnect the docker run process from the process owned by the docker daemon when cancelling.

[toolmantim]

@graemej we do in fact send a SIGTERM (see

agent/buildkite/job.go

Line 79 in 1636d0e

j.process.Kill()

and

agent/buildkite/script.go

Line 188 in 76417b9

err := p.signal(syscall.SIGTERM)

). We only send a SIGKILL if it's still alive 10 seconds after sending a SIGTERM.

This issue is about having the agent kill the process that's invoked by the bootstrap.sh, rather than the bootstrap.sh process itself.

[toolmantim]

@graemej I've seen that with docker though! Especially if you don't use the [] syntax in the CMD of the Docker file, because the docker run command will be running using bash eval.

[lox]

This should be fixed now!

[keithpitt]

@lox oh right! Since the bootstrap swallows signals and just forwards them onto the child process, it means the bootstrap has a chance to do all the things?

[lox]

Urgh, y'know, I mis-read this. I was actually looking for specifically this bug report to mention that pre-exit needs to be added to the list of things that don't run after cancel.

[lox]

As it stands, cancel will send a SIGTERM to the bootstrap, which will send it to any sub-process-groups it has created and then exit. We need to add some handling to run specific hooks in the bootstrap post-killing of it's subprocesses but before it exits itself.

Which hooks should run? I'm kind of tempted to keep the current behaviour given it's been the status quo for so long. Perhaps we should add a pre-cancel and post-cancel hook?

[lox]

I think unfortunately this one is going to be bumped to 3.1.0.

[ticky]

Just going to put this here because it might be related: do we have any plans to make the force-kill timeout user-configurable? It seems that ten seconds is not enough time for some applications to clean up after themselves.

[amitsaha]

As it stands, cancel will send a SIGTERM to the bootstrap, which will send it to any sub-process-groups it has created and then exit. We need to add some handling to run specific hooks in the bootstrap post-killing of it's subprocesses but before it exits itself.

@lox it will be useful to consider windows OS when discussing this, since on Windows:

• No concept of signals
• AFAIK, no idea of process groups

Hence, I think issue #794 may be worth considering.

Which hooks should run? I'm kind of tempted to keep the current behaviour given it's been the status quo for so long. Perhaps we should add a pre-cancel and post-cancel hook?

These separate hooks would be very useful to have. This in addition to #794 would improve the experience on Windows greatly.

[lox]

It turns out that we do run pre-exit after cancellation! We added a test recently in #789. It looks like it's not acting as hoped under windows though.

[lox]

It's now acting correctly under windows in #879.