Traverser: Bring task system down gracefully on error

6 files changed, 137 insertions, 41 deletions

diff --git a/src/buildtool/execution_engine/traverser/traverser.hpp b/src/buildtool/execution_engine/traverser/traverser.hpp
index 930cf0fb..4d3fc7e9 100644
--- a/src/buildtool/execution_engine/traverser/traverser.hpp
+++ b/src/buildtool/execution_engine/traverser/traverser.hpp
@@ -1,6 +1,8 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_ENGINE_TRAVERSER_TRAVERSER_HPP
 #define INCLUDED_SRC_BUILDTOOL_EXECUTION_ENGINE_TRAVERSER_TRAVERSER_HPP
 
+#include <atomic>
+
 #include "gsl-lite/gsl-lite.hpp"
 #include "src/buildtool/execution_engine/dag/dag.hpp"
 #include "src/buildtool/logging/logger.hpp"
@@ -27,8 +29,14 @@ concept Runnable = requires(T const r,
 template <Runnable Executor>
 class Traverser {
   public:
-    Traverser(Executor const& r, DependencyGraph const& graph, std::size_t jobs)
-        : runner_{r}, graph_{graph}, tasker_{jobs} {}
+    Traverser(Executor const& r,
+              DependencyGraph const& graph,
+              std::size_t jobs,
+              gsl::not_null<std::atomic<bool>*> fail_flag)
+        : runner_{r},
+          graph_{graph},
+          failed_{std::move(fail_flag)},
+          tasker_{jobs} {}
     Traverser() = delete;
     Traverser(Traverser const&) = delete;
     Traverser(Traverser&&) = delete;
@@ -52,6 +60,7 @@ class Traverser {
   private:
     Executor const& runner_{};
     DependencyGraph const& graph_;
+    gsl::not_null<std::atomic<bool>*> failed_;
     TaskSystem tasker_{};  // THIS SHOULD BE THE LAST MEMBER VARIABLE
 
     // Visits discover nodes and queue visits to their children nodes.
@@ -77,7 +86,7 @@ class Traverser {
     void QueueVisit(NodeTypePtr node) noexcept {
         // in case the node was already discovered, there is no need to queue
         // the visit
-        if (node->TraversalState()->GetAndMarkDiscovered()) {
+        if (failed_->load() or node->TraversalState()->GetAndMarkDiscovered()) {
             return;
         }
         tasker_.QueueTask([this, node]() noexcept { Visit(node); });
@@ -89,7 +98,7 @@ class Traverser {
     // was successful
     template <typename NodeTypePtr>
     void QueueProcessing(NodeTypePtr node) noexcept {
-        if (not node->TraversalState()->IsRequired() or
+        if (failed_->load() or not node->TraversalState()->IsRequired() or
             node->TraversalState()->GetAndMarkQueuedToBeProcessed()) {
             return;
         }
@@ -99,12 +108,16 @@ class Traverser {
                 NotifyAvailable(node);
             }
             else {
-                Logger::Log(LogLevel::Error, "Build failed.");
-                std::exit(EXIT_FAILURE);
+                Abort();
             }
         };
         tasker_.QueueTask(process_node);
     }
+
+    void Abort() noexcept {
+        failed_->store(true);
+        tasker_.Shutdown();  // skip execution of pending tasks
+    }
 };
 
 template <Runnable Executor>
@@ -116,6 +129,7 @@ auto Traverser<Executor>::Traverse(
             QueueVisit(artifact_node);
         }
         else {
+            Abort();
             Logger::Log(
                 LogLevel::Error,
                 "artifact with id {} can not be found in dependency graph.",
diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp
index fac93e27..c00fe61b 100644
--- a/src/buildtool/graph_traverser/graph_traverser.hpp
+++ b/src/buildtool/graph_traverser/graph_traverser.hpp
@@ -324,20 +324,21 @@ class GraphTraverser {
         Executor executor{&(*api_),
                           RemoteExecutionConfig::PlatformProperties(),
                           clargs_.build.timeout};
-        bool result{};
+        bool traversing{};
         std::atomic<bool> done = false;
+        std::atomic<bool> failed = false;
         std::condition_variable cv{};
         auto observer =
             std::thread([this, &done, &cv]() { reporter_(&done, &cv); });
         {
-            Traverser t{executor, g, clargs_.jobs};
-            result =
+            Traverser t{executor, g, clargs_.jobs, &failed};
+            traversing =
                 t.Traverse({std::begin(artifact_ids), std::end(artifact_ids)});
         }
         done = true;
         cv.notify_all();
         observer.join();
-        return result;
+        return traversing and not failed;
     }
 
     [[nodiscard]] auto TraverseRebuild(
@@ -350,18 +351,19 @@ class GraphTraverser {
                            &(*api_cached),
                            RemoteExecutionConfig::PlatformProperties(),
                            clargs_.build.timeout};
-        bool success{false};
+        bool traversing{false};
+        std::atomic<bool> failed{false};
         {
-            Traverser t{executor, g, clargs_.jobs};
-            success =
+            Traverser t{executor, g, clargs_.jobs, &failed};
+            traversing =
                 t.Traverse({std::begin(artifact_ids), std::end(artifact_ids)});
         }
 
-        if (success and clargs_.rebuild->dump_flaky) {
+        if (traversing and not failed and clargs_.rebuild->dump_flaky) {
             std::ofstream file{*clargs_.rebuild->dump_flaky};
             file << executor.DumpFlakyActions().dump(2);
         }
-        return success;
+        return traversing and not failed;
     }
 
     /// \brief Retrieves nodes corresponding to artifacts with ids in artifacts.
@@ -463,7 +465,7 @@ class GraphTraverser {
 
         if (clargs_.rebuild ? not TraverseRebuild(*graph, artifact_ids)
                             : not Traverse(*graph, artifact_ids)) {
-            Logger::Log(LogLevel::Error, "traversing graph failed.");
+            Logger::Log(LogLevel::Error, "Build failed.");
             return std::nullopt;
         }
 
diff --git a/test/buildtool/execution_engine/traverser/traverser.test.cpp b/test/buildtool/execution_engine/traverser/traverser.test.cpp
index 693097d5..5f06cc00 100644
--- a/test/buildtool/execution_engine/traverser/traverser.test.cpp
+++ b/test/buildtool/execution_engine/traverser/traverser.test.cpp
@@ -212,14 +212,16 @@ TEST_CASE("Executable", "[traverser]") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Traverse()") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -235,7 +237,7 @@ TEST_CASE("Executable", "[traverser]") {
     SECTION("Traverse(executable)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
 
             auto const exec_id = ArtifactFactory::Identifier(
                 ArtifactFactory::DescribeActionArtifact("action",
@@ -272,14 +274,16 @@ TEST_CASE("Executable depends on library", "[traverser]") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Full build (without specifying artifacts)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -295,12 +299,13 @@ TEST_CASE("Executable depends on library", "[traverser]") {
     SECTION("Full build (executable)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const exec_id = ArtifactFactory::Identifier(
                 ArtifactFactory::DescribeActionArtifact("make_exe",
                                                         "executable"));
             CHECK(traverser.Traverse({exec_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -318,9 +323,10 @@ TEST_CASE("Executable depends on library", "[traverser]") {
             ArtifactFactory::DescribeActionArtifact("make_lib", "library"));
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({lib_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -354,14 +360,16 @@ TEST_CASE("Two artifacts depend on another", "[traverser]") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Full build") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -379,9 +387,10 @@ TEST_CASE("Two artifacts depend on another", "[traverser]") {
             ArtifactFactory::DescribeActionArtifact("action1", "toplevel1"));
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({toplevel1_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -406,14 +415,16 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Traverse()") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -430,7 +441,7 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") {
 
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({output1_id});
             CHECK(traversed);
         }
@@ -449,7 +460,7 @@ TEST_CASE("Action with two outputs, no deps", "[traverser]") {
     SECTION("Traverse(output1, output2)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({output1_id, output2_id});
             CHECK(traversed);
         }
@@ -480,14 +491,16 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Traverse()") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -503,7 +516,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") {
     SECTION("Traverse(output1)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({output1_id});
             CHECK(traversed);
         }
@@ -522,7 +535,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") {
     SECTION("Traverse(output1, output2)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({output1_id, output2_id});
             CHECK(traversed);
         }
@@ -543,7 +556,7 @@ TEST_CASE("Action with two outputs, one dep", "[traverser]") {
             ArtifactFactory::DescribeLocalArtifact("dep", "repo"));
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({dep_id, output2_id});
             CHECK(traversed);
         }
@@ -583,14 +596,16 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs",
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted";
     build_info.SetName(name);
     SECTION("Traverse()") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -606,7 +621,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs",
     SECTION("Traverse(exec1)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({exec1_id});
             CHECK(traversed);
         }
@@ -625,7 +640,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs",
     SECTION("Traverse(exec2, output1)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({output1_id, exec2_id});
             CHECK(traversed);
         }
@@ -644,7 +659,7 @@ TEST_CASE("Action with two outputs, actions depend on each of outputs",
     SECTION("Traverse(exec1, exec2)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             auto const traversed = traverser.Traverse({exec1_id, exec2_id});
             CHECK(traversed);
         }
@@ -697,14 +712,16 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     DependencyGraph g;
     CHECK(p.FillGraph(&g));
     TestBuildInfo build_info;
+    std::atomic<bool> failed{};
     std::string name = "This is a long name that shouldn't be corrupted ";
     build_info.SetName(name);
     SECTION(" Full build(without specifying artifacts) ") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse());
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -720,9 +737,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     SECTION("Full build (executable)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({exec_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -738,9 +756,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     SECTION("Full build (executable + lib1)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({exec_id, lib1_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -756,9 +775,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     SECTION("Full build (executable + lib2)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({exec_id, lib2_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -774,9 +794,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     SECTION("Full build (executable + lib1 + lib2)") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({exec_id, lib1_id, lib2_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -792,10 +813,11 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
     SECTION("First call does not build all artifacts") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({lib1_id}));
             CHECK(traverser.Traverse({exec_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
@@ -814,9 +836,10 @@ TEST_CASE("lib2 depends on lib1, executable depends on lib1 and lib2") {
         "action") {
         {
             TestExecutor runner{&build_info};
-            Traverser traverser(runner, g, kNumJobs);
+            Traverser traverser(runner, g, kNumJobs, &failed);
             CHECK(traverser.Traverse({lib2_id}));
         }
+        CHECK_FALSE(failed);
         CHECK_THAT(
             build_info.CorrectlyBuilt(),
             HasSameUniqueElementsAs<std::unordered_set<ArtifactIdentifier>>(
diff --git a/test/end-to-end/TARGETS b/test/end-to-end/TARGETS
index 4737c7aa..a5586446 100644
--- a/test/end-to-end/TARGETS
+++ b/test/end-to-end/TARGETS
@@ -9,6 +9,7 @@
     , [["./", "targets", "TESTS"], "targets"]
     , [["./", "user-errors", "TESTS"], "user-errors"]
     , [["./", "built-in-rules", "TESTS"], "built-in-rules"]
+    , [["./", "build-fails", "TESTS"], "build-fails"]
     ]
   }
 }
diff --git a/test/end-to-end/build-fails/TARGETS b/test/end-to-end/build-fails/TARGETS
new file mode 100644
index 00000000..05e2c957
--- /dev/null
+++ b/test/end-to-end/build-fails/TARGETS
@@ -0,0 +1,9 @@
+{ "single_fail_dep":
+  { "type": ["@", "rules", "shell/test", "script"]
+  , "name": ["single_fail_dep"]
+  , "test": ["single_fail_dep.sh"]
+  , "deps": [["test/end-to-end", "tool-under-test"]]
+  }
+, "TESTS":
+  {"type": "install", "tainted": ["test"], "deps": ["single_fail_dep"]}
+}
diff --git a/test/end-to-end/build-fails/single_fail_dep.sh b/test/end-to-end/build-fails/single_fail_dep.sh
new file mode 100755
index 00000000..c4e713b3
--- /dev/null
+++ b/test/end-to-end/build-fails/single_fail_dep.sh
@@ -0,0 +1,47 @@
+#!/bin/bash
+
+set -eu
+
+readonly NUM_DEPS=100
+readonly FAIL_DEP=42
+
+create_target() {
+  local ID=$1
+  if [ $ID = $FAIL_DEP ]; then
+    # failing target
+    echo '{"type": "generic", "cmds": ["false"], "out_dirs": ["out_'$ID'"]}'
+  else
+    # success target
+    echo '{"type": "generic", "cmds": ["echo '$ID' > out_'$ID'/id"], "out_dirs": ["out_'$ID'"]}'
+  fi
+}
+
+mkdir -p .buildroot
+
+touch ROOT
+
+cat <<EOF > TARGETS
+{ $(for i in $(seq $NUM_DEPS); do
+      if [ $i = 1 ]; then echo -n ' '; else echo -n '  , '; fi;
+      echo '"dep_'$i'": '$(create_target $i); done)
+, "main":
+  { "type": "generic"
+  , "cmds": ["cat out_*/id | sort -n > id"]
+  , "outs": ["id"]
+  , "deps":
+    [ $(for i in $(seq $NUM_DEPS); do
+          if [ $i = 1 ]; then echo -n ' '; else echo -n '    , '; fi;
+          echo '"dep_'$i'"';
+        done)
+    ]
+  }
+}
+EOF
+
+cat TARGETS
+
+echo "main" >&2
+bin/tool-under-test build --local-build-root .buildroot main && exit 1 || [ $? = 1 ]
+echo "done" >&2
+
+exit