improve infinite recursion errors

src/nix-eval-jobs.cc

@@ -104,7 +104,7 @@ struct State {
     std::exception_ptr exc;
 };
 
-void handleBrokenWorkerPipe(Proc &proc) {
+void handleBrokenWorkerPipe(Proc &proc, std::string_view msg) {
     // we already took the process status from Proc, no
     // need to wait for it again to avoid error messages
     pid_t pid = proc.pid.release();
@@ -113,27 +113,49 @@ void handleBrokenWorkerPipe(Proc &proc) {
         int rc = waitpid(pid, &status, WNOHANG);
         if (rc == 0) {
             kill(pid, SIGKILL);
-            throw Error("BUG: worker pipe closed but worker still running?");
+            throw Error(
+                "BUG: while %s, worker pipe got closed but evaluation worker still running?",
+                msg);
         } else if (rc == -1) {
             kill(pid, SIGKILL);
-            throw Error("BUG: waitpid waiting for worker failed: %s",
+            throw Error("BUG: while %s, waitpid for evaluation worker failed: %s",
-                        strerror(errno));
+                        msg, strerror(errno));
         } else {
             if (WIFEXITED(status)) {
-                throw Error("evaluation worker exited with %d",
+                if (WEXITSTATUS(status) == 1) {
-                            WEXITSTATUS(status));
+                    throw Error(
+                        "while %s, evaluation worker exited with exit code 1, "
+                        "(possibly an infinite recursion)",
+                        msg);
+                }
+                throw Error("while %s, evaluation worker exited with %d",
+                            msg, WEXITSTATUS(status));
             } else if (WIFSIGNALED(status)) {
                 if (WTERMSIG(status) == SIGKILL) {
-                    throw Error("evaluation worker killed by SIGKILL, maybe "
+                    throw Error(
-                                "memory limit reached?");
+                        "while %s, evaluation worker got killed by SIGKILL, maybe "
+                        "memory limit reached?",
+                        msg);
                 }
-                throw Error("evaluation worker killed by signal %d (%s)",
+                throw Error(
-                            WTERMSIG(status), strsignal(WTERMSIG(status)));
+                    "while %s, evaluation worker got killed by signal %d (%s)",
+                    msg, WTERMSIG(status), strsignal(WTERMSIG(status)));
             } // else ignore WIFSTOPPED and WIFCONTINUED
         }
     }
 }
 
+std::string joinAttrPath(json &attrPath) {
+    std::string joined;
+    for (auto& element : attrPath) {
+        if (!joined.empty()) {
+            joined += '.';
+        }
+        joined += element.get<std::string>();
+    }
+    return joined;
+}
+
 void collector(Sync<State> &state_, std::condition_variable &wakeup) {
     try {
         std::optional<std::unique_ptr<Proc>> proc_;
@@ -151,7 +173,7 @@ void collector(Sync<State> &state_, std::condition_variable &wakeup) {
             /* Check whether the existing worker process is still there. */
             auto s = fromReader->readLine();
             if (s.empty()) {
-                handleBrokenWorkerPipe(*proc.get());
+                handleBrokenWorkerPipe(*proc.get(), "checking worker process");
             } else if (s == "restart") {
                 proc_ = std::nullopt;
                 fromReader_ = std::nullopt;
@@ -176,7 +198,7 @@ void collector(Sync<State> &state_, std::condition_variable &wakeup) {
                 if ((state->todo.empty() && state->active.empty()) ||
                     state->exc) {
                     if (tryWriteLine(proc->to.get(), "exit") < 0) {
-                        handleBrokenWorkerPipe(*proc.get());
+                        handleBrokenWorkerPipe(*proc.get(), "sending exit");
                     }
                     return;
                 }
@@ -191,13 +213,15 @@ void collector(Sync<State> &state_, std::condition_variable &wakeup) {
 
             /* Tell the worker to evaluate it. */
             if (tryWriteLine(proc->to.get(), "do " + attrPath.dump()) < 0) {
-                handleBrokenWorkerPipe(*proc.get());
+                auto msg = "sending attrPath '" + joinAttrPath(attrPath) + "'";
+                handleBrokenWorkerPipe(*proc.get(), msg);
             }
 
             /* Wait for the response. */
             auto respString = fromReader->readLine();
             if (respString.empty()) {
-                handleBrokenWorkerPipe(*proc.get());
+                auto msg = "reading result for attrPath '" + joinAttrPath(attrPath) + "'";
+                handleBrokenWorkerPipe(*proc.get(), msg);
             }
             json response;
             try {

tests/assets/flake.nix

@@ -8,6 +8,22 @@
     {
       hydraJobs = import ./ci.nix { inherit pkgs; };
 
-      legacyPackages.x86_64-linux.brokenPackage = throw "this is an evaluation error";
+      legacyPackages.x86_64-linux = {
+        brokenPkgs = {
+          brokenPackage = throw "this is an evaluation error";
+        };
+        infiniteRecursionPkgs = {
+          packageWithInfiniteRecursion =
+            let
+              recursion = [ recursion ];
+            in
+            derivation {
+              inherit (pkgs) system;
+              name = "drvB";
+              recursiveAttr = recursion;
+              builder = ":";
+            };
+        };
+      };
     };
 }

tests/test_eval.py

@@ -1,10 +1,10 @@
 #!/usr/bin/env python3
 
-import subprocess
 import json
-from tempfile import TemporaryDirectory
+import subprocess
 from pathlib import Path
-from typing import List, Dict, Any
+from tempfile import TemporaryDirectory
+from typing import Any, Dict, List
 
 TEST_ROOT = Path(__file__).parent.resolve()
 PROJECT_ROOT = TEST_ROOT.parent
@@ -77,8 +77,10 @@ def test_eval_error() -> None:
             "--gc-roots-dir",
             tempdir,
             "--meta",
+            "--workers",
+            "1",
             "--flake",
-            ".#legacyPackages.x86_64-linux",
+            ".#legacyPackages.x86_64-linux.brokenPkgs",
         ]
         res = subprocess.run(
             cmd,
@@ -86,6 +88,31 @@ def test_eval_error() -> None:
             text=True,
             stdout=subprocess.PIPE,
         )
+        print(res.stdout)
         attrs = json.loads(res.stdout)
         assert attrs["attr"] == "brokenPackage"
         assert "this is an evaluation error" in attrs["error"]
+
+
+def test_recursion_error() -> None:
+    with TemporaryDirectory() as tempdir:
+        cmd = [
+            str(BIN),
+            "--gc-roots-dir",
+            tempdir,
+            "--meta",
+            "--workers",
+            "1",
+            "--flake",
+            ".#legacyPackages.x86_64-linux.infiniteRecursionPkgs",
+        ]
+        res = subprocess.run(
+            cmd,
+            cwd=TEST_ROOT.joinpath("assets"),
+            text=True,
+            stderr=subprocess.PIPE,
+        )
+        assert res.returncode == 1
+        print(res.stderr)
+        assert 'packageWithInfiniteRecursion' in res.stderr
+        assert "possible infinite recursion" in res.stderr