Big process review & refactor by DrJosh9000 · Pull Request #3814 · buildkite/agent

DrJosh9000 · 2026-04-13T08:04:58Z

Description

Review and refactor of chunks of the process package, because it was getting pretty crusty, but also to solve a specific problem to do with interrupting programs on Windows.

Context

https://linear.app/buildkite/issue/A-1075

The current mechanism used to interrupt processes on Windows is to send a "Ctrl-Break" event (as though someone was typing that key combination on the console window). This can be applied to all processes in a process group (created by an option on the Win32 CreateProcess call) and is understood to be vaguely similar to SIGTERM on *nixes...but it really isn't the same.

Go binaries seem to understand Ctrl-Break as a signal, so we don't need to change how the agent signals the bootstrap. But some processes (PowerShell, ping, probably others) absorb Ctrl-Break and do something other than exit.

Ctrl-C also exists as a concept on Windows and is supported as a generated console event. Unfortunately there are Windows-isms in the way - GenerateConsoleCtrlEvent understands CTRL_C_EVENT, but can't target it to a process group, only the current console. So we have to create the process in a new console, then detach console, attach to the other console, set an event handler, send the Ctrl-C, then detach and reattach to the original console and clean up the event handler. And I can't get that to work!

There's no particular reason we can't support SIGKILL as an "interrupt" signal either. Maybe the user just wants to kill stuff right away? But it isn't a handle-able signal, so we should avoid using it to interrupt the bootstrap (as a subprocess of start or kubernetes-bootstrap), otherwise post-command, pre-exit, artifact etc hooks won't run.

But... interrupting with SIGKILL will let us interrupt processes that absorb Ctrl-Break. So the solution is this PR, plus pass --cancel-signal=SIGKILL.

Changes

Add SIGKILL as an understood cancel signal
For Windows, make SIGKILL call terminateProcessGroup - this isn't needed on *nixes because sending an actual SIGKILL to an actual process group is how that works.
Refactors:
- Move process into internal, because it's not an intended API surface
- Split out setup, start, startWithPTY, startWithoutPTY, copyPTYToStdout, complete from Run
- Use exec.CommandContext and Command.Cancel to set up the interrupt-grace period-terminate handling, rather than running a goroutine waiting for context cancellation the whole time
- Use cmp.Or a bit more
- Remove the waitgroup, instead close the channel from copyPTYToStdout
- Fix a few wrong comments
- Fix some weak mutex coverage
- Make some tests in shell table-driven to exercise the range of interrupt signals

Testing

Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
Code is formatted (with go tool gofumpt -extra -w .)

Disclosures / Credits

I did not use AI tools, but Codex is probably going to review it when I mark it ready whether I want it to or not

zhming0

I think I'd need to proceed after some commit wrangling to make the diff viewable in the core file.

But I am also happy to generate a diff locally if it's easier, lmk 🙏🏿

zhming0 · 2026-04-21T00:39:17Z

+		// We don't SIGKILL the bootstrap as a cancel signal, because that
+		// prevents post-checkout/command hooks running. (Change the signal
+		// grace period instead.) But the user may want the bootstrap to SIGKILL
+		// the command that it runs as an "interrupt".
+		cancelSignal := conf.CancelSignal
+		if cancelSignal == process.SIGKILL {
+			cancelSignal = process.SIGTERM
+		}


The comment does not seem to explain why we reset cancelSignal to SIGTERM when conf.CancelSignal is process.SIGKILL?

The comment seem to suggest that should allow SIGKILL setting to be propagated to child process?

After reading a bit more context, I think I get the gist but still feeling it's going to be a very hard to understand piece of logic for future travellers.

I've changed the comment a bit, let me know what you think!

zhming0

LGTM 👍🏿 . Please correct me if I understood it wrong. For Windows user, they can set cancel signal to be SIGKILL, which will let agent start to simulate a SIGKILL to its subprocesses via windows.CloseHandle(windows.Handle(p.winJobHandle)).

The main benefit is that now cancel signal won't get swallowed by other mechanisms in Windows.

But what I don't get is if this solves the original issue which is about job lifecycle hooks being missed?

Nonetheless the change itself look good, I will give it tick.

zhming0 · 2026-04-22T01:55:46Z

+		// CancelSignal == SIGKILL means the user wants the command to be killed
+		// instead of signaled more gracefully (SIGTERM, SIGINT, etc).
+		// We don't send SIGKILL to the bootstrap itself as a cancel signal,
+		// because that would kill the bootstrap immediately, which would
+		// prevent capturing the exit status of the command, executing various
+		// pre-exit hooks, and other cleanup.
+		cancelSignal := conf.CancelSignal
+		if cancelSignal == process.SIGKILL {
+			cancelSignal = process.SIGTERM
+		}


Should we comment here that the only reasonable use case for specifying SIGKILL is for non-Linux systems?

I don't see why we should specifically discourage SIGKILL for Linux users, it just means there's effectively no signal/cancel grace period.

- Move the files - Cmd-Shift-F, replace "agent/v3/process" with "agent/v3/internal/process" - bazel run :gazelle

This will enable tests to wait for the process to start or complete, rather than having to sleep.

In the unlikely event that creating the job object or attaching the process to the job object fails, we should fall back to terminating just the process.

- Split Run into setup, start, startWithPTY, startWithoutPTY, copyPTYToStdout, complete, onContextCancel. - Use CommandContext and Command.Cancel to handle context cancellation - Fix missing mutex coverage of some fields

DrJosh9000 · 2026-04-22T03:14:34Z

But what I don't get is if this solves the original issue which is about job lifecycle hooks being missed?

If a command swallows Ctrl-Break and doesn't exit, then the bootstrap keeps waiting until the signal grace period, then kills the process. At the same time, the agent is also waiting for the signal grace period, and then kills the bootstrap. Since the bootstrap is what executes the job lifecycle hooks, and the post-command and pre-exit hooks must run after the command, the bootstrap has no time to execute them before being killed.

zhming0 · 2026-04-22T04:00:54Z

It all make senses how. 👍🏿

DrJosh9000 force-pushed the a-1075-add-hard-interrupt-on-windows-experiment branch 17 times, most recently from 128a058 to 0b53623 Compare April 14, 2026 07:26

DrJosh9000 marked this pull request as ready for review April 14, 2026 07:44

DrJosh9000 requested review from a team as code owners April 14, 2026 07:44