Skip to content

Fix destruction of DerivationBuilder implementations#15072

Merged
Ericson2314 merged 1 commit into
masterfrom
fix-interrupted-linux-derivation-builder
Jan 24, 2026
Merged

Fix destruction of DerivationBuilder implementations#15072
Ericson2314 merged 1 commit into
masterfrom
fix-interrupted-linux-derivation-builder

Conversation

@xokdvium
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium commented Jan 24, 2026

Motivation

This unsures that we call the correct virtual functions when destroying a particular
DerivationBuilder.

Usually the order of destructors is in the reverse order of inheritance:

ChrootLinuxDerivationBuilder -> ChrootDerivationBuilder -> DerivationBuilderImpl

autoDelChroot was being destroyed before the DerivationBuilderImpl::killChild was run and it would fail to clean up the chroot directory, since there were still processes writing to it. Note that ChrootLinuxDerivationBuilder::killSandbox was never run in the interrupted case at all, since virtual functions in destructors do not call derived class methods.

I could reproduce the issue with the following derivation:

let
  pkgs = import <nixpkgs> { };
in
pkgs.runCommand "chroot-cleanup-race" { } ''
  mkdir -p $out

  for i in $(seq 1 200); do
    (
      mkfifo $out/fifo$i
      cat $out/fifo$i > /dev/null &

      while true; do
        : > $out/file$i
      done
    ) &
  done

  sleep 0.05
  echo done > $out/main
''

While interrupting it manually when it would hang.

Wrapping the unique pointer in a custom deleter function we can run all of the necessary clean up code consistently and calling the right virtual functions. Ideally we'd have a lint that bans the usage of virtual functions in destructors completely.

Context


Add 👍 to pull requests you find important.

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

@xokdvium xokdvium requested a review from Ericson2314 as a code owner January 24, 2026 20:22
@xokdvium xokdvium force-pushed the fix-interrupted-linux-derivation-builder branch from 296b203 to 533af39 Compare January 24, 2026 20:24
This unsures that we call the correct virtual functions when destroying a particular
DerivationBuilder.

Usually the order of destructors is in the reverse order of inheritance:

ChrootLinuxDerivationBuilder -> ChrootDerivationBuilder -> DerivationBuilderImpl

autoDelChroot was being destroyed before the DerivationBuilderImpl::killChild was
run and it would fail to clean up the chroot directory, since there were still processes
writing to it. Note that ChrootLinuxDerivationBuilder::killSandbox was never run in
the interrupted case at all, since virtual functions in destructors do not call derived class
methods.

I could reproduce the issue with the following derivation:

let
  pkgs = import <nixpkgs> { };
in
pkgs.runCommand "chroot-cleanup-race" { } ''
  mkdir -p $out

  for i in $(seq 1 200); do
    (
      mkfifo $out/fifo$i
      cat $out/fifo$i > /dev/null &

      while true; do
        : > $out/file$i
      done
    ) &
  done

  sleep 0.05
  echo done > $out/main
''

While interrupting it manually when it would hang.

Wrapping the unique pointer in a custom deleter function we can run all
of the necessary clean up code consistently and calling the right virtual
functions. Ideally we'd have a lint that bans the usage of virtual functions
in destructors completely.
@xokdvium xokdvium force-pushed the fix-interrupted-linux-derivation-builder branch from 533af39 to b752c5c Compare January 24, 2026 20:31
@xokdvium xokdvium changed the title ChrootLinuxDerivationBuilder: Override destructor to call killChild Fix destruction of DerivationBuilder implementations Jan 24, 2026
@Ericson2314 Ericson2314 added this pull request to the merge queue Jan 24, 2026
Merged via the queue into master with commit 943c18f Jan 24, 2026
20 checks passed
@Ericson2314 Ericson2314 deleted the fix-interrupted-linux-derivation-builder branch January 24, 2026 21:58
@xokdvium xokdvium added the backport 2.33-maintenance Automatically creates a PR against the branch label Jan 24, 2026
@internal-nix-ci
Copy link
Copy Markdown

Backport failed for 2.33-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.33-maintenance
git worktree add -d .worktree/backport-15072-to-2.33-maintenance origin/2.33-maintenance
cd .worktree/backport-15072-to-2.33-maintenance
git switch --create backport-15072-to-2.33-maintenance
git cherry-pick -x b752c5cb64c2675dc51aef6eb6b97d16a2a477e4

philiptaron added a commit to philiptaron/nixpkgs that referenced this pull request Feb 7, 2026
## Bug fixes

- Fix destruction of DerivationBuilder implementations (NixOS/nix#15072)
- Don't report cancelled goals as failures (NixOS/nix#14972)
- Fix `linux` build on fresh `glibc` and `gcc` (NixOS/nix#15011)

## S3 binary cache improvements

- Add AWS SSO support for S3 authentication (NixOS/nix#14645)
- Respect `AWS_PROFILE` environment variable (NixOS/nix#14645)
- Add STS support for default profile (NixOS/nix#14645)
- Skip `Accept-Encoding` header for S3 SigV4 requests (NixOS/nix#15048)
- Restart source before upload retries (NixOS/nix#15047)
- Route AWS CRT logs through Nix logger (NixOS/nix#15059)

The glibc 2.42 build fix patch is dropped as it is now included upstream.

https://github.com/NixOS/nix/releases/tag/2.33.2
brittonr pushed a commit to brittonr/nix that referenced this pull request Apr 1, 2026
…ation-builder

Fix destruction of DerivationBuilder implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.33-maintenance Automatically creates a PR against the branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants