Commit afb37c9e authored by Dylan Griffith's avatar Dylan Griffith

Find IndexStatus find_or_create race condition

**TL;DR** Fix a race condition where `IndexStatus` is not created and
does not raise error. Need to use `!` and get rid of problematic unique
model validation.

Looking at the code I notice a logical problem with using
`find_or_create_by` alongside a unique model validation. The problem is
a race condition which comes from `find_or_create` first checking to see
if the record exists. If it does not it tries creating which then
invokes the validation which checks again to see if the record exists
which finally returns a validation error. But see here we are not using
`!` version of `find_or_create` and so this validation error is silently
ignored.

Since we already have a
[`safe_find_or_create_by!`](
https://gitlab.com/gitlab-org/gitlab/-/blob/2b01f1f3420d9177e27418df063aad2e1bda1216/app/models/application_record.rb#L37
)
which is designed for handling this race condition using unique indexes
and rescue and seeing that this model already has the [unique
index](
https://gitlab.com/gitlab-org/gitlab/-/blob/2b01f1f3420d9177e27418df063aad2e1bda1216/db/structure.sql#L19266
)
we should just get rid of the model validation since it just complicates
the error handling.

Since this is a pretty weird race condition it seems tricky/contrived to
try to create a proper unit test for this case.
parent c33411a0
...@@ -7,7 +7,7 @@ class IndexStatus < ApplicationRecord ...@@ -7,7 +7,7 @@ class IndexStatus < ApplicationRecord
sha_attribute :last_wiki_commit sha_attribute :last_wiki_commit
validates :project_id, uniqueness: true, presence: true validates :project_id, presence: true
scope :for_project, ->(project_id) { where(project_id: project_id) } scope :for_project, ->(project_id) { where(project_id: project_id) }
end end
---
title: Fix bug with IndexStatus not being set on indexing project
merge_request: 36266
author:
type: fixed
...@@ -163,12 +163,7 @@ module Gitlab ...@@ -163,12 +163,7 @@ module Gitlab
# An index_status should always be created, # An index_status should always be created,
# even if the repository is empty, so we know it's been looked at. # even if the repository is empty, so we know it's been looked at.
@index_status ||= @index_status ||= IndexStatus.safe_find_or_create_by!(project_id: project.id)
begin
IndexStatus.find_or_create_by(project_id: project.id)
rescue ActiveRecord::RecordNotUnique
retry
end
attributes = attributes =
if index_wiki? if index_wiki?
...@@ -177,7 +172,8 @@ module Gitlab ...@@ -177,7 +172,8 @@ module Gitlab
{ last_commit: to_sha, indexed_at: Time.now } { last_commit: to_sha, indexed_at: Time.now }
end end
@index_status.update(attributes) @index_status.update!(attributes)
project.reload_index_status project.reload_index_status
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment