diff --git a/lix/libstore/platform/linux.cc b/lix/libstore/platform/linux.cc index 229136a93..f8b721475 100644 --- a/lix/libstore/platform/linux.cc +++ b/lix/libstore/platform/linux.cc @@ -1007,9 +1007,33 @@ Pid LinuxLocalDerivationGoal::startChild(std::function openSlave) return pid; } +void LinuxLocalDerivationGoal::cleanupHookFinally() +{ + /* This hook is used to release the build users + * and release the lock on this UID. + * + * So we need to ensure that our cgroup business + * is already done before releasing it, + * otherwise, another build may grab the UID + * and start a cgroup with it, resulting + * in a confusing set of errors. + * + * Statistics are stored inside the cgroup + * object so that `killSandbox` can retrieve + * them later. + */ + if (context.cgroup) { + context.cgroup->destroy(); + } + + LocalDerivationGoal::cleanupHookFinally(); +} + void LinuxLocalDerivationGoal::killSandbox(bool getStats) { if (context.cgroup) { + /* This might have already been killed + * by the clean-up hook above. */ context.cgroup->kill(); if (getStats) { auto stats = context.cgroup->getStatistics(); diff --git a/lix/libstore/platform/linux.hh b/lix/libstore/platform/linux.hh index 38c8b6d3a..9dba7f1de 100644 --- a/lix/libstore/platform/linux.hh +++ b/lix/libstore/platform/linux.hh @@ -34,6 +34,13 @@ public: using LocalDerivationGoal::LocalDerivationGoal; private: + /* + * Destroy the cgroup otherwise another build + * may grab the current UID which is used in the cgroup name + * and then mess with a cgroup we might be reading statistics from. + */ + void cleanupHookFinally() override; + /** * Create and populate chroot */ diff --git a/lix/libutil/cgroup.cc b/lix/libutil/cgroup.cc index f05dd6a1c..5ecc96092 100644 --- a/lix/libutil/cgroup.cc +++ b/lix/libutil/cgroup.cc @@ -291,25 +291,32 @@ AutoDestroyCgroup::AutoDestroyCgroup( delegation_ = {.uid = uid, .gid = gid}; } +void AutoDestroyCgroup::destroy() +{ + std::visit( + overloaded{ + [&, this](const Path & aliveCgroup) { + auto maybeStats = destroyCgroup(name_, aliveCgroup); + if (!maybeStats) { + warn( + "cgroup '%s' was destroyed unexpectedly (something else removed the " + "cgroup).", + aliveCgroup + ); + } else { + cgroup_ = *maybeStats; + } + }, + [&](const CgroupStats & stats) {} + }, + cgroup_ + ); +} + AutoDestroyCgroup::~AutoDestroyCgroup() { try { - std::visit( - overloaded{ - [&, this](const Path & aliveCgroup) { - auto maybeStats = destroyCgroup(name_, aliveCgroup); - if (!maybeStats) { - warn( - "cgroup '%s' was destroyed unexpectedly (something else removed the " - "cgroup).", - aliveCgroup - ); - } - }, - [&](const CgroupStats & stats) {} - }, - cgroup_ - ); + destroy(); } catch (...) { ignoreExceptionInDestructor(); } @@ -347,7 +354,10 @@ void AutoDestroyCgroup::kill() { auto path = std::get_if(&cgroup_); if (!path) { - throw SysError("killing cgroup '%s' but it went away", name_); + /* If the cgroup already disappeared, + * processes already got killed. + */ + return; } killCgroup(name_, *path); diff --git a/lix/libutil/cgroup.hh b/lix/libutil/cgroup.hh index 89d6bc916..4833017a9 100644 --- a/lix/libutil/cgroup.hh +++ b/lix/libutil/cgroup.hh @@ -141,9 +141,6 @@ private: */ AutoDelete stateRecord; - /* Kill all processes under its hierarchy and tear down the cgroup */ - void destroy(); - /* * Cleanse all previous instances of this cgroup where the deletion process * might have been interrupted and record ourself in stead. @@ -167,6 +164,9 @@ public: ); ~AutoDestroyCgroup(); + /* Kill all processes under its hierarchy and tear down the cgroup */ + void destroy(); + std::optional path() const { return std::visit(