Auto merge of #515 - pietroalbini:fix-agent-hanging, r=pietroalbini

Handle skipped crates added after experiment creation

Before this PR, if a new crate was added to the blacklist after an experiment was created a deadlock occured:

1. During experiment creation, the crate is added to the list of crates test during such experiment.
2. The crate is blacklisted, and the server is restarted to apply the blacklist change.
3. The experiment starts running.
4. An agent asks for crates to test during an experiment, and receives the blacklisted crate.
5. The agent ignores the blacklisted crate as it's blacklisted, and replies "I'm done" to the server without actually recording any result for the skipped crate.
6. The server readds the blacklisted crate to the queue for that agent, as it thinks that crate has to be tested and the agent didn't do it.

This PR fixes the deadlock by adding a new "Skip" task to the graph, which sends a "skipped" result for blacklisted crates to the server instead of silently ignoring them.

Author: bors

src/report/html.rs

@@ -42,6 +42,7 @@ impl ResultColor for TestResult {
             TestResult::TestFail(_) => Color::Single("#65461e"),
             TestResult::TestSkipped | TestResult::TestPass => Color::Single("#62a156"),
             TestResult::Error => Color::Single("#d77026"),
+            TestResult::Skipped => Color::Single("#494b4a"),
         }
     }
 }
@@ -81,6 +82,7 @@ impl ResultName for TestResult {
             TestResult::TestSkipped => "test skipped".into(),
             TestResult::TestPass => "test passed".into(),
             TestResult::Error => "error".into(),
+            TestResult::Skipped => "skipped".into(),
         }
     }
 }

src/report/mod.rs

@@ -391,6 +391,7 @@ fn compare(
             | (TestFail(_), BuildFail(_)) => Comparison::Regressed,
 
             (Error, _) | (_, Error) => Comparison::Error,
+            (Skipped, _) | (_, Skipped) => Comparison::Skipped,
             (BrokenCrate(_), _) | (_, BrokenCrate(_)) => Comparison::Broken,
             (TestFail(_), TestSkipped)
             | (TestPass, TestSkipped)
@@ -687,6 +688,18 @@ mod tests {
                 TestFail(Unknown), Error => Error;
                 BuildFail(Unknown), Error => Error;
 
+                // Skipped
+                Skipped, Skipped => Skipped;
+                Skipped, TestPass => Skipped;
+                Skipped, TestSkipped => Skipped;
+                Skipped, TestFail(Unknown) => Skipped;
+                Skipped, BuildFail(Unknown) => Skipped;
+                TestPass, Skipped => Skipped;
+                TestSkipped, Skipped => Skipped;
+                TestFail(Unknown), Skipped => Skipped;
+                BuildFail(Unknown), Skipped => Skipped;
+
+
                 // Broken
                 BrokenCrate(BrokenReason::Unknown), TestPass => Broken;
                 BrokenCrate(BrokenReason::Unknown), TestSkipped => Broken;

src/results/mod.rs

@@ -200,6 +200,7 @@ test_result_enum!(pub enum TestResult {
     without_reason {
         TestSkipped => "test-skipped",
         TestPass => "test-pass",
+        Skipped => "skipped",
         Error => "error",
     }
 });

src/runner/graph.rs

@@ -260,6 +260,16 @@ pub(super) fn build_graph(ex: &Experiment, crates: &[Crate], config: &Config) ->
 
     for krate in crates {
         if !ex.ignore_blacklist && config.should_skip(krate) {
+            for tc in &ex.toolchains {
+                let id = graph.add_task(
+                    Task {
+                        krate: krate.clone(),
+                        step: TaskStep::Skip { tc: tc.clone() },
+                    },
+                    &[],
+                );
+                graph.add_crate(&[id]);
+            }
             continue;
         }
 

src/runner/tasks.rs

@@ -51,6 +51,7 @@ impl<'ctx, DB: WriteResults + 'ctx> TaskCtx<'ctx, DB> {
 pub(super) enum TaskStep {
     Prepare,
     Cleanup,
+    Skip { tc: Toolchain },
     BuildAndTest { tc: Toolchain, quiet: bool },
     BuildOnly { tc: Toolchain, quiet: bool },
     CheckOnly { tc: Toolchain, quiet: bool },
@@ -64,6 +65,7 @@ impl fmt::Debug for TaskStep {
         let (name, quiet, tc) = match *self {
             TaskStep::Prepare => ("prepare", false, None),
             TaskStep::Cleanup => ("cleanup", false, None),
+            TaskStep::Skip { ref tc } => ("skip", false, Some(tc)),
             TaskStep::BuildAndTest { ref tc, quiet } => ("build and test", quiet, Some(tc)),
             TaskStep::BuildOnly { ref tc, quiet } => ("build", quiet, Some(tc)),
             TaskStep::CheckOnly { ref tc, quiet } => ("check", quiet, Some(tc)),
@@ -105,7 +107,8 @@ impl Task {
             // runner will not reach the prepare task in that case.
             TaskStep::Prepare => true,
             // Build tasks should only be executed if there are no results for them
-            TaskStep::BuildAndTest { ref tc, .. }
+            TaskStep::Skip { ref tc }
+            | TaskStep::BuildAndTest { ref tc, .. }
             | TaskStep::BuildOnly { ref tc, .. }
             | TaskStep::CheckOnly { ref tc, .. }
             | TaskStep::Clippy { ref tc, .. }
@@ -127,7 +130,8 @@ impl Task {
     ) -> Fallible<()> {
         match self.step {
             TaskStep::Prepare | TaskStep::Cleanup => {}
-            TaskStep::BuildAndTest { ref tc, .. }
+            TaskStep::Skip { ref tc }
+            | TaskStep::BuildAndTest { ref tc, .. }
             | TaskStep::BuildOnly { ref tc, .. }
             | TaskStep::CheckOnly { ref tc, .. }
             | TaskStep::Clippy { ref tc, .. }
@@ -221,6 +225,23 @@ impl Task {
                     crate::runner::unstable_features::find_unstable_features,
                 )?;
             }
+            TaskStep::Skip { ref tc } => {
+                // If a skipped crate is somehow sent to the agent (for example, when a crate was
+                // added to the experiment and *then* blacklisted) report the crate as skipped
+                // instead of silently ignoring it.
+                db.record_result(
+                    ex,
+                    tc,
+                    &self.krate,
+                    None,
+                    config,
+                    EncodingType::Plain,
+                    || {
+                        warn!("crate skipped");
+                        Ok(TestResult::Skipped)
+                    },
+                )?;
+            }
         }
 
         Ok(())