Fixes #38169 - Fix operatingsystem race condition with concurrent registrations

[jeremylenz]

While testing concurrent Katello registrations in batches we see the following error leading to some hosts (2, 3 usually) with the same OS version failing to register:

PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_operatingsystems_on_title"
DETAIL: Key (title)=(RedHat 9.2) already exists.

After these errors, the rest of registrations work without problems.
The error occurs with 50 concurrent registrations but does not occur with >120 concurrent registrations.

This PR switches from find_or_create_by to create_or_find_by. Per the Active Record documentation:

Attempts to create a record with the given attributes in a table that has a unique database constraint on one or several of its columns. If a row already exists with one or several of these unique constraints, the exception such an insertion would normally raise is caught, and the existing record with those attributes is found using #find_by!.

This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT and the INSERT, as that method needs to first query the table, then attempt to insert a row if none is found.

In our testing we found create_or_find_by would successfully avoid the unique constraint error, but then because

This method will return a record if all given attributes are covered by unique constraints (unless the INSERT -> DELETE -> SELECT race condition is triggered), but if creation was attempted and failed due to validation errors it won’t be persisted, you get what #create returns in such situation.

the Rails validation for :name would cause it to return an unpersisted object with errors, and cause

ERROR: ERF42-2587 [Foreman::Exception]: Must provide an operating system 

We get around this with the second code change here, doing another find_by.

With these two changes, Pablo says he ran the test 15 times and did not get any errors.

Considerations

The tradeoff with this solution is that you may end up with some (harmless) errors in your Postgres logs.

Another possible approach is to use active_record_retry from Katello: https://github.com/Katello/katello/blob/882b257afce348d1795db28fce81611543287b9e/app/lib/katello/util/support.rb#L96

But that would require moving that code to Foreman, and we have not yet tested that it would work as well as this.

[jeremylenz]

I also just noticed this:

The primary key may auto-increment on each create, even if it fails. This can accelerate the problem of running out of integers, if the underlying table is still stuck on a primary key of type int (note: All Rails apps since 5.1+ have defaulted to bigint, which is not liable to this problem).

If this solution is accepted, I probably will need to add a migration to change the operatingsystem id column to bigint.

[parthaa]

APJ with a test and migration

[jeremylenz]

Updated with test and also with an additional find_by as Partha and I discussed offline. This should hopefully avoid the need for a migration.

[ofedoren]

The test failures are related :/

[jeremylenz]

🍏

[parthaa]

ACK

[parthaa]

ACK

[ofedoren]

Thanks, @jeremylenz and @parthaa !