Skip to content

Fold the build hook into libstore while keeping custom build-hook programs working - #16587

Open
ArkhamKnight25 wants to merge 6 commits into
NixOS:masterfrom
ArkhamKnight25:hook-fold
Open

ArkhamKnight25 wants to merge 6 commits into
NixOS:masterfrom
ArkhamKnight25:hook-fold

Conversation

@ArkhamKnight25

Copy link
Copy Markdown

Motivation

Remote builds go through the build hook, which picks a builder and runs the build there. Today the default hook is a second Nix process: the worker forks, execs nix __build-remote, and sends it every setting over a pipe so the new process can rebuild its configuration.

This moves the hook into libstore and runs the default hook in a forked child of the worker, without exec. The child opens its own store from the worker's StoreConfig and speaks the existing protocol over the existing pipes. Nix no longer has to find its own executable or dump its settings for the default case, and the hook code can be tested as a library. A custom build-hook program is still started with fork+exec, exactly as before.

Context

Part of my GSoC project on remote building, mentored by @lisanna-dettwyler.

Related: #15533 (future of the build-hook mechanism), #16504 (spawnProgram, merged), and #16579, which moves HookInstance onto spawnProgram and touches the same code (see the review guide).

Implementation strategy

Six commits:

  1. Move remote-builder selection into libstore. The slot-lock, ranking and upload-lock code of nix __build-remote is extracted into acquireMachineSlot, openUploadLock and their helpers in machine-selection.{hh,cc}. The existing program is switched to the new functions.

  2. Make the hook a library function. The body of nix __build-remote becomes serveBuildHook() in src/libstore/unix/build/build-remote.cc; the command is now a thin wrapper that reads the settings dump, opens a store and calls it. One behaviour change in the move: the first request is read before # decline-permanently is written. Before, the parent's write could fail with EPIPE and it would restart the hook.

  3. Run the default hook without exec. HookInstance::builtin() forks with startProcess and calls serveBuildHook() in the child. The child closes the inherited descriptors except SQLite's (its own connection reuses the parent's per-process SQLite state, which refers to them by number), keeps the saved mount-namespace descriptors that a root client needs to start ssh, switches to the JSON logger and opens its own store. Where the path behind a descriptor cannot be looked up (anything other than Linux and macOS), the default runs nix __build-remote found on PATH (before, the absolute path of the running binary). Custom hooks go through HookInstance::external(), which is the old constructor unchanged.

    Choosing between the two uses a new Setting::isDefault(). isOverridden() cannot be used: it is reset after the system nix.conf is read, so a hook configured there would be taken for the default. I also removed the code in main.cc that replaced the default with the absolute path of the running binary; it made every installation look as if it had a custom hook. As a side effect, nix config show build-hook now prints nix __build-remote.

  4. Signals in the built-in hook. The child skips initNix, so it starts the signal-handler thread itself. This changes behaviour: the old program reset SIGTERM to its default and died on it, while the built-in hook treats SIGTERM as an interrupt and unwinds at the next interrupt check; a hook blocked inside ssh is still killed by the parent after build-hook-kill-timeout. The same thread restores the SIGPIPE blocking that the old program had through initNix and that the fork loses in commonChildInit, so a dropped ssh connection does not kill the hook silently.

  5. No fork without builders. If builders resolves to no machine (including the default @/etc/nix/machines when the file does not exist), the parent declines on the spot instead of forking and opening a second database connection.

  6. Tripwire for the SQLite WAL index. If fd 4 or 5 is the store's db.sqlite-shm, the child fails instead of dup2ing over it. Not reachable as long as descriptors 0–2 are open when the store is opened (the big lock, the database and its WAL come first, so -shm lands at 6 or above); it turns a silent hang into an error.

Alternatives

  1. spawnProgram. Keeps the self-exec and the settings dump, but starts a clean process and avoids forking while SQLite connections and other threads are live. This is what libstore/unix: Use spawnProgram for the build hook  #16579 does for HookInstance.
  2. Close every inherited descriptor. The child's new SQLite connection then fails with SQLITE_PROTOCOL and retries forever; the hook hangs. That is why the SQLite descriptors are kept.
  3. Fall back to exec when fd 4 or 5 is unsafe. I chose to fail loudly instead, since with descriptors 0–2 open the case cannot occur.
  4. A thread instead of a process. The hook changes process-global state (settings, logger, working directory, environment, SIGALRM and alarm()) and blocks in ssh for minutes. A process keeps that away from the worker and can be killed on its own.

Open questions

  • Fork safety. The child never touches the parent's SQLite connection objects, but if another thread holds a SQLite mutex at the moment of the fork, the child deadlocks and the parent waits forever in tryBuildHook. I could not reproduce it, and tests cannot rule it out. Is this acceptable? Commits 1 and 2 stand on their own, and serveBuildHook() can be started through spawnProgram instead if the fork is not wanted.
  • SIGTERM. Unwinding instead of dying immediately: is that the behaviour we want?

User-visible changes

  • nix config show build-hook prints nix __build-remote instead of an absolute path.
  • The default hook runs in a fork of the worker and shows up in ps under the worker's command line.
  • Setting build-hook = nix __build-remote by hand selects the same in-process hook (on Linux and macOS; elsewhere both run the program).

Review guide

Commits 1 and 2 are code movement; diff them as such. Commits 3 to 6 are the new logic: child setup in HookInstance::builtin(), descriptor handling, signals, and the dispatch in tryBuildHook. #16579 renames child.hh to child-impl.hh, which this branch includes; whichever lands second rebases.


Add 👍 to pull requests you find important.

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

@github-actions github-actions Bot added the new-cli Relating to the "nix" command label Oct 10, 2026
Comment on lines +229 to +233
/* Our SQLite connection reuses the parent's WAL index descriptor;
the dup2()s below must not overwrite it. */
for (int fd : {4, 5})
if (fdIsStoreWALIndex(fd))
throw Error("descriptor %d is the store's SQLite WAL index; cannot fork the build hook", fd);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh this is the reason why this stuff should be in a separate executable. Let's not do more fork-without-exec.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to re-exec ourselves though. Several ways:

    1. Dynamically linked - just execute a binary from libexec that only links to libstore
    1. For full static builds on linux do memfd + fexecve

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-cli Relating to the "nix" command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants