From 374f8e79a195bbcf606b0c3452f7e7de67b68150 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 11:58:28 -0400
Subject: [PATCH 1/9] `DerivationBuilderImpl::unprepareBuild` Just throw error

Aftet the previous simplifications, there is no reason to catch the
error and immediately return it with a `std::variant` --- just let the
caller catch it instead.
---
 .../build/derivation-building-goal.cc         | 22 +++---
 .../nix/store/build/derivation-builder.hh     |  4 +-
 src/libstore/unix/build/derivation-builder.cc | 67 +++++++++----------
 .../unix/build/linux-derivation-builder.cc    |  2 +-
 4 files changed, 45 insertions(+), 50 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index c9b562817..d2752dfb5 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -804,21 +804,22 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
 
     trace("build done");
 
-    auto res = builder->unprepareBuild();
-    // N.B. cannot use `std::visit` with co-routine return
-    if (auto * ste = std::get_if<0>(&res)) {
+    SingleDrvOutputs builtOutputs;
+    try {
+        builtOutputs = builder->unprepareBuild();
+    } catch (BuildError & e) {
         outputLocks.unlock();
 // Allow selecting a subset of enum values
 #  pragma GCC diagnostic push
 #  pragma GCC diagnostic ignored "-Wswitch-enum"
-        switch (ste->status) {
+        switch (e.status) {
         case BuildResult::HashMismatch:
             worker.hashMismatch = true;
             /* See header, the protocols don't know about `HashMismatch`
                yet, so change it to `OutputRejected`, which they expect
                for this case (hash mismatch is a type of output
                rejection). */
-            ste->status = BuildResult::OutputRejected;
+            e.status = BuildResult::OutputRejected;
             break;
         case BuildResult::NotDeterministic:
             worker.checkMismatch = true;
@@ -828,10 +829,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
             break;
         }
 #  pragma GCC diagnostic pop
-        co_return doneFailure(std::move(*ste));
-    } else if (auto * builtOutputs = std::get_if<1>(&res)) {
+        co_return doneFailure(std::move(e));
+    }
+    {
         StorePathSet outputPaths;
-        for (auto & [_, output] : *builtOutputs)
+        for (auto & [_, output] : builtOutputs)
             outputPaths.insert(output.outPath);
         runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
 
@@ -841,9 +843,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
            (unlinked) lock files. */
         outputLocks.setDeletion(true);
         outputLocks.unlock();
-        co_return doneSuccess(BuildResult::Built, std::move(*builtOutputs));
-    } else {
-        unreachable();
+        co_return doneSuccess(BuildResult::Built, std::move(builtOutputs));
     }
 #endif
 }
diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index fd487c5fe..08708ec05 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -187,8 +187,10 @@ struct DerivationBuilder : RestrictionContext
      * processing. A status code and exception are returned, providing
      * more information. The second case indicates success, and
      * realisations for each output of the derivation are returned.
+     *
+     * @throws BuildError
      */
-    virtual std::variant<BuildError, SingleDrvOutputs> unprepareBuild() = 0;
+    virtual SingleDrvOutputs unprepareBuild() = 0;
 
     /**
      * Stop the in-process nix daemon thread.
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index c9b603db9..6a5b6934e 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -191,7 +191,7 @@ public:
 
     void startBuilder() override;
 
-    std::variant<BuildError, SingleDrvOutputs> unprepareBuild() override;
+    SingleDrvOutputs unprepareBuild() override;
 
 protected:
 
@@ -426,7 +426,7 @@ bool DerivationBuilderImpl::prepareBuild()
     return true;
 }
 
-std::variant<BuildError, SingleDrvOutputs> DerivationBuilderImpl::unprepareBuild()
+SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
 {
     // FIXME: get rid of this, rely on RAII.
     Finally releaseBuildUser([&]() {
@@ -477,49 +477,42 @@ std::variant<BuildError, SingleDrvOutputs> DerivationBuilderImpl::unprepareBuild
 
     bool diskFull = false;
 
-    try {
+    /* Check the exit status. */
+    if (!statusOk(status)) {
 
-        /* Check the exit status. */
-        if (!statusOk(status)) {
+        diskFull |= decideWhetherDiskFull();
 
-            diskFull |= decideWhetherDiskFull();
+        cleanupBuild();
 
-            cleanupBuild();
+        auto msg =
+            fmt("Cannot build '%s'.\n"
+                "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
+                Magenta(store.printStorePath(drvPath)),
+                statusToString(status));
 
-            auto msg =
-                fmt("Cannot build '%s'.\n"
-                    "Reason: " ANSI_RED "builder %s" ANSI_NORMAL ".",
-                    Magenta(store.printStorePath(drvPath)),
-                    statusToString(status));
+        msg += showKnownOutputs(store, drv);
 
-            msg += showKnownOutputs(store, drv);
+        miscMethods->appendLogTailErrorMsg(msg);
 
-            miscMethods->appendLogTailErrorMsg(msg);
+        if (diskFull)
+            msg += "\nnote: build failure may have been caused by lack of free disk space";
 
-            if (diskFull)
-                msg += "\nnote: build failure may have been caused by lack of free disk space";
-
-            throw BuildError(
-                !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure
-                                                          : BuildResult::PermanentFailure,
-                msg);
-        }
-
-        /* Compute the FS closure of the outputs and register them as
-           being valid. */
-        auto builtOutputs = registerOutputs();
-
-        /* Delete unused redirected outputs (when doing hash rewriting). */
-        for (auto & i : redirectedOutputs)
-            deletePath(store.Store::toRealPath(i.second));
-
-        deleteTmpDir(true);
-
-        return std::move(builtOutputs);
-
-    } catch (BuildError & e) {
-        return std::move(e);
+        throw BuildError(
+            !derivationType.isSandboxed() || diskFull ? BuildResult::TransientFailure : BuildResult::PermanentFailure,
+            msg);
     }
+
+    /* Compute the FS closure of the outputs and register them as
+       being valid. */
+    auto builtOutputs = registerOutputs();
+
+    /* Delete unused redirected outputs (when doing hash rewriting). */
+    for (auto & i : redirectedOutputs)
+        deletePath(store.Store::toRealPath(i.second));
+
+    deleteTmpDir(true);
+
+    return builtOutputs;
 }
 
 void DerivationBuilderImpl::cleanupBuild()
diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc
index 39b8f09ae..d474c001e 100644
--- a/src/libstore/unix/build/linux-derivation-builder.cc
+++ b/src/libstore/unix/build/linux-derivation-builder.cc
@@ -659,7 +659,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
             throw SysError("setuid failed");
     }
 
-    std::variant<BuildError, SingleDrvOutputs> unprepareBuild() override
+    SingleDrvOutputs unprepareBuild() override
     {
         sandboxMountNamespace = -1;
         sandboxUserNamespace = -1;

From 8dd289099c787440c0eb9eeac550a199801f57ae Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 12:06:06 -0400
Subject: [PATCH 2/9] Simplify `DerivationGoal::unprepareBuild::diskFull`

We only need it defined in the narrower scope
---
 src/libstore/unix/build/derivation-builder.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index 6a5b6934e..daaf0b964 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -475,12 +475,10 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
             ((double) buildResult.cpuSystem->count()) / 1000000);
     }
 
-    bool diskFull = false;
-
     /* Check the exit status. */
     if (!statusOk(status)) {
 
-        diskFull |= decideWhetherDiskFull();
+        bool diskFull = decideWhetherDiskFull();
 
         cleanupBuild();
 

From 4db6bf96b77d5027526f487bbda9966518d69187 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 12:18:40 -0400
Subject: [PATCH 3/9] Give `DerivationBuilderImpl::cleanupBuild` bool arg

Do this to match `DerivationBuilder::deleteTmpDir`, which we'll want to
combine it with next.

Also chenge one caller from `deleteTmpDir(true)` to `cleanupBuild(true)`
now that this is done, because it will not make a difference.

This should be a pure refactor with no behavioral change.
---
 src/libstore/unix/build/chroot-derivation-builder.cc |  7 +++++--
 src/libstore/unix/build/derivation-builder.cc        | 10 +++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/libstore/unix/build/chroot-derivation-builder.cc b/src/libstore/unix/build/chroot-derivation-builder.cc
index 887bb47f0..20a4bd6bf 100644
--- a/src/libstore/unix/build/chroot-derivation-builder.cc
+++ b/src/libstore/unix/build/chroot-derivation-builder.cc
@@ -166,9 +166,12 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
         return !needsHashRewrite() ? chrootRootDir + p : store.toRealPath(p);
     }
 
