diff --git a/rust/crates/du-db/src/denovo.rs b/rust/crates/du-db/src/denovo.rs index 7105bcfb..c8b4c322 100644 --- a/rust/crates/du-db/src/denovo.rs +++ b/rust/crates/du-db/src/denovo.rs @@ -165,9 +165,36 @@ fn confidence(support: Option) -> &'static str { } } +/// Named Y catalog rows that carry **no `hs1` coordinate**. This is a load-blocking +/// precondition, not a statistic: [`resolve_variant`] / [`prefill_vcache`] reuse a +/// catalog row only by matching `coordinates @> {'hs1': …}`, so a named variant that +/// hasn't been lifted to hs1 yet is *invisible* to the match — and the loader quietly +/// mints a second, coordinate-named row for a marker YBrowse already named. +/// +/// The failure is silent and permanent: the duplicate is what gets linked to the branch, +/// so the marker's real name is stranded on an unlinked catalog row and the branch row +/// lands in the curator naming queue as if it were a novel discovery. Getting this wrong +/// once produced ~130k such duplicates (ybrowse-ingest → de-novo load → variant-coord-lift, +/// when the lift has to come *second*). +pub async fn unlifted_y_catalog_count(pool: &PgPool) -> Result { + Ok(sqlx::query_scalar( + "SELECT count(*) FROM core.variant \ + WHERE defining_haplogroup_id IS NULL \ + AND canonical_name IS NOT NULL AND canonical_name NOT LIKE 'chr%:%' \ + AND NOT (coordinates ? 'hs1') \ + AND (coordinates->'GRCh38'->>'contig' = 'chrY' \ + OR coordinates->'GRCh37'->>'contig' = 'chrY')", + ) + .fetch_one(pool) + .await?) +} + /// Load a de-novo tree document as the tree foundation. Assumes `tree.*` is /// already cleared (greenfield). Commits the topology, then recomputes backbone /// and bumps the tree revision. +/// +/// A Y load **refuses to start** unless the Y catalog is fully hs1-lifted — see +/// [`unlifted_y_catalog_count`]. Run `du-jobs run-once variant-coord-lift` first. pub async fn load(pool: &PgPool, doc: &DenovoTree, keep_private: bool) -> Result { let dna: DnaType = match doc.haplogroup_type.as_str() { "Y_DNA" => DnaType::YDna, @@ -177,6 +204,18 @@ pub async fn load(pool: &PgPool, doc: &DenovoTree, keep_private: bool) -> Result let dna_label = pg_enum_label(&dna)?; let mut rep = LoadReport::default(); + if matches!(dna, DnaType::YDna) { + let unlifted = unlifted_y_catalog_count(pool).await?; + if unlifted > 0 { + return Err(DbError::Conflict(format!( + "{unlifted} named Y catalog variants have no hs1 coordinate — the de-novo \ + catalog match is hs1-only, so loading now would mint a duplicate \ + coordinate-named row for each of them instead of reusing its name. \ + Run `du-jobs run-once variant-coord-lift` first, then re-run this load." + ))); + } + } + let mut tx = pool.begin().await?; // 1. Resolve every defining SNP to a core.variant id (catalog reuse or mint), diff --git a/rust/crates/du-db/src/naming.rs b/rust/crates/du-db/src/naming.rs index 1d864323..9e96451d 100644 --- a/rust/crates/du-db/src/naming.rs +++ b/rust/crates/du-db/src/naming.rs @@ -188,12 +188,16 @@ pub fn established_name(aliases: &Value) -> Option { .map(str::to_string) } -/// **Adopt an established name**: ratify a variant's existing ISOGG/YBrowse name -/// (a non-DU `common_names` alias) as its canonical name instead of minting a new -/// `DU` identifier. This is the "named by definition" path — when a matching -/// locus + mutation state already has a name in the source set, the authority -/// reuses it. Sets `canonical_name` to that name and marks `NAMED`. Errors if -/// the variant is already named or carries no established name. +/// **Adopt an established name**: ratify the name this variant already has — an +/// ISOGG/YBrowse `common_names` alias, or the name of a variant at the same locus + +/// mutation state — as its canonical name, instead of minting a new `DU` identifier. +/// This is the "named by definition" path: a marker the source set already named must +/// not get a second, DecodingUs-invented identity. Sets `canonical_name` and marks +/// `NAMED`. Errors if the variant is already named or no name is available to adopt. +/// +/// Both name sources are resolved here, in [`adoptable_name`]'s order — the site twin +/// is not a fallback nicety but the *only* source for a de-novo coordinate row, which +/// carries no aliases of its own. pub async fn adopt_established_name(pool: &PgPool, id: i64) -> Result { let mut tx = pool.begin().await?; let row: Option<(Option, Value)> = sqlx::query_as( @@ -210,8 +214,19 @@ pub async fn adopt_established_name(pool: &PgPool, id: i64) -> Result n, + None => sqlx::query_as::<_, (i64, String)>(SITE_TWIN_SQL) + .bind(id) + .fetch_all(&mut *tx) + .await? + .into_iter() + .next() + .map(|(_, n)| n) + .ok_or_else(|| { + DbError::Conflict("variant has no established name to adopt".into()) + })?, + }; // Canonical identity is (name + defining branch) — the recurrence model that // replaces ISOGG's L270.1/L270.2 suffixing: the same SNP name is canonical on // each branch it defines, scoped by `defining_haplogroup_id`. A clash only @@ -243,34 +258,185 @@ pub async fn adopt_established_name(pool: &PgPool, id: i64) -> ResultC -/// vs Y17125 A>G), so a same-position-different-allele variant is NOT a duplicate. +/// Other **named** variants at the same locus **and mutation state** — contig + +/// position + ancestral + derived — on this variant's preferred build (`hs1` first, +/// else `GRCh38`). Matching by locus alone is wrong: two distinct SNPs can share a +/// position with different alleles (e.g. Z12236 A>C vs Y17125 A>G), so a +/// same-position-different-allele variant is NOT a duplicate. +/// +/// Coordinate placeholders (`chrY:…`) are excluded: a placeholder is not a name, and +/// without this filter the authority reports "a named variant already exists at this +/// locus: `chrY:10249542C>G`" — contradicting every other path in this module, which +/// treats that string as *unnamed* (see [`is_placeholder_name`]). +/// +/// The ordering **is the adoption preference** — the head of this list is what +/// [`adopt_established_name`] ratifies: the catalog representative first, then an +/// external name over one of our own `DU` mints, then alphabetical for determinism. +const SITE_TWIN_SQL: &str = "WITH b AS ( \ + SELECT CASE WHEN coordinates ? 'hs1' THEN 'hs1' \ + WHEN coordinates ? 'GRCh38' THEN 'GRCh38' END AS build \ + FROM core.variant WHERE id = $1), \ + me AS ( \ + SELECT b.build, \ + v.coordinates->b.build->>'contig' AS c, \ + v.coordinates->b.build->>'position' AS p, \ + v.coordinates->b.build->>'ancestral' AS a, \ + v.coordinates->b.build->>'derived' AS d \ + FROM core.variant v, b WHERE v.id = $1) \ + SELECT v.id, v.canonical_name FROM core.variant v, me \ + WHERE v.id <> $1 AND v.canonical_name IS NOT NULL \ + AND v.canonical_name NOT LIKE 'chr%:%' \ + AND me.build IS NOT NULL \ + AND me.c IS NOT NULL AND me.p IS NOT NULL \ + AND v.coordinates->me.build->>'contig' = me.c \ + AND v.coordinates->me.build->>'position' = me.p \ + AND v.coordinates->me.build->>'ancestral' IS NOT DISTINCT FROM me.a \ + AND v.coordinates->me.build->>'derived' IS NOT DISTINCT FROM me.d \ + ORDER BY v.catalog_representative DESC, (v.canonical_name LIKE 'DU%'), v.canonical_name \ + LIMIT 10"; + +/// **Dedup check** before naming — see [`SITE_TWIN_SQL`]. pub async fn dedup_by_site(pool: &PgPool, id: i64) -> Result, DbError> { - Ok(sqlx::query_as( - "WITH b AS ( \ - SELECT CASE WHEN coordinates ? 'hs1' THEN 'hs1' \ - WHEN coordinates ? 'GRCh38' THEN 'GRCh38' END AS build \ - FROM core.variant WHERE id = $1), \ - me AS ( \ - SELECT b.build, \ - v.coordinates->b.build->>'contig' AS c, \ - v.coordinates->b.build->>'position' AS p, \ - v.coordinates->b.build->>'ancestral' AS a, \ - v.coordinates->b.build->>'derived' AS d \ - FROM core.variant v, b WHERE v.id = $1) \ - SELECT v.id, v.canonical_name FROM core.variant v, me \ - WHERE v.id <> $1 AND v.canonical_name IS NOT NULL AND me.build IS NOT NULL \ - AND me.c IS NOT NULL AND me.p IS NOT NULL \ - AND v.coordinates->me.build->>'contig' = me.c \ - AND v.coordinates->me.build->>'position' = me.p \ - AND v.coordinates->me.build->>'ancestral' IS NOT DISTINCT FROM me.a \ - AND v.coordinates->me.build->>'derived' IS NOT DISTINCT FROM me.d \ - ORDER BY v.canonical_name LIMIT 10", - ) - .bind(id) - .fetch_all(pool) - .await?) + Ok(sqlx::query_as(SITE_TWIN_SQL).bind(id).fetch_all(pool).await?) +} + +/// The name this variant would **adopt**, from the two places a real name can live: +/// its own established (ISOGG/YBrowse) alias, else a named variant at the same locus + +/// mutation state ([`dedup_by_site`]). +/// +/// The second source is the one that matters in practice. The de-novo loader mints its +/// coordinate-named rows with **no aliases at all**, so the alias source never fires for +/// them — a de-novo branch row's real name lives only on the separate catalog row at the +/// same site. Consulting aliases alone made the UI offer no way to act on its own "a named +/// variant already exists here" warning. +/// +/// Resolution order must stay in step with [`adopt_established_name`], which decides what +/// the button actually writes. +pub async fn adoptable_name( + pool: &PgPool, + id: i64, + aliases: &Value, +) -> Result, DbError> { + if let Some(n) = established_name(aliases) { + return Ok(Some(n)); + } + Ok(dedup_by_site(pool, id).await?.into_iter().next().map(|(_, n)| n)) +} + +/// Outcome of [`reconcile_placeholder_names`]. +#[derive(Debug, Default)] +pub struct NameReconcile { + /// Coordinate-placeholder rows examined. + pub scanned: i64, + /// Of those, ones that have a named variant at the same hs1 site + mutation state. + pub matched: i64, + /// Rows folded onto that existing name (`NAMED`). + pub adopted: i64, + /// Matched but **not** written: the name is already canonical for that same branch. + /// That is a real duplicate or an unmodelled recurrence — a merge-review question, + /// not something to resolve by overwriting. Left in the queue deliberately. + pub conflicted: i64, +} + +/// One batch of the reconcile below: take the next `$1` placeholder rows after id `$2`, +/// find each one's named twin at the same hs1 site + mutation state, and adopt that name +/// where doing so doesn't collide with the `(name + defining branch)` unique key. +/// +/// Scoped to **branch-defining** rows — the same population as the `needs_name` queue, so +/// what this clears is exactly what a curator would otherwise have to clear by hand. The +/// loader also leaves behind coordinate-named rows that define *no* branch and are linked +/// to nothing (94.6k of them at last count, ~67k of which duplicate an already-named +/// catalog row). Those are catalog dead weight, not naming work: they never surface in the +/// queue, and resolving them means merging rows ([`crate::merge`]), not renaming one — a +/// rename would just collide with the catalog row on `(name, NULL)`. Left alone here +/// deliberately; folding them is a separate cleanup. +/// +/// The `ok` CTE deduplicates *within* the batch as well as against the table — two +/// placeholder rows on one branch can resolve to the same name when the catalog itself +/// carries that name at more than one site, and the second write would violate +/// `variant_canonical_name_key`. +const RECONCILE_BATCH_SQL: &str = "WITH win AS ( \ + SELECT v.id, v.defining_haplogroup_id AS branch, v.coordinates->'hs1' AS hs1 \ + FROM core.variant v \ + WHERE v.canonical_name LIKE 'chr%:%' AND v.coordinates ? 'hs1' \ + AND v.defining_haplogroup_id IS NOT NULL AND v.id > $2 \ + ORDER BY v.id LIMIT $1), \ + cand AS ( \ + SELECT DISTINCT ON (w.id) w.id, w.branch, n.canonical_name AS name \ + FROM win w \ + JOIN core.variant n \ + ON n.id <> w.id \ + AND n.canonical_name IS NOT NULL \ + AND n.canonical_name NOT LIKE 'chr%:%' \ + AND n.coordinates ? 'hs1' \ + AND n.coordinates->'hs1'->>'contig' = w.hs1->>'contig' \ + AND n.coordinates->'hs1'->>'position' = w.hs1->>'position' \ + AND n.coordinates->'hs1'->>'ancestral' = w.hs1->>'ancestral' \ + AND n.coordinates->'hs1'->>'derived' = w.hs1->>'derived' \ + ORDER BY w.id, n.catalog_representative DESC, (n.canonical_name LIKE 'DU%'), n.canonical_name), \ + ok AS ( \ + SELECT DISTINCT ON (c.name, COALESCE(c.branch, -1)) c.id, c.name \ + FROM cand c \ + WHERE NOT EXISTS ( \ + SELECT 1 FROM core.variant o \ + WHERE o.canonical_name = c.name \ + AND COALESCE(o.defining_haplogroup_id, -1) = COALESCE(c.branch, -1)) \ + ORDER BY c.name, COALESCE(c.branch, -1), c.id), \ + upd AS ( \ + UPDATE core.variant v \ + SET canonical_name = ok.name, naming_status = 'NAMED', updated_at = now() \ + FROM ok WHERE v.id = ok.id \ + RETURNING v.id) \ + SELECT (SELECT max(id) FROM win) AS cursor, \ + (SELECT count(*) FROM win) AS scanned, \ + (SELECT count(*) FROM cand) AS matched, \ + (SELECT count(*) FROM upd) AS adopted"; + +/// **Fold coordinate-placeholder variants onto the name they already have.** +/// +/// The de-novo loader mints a `chrY:10249542C>G` row whenever its hs1 catalog match +/// misses, and that row — not the named catalog row — is what gets linked to the branch. +/// Every such row then surfaces in the curator naming queue as if it were a novel +/// discovery, where the only offered action (Mint DU name) would give a marker YBrowse +/// already named a second, conflicting identity. +/// +/// This job resolves them the way a curator would: adopt the existing name. It is the bulk +/// form of [`adopt_established_name`] and uses the same site match and same preference +/// order, so a row cleared here and a row cleared by the button get the same name. +/// +/// Idempotent and safe to re-run: an adopted row no longer matches `chr%:%`, so it drops +/// out of the scan. Worth running **recurrently**, not just once — a YBrowse ingest that +/// names a marker after a tree load creates exactly this state again, and the fix is the +/// same fold. Rows whose name is already taken on their branch are counted in +/// `conflicted` and left alone for merge review. +/// +/// Batched by id cursor so it never long-locks the catalog. +pub async fn reconcile_placeholder_names( + pool: &PgPool, + batch: i64, +) -> Result { + let batch = batch.clamp(1, 50_000); + let mut rep = NameReconcile::default(); + let mut cursor: i64 = 0; + loop { + let (next, scanned, matched, adopted): (Option, i64, i64, i64) = + sqlx::query_as(RECONCILE_BATCH_SQL) + .bind(batch) + .bind(cursor) + .fetch_one(pool) + .await?; + rep.scanned += scanned; + rep.matched += matched; + rep.adopted += adopted; + rep.conflicted += matched - adopted; + // `win` empty ⇒ no placeholder rows past the cursor ⇒ done. Advancing the cursor + // past every *scanned* row (not just adopted ones) is what makes this terminate: + // conflicted rows are never written, so a cursor keyed on progress-made would + // re-select them forever. + match next { + Some(id) => cursor = id, + None => break, + } + } + Ok(rep) } diff --git a/rust/crates/du-db/tests/variant_naming.rs b/rust/crates/du-db/tests/variant_naming.rs index 28393342..5bf139a0 100644 --- a/rust/crates/du-db/tests/variant_naming.rs +++ b/rust/crates/du-db/tests/variant_naming.rs @@ -51,6 +51,135 @@ async fn mk_variant( .expect("insert variant") } +/// An **hs1**-framed variant — the frame the de-novo loader and the site match work in. +async fn mk_hs1_variant( + pool: &PgPool, + name: Option<&str>, + status: &str, + pos: i64, + anc: &str, + der: &str, + defining: Option, +) -> i64 { + let coords = serde_json::json!({ + "hs1": { "contig": "chrY", "position": pos, "ancestral": anc, "derived": der } + }); + sqlx::query_scalar( + "INSERT INTO core.variant (canonical_name, mutation_type, naming_status, coordinates, defining_haplogroup_id) \ + VALUES ($1, 'SNP'::core.mutation_type, $2::core.naming_status, $3, $4) RETURNING id", + ) + .bind(name) + .bind(status) + .bind(coords) + .bind(defining) + .fetch_one(pool) + .await + .expect("insert hs1 variant") +} + +/// The real-world shape the authority got wrong: one physical SNP stored as **two rows** — +/// a named, branch-less catalog row (YBrowse) and the de-novo loader's coordinate-named +/// branch row, which carries *no aliases at all*. The name lives only on the sibling. +/// +/// Regression cover for three defects: the dedup warning offering a placeholder as a +/// "named variant"; the Reuse button never appearing for exactly the rows the warning +/// fires on (aliases-only name source ⇒ Mint DU name, which forks the marker's identity, +/// was the only action on screen); and no bulk path to fold the ~130k such rows a single +/// mis-ordered load produced. +#[tokio::test] +async fn adopts_name_from_site_twin_not_just_aliases() { + let Some(url) = database_url() else { + eprintln!("DATABASE_URL unset — skipping site-twin naming test"); + return; + }; + let db = du_db::testing::ephemeral_db(&url).await.expect("ephemeral db"); + let pool = db.pool().clone(); + + let branch = mk_hg(&pool, "TEST-TWIN-A").await; + let branch2 = mk_hg(&pool, "TEST-TWIN-B").await; + let branch3 = mk_hg(&pool, "TEST-TWIN-C").await; + + // The catalog row: named, defines no branch (YBrowse reference data). + let catalog = mk_hs1_variant(&pool, Some("TESTFT186008"), "NAMED", 10249542, "C", "G", None).await; + // The de-novo row: same site + mutation state, placeholder name, empty aliases, + // and it is the one actually wired to the branch. + let denovo = + mk_hs1_variant(&pool, Some("chrY:10249542C>G"), "UNNAMED", 10249542, "C", "G", Some(branch)).await; + // A same-position, different-state SNP must never be treated as the same marker. + let other_state = + mk_hs1_variant(&pool, Some("TESTOTHER"), "NAMED", 10249542, "C", "T", None).await; + + // The de-novo row is naming work… + let q = du_db::naming::queue(&pool, "needs_name", 1, 500).await.expect("queue"); + assert!(q.items.iter().any(|i| i.id == denovo), "placeholder branch row is in the queue"); + + // …and the site match finds its real name on the sibling row. + let dups = du_db::naming::dedup_by_site(&pool, denovo).await.expect("dedup"); + assert!( + dups.iter().any(|(id, n)| *id == catalog && n == "TESTFT186008"), + "site twin found: {dups:?}" + ); + assert!(!dups.iter().any(|(id, _)| *id == other_state), "different mutation state is not a twin"); + // A placeholder is not a name and must never be reported as one. + assert!( + !dups.iter().any(|(_, n)| du_db::naming::is_placeholder_name(n)), + "dedup must not offer a coordinate placeholder as an existing name: {dups:?}" + ); + + // The row has no aliases, so the old aliases-only lookup found nothing to reuse… + let d = du_db::naming::get(&pool, denovo).await.unwrap().unwrap(); + assert!(du_db::naming::established_name(&d.aliases).is_none(), "no aliases on a de-novo row"); + // …but the name is adoptable all the same, because the twin has it. + let offer = du_db::naming::adoptable_name(&pool, denovo, &d.aliases).await.expect("adoptable"); + assert_eq!(offer.as_deref(), Some("TESTFT186008"), "the Reuse offer is backed by the site twin"); + + // Adopting writes that name — no DU identifier is minted for a marker already named. + let adopted = du_db::naming::adopt_established_name(&pool, denovo).await.expect("adopt"); + assert_eq!(adopted, "TESTFT186008"); + let d = du_db::naming::get(&pool, denovo).await.unwrap().unwrap(); + assert_eq!(d.canonical_name.as_deref(), Some("TESTFT186008")); + assert_eq!(d.naming_status, "NAMED"); + // (name + defining branch) is the canonical identity: the same name on the branch-less + // catalog row and on branch A coexist by design. + let q = du_db::naming::queue(&pool, "needs_name", 1, 500).await.expect("queue"); + assert!(!q.items.iter().any(|i| i.id == denovo), "adopted row leaves the queue"); + + // ── bulk fold ──────────────────────────────────────────────────────────────── + // Two more de-novo duplicates: one adoptable, one whose name is already canonical on + // its own branch (a true duplicate / unmodelled recurrence — must NOT be overwritten). + let dn2 = + mk_hs1_variant(&pool, Some("chrY:10249542C>G"), "UNNAMED", 10249542, "C", "G", Some(branch2)).await; + let taken = + mk_hs1_variant(&pool, Some("TESTFT186008"), "NAMED", 10249542, "C", "G", Some(branch3)).await; + let dn3 = + mk_hs1_variant(&pool, Some("chrY:10249542C>G"), "UNNAMED", 10249542, "C", "G", Some(branch3)).await; + + let r = du_db::naming::reconcile_placeholder_names(&pool, 2).await.expect("reconcile"); + assert!(r.adopted >= 1, "folded the adoptable duplicate: {r:?}"); + assert!(r.conflicted >= 1, "left the already-taken name for merge review: {r:?}"); + + let g2 = du_db::naming::get(&pool, dn2).await.unwrap().unwrap(); + assert_eq!(g2.canonical_name.as_deref(), Some("TESTFT186008"), "dn2 folded onto the catalog name"); + assert_eq!(g2.naming_status, "NAMED"); + let g3 = du_db::naming::get(&pool, dn3).await.unwrap().unwrap(); + assert_eq!( + g3.canonical_name.as_deref(), + Some("chrY:10249542C>G"), + "dn3's name is already canonical on branch3 — left alone, not silently overwritten" + ); + + // Idempotent: a second pass finds nothing new to adopt. + let again = du_db::naming::reconcile_placeholder_names(&pool, 2).await.expect("reconcile again"); + assert_eq!(again.adopted, 0, "re-running adopts nothing: {again:?}"); + + for id in [denovo, dn2, dn3, taken, catalog, other_state] { + let _ = sqlx::query("DELETE FROM core.variant WHERE id = $1").bind(id).execute(&pool).await; + } + for hg in [branch, branch2, branch3] { + let _ = sqlx::query("DELETE FROM tree.haplogroup WHERE id = $1").bind(hg).execute(&pool).await; + } +} + #[tokio::test] async fn variant_naming_authority_flow() { let Some(url) = database_url() else { diff --git a/rust/crates/du-jobs/src/main.rs b/rust/crates/du-jobs/src/main.rs index ee8f1f76..4a977c0d 100644 --- a/rust/crates/du-jobs/src/main.rs +++ b/rust/crates/du-jobs/src/main.rs @@ -111,6 +111,26 @@ async fn main() -> anyhow::Result<()> { let fixed = du_db::variant::normalize_naming_status(&pool).await?; tracing::info!(fixed, "naming-status-normalize complete"); } + // Fold de-novo coordinate-placeholder variants (chrY:10249542C>G) onto the + // name a catalog row already carries at the same hs1 site + mutation state. + // These are the loader's duplicates for markers YBrowse had already named; + // without this they sit in the curator naming queue looking like novel + // discoveries whose only offered action would fork the marker's identity. + // Idempotent — re-run after any ybrowse ingest that names a marker post-load. + "variant-name-reconcile" => { + let r = du_db::naming::reconcile_placeholder_names(&pool, 5_000).await?; + tracing::info!( + scanned = r.scanned, matched = r.matched, adopted = r.adopted, + conflicted = r.conflicted, "variant-name-reconcile complete" + ); + if r.conflicted > 0 { + tracing::warn!( + conflicted = r.conflicted, + "placeholders whose name is already canonical on their branch — \ + left for merge review (duplicate or unmodelled recurrence)" + ); + } + } // Load the T2T-CHM13v2.0 (hs1) Y structural regions (AZF/DYZ/ // ampliconic/palindrome/inverted-repeat) into core.genome_region. // One-shot: the source BEDs are tiny + versioned, not a daily feed. @@ -322,7 +342,7 @@ async fn main() -> anyhow::Result<()> { ena::enrich_studies(&pool, &client).await?; } other => anyhow::bail!( - "unknown run-once job '{other}' (known: ybrowse, reconcile, variant-representatives, yregions, branch-age, ftdna-str, sequencer-consensus, discovery-consensus, coverage-norms, ibd-discovery-recompute, exchange-expire, tree-samples-recompute, dedup-candidates, consolidate-donors, link-federated-subjects, import-kit-identifiers, mt-rcrs-lift, variant-coord-lift, crawl-project, publication-topic-prune, name-private-nodes, publication-update, publication-discovery, publication-pubmed-update, ena-study-enrichment)" + "unknown run-once job '{other}' (known: ybrowse, reconcile, variant-representatives, naming-status-normalize, variant-name-reconcile, yregions, branch-age, ftdna-str, sequencer-consensus, discovery-consensus, coverage-norms, ibd-discovery-recompute, exchange-expire, tree-samples-recompute, dedup-candidates, consolidate-donors, link-federated-subjects, import-kit-identifiers, mt-rcrs-lift, variant-coord-lift, crawl-project, publication-topic-prune, name-private-nodes, publication-update, publication-discovery, publication-pubmed-update, ena-study-enrichment)" ), } return Ok(()); diff --git a/rust/crates/du-web/src/routes/naming.rs b/rust/crates/du-web/src/routes/naming.rs index 112b978c..d74e08f4 100644 --- a/rust/crates/du-web/src/routes/naming.rs +++ b/rust/crates/du-web/src/routes/naming.rs @@ -230,10 +230,15 @@ async fn build_detail(st: &AppState, id: i64, notice: Option) -> Result< // "already named". let placeholder = i.canonical_name.as_deref().is_some_and(du_db::naming::is_placeholder_name); let can_assign = i.naming_status != "NAMED" || placeholder; - // "Named by definition": the variant already carries an established (non-DU) - // name for its locus+mutation-state — reuse it rather than mint a new DU id. + // "Named by definition": a real name for this locus + mutation state already exists — + // on the variant's own aliases, or on the catalog row at the same site. Reuse it rather + // than mint a DU id. Sourced from `adoptable_name` (not `established_name`) so that the + // offer is backed by the same lookup as the dedup warning above: a de-novo coordinate + // row has no aliases, so the aliases-only source left the curator staring at "a named + // variant already exists" with no button that could reuse it, and Mint DU name — which + // would fork the marker's identity — as the only action on screen. let established = if can_assign && (i.canonical_name.is_none() || placeholder) { - du_db::naming::established_name(&i.aliases) + du_db::naming::adoptable_name(&st.pool, id, &i.aliases).await? } else { None }; diff --git a/rust/docs/variant-name-reconcile-deploy.md b/rust/docs/variant-name-reconcile-deploy.md new file mode 100644 index 00000000..db52fd7c --- /dev/null +++ b/rust/docs/variant-name-reconcile-deploy.md @@ -0,0 +1,211 @@ +# Variant name reconcile — production deployment + +Deployment notes for the fix to the Variant Naming Authority duplicate-name defect. +Verified end-to-end on `decodingus_cutover` on 2026-07-13; **not yet applied to production.** + +## What was wrong + +One physical SNP was stored as two `core.variant` rows: the named YBrowse catalog row +(`FT186008`, defines no branch) and a de-novo coordinate row (`chrY:10249542C>G`, defines +branch `R-BY18864`). The coordinate row is the one linked to the branch, so the marker's +real name was stranded on an unlinked row and the branch row surfaced in the curator +naming queue as if it were a novel discovery. + +**The loader's reuse check was never missing — it ran too early.** `resolve_variant` / +`prefill_vcache` (`du-db/src/denovo.rs`) match the catalog by `coordinates @> {'hs1': …}`. +The Y catalog's `hs1` coordinates are populated by a *separate later job*, +`variant-coord-lift`. The cutover ran: + +``` +ybrowse-ingest → de-novo load → variant-coord-lift ← WRONG +``` + +At load time `FT186008` had only GRCh37/GRCh38 coords, so the hs1 match found nothing and +the loader minted a duplicate. It needed to be: + +``` +ybrowse-ingest → variant-coord-lift → de-novo load ← CORRECT +``` + +That single mis-ordering produced ~130k duplicate rows and left the naming queue 86% noise. +Worse, the curator UI showed those rows with a "a named variant already exists here" +warning but **no button that could reuse it** — the Reuse offer read the row's own +`aliases`, which a de-novo row does not have. The only actions on screen were *Mint DU +name*, which would have given an already-named marker a second conflicting identity, and +*Flag for review*. + +## What shipped + +| Change | File | +|---|---| +| Y load refuses to start unless the Y catalog is hs1-lifted (`unlifted_y_catalog_count`) | `du-db/src/denovo.rs` | +| `dedup_by_site` excludes coordinate placeholders; ordering is the adoption preference | `du-db/src/naming.rs` | +| `adoptable_name()` — the Reuse offer now reads the site twin, not just aliases | `du-db/src/naming.rs` | +| `adopt_established_name()` writes from either source | `du-db/src/naming.rs` | +| `reconcile_placeholder_names()` — bulk fold, idempotent, id-cursor batched | `du-db/src/naming.rs` | +| `run-once variant-name-reconcile` | `du-jobs/src/main.rs` | +| Two indexes for the site lookup (was a full scan of 3.1M rows per panel click) | `migrations/0068_variant_hs1_site_index.sql` | +| Nightly wiring, between `resync-ybrowse` and `name-private-nodes` | `scripts/nightly-maintenance.sh` | +| Dedup hint text (all three locales) | `locales/{en,es,fr}.txt` | + +## Cutover result (the rehearsal) + +``` +scanned 157,056 matched 130,553 adopted 130,553 conflicted 0 +needs_name queue: 151,318 → 20,765 +``` + +Zero conflicts — no genuine `(name + branch)` collisions exist. Re-running adopts 0. + +--- + +## Production steps + +### 0. Decide which situation you're in + +If production is going to be **replaced by a fresh ship of the now-fixed cutover DB**, the +data half of this is already done — skip to steps 2 and 4 (code, migration, nightly job) +and skip the backfill entirely. + +If production is **already carrying data loaded by the old sequence**, it has the same +duplicates. Confirm and size it: + +```sql +-- How many branch-defining placeholder rows have a named twin at the same hs1 site? +SELECT count(*) FROM core.variant v +WHERE v.canonical_name LIKE 'chr%:%' + AND v.coordinates ? 'hs1' + AND v.defining_haplogroup_id IS NOT NULL + AND EXISTS ( + SELECT 1 FROM core.variant n + WHERE n.id <> v.id + AND n.canonical_name IS NOT NULL AND n.canonical_name NOT LIKE 'chr%:%' + AND n.coordinates ? 'hs1' + AND n.coordinates->'hs1'->>'contig' = v.coordinates->'hs1'->>'contig' + AND n.coordinates->'hs1'->>'position' = v.coordinates->'hs1'->>'position' + AND n.coordinates->'hs1'->>'ancestral' = v.coordinates->'hs1'->>'ancestral' + AND n.coordinates->'hs1'->>'derived' = v.coordinates->'hs1'->>'derived'); +``` + +Expect a number near 130,553. **Run this before the indexes exist and it is a full scan of +a 4.7 GB table** — give it minutes, and don't kill the client and assume the query died: +a `pkill` on the shell leaves the backend running server-side. Check +`pg_stat_activity` and use `pg_terminate_backend()` if you need to stop it. + +### 1. Pre-create the indexes CONCURRENTLY — *before* deploying + +`core.variant` is **4.7 GB / 3.15M rows**. Migration `0068` uses plain `CREATE INDEX`, +which takes a `SHARE` lock and **blocks all writes to `core.variant` for the whole build**. +`du-web` runs migrations on boot (`du-web/src/main.rs:61`), so deploying without this step +stalls variant writes during startup. + +Build them concurrently against live prod first. `0068` is written with `IF NOT EXISTS`, so +the migration then finds them present and is a no-op — this is exactly the sequence used on +cutover. + +```sql +-- Not in a transaction. Safe on a live table; no write lock. +CREATE INDEX CONCURRENTLY IF NOT EXISTS variant_hs1_site_named_idx + ON core.variant ((coordinates -> 'hs1' ->> 'contig'), + (coordinates -> 'hs1' ->> 'position'), + (coordinates -> 'hs1' ->> 'ancestral'), + (coordinates -> 'hs1' ->> 'derived')) + WHERE coordinates ? 'hs1' + AND canonical_name IS NOT NULL + AND canonical_name NOT LIKE 'chr%:%'; + +CREATE INDEX CONCURRENTLY IF NOT EXISTS variant_placeholder_branch_idx + ON core.variant (id) + WHERE canonical_name LIKE 'chr%:%' AND defining_haplogroup_id IS NOT NULL; + +ANALYZE core.variant; +``` + +The index expressions must stay **character-identical** to the query predicates or the +planner will not use them. If a `CONCURRENTLY` build fails it leaves an `INVALID` index — +check `pg_index.indisvalid`, `DROP` it, and retry. + +### 2. Deploy the code + +Migration `0068` applies as a no-op given step 1. Nothing else in the migration touches +data. + +### 3. Backfill (skip if prod is a fresh ship from fixed cutover) + +```bash +DATABASE_URL=postgres://... /opt/decoding-us/bin/decodingus-jobs run-once variant-name-reconcile +``` + +Took ~30s on cutover. Batched by id cursor (5,000/batch), so it never long-locks the +catalog — safe to run against a live prod. Idempotent: re-running is free. + +Expect `conflicted=0`. **If `conflicted > 0`, stop and look at it** — it means a name is +already canonical on the same branch, i.e. a true duplicate or an unmodelled recurrence. +The job deliberately leaves those rows alone rather than overwrite; they need merge review. + +### 4. Verify + +```sql +-- The naming queue should collapse to genuinely novel variants only. +SELECT count(*) FROM core.variant v +WHERE v.defining_haplogroup_id IS NOT NULL + AND EXISTS (SELECT 1 FROM tree.haplogroup h + WHERE h.id = v.defining_haplogroup_id AND h.haplogroup_type = 'Y_DNA') + AND (v.canonical_name IS NULL OR v.canonical_name LIKE 'chr%:%' + OR v.naming_status = 'PENDING_REVIEW'); +``` + +Then open **Curator → Variant Naming Authority** and click a row. Any row still showing the +"a named variant already exists at this locus" warning must now also show a green +**Reuse ⟨name⟩** button. Warning without a Reuse button is the original bug. + +### 5. Nightly job + +`scripts/nightly-maintenance.sh` already runs `variant-name-reconcile` between +`resync-ybrowse` and `name-private-nodes`. No systemd change needed — the timer invokes the +script, not individual jobs. Confirm it appears in the next nightly log. + +It has to stay recurring, not one-shot: a YBrowse refresh that names a marker **after** a +tree load recreates exactly this state, and the fix is the same fold. It must run *before* +`name-private-nodes`, which picks node names from the variants on a branch and can only see +a name once the variant actually carries it. + +--- + +## The durable rule + +**`variant-coord-lift` must complete before any de-novo Y tree load.** This is now enforced +— `denovo::load` refuses to start for a Y tree while any named Y catalog row lacks an hs1 +coordinate, and names the count. If you see: + +``` +N named Y catalog variants have no hs1 coordinate — the de-novo catalog match is hs1-only, +so loading now would mint a duplicate coordinate-named row for each of them instead of +reusing its name. Run `du-jobs run-once variant-coord-lift` first, then re-run this load. +``` + +that guard just saved you another 130k duplicates. Do not work around it. + +## Known residue (not fixed here) + +**94,576 branch-less coordinate rows**, ~67k of which duplicate an already-named catalog +row. They define no branch and are linked to nothing (`tree.haplogroup_variant` and +`defining_haplogroup_id` agree exactly — 157,056 linked / 94,576 unlinked, no overlap), so +they never reach the naming queue and no curator ever sees them. They are catalog dead +weight: they inflate variant counts and every same-site lookup. + +They are deliberately **out of scope for the reconcile**, which is scoped to +`defining_haplogroup_id IS NOT NULL`. Renaming them cannot work — they would collide with +the catalog row on `(name, NULL)`. Resolving them means *merging rows* (`du-db/src/merge.rs`, +`merge_into`), which has FK implications. An unscoped reconcile reports these as 67,131 +"conflicts", which is misleading noise; that is why the job is scoped. + +Separate cleanup pass; file it before it becomes folklore. + +## Rollback + +The backfill overwrites `canonical_name` on branch-defining rows, replacing a synthetic +placeholder with a real name. The placeholder is deterministically reconstructible from the +row's own hs1 coordinates (`{contig}:{position}{ancestral}>{derived}`), so the write is +reversible in principle, but there is no automated undo. Take a snapshot before step 3 if +production data is not otherwise recoverable. diff --git a/rust/locales/en.txt b/rust/locales/en.txt index 3942733c..026227df 100644 --- a/rust/locales/en.txt +++ b/rust/locales/en.txt @@ -470,7 +470,7 @@ nm.type=Mutation type nm.defines=Defines branch nm.aliases=Known as nm.dedup.warn=A named variant already exists at this locus and mutation state: -nm.dedup.hint=Consider reusing it (add as an alias on the branch) instead of minting a new DU name. +nm.dedup.hint=This marker is already named. Reuse that name rather than minting a DU identifier, which would give it a second, conflicting identity. nm.adopt=Reuse nm.adopt.confirm=Reuse the established name nm.assign=Mint DU name diff --git a/rust/locales/es.txt b/rust/locales/es.txt index 2dce7212..ef00ada5 100644 --- a/rust/locales/es.txt +++ b/rust/locales/es.txt @@ -405,7 +405,7 @@ nm.type=Tipo de mutación nm.defines=Define la rama nm.aliases=Conocida como nm.dedup.warn=Ya existe una variante con nombre en este locus y estado de mutación: -nm.dedup.hint=Considere reutilizarla (añadirla como alias en la rama) en lugar de acuñar un nuevo nombre DU. +nm.dedup.hint=Este marcador ya tiene nombre. Reutilícelo en lugar de acuñar un identificador DU, que le daría una segunda identidad en conflicto. nm.adopt=Reutilizar nm.adopt.confirm=¿Reutilizar el nombre establecido nm.assign=Acuñar nombre DU diff --git a/rust/locales/fr.txt b/rust/locales/fr.txt index 432996f6..c7bcdf73 100644 --- a/rust/locales/fr.txt +++ b/rust/locales/fr.txt @@ -405,7 +405,7 @@ nm.type=Type de mutation nm.defines=Définit la branche nm.aliases=Connue sous nm.dedup.warn=Une variante nommée existe déjà à ce locus et état de mutation : -nm.dedup.hint=Envisagez de la réutiliser (l'ajouter comme alias sur la branche) au lieu de frapper un nouveau nom DU. +nm.dedup.hint=Ce marqueur porte déjà un nom. Réutilisez-le au lieu de frapper un identifiant DU, qui lui donnerait une seconde identité contradictoire. nm.adopt=Réutiliser nm.adopt.confirm=Réutiliser le nom établi nm.assign=Frapper un nom DU diff --git a/rust/migrations/0068_variant_hs1_site_index.sql b/rust/migrations/0068_variant_hs1_site_index.sql new file mode 100644 index 00000000..0ff283e1 --- /dev/null +++ b/rust/migrations/0068_variant_hs1_site_index.sql @@ -0,0 +1,37 @@ +-- Site lookup for the naming authority: "is there already a named variant at this +-- exact hs1 locus + mutation state?" +-- +-- DEPLOY NOTE: core.variant is ~4.7 GB / 3.1M rows. Plain CREATE INDEX takes a SHARE lock +-- and blocks writes to the table for the whole build, and du-web runs migrations on boot. +-- Build these CONCURRENTLY against live prod BEFORE deploying; the IF NOT EXISTS below then +-- makes this migration a no-op. See docs/variant-name-reconcile-deploy.md. +-- +-- This question is asked three ways and, until now, always by full scan of a ~3M-row +-- catalog: the curator dedup warning (du_db::naming::dedup_by_site), the Reuse-name +-- offer behind it (adoptable_name), and the placeholder reconcile job that folds +-- de-novo coordinate rows onto the name they already have +-- (naming::reconcile_placeholder_names, which probes once per placeholder row). +-- +-- Partial + expression index on the *named* side only — that is the side being probed, +-- and it excludes both NULL names and the loader's coordinate placeholders (chrY:…), +-- which are not names and must never satisfy a "already named here?" lookup. +-- The expressions must stay character-identical to the query predicates or the planner +-- will not use this index. +CREATE INDEX IF NOT EXISTS variant_hs1_site_named_idx + ON core.variant ( + (coordinates -> 'hs1' ->> 'contig'), + (coordinates -> 'hs1' ->> 'position'), + (coordinates -> 'hs1' ->> 'ancestral'), + (coordinates -> 'hs1' ->> 'derived') + ) + WHERE coordinates ? 'hs1' + AND canonical_name IS NOT NULL + AND canonical_name NOT LIKE 'chr%:%'; + +-- The probing side: the branch-defining placeholder rows the reconcile job walks in id +-- order. Scoped to defining_haplogroup_id IS NOT NULL to match the job (and the +-- needs_name queue) — the loader's branch-less coordinate rows are catalog dead weight, +-- not naming work, and are not scanned. +CREATE INDEX IF NOT EXISTS variant_placeholder_branch_idx + ON core.variant (id) + WHERE canonical_name LIKE 'chr%:%' AND defining_haplogroup_id IS NOT NULL; diff --git a/rust/scripts/nightly-maintenance.sh b/rust/scripts/nightly-maintenance.sh index fe60a683..dd97215c 100755 --- a/rust/scripts/nightly-maintenance.sh +++ b/rust/scripts/nightly-maintenance.sh @@ -29,7 +29,14 @@ step() { echo ">>> $*" >&2; if "$@"; then echo "<<< ok" >&2; else echo "!!! FAIL job() { step "${JOBS[@]}" run-once "$@"; } # 1. YBrowse catalog refresh (diff-based) + name the de-novo nodes it made namable. +# variant-name-reconcile sits BETWEEN the two on purpose. A refresh that names a marker +# the de-novo loader had already minted as a coordinate row (chrY:10249542C>G) leaves that +# row unnamed and sitting in the curator naming queue, where the only offered action would +# mint a DU id and fork the marker's identity. The reconcile folds it onto the name YBrowse +# just published — and it must run before name-private-nodes, which picks node names from +# the variants on a branch and can only see a name once the variant actually carries it. step "$HERE/resync-ybrowse.sh" +job variant-name-reconcile job name-private-nodes --apply # 2. External metadata enrichment (OpenAlex + PubMed + ENA). Skipped internally if the