From 0e2603961183925cc637172f8fa3878f2ac1e994 Mon Sep 17 00:00:00 2001
From: Mark Rousskov <mark.simulacrum@gmail.com>
Date: Wed, 11 Dec 2024 19:59:49 -0500
Subject: [PATCH] Fix marking experiment crates as complete

When processing GitHub repositories, our list usually (always?) contains
a repository without any form of commit hash. Crater agents checkout
that repository and, as part of building it, record the commit hash they
used. The agent then submits that hash to the db for storage.

When storing the hash, we *replace* the "crate name" (owner.reponame)
with a more specific ID (e.g., owner.reponame.$cratehash). This means
that the set of crates we tested has effectively *changed* at this
point from our perspective. Next we store results (previously under the
original name, now under the new name) and also update the (previous
old, now new) experiment_crates record to mark it complete.

The net effect is that prior to this commit (and likely since ~Aug 31)
every GitHub repository has been repeatedly tested by Crater until we
eventually hit count(results) >= count(experiment_crates). This is
basically just a random point in time though, AFAICT there's no
relationship between the set of crates we wanted to test and the set of
results we have. One saving factor is there's some amount of fixed point
-- if the GitHub repository we test doesn't receive any new commits
between attempts to run it, we'll re-test the same code and the old/new
IDs will match, letting us mark it as complete. But this is at best a
minor improvement, it's not actually a mitigating factor.

As a future TODO, we probably should update the "finish condition" from
counting results and experiment_crates and instead use something like
"are there any experiment_crates with a status of queued" which makes
much more sense.
---
 src/results/db.rs | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/results/db.rs b/src/results/db.rs
index 03013221..8b0e6d17 100644
--- a/src/results/db.rs
+++ b/src/results/db.rs
@@ -83,9 +83,31 @@ impl<'a> DatabaseDB<'a> {
         data: &ProgressData,
         encoding_type: EncodingType,
     ) -> Fallible<()> {
+        let krate = if let Some((old, new)) = &data.version {
+            // If we're updating the name of the crate (typically changing the hash we found on
+            // github) then we ought to also use that new name for marking the crate as complete.
+            // Otherwise, we leave behind the old (unversioned) name and end up running this crate
+            // many times, effectively never actually completing it.
+            self.update_crate_version(ex, old, new)?;
+
+            // sanity check that the previous name of the crate is the one we intended to run.
+            if old.id() != data.result.krate.id() {
+                log::warn!(
+                    "Storing result under {} despite job intended for {} (with wrong name old={})",
+                    new.id(),
+                    data.result.krate.id(),
+                    old.id(),
+                );
+            }
+
+            new
+        } else {
+            &data.result.krate
+        };
+
         self.store_result(
             ex,
-            &data.result.krate,
+            krate,
             &data.result.toolchain,
             &data.result.result,
             &base64::engine::general_purpose::STANDARD
@@ -94,11 +116,7 @@ impl<'a> DatabaseDB<'a> {
             encoding_type,
         )?;
 
-        if let Some((old, new)) = &data.version {
-            self.update_crate_version(ex, old, new)?;
-        }
-
-        self.mark_crate_as_completed(ex, &data.result.krate)?;
+        self.mark_crate_as_completed(ex, krate)?;
 
         Ok(())
     }