-    void cleanupBuild() override
+    void cleanupBuild(bool force) override
     {
-        DerivationBuilderImpl::cleanupBuild();
+        DerivationBuilderImpl::cleanupBuild(force);
+
+        if (force)
+            return;
 
         /* Move paths out of the chroot for easier debugging of
            build failures. */
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index daaf0b964..bd6cac522 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -350,7 +350,7 @@ public:
 
 protected:
 
-    virtual void cleanupBuild();
+    virtual void cleanupBuild(bool force);
 
 private:
 
@@ -480,7 +480,7 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
 
         bool diskFull = decideWhetherDiskFull();
 
-        cleanupBuild();
+        cleanupBuild(false);
 
         auto msg =
             fmt("Cannot build '%s'.\n"
@@ -508,14 +508,14 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
     for (auto & i : redirectedOutputs)
         deletePath(store.Store::toRealPath(i.second));
 
-    deleteTmpDir(true);
+    cleanupBuild(true);
 
     return builtOutputs;
 }
 
-void DerivationBuilderImpl::cleanupBuild()
+void DerivationBuilderImpl::cleanupBuild(bool force)
 {
-    deleteTmpDir(false);
+    deleteTmpDir(force);
 }
 
 static void chmod_(const Path & path, mode_t mode)

From 557bbe969e2d3caa71ac4c2c3075371b5df0c7de Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 12:26:29 -0400
Subject: [PATCH 4/9] Combine `cleanupBuild` and `deleteTmpDir`

It's hard to tell if I changed any behavior, but if I did, I think I
made it better, because now we explicitly move stuff out of the chroot
(if we were going to) before trying to delete the chroot.
---
 src/libstore/build/derivation-building-goal.cc     |  2 +-
 .../include/nix/store/build/derivation-builder.hh  |  5 ++++-
 .../unix/build/chroot-derivation-builder.cc        | 14 +++-----------
 src/libstore/unix/build/derivation-builder.cc      | 13 ++-----------
 4 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index d2752dfb5..24244ebd4 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -66,7 +66,7 @@ DerivationBuildingGoal::~DerivationBuildingGoal()
             ignoreExceptionInDestructor();
         }
         try {
-            builder->deleteTmpDir(false);
+            builder->cleanupBuild(false);
         } catch (...) {
             ignoreExceptionInDestructor();
         }
diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index 08708ec05..512d001e0 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -200,8 +200,11 @@ struct DerivationBuilder : RestrictionContext
 
     /**
      * Delete the temporary directory, if we have one.
+     *
+     * @param force We know the build suceeded, so don't attempt to
+     * preseve anything for debugging.
      */
