feat!(prepro): validate user-submitted external accessions

[maverbiest]

resolves #6859, #7250

Breaking change

Non-INSDC ingested entries that provide accessions of umbrella projects in bioprojectAccession will be rejected after this change. Also accessions that don't match the following patterns will error:

ACCESSION_PATTERNS: dict[EnaAccessionType, re.Pattern[str]] = {
    EnaAccessionType.BIOPROJECT: re.compile(r"PRJ[EDN][A-Z][0-9]+"),
    EnaAccessionType.BIOSAMPLE: re.compile(r"SAM[EDN][A-Z]?[0-9]+"),
    EnaAccessionType.RAW_READS: re.compile(r"[EDS]RR[0-9]{6,}"),
}

Summary

This PR adds a validation for values provided by direct submitters in the bioprojectAccession, biosampleAccession, and insdcRawReadsAccession fields (validation is skipped for INSDC-ingested data). This is done by checking if the provided accessions are public on ENA. The ENA checking implementation is based on the ENAVisibilityChecker in ena-submission/src/ena_deposition/check_external_visibility.py

New behaviour summary:

• ENA unreachable after retries: add warning
• Accession not public on ENA: add warning
• Accession is unexpected format (e.g., bioproject format in biosample field): add error
• Accession is an umbrella project: add error (we can't submit into this as broker, see Support of Umbrella bioprojects #6859)

Most of the LOC are from a parametrized test I asked Claude to add. I thought it was quite nice, but let me know if it's too exhaustive and I will cut it down.

Will this have a big impact on reprocessing?

This change means also that we will re-validate each non-INSDC-ingested value in these three fields when we do a full reprocessing of our data. Luckily, it seems we only have 448 direct-submission entries in the DB where one or more of these fields are set.

I found this out by running this query against the staging DB:

SELECT * FROM public.sequence_entries
WHERE group_id != 1 AND (
	COALESCE(submitted_data->'metadata'->>'bioprojectAccession', '') != ''
	OR COALESCE(submitted_data->'metadata'->>'biosampleAccession', '') != ''
	OR COALESCE(submitted_data->'metadata'->>'insdcRawReadsAccession', '') != ''
)
ORDER BY accession ASC, version ASC;

Naively, this is 448 * 3 = 1344 calls to ENA. Claude times these between 0.1 and 0.4 seconds, so ~538 seconds added to reprocessing all of PPX at most. This does not take into account caching, timeouts, and retries, though.

We could in theory record in the DB whether an accession has passed validation in the past so we never check it again, but this would be quite a big change for little gain IMO.

Screenshot

PR Checklist

• All necessary documentation has been adapted.
• The implemented feature is covered by appropriate, automated tests.
• Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://perpro-check-accessions.loculus.org

[claude]

Claude finished @maverbiest's task in 1m 3s —— View job

---

Code review: validate user-submitted external accessions

• Read the PR diff and related code (RequestCache, call_function, tests)
• Post inline comments
• Post summary

Overall this is clean and easy to follow. The accession regexes, the INSDC-ingest bypass and the umbrella-project detection look correct. The parametrised tests cover the main paths well: wrong type, comma-separated values, case, 404, 5xx, retries exhausted and ingest bypass.

Main concern (inline): if ENA is down, the check can hold up the pipeline badly. Each lookup can take about 3.5 minutes because of 5 retries with a 30s timeout plus backoff. Failures aren't cached and there can be 3 lookups per entry, so a whole batch could stall even though the result is only a warning. I suggest fewer retries, a shorter timeout and a simple circuit breaker.

Other inline points:

• An uncaught ET.ParseError becomes an internal error, which is inconsistent with how the other ENA failures are handled.
• The swallowed RequestException should be logged.
• Nit: strip whitespace instead of rejecting it.

General:

• The preprocessing blocks are added to defaultOrganismConfig. This means every organism now calls ENA for these fields, including instances without ENA deposition. That may be intended; if not, consider gating it.
• check_ena_accession hard-codes input_data.get("accession"). This matches the config, but a short docstring on the function would help people writing configs.
• Missing test: a 200 response with malformed XML.
• The PR checklist and manual testing notes are still empty.

[claude]

Performance when ENA is slow or down: this reuses RequestCache's default retries=5 with backoff_factor=1, and uses a 30s timeout. If ENA hangs or keeps returning 5xx/429, one lookup can block for about 6×30s plus ~31s of backoff, so roughly 3.5 minutes. Failed responses are never cached, and each entry can have up to 3 accessions, so every entry in the batch waits the full time again. An ENA outage could stall preprocessing for hours, even though the result is only a warning.

Suggestions:

• Use fewer retries for this cache (e.g. RequestCache(max_size=16, retries=1)) and a shorter timeout (5–10s).
• Add a small circuit breaker: after an "unreachable" result, skip ENA for N seconds and return the warning straight away.

[maverbiest]

I'll reduce retries and timeout as suggested, let's skip the circuit breaker for now, we can revisit if it's problematic

[claude]

If ENA returns a 200 with a body that isn't valid XML (truncated body, an HTML maintenance page, etc.), ET.ParseError isn't caught here. call_function then turns it into an internal error, so the submission is blocked. Every other ENA problem only produces a warning. Consider catching ET.ParseError and returning the "unable to check" warning instead, and add a test case for it. Using defusedxml would also remove the need for the S314 noqa.

[maverbiest]

It's probably a bit overboard to guard against ENA returning malformed XML on a 200, we also don't handle this specifically elsewhere

[anna-parker]

we should probably link to the docs here: https://ena-docs.readthedocs.io/en/latest/submit/general-guide/accessions.html

[anna-parker]

Hmm... if the bioproject was once public and then is removed we cache the successful query... I guess in practice this is a small cache however as we probably wont get many bioprojects I wonder if this could be an issue

[maverbiest]

We could add some time limit to the cache that makes entries stale after a while?

But then again, a bioproject could also be removed in the interim between PPX submission and ENA deposition, so we can't guard against this type of thing completely in any case

[anna-parker]

yeah I think its ok to ignore for now, just good to be aware (we might want to add a commetn)

[maverbiest]

This also raises the question: what do we do if a bioproject exists when someone submits and when we propagate to ENA, but then three months later we bump the pipeline version and discover the bioproject has been deleted?

This would block reprossessing, right? What would we do in that situation, a DB surgery to remove the external accessions from the entry?

[anna-parker]

yeah it would block reprocessing but I think we can just ignore that case for now as I think its unlikely (and tbf would be good for us to know if this happened lol)

[anna-parker]

I think this is actually checked by checking if expected_url is actually null

[anna-parker]

This is amazing thanks!

Before we merge this could you quickly confirm that for the existing bioproject, biosamples and runs that have been directly submitted to PPX none are not live? (should hopefully not be the case)