SecurityFinding::CreateMergeRequestService vulnerability leftover on failure (separate sec DB)

Summary

When it fails SecurityFinding::CreateMergeRequestService doesn't properly roll back, and it doesn't remove the vulnerability it creates. This only occurs on a multi-database setup where there's a separate sec database. That's because vulnerabilities are created in a transaction that's opened to create MRs, and this can't be rolled back.

Further details

The following spec fails:

  1) Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute when a error occurs during the merge request creation behaves like an error occurs does not create a new Vulnerability, MergeRequest, and MergeRequestLink
     Failure/Error:
       expect { subject }.to change {
         project.vulnerabilities.count
       }.by(0)
        .and(change(Vulnerabilities::MergeRequestLink, :count).by(0))
        .and(change(Vulnerabilities::MergeRequestLink, :count).by(0))
     
       expected `project.vulnerabilities.count` to have changed by 0, but was changed by 1
     Shared Example Group: "an error occurs" called from ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:167

There's a cross-DB transaction which is currently ignored.

See https://gitlab.com/gitlab-org/gitlab/-/blob/e2cb38b86854eba8b12c4a5c496029f72a2025b8/ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb#L12


Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.temporary_ignore_tables_in_transaction(
          %w[
            internal_ids
            merge_requests
            merge_request_user_mentions
            vulnerability_merge_request_links
          ], url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/480003'
        ) do
          merge_request = ApplicationRecord.transaction do
            vulnerability = find_or_create_vulnerability
            create_merge_request(vulnerability).tap do |merge_request|
              create_vulnerability_merge_request_link(merge_request, vulnerability)
            end
          end
        end

Proposal

  • Remove temporary_ignore_tables_in_transaction from Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute.
  • Implement the rollback mechanism in the service class itself, and destroy objects when the service fails. We can rely on the ActiveRecord::Rollback exception that's already raised when any of the services CreateMergeRequestService depends on fails.

Same as Resolve cross join issues in ee/app/services/vu... (#480359 - closed), so doing one issue should close the other.

Edited by Fabien Catteau