Fixes #39626 - Recover gracefully from concurrent create races on Subnet and Operatingsystem - #11161
Draft
pablomh wants to merge 2 commits into
Draft
Fixes #39626 - Recover gracefully from concurrent create races on Subnet and Operatingsystem#11161pablomh wants to merge 2 commits into
pablomh wants to merge 2 commits into
Conversation
pablomh
force-pushed
the
feat/harden-os-subnet-create-race
branch
3 times, most recently
from
August 14, 2026 16:10
b1684a5 to
c99be43
Compare
Location/Organization uniqueness (Taxonomy#validates :name, uniqueness:
{scope: [:ancestry, :type]}) is application-level only - a SELECT before
INSERT, with no backing DB constraint. Two concurrent POST /api/locations
(or /api/organizations) for the same name both pass the check before
either commits, both INSERT cleanly, and you end up with duplicate rows.
Confirmed live against a real Satellite 6.19/foreman-3.18 instance (5
concurrent identical creates produced 5 distinct duplicate Location rows).
Adds a unique index matching the model's existing validation scope:
type, COALESCE(ancestry, ''), lower(name). A plain UNIQUE index on the
raw ancestry column does nothing for root taxonomies (NULL is distinct
from NULL); COALESCE was caught and verified live.
Api::V2::TaxonomiesController#create recovers from
ActiveRecord::RecordNotUnique via process_create_with_record_not_unique
and returns the winning record as 201.
If duplicate rows already exist, the migration aborts and lists their
ids. It does not merge or delete them, so an upgrade hard-blocks until
an admin resolves the extras. That is intentional: auto-heal has a large
association surface and raced creates are not actually unused.
Tests cover the DB constraint (validations bypassed), API recovery, the
migration abort path, index creation, and reversibility.
Co-authored-by: Cursor <cursoragent@cursor.com>
…net and Operatingsystem Subnet (unique index on name since 20180806151925) and Operatingsystem (unique index on name+major+minor since 20150713143226) both already have real DB-level uniqueness constraints, but their controllers' create actions never handled the ActiveRecord::RecordNotUnique a concurrent duplicate create raises - it surfaced as a raw 500 instead of the graceful 'return the existing record' behavior a client would expect from a race on an idempotent-looking create call. Wires both into the process_create_with_record_not_unique helper introduced for Taxonomy on the branch this stacks on (fix/taxonomy-location-create-race). No schema changes needed here - these two already had the DB constraint the recovery path depends on, unlike Taxonomy. Related but non-overlapping: theforeman#11059 fixes a different concurrency issue on the same Operatingsystem model, in Katello::RhsmFactParser's internal create_or_find_by call during RHSM registration (a service-class code path, not the REST API controller this PR touches) - zero file overlap with this PR. Verified live on a Satellite 6.19/foreman-3.18 instance: concurrent identical POST /api/subnets and POST /api/operatingsystems both return 201 with the same id for the racing requests (plus the pre-existing, unaffected 422 for non-racing ones), confirmed no duplicate rows in either case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pablomh
force-pushed
the
feat/harden-os-subnet-create-race
branch
from
August 14, 2026 16:40
c99be43 to
3efb1f3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Subnet(unique index onnamesince20180806151925) andOperatingsystem(unique index onname+major+minorsince20150713143226) both already have real DB-level uniqueness constraints, but their controllers'createactions never handled theActiveRecord::RecordNotUniquea concurrent duplicate create raises — it surfaced as a raw 500 instead of the graceful "return the existing record" behavior a client would expect from a race on an idempotent-looking create call.Fixes #39626.
Changes
Wires both into the
process_create_with_record_not_uniquehelper introduced in #11160 forTaxonomy, matching the exact same pattern. No schema changes needed here — these two already had the DB constraint the recovery path depends on, unlikeTaxonomy.Stacked on #11160 — this branch is based on top of it (needs
process_create_with_record_not_uniqueinApi::BaseController). Please review/merge #11160 first.Testing done
Live-verified against the same real Satellite 6.19/foreman-3.18 instance used for #11160 (patched both controller files, restarted the service):
POST /api/subnets: 4/5 requests returned201with the same id (the 5th got the pre-existing, unaffected422"already taken" from the normal validation path — the recovery is additive, not a replacement). DB confirmed exactly one row.POST /api/operatingsystems: 3/5 returned201with the same id, 2/5 got the normal422. DB confirmed exactly one row.Ruby syntax-checked all changed files locally. Same caveat as #11160 - wasn't able to run the full Rails test suite locally (no local Postgres set up), relying on CI plus the live verification above.