fix(backend): discard processed data whose sequence entry was edited - #7137
corneliusroemer-agent wants to merge 5 commits into
Conversation
Editing a sequence entry deletes its preprocessed data for all pipeline versions, so a pipeline that is mid-flight on that entry can no longer submit: insertProcessedData matched no row and threw, which rolled back the entire submit-processed-data batch because the service is @transactional. The unaffected entries in the batch stayed IN_PROCESSING until the stale reaper cleared them. Discard such a result instead, logging at WARN and reporting the count in the summary log line. Unknown accession versions and rows in an unexpected state still fail with 422. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @corneliusroemer's task in 3m 6s —— View job Review
The approach is right: a Five things, in inline comments. Two are worth acting on before merge: Files of a discarded result are still published to S3 (comment). No tests. The scenario from #7128 is directly constructible with the existing helpers — The rest are smaller: the revoked check also swallows results for entries that were never in processing and costs two extra queries per failing row (comment) — the "never claimed" case is the one I'd most like a deliberate decision on, since it turns a real client bug into a silent success; plus a naming nit on the new boolean return and a suggestion to add a discard counter to Not verified: I did not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } else { | ||
| discardedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | ||
| } |
There was a problem hiding this comment.
Files of a discarded result still get published.
validateFileMappingsAndCollectFileIds (line 377) writes into processedFiles before insertProcessedData runs, and the block at lines 320-331 later calls s3Service.setFileToPublic(fileId) for every entry that is already released. So when a result is discarded, its processed data is dropped but its files are still made public — and nothing references them anymore.
Reachable path: released entry gets reprocessed by pipeline version N+1 → that claim goes stale → cleanUpStaleSequencesInProcessing deletes the row → the pipeline submits late → discarded here, files published.
| } else { | |
| discardedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | |
| } | |
| } else { | |
| discardedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | |
| processedFiles.remove( | |
| AccessionVersion(submittedProcessedData.accession, submittedProcessedData.version), | |
| ) | |
| } |
| private fun claimWasRevoked(accessionVersion: AccessionVersionInterface, pipelineVersion: Long): Boolean { | ||
| val sepd = SequenceEntriesPreprocessedDataTable | ||
| val claimExists = sepd | ||
| .select(sepd.accessionColumn) | ||
| .where { | ||
| sepd.accessionVersionEquals(accessionVersion) and | ||
| (sepd.pipelineVersionColumn eq pipelineVersion) | ||
| } | ||
| .limit(1) | ||
| .count() > 0 | ||
| if (claimExists) { | ||
| return false | ||
| } | ||
| return SequenceEntriesTable | ||
| .select(SequenceEntriesTable.accessionColumn) | ||
| .where { SequenceEntriesTable.accessionVersionIsIn(listOf(accessionVersion)) } | ||
| .limit(1) | ||
| .count() > 0 | ||
| } |
There was a problem hiding this comment.
Two concerns here.
1. This also silently swallows results for entries that were never in processing. The predicate is "entry exists, but no preprocessed row for this pipeline version". An entry in RECEIVED that was never handed out to pipeline version N matches that just as well as an edited entry. So a pipeline submitting results for an entry it never claimed now gets a 200 instead of the previous 422 ... does not exist or is not awaiting any processing results. That's an error signal we lose for a genuine client bug.
The three existing 422 tests still pass (non-existent accession/version fail the SequenceEntriesTable lookup; the PROCESSED entry has a row for the same pipeline version), so nothing catches this. Worth a deliberate decision + a test either way — see the top-level comment.
2. Duplicated query work on the failure path. throwInsertFailedException (line 618) already selects every preprocessed row for the accession version and reasons about status/pipeline version. claimWasRevoked issues two more queries to answer an overlapping question, and it runs per row — during an edit sweep (the #7128 scenario) that's the common path, so a 100-row batch costs 200 extra round trips in a method that was otherwise deliberately written to batch its DB lookups (see the comment on line 296).
Folding the revoked check into throwInsertFailedException would reuse the one select it already does and keep all "why did the update not match" reasoning in one place: if the select returns no rows at all, check SequenceEntriesTable once and signal "revoked" instead of throwing.
| organism: Organism, | ||
| pipelineVersion: Long, | ||
| ) { | ||
| ): Boolean { |
There was a problem hiding this comment.
if (insertProcessedData(...)) at the call site (line 310) doesn't say what the boolean means — a reader has to jump here to find out that true means stored and false means discarded. The KDoc that explained this was removed in c940c33, so consider letting the name carry it instead, e.g. storeProcessedDataUnlessClaimRevoked(...), or returning a two-value enum. Either way the if/else at line 310 becomes readable on its own.
|
|
||
| log.info { | ||
| "Updated ${processedAccessionVersions.size} sequences to $PROCESSED. " + | ||
| "Updated ${processedAccessionVersions.size} sequences to $PROCESSED, " + |
There was a problem hiding this comment.
The PR description notes that a repeating Updated 0, discarded 100 signals a lease threshold set below batch duration — that's exactly the kind of thing operators should be able to alert on, and a log line makes it grep-only. SubmissionMetrics already has the Counter pattern (recordProcessedSequencesStored, line 348); a matching recordDiscardedProcessedResults(organism, count) would be a small addition and makes the condition observable.
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in the backend preprocessing submission flow where processed results for a sequence are rejected (422) if the sequence entry was edited while a higher preprocessing pipeline version was still running. The change treats “missing claim row for this pipeline version” as a revoked claim and discards the result instead of failing the whole batch.
Changes:
- Make
insertProcessedData(...)returnBooleanto indicate whether a processed result was stored or discarded due to a revoked claim. - Add a revoked-claim detection query (
claimWasRevoked) and log/summary reporting for discarded results.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| submittedProcessedDataBatch.forEach { submittedProcessedData -> | ||
| val processingResult = submittedProcessedData.processingResult() | ||
|
|
||
| insertProcessedData(submittedProcessedData, organism, pipelineVersion) | ||
| processedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | ||
| processingResultCounts.merge(processingResult, 1, Int::plus) | ||
| if (insertProcessedData(submittedProcessedData, organism, pipelineVersion)) { | ||
| processedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | ||
| processingResultCounts.merge(processingResult, 1, Int::plus) | ||
| } else { | ||
| discardedAccessionVersions.add(submittedProcessedData.displayAccessionVersion()) | ||
| } |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A discarded result's file IDs were collected before the result was stored, so its files were still made public for an already released entry while nothing referenced them any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| .assertStatusIs(Status.RECEIVED) | ||
| } | ||
|
|
||
| // A discarded result must not publish its files, they end up referenced by nothing. |
There was a problem hiding this comment.
| // A discarded result must not publish its files, they end up referenced by nothing. |
| ).andExpect(status().isNoContent) | ||
| convenienceClient.approveProcessedSequenceEntries(listOf(AccessionVersion(accession, 1))) | ||
|
|
||
| // Pipeline version 2 submits a result for the released entry without holding a claim on it. |
There was a problem hiding this comment.
| // Pipeline version 2 submits a result for the released entry without holding a claim on it. |
corneliusroemer
changed the title
Rethinking this PR I'm now not sure we need it - it only affects reprocessing where a 10min delay on a few sequences isn't a big deal.
Fixes #7128.
When a user edits a sequence entry (which is only allowed when the current prepro version's entry is
PROCESSED) we delet sequence_entries_preprocessed_data rows all pipeline versions of that accessionVersion. Preprocessing could already be underway for a a higher prepro version than current, when that delete happens. When that pipeline then submits it would get a 422 for the whole batch which would only get reprocessed once the cleanup job clears them (10min for PPX).This isn't so bad as it only happens during reprocessing - so not sure we need to make this change here.
Changes
A result whose claim no longer exists is now discarded: WARN log, counted separately, and the summary line reads
Updated N sequences to PROCESSED, discarded M results whose claim was revoked. The entry is handed out again and reprocessed, as it already was. Unknown accession versions and rows in an unexpected state still fail with 422.Updated 0, discarded 100repeating would indicate a lease threshold set below batch duration.Not addressed: the update has no fencing token, so two pods of the same pipeline version can still race an edit and land pre-edit data on a fresh claim. That needs a claim id in the protocol. However, this should be a very rare issue and it would resolve on the next reprocessing so it's not permanent.
Alternatives considered
We could block editing a sequence when any entry greater or equal to current is
IN_PROCESSING- however that would mean people could not edit while a reprocessing is in process which could be hours and is not a good user experience.🚀 Preview: Add
previewlabel to enable