-    virtual void deleteTmpDir(bool force) = 0;
+    virtual void cleanupBuild(bool force) = 0;
 
     /**
      * Kill any processes running under the build user UID or in the
diff --git a/src/libstore/unix/build/chroot-derivation-builder.cc b/src/libstore/unix/build/chroot-derivation-builder.cc
index 20a4bd6bf..8c9359533 100644
--- a/src/libstore/unix/build/chroot-derivation-builder.cc
+++ b/src/libstore/unix/build/chroot-derivation-builder.cc
@@ -22,13 +22,6 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
 
     PathsInChroot pathsInChroot;
 
-    void deleteTmpDir(bool force) override
-    {
-        autoDelChroot.reset(); /* this runs the destructor */
-
-        DerivationBuilderImpl::deleteTmpDir(force);
-    }
-
     bool needsHashRewrite() override
     {
         return false;
@@ -170,12 +163,9 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
     {
         DerivationBuilderImpl::cleanupBuild(force);
 
-        if (force)
-            return;
-
         /* Move paths out of the chroot for easier debugging of
            build failures. */
-        if (buildMode == bmNormal)
+        if (!force && buildMode == bmNormal)
             for (auto & [_, status] : initialOutputs) {
                 if (!status.known)
                     continue;
@@ -185,6 +175,8 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl
                 if (pathExists(chrootRootDir + p))
                     std::filesystem::rename((chrootRootDir + p), p);
             }
+
+        autoDelChroot.reset(); /* this runs the destructor */
     }
 
     std::pair<Path, Path> addDependencyPrep(const StorePath & path)
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index bd6cac522..241d98ace 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -344,14 +344,10 @@ private:
 
 public:
 
-    void deleteTmpDir(bool force) override;
+    void cleanupBuild(bool force) override;
 
     void killSandbox(bool getStats) override;
 
-protected:
-
-    virtual void cleanupBuild(bool force);
-
 private:
 
     bool decideWhetherDiskFull();
@@ -513,11 +509,6 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild()
     return builtOutputs;
 }
 
-void DerivationBuilderImpl::cleanupBuild(bool force)
-{
-    deleteTmpDir(force);
-}
-
 static void chmod_(const Path & path, mode_t mode)
 {
     if (chmod(path.c_str(), mode) == -1)
@@ -1821,7 +1812,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
     return builtOutputs;
 }
 
