Recover gracefully from agent failure

Mark-Simulacrum · Mark-Simulacrum · commit ae1099a799d0 · 2022-02-01T22:29:01.000-05:00
Previously, any agent reporting an error would cause an experiment to come to a
hard stop. This is noisy (leads to a user-visible report) and, at least lately,
not actually representative of a terminal failure. Instead, we take the approach
of re-queueing any crates assigned to that agent and logging an error on the
server host specifying which agent failed.

As the error message is now not typically posted publicly (and no longer needs
to be short for being in a GitHub comment), we will also have more opportunity
to add extra metadata over time to that message (in future PRs) enabling
diagnosing and fixing these errors.

In the future, we will likely also want some tracking/reporting of these
failures such that hard errors across all agents lead to alerting the infra
team, but this may be best done through Prometheus metrics rather than GitHub
"end-user" visible comments (which are likely not the best way to reach infra
team members regardless).
diff --git a/src/experiments.rs b/src/experiments.rs
@@ -485,8 +485,9 @@ impl Experiment {
         }
     }
 
-    pub fn report_failure(&mut self, db: &Database, agent: &Assignee) -> Fallible<()> {
-        // Mark all the running crates from this agent as failed as well if the experiment failed
+    pub fn handle_failure(&mut self, db: &Database, agent: &Assignee) -> Fallible<()> {
+        // Mark all the running crates from this agent as queued (so that they
+        // run again)
         db.execute(
             "
             UPDATE experiment_crates
@@ -495,14 +496,13 @@ impl Experiment {
             AND assigned_to = ?4
             ",
             &[
-                &Status::Failed.to_string(),
+                &Status::Queued.to_string(),
                 &self.name,
                 &Status::Running.to_string(),
                 &agent.to_string(),
             ],
         )?;
-
-        self.set_status(db, Status::Failed)
+        Ok(())
     }
 
     pub fn set_status(&mut self, db: &Database, status: Status) -> Fallible<()> {
@@ -1063,6 +1063,8 @@ mod tests {
         assert_eq!(uncompleted_crates.len(), 0);
     }
 
+    // A failure is handled by re-queueing any running crates for a given agent,
+    // to be picked up by the next agent to ask for them.
     #[test]
     fn test_failed_experiment() {
         let db = Database::temp().unwrap();
@@ -1079,9 +1081,12 @@ mod tests {
             .get_uncompleted_crates(&db, &config, &agent1)
             .unwrap()
             .is_empty());
-        ex.report_failure(&db, &agent1).unwrap();
-        assert!(!Experiment::next(&db, &agent1).unwrap().is_some());
-        assert_eq!(ex.status, Status::Failed);
-        assert!(ex.get_running_crates(&db, &agent1).unwrap().is_empty());
+        ex.handle_failure(&db, &agent1).unwrap();
+        assert!(Experiment::next(&db, &agent1).unwrap().is_some());
+        assert_eq!(ex.status, Status::Running);
+        assert!(!ex
+            .get_uncompleted_crates(&db, &config, &agent1)
+            .unwrap()
+            .is_empty());
     }
 }
diff --git a/src/server/routes/agent.rs b/src/server/routes/agent.rs
@@ -75,7 +75,6 @@ pub fn routes(
         .and(warp::path::end())
         .and(warp::body::json())
         .and(mutex_filter)
-        .and(github_data_filter)
         .and(auth_filter(data, TokenType::Agent))
         .map(endpoint_error);
 
@@ -197,38 +196,22 @@ fn endpoint_heartbeat(data: Arc<Data>, auth: AuthDetails) -> Fallible<Response<B
 fn endpoint_error(
     error: ExperimentData<HashMap<String, String>>,
     mutex: Arc<Mutex<Data>>,
-    github_data: Option<Arc<GithubData>>,
     auth: AuthDetails,
 ) -> Fallible<Response<Body>> {
+    log::error!(
+        "agent {} failed while running {}: {:?}",
+        auth.name,
+        error.experiment_name,
+        error.data.get("error")
+    );
+
     let data = mutex.lock().unwrap();
     let mut ex = Experiment::get(&data.db, &error.experiment_name)?
         .ok_or_else(|| err_msg("no experiment run by this agent"))?;
 
     //also set status to failed
-    ex.report_failure(&data.db, &Assignee::Agent(auth.name))?;
+    ex.handle_failure(&data.db, &Assignee::Agent(auth.name))?;
 
-    if let Some(github_data) = github_data.as_ref() {
-        if let Some(ref github_issue) = ex.github_issue {
-            Message::new()
-                .line(
-                    "rotating_light",
-                    format!(
-                        "Experiment **`{}`** has encountered an error: {}",
-                        ex.name,
-                        error.data.get("error").unwrap_or(&String::from("no error")),
-                    ),
-                )
-                .line(
-                    "hammer_and_wrench",
-                    "If the error is fixed use the `retry` command.",
-                )
-                .note(
-                    "sos",
-                    "Can someone from the infra team check in on this? @rust-lang/infra",
-                )
-                .send(&github_issue.api_url, &data, github_data)?;
-        }
-    }
     Ok(ApiResponse::Success { result: true }.into_response()?)
 }
 
