Kill build hook with SIGTERM instead of SIGKILL #15105

lisanna-dettwyler · 2026-01-28T08:41:35Z

This feels like the most straightforward way to handle #14760. If there's a more preferred solution, I'd be happy to change my patch.

Motivation

This gives custom build hooks the chance to perform any needed cleanup, which is impossible when killed with SIGKILL. After 500ms, if the hook still hasn't exited, it will be forcibly killed with SIGKILL.

Context

#14760

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

xokdvium · 2026-01-28T18:20:59Z

src/libstore/unix/build/hook-instance.cc

+    /* Give custom build hooks the chance to cleanup. */
+    pid.setKillSignal(SIGTERM);


Hm doesn't this mean that we'd use SIGTERM in the destructor and could get stuck on waitpid after an interrupt indefinitely for a misbehaving build hook? Shouldn't we still use SIGKILL in the destructor and have a timeout in the happy path (busywait?).

How's this? The alternative is adding a busywait + kill directly to Pid::wait, which seems messy.

src/libutil/unix/processes.cc

xokdvium · 2026-01-29T23:37:50Z

src/libutil/unix/processes.cc

+    if (killTimeout > 0)
+        killThread = std::thread([&]() {


Hm, should we bother doing this if we are using SIGKILL already?

Not sure I follow, this patch uses SIGTERM initially, and falls back to SIGKILL (via the killThread) if SIGTERM doesn't do the trick after 500ms. Since SIGKILL is unhandleable, we need to use SIGTERM for the build hook if we want to give them the ability to do any sort of cleanup.

Right, I meant for the non-hook case. Don't make sense to spawn a thread to kill a derivation builder for example, but I guess that is guarded by the timeout, which is 0 by default.

Yeah, that was the intention behind the guard. I'll make the logic a little clearer by also checking killSignal.

xokdvium · 2026-01-30T00:11:22Z

src/libutil/include/nix/util/processes.hh

    pid_t pid = -1;
    bool separatePG = false;
    int killSignal = SIGKILL;
+    unsigned killTimeout = 0;


Maybe std::chrono::duration would be a better fit?

I used std::chrono::milliseconds to avoid templating the function and because all chrono literals are automatically convertible.

xokdvium

Also one trivial improvement I think we could make is to have the progress bar indicate that nix is shutting down is isInterrupted() is set. Since the bar is rendered by another thread that should be doable and would provide immediate feedback to users in such cases.

lisanna-dettwyler · 2026-01-30T17:07:34Z

Example with a misbehaving build hook getting killed: https://asciinema.org/a/GMoTcJkTYHLvRfH7

src/libutil/include/nix/util/processes.hh

xokdvium · 2026-01-31T12:30:32Z

src/libutil/include/nix/util/logging.hh

    actPostBuildHook = 110,
    actBuildWaiting = 111,
    actFetchTree = 112,
+    actShuttingDown = 113,


Hm, so you think there could be a way to avoid adding a new activity type to handle this? The progress bar thread could install a callback for interrupts with ReceiveCallbacks.

Using a static activity seems hacky to me too.

Yeah I didn't really like it either. I added a callback which just redraws the progress bar to display the shutdown message. https://asciinema.org/a/zpp6NYB15xtdjRDu

edolstra · 2026-02-04T20:50:01Z

src/libutil/unix/processes.cc

+    if (killTimeout > 0ms && killSignal != SIGKILL)
+        killThread = std::thread([&]() {
+            auto elapsed = 0ms;
+            while (elapsed < killTimeout) {


This will block the Worker::run() loop from making progress for up to killTimeout milliseconds, right? That's probably not too bad, but not ideal...

In an ideal world we could try doing async cleanup if we had better facilities for async things (think a task that either polls pidfd or hits a timeout). I think ASIO + Boost.process2 might have some helpers for this sort of thing

Only if these conditions are met:

The user interrupts nix

The user is using a nonstandard build hook (the regular build hook responds timely to SIGTERM)

The build hook is misbehaving during shutdown

This gives custom build hooks the chance to perform any needed cleanup, which is impossible when killed with SIGKILL. After 500ms, if the hook still hasn't exited, it will be forcibly killed with SIGKILL. Resolves NixOS#14760 Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>

Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>

lisanna-dettwyler requested review from Ericson2314 and edolstra as code owners January 28, 2026 08:41

github-actions bot added the new-cli Relating to the "nix" command label Jan 28, 2026

xokdvium reviewed Jan 28, 2026

View reviewed changes

lisanna-dettwyler force-pushed the build-hook-signals branch 2 times, most recently from a71d6b1 to f72e16a Compare January 28, 2026 21:33

xokdvium reviewed Jan 29, 2026

View reviewed changes

src/libutil/unix/processes.cc Outdated Show resolved Hide resolved

lisanna-dettwyler force-pushed the build-hook-signals branch from f72e16a to d8e4a9c Compare January 29, 2026 13:20

xokdvium reviewed Jan 29, 2026

View reviewed changes

xokdvium reviewed Jan 30, 2026

View reviewed changes

lisanna-dettwyler force-pushed the build-hook-signals branch from d8e4a9c to 1143030 Compare January 30, 2026 17:07

xokdvium reviewed Jan 31, 2026

View reviewed changes

lisanna-dettwyler force-pushed the build-hook-signals branch 4 times, most recently from 9be29a9 to 573ad90 Compare February 1, 2026 16:23

lisanna-dettwyler requested a review from xokdvium February 4, 2026 20:43

edolstra reviewed Feb 4, 2026

View reviewed changes

lisanna-dettwyler force-pushed the build-hook-signals branch from 573ad90 to 0a2b5e5 Compare February 8, 2026 15:05

lisanna-dettwyler added 2 commits February 8, 2026 10:06

Display shutdown message on interrupt

f25ec83

Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>

lisanna-dettwyler force-pushed the build-hook-signals branch from 0a2b5e5 to f25ec83 Compare February 8, 2026 15:07

		/* Give custom build hooks the chance to cleanup. */
		pid.setKillSignal(SIGTERM);

Uh oh!

Kill build hook with SIGTERM instead of SIGKILL #15105

Are you sure you want to change the base?

Kill build hook with SIGTERM instead of SIGKILL #15105

Conversation

lisanna-dettwyler commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xokdvium left a comment

Choose a reason for hiding this comment

Uh oh!

lisanna-dettwyler commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lisanna-dettwyler Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lisanna-dettwyler commented Jan 28, 2026 •

edited

Loading

lisanna-dettwyler commented Jan 30, 2026 •

edited

Loading

lisanna-dettwyler Feb 7, 2026 •

edited

Loading