-void DerivationBuilderImpl::deleteTmpDir(bool force)
+void DerivationBuilderImpl::cleanupBuild(bool force)
 {
     if (topTmpDir != "") {
         /* As an extra precaution, even in the event of `deletePath` failing to

From 49da508f46a1a90b04faed88a7d865976ae7c6fb Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 12:40:55 -0400
Subject: [PATCH 5/9] Write a destructor for `DerivationBuilderImpl`

This allows `DerivationBuildingGoal` to know less.
---
 .../build/derivation-building-goal.cc         | 14 ++------
 .../nix/store/build/derivation-builder.hh     | 14 --------
 src/libstore/unix/build/derivation-builder.cc | 36 ++++++++++++++++---
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index 24244ebd4..61f726bf9 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -59,18 +59,8 @@ DerivationBuildingGoal::~DerivationBuildingGoal()
         ignoreExceptionInDestructor();
     }
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
-    if (builder) {
-        try {
-            builder->stopDaemon();
-        } catch (...) {
-            ignoreExceptionInDestructor();
-        }
-        try {
-            builder->cleanupBuild(false);
-        } catch (...) {
-            ignoreExceptionInDestructor();
-        }
-    }
+    if (builder)
+        builder.reset();
 #endif
     try {
         closeLogFile();
diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index 512d001e0..65d044a79 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -192,20 +192,6 @@ struct DerivationBuilder : RestrictionContext
      */
     virtual SingleDrvOutputs unprepareBuild() = 0;
 
-    /**
-     * Stop the in-process nix daemon thread.
-     * @see startDaemon
-     */
-    virtual void stopDaemon() = 0;
-
-    /**
-     * Delete the temporary directory, if we have one.
-     *
-     * @param force We know the build suceeded, so don't attempt to
-     * preseve anything for debugging.
-     */
-    virtual void cleanupBuild(bool force) = 0;
-
     /**
      * Kill any processes running under the build user UID or in the
      * cgroup of the build.
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index 241d98ace..4678dae42 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -85,6 +85,22 @@ public:
     {
     }
 
+    ~DerivationBuilderImpl()
+    {
+        /* Careful: we should never ever throw an exception from a
+           destructor. */
+        try {
+            stopDaemon();
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
+        try {
+            cleanupBuild(false);
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
+    }
+
 protected:
 
     /**
@@ -285,9 +301,11 @@ private:
      */
     void startDaemon();
 
-public:
-
-    void stopDaemon() override;
+    /**
+     * Stop the in-process nix daemon thread.
+     * @see startDaemon
+     */
+    void stopDaemon();
 
 protected:
 
@@ -342,9 +360,17 @@ private:
      */
     SingleDrvOutputs registerOutputs();
 
-public:
+protected:
 
-    void cleanupBuild(bool force) override;
+    /**
+     * Delete the temporary directory, if we have one.
+     *
+     * @param force We know the build suceeded, so don't attempt to
+     * preseve anything for debugging.
+     */
+    virtual void cleanupBuild(bool force);
+
+public:
 
     void killSandbox(bool getStats) override;
 

From 4388e3dcb588ef960c92128040242c80bfb10361 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 13:43:26 -0400
Subject: [PATCH 6/9] Create `DerivationBuilder::killChild`

Then the derivation building goal doesn't need to snoop around as much.
---
 .../build/derivation-building-goal.cc         | 16 +-----------
 .../nix/store/build/derivation-builder.hh     |  8 +++---
 src/libstore/unix/build/derivation-builder.cc | 26 ++++++++++++++++++-
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index 61f726bf9..5af385aed 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -84,22 +84,8 @@ void DerivationBuildingGoal::killChild()
     hook.reset();
 #endif
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
-    if (builder && builder->pid != -1) {
+    if (builder && builder->killChild())
         worker.childTerminated(this);
-
-        // FIXME: move this into DerivationBuilder.
-
-        /* If we're using a build user, then there is a tricky race
-           condition: if we kill the build user before the child has
-           done its setuid() to the build user uid, then it won't be
-           killed, and we'll potentially lock up in pid.wait().  So
-           also send a conventional kill to the child. */
-        ::kill(-builder->pid, SIGKILL); /* ignore the result */
-
-        builder->killSandbox(true);
-
-        builder->pid.wait();
-    }
 #endif
 }
 
diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index 65d044a79..3e8903e8a 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -193,10 +193,12 @@ struct DerivationBuilder : RestrictionContext
     virtual SingleDrvOutputs unprepareBuild() = 0;
 
     /**
-     * Kill any processes running under the build user UID or in the
-     * cgroup of the build.
+     * Forcibly kill the child process, if any.
+     *
+     * @returns whether the child was still alive and needed to be
+     * killed.
      */
-    virtual void killSandbox(bool getStats) = 0;
+    virtual bool killChild() = 0;
 };
 
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index 4678dae42..dff7d0eaa 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -370,9 +370,15 @@ protected:
      */
     virtual void cleanupBuild(bool force);
 
+    /**
+     * Kill any processes running under the build user UID or in the
+     * cgroup of the build.
+     */
+    virtual void killSandbox(bool getStats);
+
 public:
 
-    void killSandbox(bool getStats) override;
+    bool killChild() override;
 
 private:
 
@@ -435,6 +441,24 @@ void DerivationBuilderImpl::killSandbox(bool getStats)
     }
 }
 
