Skip to content

Fix IndexStatus find_or_create race condition

Dylan Griffith requested to merge update-bang-elastic-commit-indexer into master

What does this MR do?

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! which is designed for handling this race condition using unique indexes and rescue and seeing that this model already has the unique index we should just get rid of the model validation since it just complicates the error handling as we'd need a 2nd rescue/retry case to avoid missing these saves.

Since this is a pretty weird race condition it seems tricky/contrived to try to create a proper unit test for this case.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • 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 Dylan Griffith

Merge request reports