Commit namespace statistics in its own transaction

What this changes

Security::Ingestion::IngestSliceBaseService currently runs all of a slice's sec-database tasks inside one SecApplicationRecord.transaction. This moves one task, IngestVulnerabilityNamespaceStatistics, into its own transaction that opens after the slice transaction commits. It is the only write in the 18-task chain whose conflict key is not project-scoped, so it is the only source of cross-project lock contention.

Added to the base class:

  • OWN_TRANSACTION_TASKS = %i[IngestVulnerabilityNamespaceStatistics].freeze
  • sec_db_tasks, which returns self.class::SEC_DB_TASKS unchanged when the flag is off and self.class::SEC_DB_TASKS - OWN_TRANSACTION_TASKS when it is on
  • run_own_transaction_tasks, which opens a second SecApplicationRecord.transaction and runs the task there, returning early when the flag is off
  • execute now calls run_tasks_in_sec_db, then run_own_transaction_tasks, then run_tasks_in_main_db

Behind a new feature flag ingest_namespace_statistics_in_own_transaction, type gitlab_com_derisk, default_enabled: false. The actor is pipeline&.project || :instance, because continuous vulnerability scanning calls this service with a nil pipeline. With the flag off, behaviour is byte-identical to today.

The change lives in the base class, so both subclasses get it: IngestReportSliceService and IngestCvsSliceService. Both declare SEC_DB_TASKS with symbols, which the array subtraction relies on.

Why only this task

IngestVulnerabilityNamespaceStatistics calls Vulnerabilities::NamespaceStatistics::UpdateService, which expands traversal_ids: a project whose traversal_ids are {1,2,3} upserts rows for namespaces 1, 2 and 3. Every project in a group hierarchy therefore writes that hierarchy's root namespace row. Every other conflict key in the chain is project, finding or occurrence scoped, so two different projects can never contend on the same row. The linked root cause analysis calls this "Phase 2 — remove the single cross-project source".

The other two aggregate tasks stay in the slice transaction, for different reasons:

  • IncreaseCountersTask increments one security_statistics row per project. There is no reconciliation path for that table anywhere in the codebase, and a retry could not repair a missed increment: the task only counts finding maps whose new_record flag is set, and that flag is only set where IngestVulnerabilities::Create actually creates the vulnerability. On a second run the vulnerability already exists, so the count is zero. Moving it out would turn any failure of the second transaction into permanently wrong counters. Today it is atomic with the findings and correct.
  • IngestVulnerabilityStatistics upserts vulnerability_statistics with ON CONFLICT (project_id), one row per project. That is same-project contention, not cross-project, so moving it does nothing for the problem here.

Trade-off

Splitting the transaction gives up atomicity between the findings and the namespace statistics. If the second transaction fails after the first has committed, the namespace rows are left stale.

That is acceptable for this table specifically, because it has a reconciliation path: Vulnerabilities::NamespaceStatistics::ScheduleWorker is cron-scheduled at 0 8 * * 0 and fans out to RecalculateNamespaceStatisticsWorker, which recomputes the rows. The worst case is stale namespace statistics until the next weekly run, not permanent drift — but the cadence is weekly, so the stale window can be up to seven days.

Second interaction worth recording: an exception from the second transaction propagates to Security::Ingestion::IngestReportService#ingest_slice, which rescues StandardError and records an IngestionError on the security scan. A namespace-statistics failure will therefore mark the scan even though the findings did commit. The alternative — rescue and log inside the new transaction — was rejected because it trades a visible failure for silent drift, and because the linked RCA's highest-priority item is about not silently dropping things.

Relationship to !256593

!256593 reorders the three aggregate tasks so they run last inside the slice transaction. That is an interim mitigation: it shortens how long the root namespace row lock is held, but leaves the serialisation in place. This MR removes the namespace task from that transaction entirely. The two touch different files — !256593 changes ingest_report_slice_service.rb, this changes ingest_slice_base_service.rb — so they do not conflict.

Testing

Specs added to ee/spec/services/security/ingestion/ingest_slice_base_service_spec.rb, using the synthetic subclass pattern already in that file. They cover:

  • the task runs after the rest of the slice
  • when the task raises, writes the slice already committed are kept
  • with the flag disabled, those same writes are rolled back instead, because the task then shares the slice transaction

That last pair is what pins the transaction boundary rather than just call ordering. RuboCop is clean on both changed files.

Closes #629996

  • Root cause analysis, including the phase plan: #629530
  • Parent epic: &23596

Merge request reports

Loading
Loading