diff --git a/doc/manual/rl-next/aggressive-derivation-output-cleanups.md b/doc/manual/rl-next/aggressive-derivation-output-cleanups.md new file mode 100644 index 000000000..786a0332b --- /dev/null +++ b/doc/manual/rl-next/aggressive-derivation-output-cleanups.md @@ -0,0 +1,15 @@ +--- +synopsis: "Always clean up scratch paths after derivations failed to build" +issues: [] +cls: [3420] +category: "Fixes" +credits: ["raito", "horrors"] +--- + +Previously, scratch paths created during builds were not always cleaned up if +the derivation failed, potentially leaving behind unnecessary temporary files +or directories in the Nix store. + +This fix ensures that such paths are consistently removed after a failed build, +improving Nix store hygiene, hardening Lix against mis-reuse of failed builds +scratch paths. diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 64075852f..cfcda1311 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -363,9 +363,13 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck() void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() { - /* Delete unused redirected outputs (when doing hash rewriting). */ - for (auto & i : redirectedOutputs) - deletePath(worker.store.Store::toRealPath(i.second)); + /* In the past, redirected outputs were manually tracked for deletion. + * Now that we have the scratch outputs cleaner which are a superset of + * redirected outputs, we just fire all uncancelled automatic deleters now. + * + * This should clean up any paths that IS NOT registered in the database. + */ + scratchOutputsCleaner.clear(); /* Delete the chroot (if we were using one). */ autoDelChroot.reset(); /* this runs the destructor */ @@ -523,6 +527,10 @@ void LocalDerivationGoal::startBuilder() to use a temporary path */ makeFallbackPath(status.known->path); scratchOutputs.insert_or_assign(outputName, scratchPath); + /* Schedule this scratch output path for automatic deletion + * if we do not cancel it, e.g. when registering the outputs. + */ + scratchOutputsCleaner.insert_or_assign(outputName, worker.store.printStorePath(scratchPath)); /* Substitute output placeholders with the scratch output paths. We'll use during the build. */ @@ -545,8 +553,6 @@ void LocalDerivationGoal::startBuilder() std::string h2 { scratchPath.hashPart() }; inputRewrites[h1] = h2; } - - redirectedOutputs.insert_or_assign(std::move(fixedFinalPath), std::move(scratchPath)); } /* Construct the environment passed to the builder. */ @@ -2376,6 +2382,10 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() localStore.registerValidPaths({{oldInfo.path, oldInfo}}); } + /* Don't register anything, since we already have the + previous versions which we're comparing. + NOTE: this means that the `.check` path will be automatically deleted. + */ continue; } @@ -2399,8 +2409,13 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() /* If it's a CA path, register it right away. This is necessary if it isn't statically known so that we can safely unlock the path before the next iteration */ - if (newInfo.ca) + if (newInfo.ca) { localStore.registerValidPaths({{newInfo.path, newInfo}}); + /* Cancel automatic deletion of that output if it was a scratch output. */ + if (auto cleaner = scratchOutputsCleaner.extract(outputName)) { + cleaner.mapped().cancel(); + } + } infos.emplace(outputName, std::move(newInfo)); } @@ -2428,6 +2443,13 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() infos2.insert_or_assign(newInfo.path, newInfo); } localStore.registerValidPaths(infos2); + + /* Cancel automatic deletion of that output if it was a scratch output that we just registered. */ + for (auto & [outputName, _ ] : infos) { + if (auto cleaner = scratchOutputsCleaner.extract(outputName)) { + cleaner.mapped().cancel(); + } + } } /* In case of a fixed-output derivation hash mismatch, throw an @@ -2461,6 +2483,13 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() builtOutputs.emplace(outputName, thisRealisation); } + /* NOTE: At this point, all outputs MAY NOT have been registered. + * Therefore, there may remains auto-deleters pending in the cleaner list (`scratchOutputsCleaner`). + * + * They will be finally deleted but we have no way to assert they all have been, e.g. + * `assert(scratchOutputsCleaner.size() == 0)` cannot be written. + */ + return builtOutputs; } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 05e7588ac..cdf9ac51d 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -107,8 +107,6 @@ struct LocalDerivationGoal : public DerivationGoal * Hash rewriting. */ StringMap inputRewrites, outputRewrites; - typedef map RedirectedOutputs; - RedirectedOutputs redirectedOutputs; /** * The outputs paths used during the build. @@ -125,6 +123,19 @@ struct LocalDerivationGoal : public DerivationGoal * self-references. */ OutputPathMap scratchOutputs; + /** + * Output paths used during the build are scheduled for + * automatic cleanup unless they have been successfully built. + * + * `registerOutputs` take care of cancelling the cleanups + * and clearing this vector. + * + * `startBuilder` take care of filling this vector + * as `scratchOutputs` gets filled. + * + * This is a map from output names to automatic delete handles. + */ + std::map scratchOutputsCleaner; /** * Path registration info from the previous round, if we're diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index e48d47559..5e4b0f896 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -146,6 +146,9 @@ in symlinkResolvconf = runNixOSTestFor "x86_64-linux" ./symlink-resolvconf.nix; + # Use this test to test things that cannot easily be tested under chroot Nix stores in functional test suite. + non-chroot-misc = runNixOSTestFor "x86_64-linux" ./non-chroot-misc; + noNewPrivilegesInSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/sandbox.nix; noNewPrivilegesOutsideSandbox = runNixOSTestFor "x86_64-linux" ./no-new-privileges/no-sandbox.nix; diff --git a/tests/nixos/non-chroot-misc/default.nix b/tests/nixos/non-chroot-misc/default.nix new file mode 100644 index 000000000..93a66a595 --- /dev/null +++ b/tests/nixos/non-chroot-misc/default.nix @@ -0,0 +1,34 @@ +{ ... }: +# Misc things we want to test inside of a non redirected, non chroot Nix store. +let + nonAutoCleaningFailingDerivationCode = '' + derivation { + name = "scratch-failing"; + system = builtins.currentSystem; + builder = "/bin/sh"; + args = [ (builtins.toFile "builder.sh" "echo bonjour > $out; echo out: $out; false") ]; + } + ''; +in +{ + name = "non-chroot-sandbox-misc"; + + nodes.machine = { + }; + + testScript = { nodes }: '' + import re + start_all() + + # You might ask yourself why write such a convoluted thing? + # The condition for fooling Nix into NOT cleaning up the output path are non trivial and unclear. + # This is one of those: create a derivation, mkdir or touch the $out path, communicate it back. + # Even with a sandboxed Lix, you will observe leftovers before 2.93.0. After this version, this test passes. + result = machine.fail("""nix-build --substituters "" -E '${nonAutoCleaningFailingDerivationCode}' 2>&1""") + match = re.search(r'out: (\S+)', result) + assert match is not None, "Did not find Nix store path in the result of the failing build" + outpath = match.group(1).strip() + print(f"Found Nix store path: {outpath}") + machine.fail(f'stat {outpath}') + ''; +}