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.
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_transactionfrom 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::Rollbackexception that's already raised when any of the servicesCreateMergeRequestServicedepends 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