Draft: (Throwaway) Fix registry race condition

What does this MR do and why?

Just a throwaway MR to analyze / test !75483 (merged) .

I've added an extra test that shows that the current fix can return non-persisted ContainerRepository records.

Analysis of the current solution

The problem with the solution of using .safe_find_or_create_by instead of .safe_find_or_create_by! is that the former also runs the model validations, but returns an initialized non-persisted record instead of throwing ActiveRecord::RecordInvalid.

From ActiveRecord::Relation.create docs:

Tries to create a new record with the same scoped attributes defined in the relation. Returns the initialized object if validation fails.

So, the all.create(*args, &block) call in .safe_find_or_create_by will run the model validations and return an initialized ContainerRepository if another concurrent call has already created the record.

From my testing, the solution from !75483 (merged) only works correctly if the name uniqueness validation is removed. In that case, the all.create(*args, &block) actually triggers a DB insert that then throws ActiveRecord::RecordNotUnique, which is rescued and causes a retrieval from the DB.

Analysis of .safe_find_or_create_by

This makes me think the .safe_find_or_create_by method is a bit misleading, and it may be used incorrectly in other places 🤔 ? However, I 'm not sure if this is a case of that method not working as intended, or a case of it working as intended but we not understanding the exact intention 😅 .

The exact intention is not clear to me from the following:

1. The code could use some more comments to explain the exact intention and even recommended usage.
2. The dev docs make it more explicit that the idea is to avoid race conditions and unique constraint violations caused by them. But it doesn't talk about model validations.
3. The specs:
   • The 'creates the suggestion avoiding race conditions' test is ambiguous because it uses mocking to artificially throw the ActiveRecord::RecordNotUnique error.
   • But the 'does not create a record when is not valid' test makes me think the method is working as intended. And the record.validate! unless record.persisted? line in .safe_find_or_create_by! makes me think the same.

Conclusion / alternative solution

I think what we need is not provided by either .safe_find_or_create_by or .safe_find_or_create_by! 🤔 .

One way .safe_find_or_create_by would work as needed by ContainerRepository is if it would use .create! instead of .create internally and then have a more robust way to test for uniqueness model validation errors and call find_by a second time in case of one (as it is doing for ActiveRecord::RecordNotUnique errors).

But I think we don't want to mess with that method and introduce breaking changes. So, another solution could be calling .safe_find_or_create_by and handle the case when the returned record is not persisted, which would mean the call tried to create a duplicate record. So, in that case we can do an extra find_by_path! call:

# In app/models/container_repository.rb
def self.create_from_path!(path)
  repository = safe_find_or_create_by(project: path.repository_project, name: path.repository_name)
  return repository if repository.persisted?
  
  find_by_path!(path)
end

With this change, the added test should pass 💚 :

Screenshots or screen recordings

Result of running the added test:

How to set up and validate locally

bin/rspec -f doc spec/models/container_repository_spec.rb:302

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the MR acceptance checklist for this MR.

Related to #345598 (closed)