Skip to content

Commit

Permalink
Fix marking experiment crates as complete
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Mark-Simulacrum committed Dec 12, 2024
1 parent 61ed2a4 commit 0e26039
Showing 1 changed file with 24 additions and 6 deletions.
30 changes: 24 additions & 6 deletions src/results/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(())
}
Expand Down

0 comments on commit 0e26039

Please sign in to comment.