Skip to content

Status Page: Track incident issue without subtransaction

Peter Leitzen requested to merge pl-status-page-track-subtransaction into master

What does this MR do?

This MR removes SQL subtransactions (via safe_find_or_create_by) for StatusPage::PublishedIncident#track by using the unsafe method #find_or_create_by!.

Previously, we've used safe_find_or_create_by to circumvent this fact but it uses subtransactions under the hood which is problematic in nested transactions.

In the rare event of ActiveRecord::RecordNotUnique users end up seeing a meaningful error message. This behaviour is acceptable and that's why switched to unsafe method find_or_create_by.

See &6540 (closed) for more context.

Will fix #339122 (closed).

Screenshots or Screencasts (strongly suggested)

n/a

How to setup and validate locally (strongly suggested)

In Rails console:

issue = Issue.last

# Clean slate
StatusPage::PublishedIncident.delete_all

# "Simulate" race condition
StatusPage::PublishedIncident.track(issue)

# Transaction similar to MarkForPublicationService 
StatusPage::PublishedIncident.transaction do
  StatusPage::PublishedIncident.track(issue)
  p :ok
  issue.update!(title: "foo bar #{rand(1000)}")
end

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Peter Leitzen

Merge request reports