From 3e237598342dee46188c83fba49cc30d509ee553 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Fri, 12 Jan 2024 12:38:55 +0100
Subject: [PATCH] gc-non-blocking.sh: Add explanation

Also name the _NIX_TEST_GC_SYNC environment variables logically.
---
 src/libstore/gc.cc                  | 10 +++++-----
 tests/functional/gc-non-blocking.sh | 20 ++++++++++++++------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index bd64e238d..80e036e7e 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -511,7 +511,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 
     /* Synchronisation point to test ENOENT handling in
        addTempRoot(), see tests/gc-non-blocking.sh. */
-    if (auto p = getEnv("_NIX_TEST_GC_SYNC_2"))
+    if (auto p = getEnv("_NIX_TEST_GC_SYNC_1"))
         readFile(*p);
 
     /* Start the server for receiving new roots. */
@@ -637,6 +637,10 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         roots.insert(root.first);
     }
 
+    /* Synchronisation point for testing, see tests/functional/gc-non-blocking.sh. */
+    if (auto p = getEnv("_NIX_TEST_GC_SYNC_2"))
+        readFile(*p);
+
     /* Helper function that deletes a path from the store and throws
        GCLimitReached if we've deleted enough garbage. */
     auto deleteFromStore = [&](std::string_view baseName)
@@ -783,10 +787,6 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         }
     };
 
-    /* Synchronisation point for testing, see tests/functional/gc-non-blocking.sh. */
-    if (auto p = getEnv("_NIX_TEST_GC_SYNC"))
-        readFile(*p);
-
     /* Either delete all garbage paths, or just the specified
        paths (for gcDeleteSpecific). */
     if (options.action == GCOptions::gcDeleteSpecific) {
diff --git a/tests/functional/gc-non-blocking.sh b/tests/functional/gc-non-blocking.sh
index 7f2aebb8b..ec280badb 100644
--- a/tests/functional/gc-non-blocking.sh
+++ b/tests/functional/gc-non-blocking.sh
@@ -6,10 +6,14 @@ needLocalStore "the GC test needs a synchronisation point"
 
 clearStore
 
-fifo=$TEST_ROOT/test.fifo
-mkfifo "$fifo"
+# This FIFO is read just after the global GC lock has been acquired,
+# but before the root server is started.
+fifo1=$TEST_ROOT/test2.fifo
+mkfifo "$fifo1"
 
-fifo2=$TEST_ROOT/test2.fifo
+# This FIFO is read just after the roots have been read, but before
+# the actual GC starts.
+fifo2=$TEST_ROOT/test.fifo
 mkfifo "$fifo2"
 
 dummy=$(nix store add-path ./simple.nix)
@@ -17,19 +21,23 @@ dummy=$(nix store add-path ./simple.nix)
 running=$TEST_ROOT/running
 touch $running
 
-(_NIX_TEST_GC_SYNC=$fifo _NIX_TEST_GC_SYNC_2=$fifo2 nix-store --gc -vvvvv; rm $running) &
+# Start GC.
+(_NIX_TEST_GC_SYNC_1=$fifo1 _NIX_TEST_GC_SYNC_2=$fifo2 nix-store --gc -vvvvv; rm $running) &
 pid=$!
 
 sleep 2
 
-(sleep 1; echo > $fifo2) &
+# Delay the start of the root server to check that the build below
+# correctly handles ENOENT when connecting to the root server.
+(sleep 1; echo > $fifo1) &
 pid2=$!
 
+# Start a build. This should not be blocked by the GC in progress.
 outPath=$(nix-build --max-silent-time 60 -o "$TEST_ROOT/result" -E "
   with import ./config.nix;
   mkDerivation {
     name = \"non-blocking\";
-    buildCommand = \"set -x; test -e $running; mkdir \$out; echo > $fifo\";
+    buildCommand = \"set -x; test -e $running; mkdir \$out; echo > $fifo2\";
   }")
 
 wait $pid