Loading
Run the aggregate counter tasks last in the slice transaction
What does this MR do and why?
Moves IncreaseCountersTask, IngestVulnerabilityStatistics and IngestVulnerabilityNamespaceStatistics to the end of SEC_DB_TASKS, in both IngestReportSliceService and IngestCvsSliceService.
These three take a row lock held until the slice transaction commits, and they are the widest locks in the chain. IncreaseCountersTask ran fourth of eighteen, so its lock on project_security_statistics was held across fourteen more tasks. Running the three last shrinks that hold to the commit itself.
The namespace one is wider than "one row per project" suggests: Vulnerabilities::NamespaceStatistics::UpdateService expands traversal_ids, so a project at namespace {1,2,3} upserts a row for namespaces 1, 2 and 3 — one per ancestor, up to the root. Two ingestion jobs for unrelated projects under the same top-level group serialise on the root namespace's row, once per 50-finding slice.
How this reaches a timeout that surfaced on task 1
The statistics locks never touch the identifier rows, but they extend the life of transactions that hold them:
- Transaction C locks the namespace row at task 14 and works toward commit.
- Transaction A blocks on that row, while still holding the identifier row locks it took at task 1.
- Transaction B arrives at task 1, collides with A's identifier rows, and waits.
B's wait is A's remaining work plus A's own wait plus commit fsync. Because the namespace row is shared across a whole group, C can be an unrelated project — so a cross-project collision propagates into same-project waits two hops away.
Why reorder rather than move them out of the transaction
The work item offers three options — after_commit, a separate transaction, or once per job. All three break atomicity, and these are incremental counters (vulnerability_count = vulnerability_count + N), so a slice that commits while the counter write fails leaves a permanently drifted count with no self-correction.
Reordering keeps them inside the transaction, so the counters stay atomic with the findings they count, while still removing most of the hold. It is a step, not the destination: moving them out entirely remains the larger option, and is better decided once this is measured.
The trade this makes: a transaction now waits for the contended row later, while holding more locks. That is acceptable only because the hold reduction is what collapses the wait — shorter holds drain the queue, which shortens waits, which shortens holds. Worth a reviewer's eye.
Risk: lock ordering during the rolling deploy
Reordering changes lock acquisition order, and old and new code run concurrently during a deploy. An old transaction locks project_security_statistics at task 4 then vulnerability_reads at task 12; a new one locks vulnerability_reads at task 11 then the statistics row at task 14. Opposite order on the same two rows for the same project can deadlock.
It is transient and Postgres resolves it in about a second by aborting one side — but per #629530 an aborted slice is silently dropped, so anything lost in that window is invisible today. Landing #629988 first would make it visible. A feature flag would not help here; it would lengthen the mixed-order window rather than shorten it.
Verification gap worth naming
IngestCvsSliceService has no spec file at all, and this MR changes its task order. The ordering guard added here covers IngestReportSliceService only.
References
Screenshots or screen recordings
No UI change.
How to set up and validate locally
bundle exec rspec ee/spec/services/security/ingestion/ingest_report_slice_service_spec.rbLocally: 4 examples, 0 failures. RuboCop clean. The existing .ordered assertions were updated to the new sequence, and a new example asserts the three counter tasks stay last so a future change cannot silently undo this.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist.
Edited by Bala Kumar