Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions rust/crates/du-db/src/grid/digest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,30 @@ impl Comparable {
}
}

impl Comparable {
/// Whether this digest carries **no** comparable call at all.
///
/// Two such digests agree with each other, because every field is absent on both sides. That is
/// correct as a comparison and disastrous as a quorum: two nodes whose analysis failed would
/// agree on nothing, canonicalize a unit with no content, and be paid for it.
///
/// So validation refuses to canonicalize on an empty digest. The node is expected to fail its
/// unit rather than submit one, but the AppView cannot rely on that — a node is untrusted by
/// construction, and that is the whole premise of adaptive replication.
pub fn is_empty(&self) -> bool {
self.sex.is_none()
&& self.y_terminal.is_none()
&& self.ancestry_superpop_argmax.is_none()
&& self.coverage_bucket.is_none()
&& self.callable_bucket.is_none()
}
}

/// Whether a digest carries anything a quorum could be about.
pub fn has_content(digest: &Value) -> bool {
!Comparable::from_digest(digest).is_empty()
}

/// The major component of a semver-ish stack version: `"1.7.0"` → `"1"`.
///
/// Only submissions from a compatible major are compared. A minor release that refactors a walker
Expand Down Expand Up @@ -203,6 +227,19 @@ mod tests {
);
}

/// …and "equally uninformative" is exactly why agreement is not enough on its own. Two nodes
/// whose analysis failed submit two empty digests, which agree. Without this check they would
/// canonicalize a unit with no content and be credited for it.
#[test]
fn an_empty_digest_has_no_content_to_agree_about() {
assert!(!has_content(&json!({"unexpected": true})));
assert!(!has_content(&json!(null)));
assert!(!has_content(&json!({"calls": {}})));
assert!(has_content(&d("XY", "R-A", "EUR", 30.0, 0.94)));
// One discrete call is enough to be about something.
assert!(has_content(&json!({"calls": {"sex": "XX"}})));
}

/// The cross-repo byte contract: key order in the source JSON must not change the hash, or a
/// node and the AppView would disagree about what was signed.
#[test]
Expand Down
9 changes: 8 additions & 1 deletion rust/crates/du-db/src/grid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,12 @@ pub struct CurationCandidate {
/// With `only_new`, samples that already have a work unit are skipped. That is the nightly path.
/// Passing `false` re-projects everything, which refreshes manifests after a re-crawl.
///
/// **The manifest carries the instrument**, because the node can not choose a mapper without it.
/// A read set with no mate can be a long read or a single-end short read, and those two need
/// different presets. A long read mapped under a short-read preset does not fail — it gives
/// alignments that look correct and are wrong. `crawl_project` already stores the instrument model
/// of ENA on the library, so this only carries a value that we hold.
///
/// **`est_bases` prefers the measured `base_count`** that ENA publishes on `read_run`, and falls
/// back to `reads × read_length` where a row predates that column. The fallback is only ever a
/// mean-length approximation and is wrong outright for variable-length long reads, so it is a
Expand Down Expand Up @@ -460,7 +466,8 @@ pub async fn curation_candidates(
'index_url', sf.http_locations->0->>'file_index_url', \
'md5', sf.checksums->0->>'checksum', \
'bytes', sf.file_size_bytes, \
'format', sf.file_format \
'format', sf.file_format, \
'instrument', sl.instrument \
)) ORDER BY sl.id, sf.id) AS manifest, \
( SELECT SUM(COALESCE(l2.base_count, l2.reads::bigint * l2.read_length::bigint))::bigint \
FROM genomics.sequence_library l2 WHERE l2.sample_guid = s.sample_guid ) AS est_bases, \
Expand Down
4 changes: 4 additions & 0 deletions rust/crates/du-db/tests/grid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,10 @@ async fn curation_projects_crawled_samples_into_work_units() {
"ftp.sra.ebi.ac.uk/vol1/run/ERR200/s1.cram.crai"
);
assert_eq!(cram.manifest[0]["md5"], "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
assert_eq!(
cram.manifest[0]["instrument"], "Illumina NovaSeq 6000",
"the node needs the instrument to choose a mapper preset"
);
assert_eq!(cram.total_bytes, Some(12_000_000_000));
assert_eq!(
cram.est_bases,
Expand Down
14 changes: 14 additions & 0 deletions rust/crates/du-jobs/src/grid_validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,20 @@ pub async fn validate(pool: &PgPool, spot_check_rate: f64) -> anyhow::Result<Val
let clusters = cluster(&subs);
let winner = &clusters[0];

// A digest with no comparable call agrees with every other such digest, because every
// field is absent on both sides. Two nodes whose analysis failed would therefore reach a
// quorum on nothing and be paid for it. The node is expected to fail its unit instead of
// sending an empty digest, and the AppView must not depend on that: a node is untrusted by
// construction, which is the premise adaptive replication rests on.
if !digest::has_content(&subs[winner[0]].digest) {
tracing::warn!(
unit = %unit.sample_accession,
submissions = winner.len(),
"grid-validate: the agreeing digests carry no calls; not canonical"
);
continue;
}

// Distinct DIDs, not distinct rows. The unique index already makes them the same thing;
// relying on it silently would leave this correct only by coincidence.
let dids: HashSet<&str> = winner.iter().map(|&i| subs[i].did.as_str()).collect();
Expand Down
Loading