+bool DerivationBuilderImpl::killChild()
+{
+    bool ret = pid != -1;
+    if (ret) {
+        /* If we're using a build user, then there is a tricky race
+           condition: if we kill the build user before the child has
+           done its setuid() to the build user uid, then it won't be
+           killed, and we'll potentially lock up in pid.wait().  So
+           also send a conventional kill to the child. */
+        ::kill(-pid, SIGKILL); /* ignore the result */
+
+        killSandbox(true);
+
+        pid.wait();
+    }
+    return ret;
+}
+
 bool DerivationBuilderImpl::prepareBuild()
 {
     if (useBuildUsers()) {

From c632c823cee268c3efdd5251375434c976827370 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 13:44:39 -0400
Subject: [PATCH 7/9] Take `DerivationBuilder::pid` private

---
 src/libstore/include/nix/store/build/derivation-builder.hh | 5 -----
 src/libstore/unix/build/derivation-builder.cc              | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index 3e8903e8a..d7f2058d1 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -150,11 +150,6 @@ struct DerivationBuilderCallbacks
  */
 struct DerivationBuilder : RestrictionContext
 {
-    /**
-     * The process ID of the builder.
-     */
-    Pid pid;
-
     DerivationBuilder() = default;
     virtual ~DerivationBuilder() = default;
 
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index dff7d0eaa..b11eb383d 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -70,6 +70,11 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder
 {
 protected:
 
+    /**
+     * The process ID of the builder.
+     */
+    Pid pid;
+
     LocalStore & store;
 
     std::unique_ptr<DerivationBuilderCallbacks> miscMethods;

From bde745cb3f8dab7a29fbc2c87eb599ff31384ab5 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 13:57:38 -0400
Subject: [PATCH 8/9] Move `killChild` call from `~DerivationBuildingGoal` to
 `~DerivationBuilder`

Sadly we cannot unexpose `DerivationBuilder::killChild` yet, because
`DerivationBuildingGoal` calls it elsewhere, but we can at least haave a
better division of labor between the two destructors.
---
 src/libstore/build/derivation-building-goal.cc | 5 -----
 src/libstore/unix/build/derivation-builder.cc  | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index 5af385aed..20a67008c 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -53,11 +53,6 @@ DerivationBuildingGoal::~DerivationBuildingGoal()
 {
     /* Careful: we should never ever throw an exception from a
        destructor. */
-    try {
-        killChild();
-    } catch (...) {
-        ignoreExceptionInDestructor();
-    }
 #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows
     if (builder)
         builder.reset();
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index b11eb383d..bc48d4256 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -94,6 +94,11 @@ public:
     {
         /* Careful: we should never ever throw an exception from a
            destructor. */
+        try {
+            killChild();
+        } catch (...) {
+            ignoreExceptionInDestructor();
+        }
         try {
             stopDaemon();
         } catch (...) {

From 3e0b1705c13ab612ca1e4f524619d12a9733eeff Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 28 Aug 2025 14:54:11 -0400
Subject: [PATCH 9/9] Move `markContentsGood` to after `DerivationBuilder`
 finishes

I think this should be fine for repairing. If anything, it is better,
because it would be weird to "mark and output good" only for it to then
fail output checks.
---
 src/libstore/build/derivation-building-goal.cc         | 10 ++++------
 .../include/nix/store/build/derivation-builder.hh      |  2 --
 src/libstore/unix/build/derivation-builder.cc          |  1 -
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc
index 20a67008c..b1920cadb 100644
--- a/src/libstore/build/derivation-building-goal.cc
+++ b/src/libstore/build/derivation-building-goal.cc
@@ -623,11 +623,6 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
                     goal.worker.childTerminated(&goal);
                 }
 
-                void markContentsGood(const StorePath & path) override
-                {
-                    goal.worker.markContentsGood(path);
-                }
-
                 Path openLogFile() override
                 {
                     return goal.openLogFile();
@@ -804,8 +799,11 @@ Goal::Co DerivationBuildingGoal::tryToBuild()
     }
     {
         StorePathSet outputPaths;
-        for (auto & [_, output] : builtOutputs)
+        for (auto & [_, output] : builtOutputs) {
+            // for sake of `bmRepair`
+            worker.markContentsGood(output.outPath);
             outputPaths.insert(output.outPath);
+        }
         runPostBuildHook(worker.store, *logger, drvPath, outputPaths);
 
         /* It is now safe to delete the lock files, since all future
diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh
index d7f2058d1..a373c4729 100644
--- a/src/libstore/include/nix/store/build/derivation-builder.hh
+++ b/src/libstore/include/nix/store/build/derivation-builder.hh
@@ -133,8 +133,6 @@ struct DerivationBuilderCallbacks
      * @todo this should be reworked
      */
     virtual void childTerminated() = 0;
-
-    virtual void markContentsGood(const StorePath & path) = 0;
 };
 
 /**
diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc
index bc48d4256..bf99c4c1a 100644
--- a/src/libstore/unix/build/derivation-builder.cc
+++ b/src/libstore/unix/build/derivation-builder.cc
@@ -1804,7 +1804,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
             }
 
             store.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences()
-            miscMethods->markContentsGood(newInfo.path);
 
             newInfo.deriver = drvPath;
             newInfo.ultimate = true;