From e753fcb414d111192203f4ca73ca7d7bc7d0dd40 Mon Sep 17 00:00:00 2001 From: Linus Heckemann Date: Wed, 7 May 2025 20:17:05 +0200 Subject: [PATCH] gc: delay throwing error until cleanup is complete Previously, paths not being deleted by gcDeleteSpecific would result in (a) hardlinks not being cleaned up, and (b) statistics not being reported correctly. By throwing the error later, we fix both of these problems. Change-Id: I8019f3e10d9f22e81ea87bb26b77f04ebc888a19 --- doc/manual/rl-next/fix-gc-links.md | 13 ++++++++++ doc/manual/rl-next/fix-gc-stats.md | 24 ++++++++++++++++++ lix/libstore/gc.cc | 40 +++++++++++++++--------------- tests/functional/dependencies.nix | 2 ++ tests/functional/gc.sh | 5 +++- 5 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 doc/manual/rl-next/fix-gc-links.md create mode 100644 doc/manual/rl-next/fix-gc-stats.md diff --git a/doc/manual/rl-next/fix-gc-links.md b/doc/manual/rl-next/fix-gc-links.md new file mode 100644 index 000000000..1a40a6969 --- /dev/null +++ b/doc/manual/rl-next/fix-gc-links.md @@ -0,0 +1,13 @@ +--- +synopsis: "nix-store --delete: always remove obsolete hardlinks" +issues: [] +cls: [3188] +category: Fixes +credits: [lheckemann] +--- + +Deleting specific paths using `nix-store --delete` or `nix store +delete` previously did not delete hard links created by `nix-store +--optimise` even if they became obsolete, unless _all_ of the given +paths were deleted successfully. Now, hard links are always cleaned +up, even if some of the given paths could not be deleted. diff --git a/doc/manual/rl-next/fix-gc-stats.md b/doc/manual/rl-next/fix-gc-stats.md new file mode 100644 index 000000000..fc417892d --- /dev/null +++ b/doc/manual/rl-next/fix-gc-stats.md @@ -0,0 +1,24 @@ +--- +synopsis: "Report GC statistics correctly" +issues: [] +cls: [3188] +category: Fixes +credits: [lheckemann] +--- + +Deleting specific paths using `nix-store --delete` or `nix store delete` previously did +not report statistics correctly when some of the paths could not be deleted, even if +others were deleted: + +``` +$ nix store delete /nix/store/9bwryidal9q3g91cjm6xschfn4ikd82q-hello-2.12.1 --delete-closure -v +finding garbage collector roots... +deleting '/nix/store/9bwryidal9q3g91cjm6xschfn4ikd82q-hello-2.12.1' +0 store paths deleted, 0.00 MiB freed +error: Cannot delete some of the given paths because they are still alive. Paths not deleted: + k9bxzr1l92r5y6mihrkbpbr3fmc8qszx-libidn2-2.3.8 + mbx9ii53lzjlrsnlrfmzpwm33ynljwdn-libunistring-1.3 + rf8hcy6bldxdqc0g6q1dcka1vh47x69s-xgcc-14.2.1.20250322-libgcc + vbrdc5wgzn0w1zdp10xd2favkjn5fk7y-glibc-2.40-66 + To find out why, use nix-store --query --roots and nix-store --query --referrers. +``` diff --git a/lix/libstore/gc.cc b/lix/libstore/gc.cc index 0aad78c30..6a9f679b8 100644 --- a/lix/libstore/gc.cc +++ b/lix/libstore/gc.cc @@ -813,10 +813,10 @@ try { } }; + PathSet kept; /* Either delete all garbage paths, or just the specified paths (for gcDeleteSpecific and gcTryDeleteSpecific). */ if (deleteSpecific) { - PathSet kept; for (auto & i : options.pathsToDelete) { TRY_AWAIT(deleteReferrersClosure(i)); if (!dead.count(i)) { @@ -831,25 +831,6 @@ try { printTalkative(path); } } - if (options.action == GCOptions::gcDeleteSpecific && !kept.empty()) { - std::ostringstream pathSummary; - for (auto const [n, path]: enumerate(kept)) { - pathSummary << "\n " << path; - const int summaryThreshold = 10; - if (n >= summaryThreshold) { - pathSummary << "\nand " << kept.size() - summaryThreshold << " others."; - break; - } - } - throw Error( - "Cannot delete some of the given paths because they are still alive. " - "Paths not deleted:" - "%1%" - "\nTo find out why, use nix-store --query --roots and nix-store --query --referrers." - , - pathSummary.str() - ); - } } else if (options.maxFreed > 0) { @@ -940,6 +921,25 @@ try { printInfo("note: currently hard linking saves %.2f MiB", ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0))); } + if (options.action == GCOptions::gcDeleteSpecific && !kept.empty()) { + std::ostringstream pathSummary; + for (auto const [n, path]: enumerate(kept)) { + pathSummary << "\n " << path; + const int summaryThreshold = 10; + if (n >= summaryThreshold) { + pathSummary << "\nand " << kept.size() - summaryThreshold << " others."; + break; + } + } + throw Error( + "Cannot delete some of the given paths because they are still alive. " + "Paths not deleted:" + "%1%" + "\nTo find out why, use nix-store --query --roots and nix-store --query --referrers." + , + pathSummary.str() + ); + } co_return result::success(); } catch (...) { co_return result::current_exception(); diff --git a/tests/functional/dependencies.nix b/tests/functional/dependencies.nix index 944371053..6463bcb32 100644 --- a/tests/functional/dependencies.nix +++ b/tests/functional/dependencies.nix @@ -16,6 +16,8 @@ let name = "dependencies-input-2"; buildCommand = '' mkdir $out + # Space-filler to test GC stats reporting + head -c 100k /dev/zero > $out/filler echo BAR > $out/bar echo ${input0} > $out/input0 ''; diff --git a/tests/functional/gc.sh b/tests/functional/gc.sh index 7e3327f4f..6df237eaa 100644 --- a/tests/functional/gc.sh +++ b/tests/functional/gc.sh @@ -69,11 +69,14 @@ nix-store --delete --skip-live $(readLink $outPath/reference-to-input-2) rm "$NIX_STATE_DIR"/gcroots/foo # with the dependent unrooted, we should be able to remove input2... -nix-store --delete --delete-closure $input2 +nix-store --delete --delete-closure $input2 > delete-output # which should remove input0, since only input2 and top depended on it and we passed --delete-closure ! test -e $input0 # but fod should be unaffected, since it's not part of input-2's closure test -e $fodOut +# and stats should be reported correctly +grep "0.10 MiB freed$" delete-output +test -z "$(grep "0 paths deleted" delete-output)" nix-collect-garbage