Rescue validation race in McpServerBlock.block!

What does this MR do and why?

Closes https://gitlab.com/gitlab-org/gitlab/-/work_items/622706

Ai::Catalog::McpServerBlock.block! (ee/app/models/ai/catalog/mcp_server_block.rb) was documented as race-safe but only rescued ActiveRecord::RecordNotUnique. The model validates uniqueness of namespace_id scoped to ai_catalog_mcp_server_id, and that validation runs before the insert reaches the database.

When two concurrent requests race, the loser can pass find_or_create_by!'s initial find, then have the winner commit its row before the loser's own uniqueness validation runs. The loser's validation catches the now-existing row and raises ActiveRecord::RecordInvalid ("Namespace has already been taken"), which was unrescued and surfaced as a 500 from the aiCatalogMcpServerSetBlock GraphQL mutation.

Data was never wrong: exactly one block row is created either way, and enforcement applies as expected. Only the losing caller's response was wrong — it reported failure for an action that had actually succeeded. Severity 4, shipped in commit 20f38338b9c9 (2026-06-28), unflagged since.

The fix adds a second rescue clause. It re-raises unless the error is specifically the namespace_id "taken" uniqueness error — so genuine validation failures still surface — and otherwise re-reads the winning row with find_by!. This follows the existing pattern in ee/app/models/search/zoekt/replica.rb, which uses the identical errors.of_kind?(:namespace_id, :taken) guard, and matches the of_kind?(..., :taken) idiom used in services like app/services/packages/npm/create_temporary_package_service.rb to distinguish a uniqueness race from a real validation failure.

No feature flag: this corrects a response for a benign race and doesn't change behavior for sequential callers.

How to validate locally

  • A deterministic 5-thread concurrency repro is available at ai-governance-notes/qa-scripts/mcp_audit_events_deep.rb (section T4), run via bin/rails runner. Requires GDK postgres and redis running. Without the fix, at least one of the 5 concurrent calls raises; with the fix, all 5 return success with exactly one block row.

  • Three examples were added to ee/spec/models/ai/catalog/mcp_server_block_spec.rb:

    • the validation race returns the existing row
    • the unique-index race returns the existing row
    • a RecordInvalid carrying any other error still re-raises

    The validation-race example was confirmed to fail without the fix (it raised RecordInvalid from block!) before being asserted to pass.

  • 27 examples pass across ee/spec/models/ai/catalog/mcp_server_block_spec.rb and ee/spec/services/ai/catalog/mcp_servers/set_block_service_spec.rb.

  • RuboCop clean.

Database

No database review or query plan needed. The new find_by!(attrs) call runs the same query the existing RecordNotUnique branch already runs, so this introduces no new SQL and no new scopes.

Notes for reviewers

  • There is currently no audit-event emission for MCP server blocks on master. That's being added by !251763 (merged), still open. Once that merges, this fix won't cause duplicate audit events: the row recovered via find_by! is not previously_new_record?, which is the condition that service gates audit emission on, so only the winner emits.

typebug backend groupcompliance devopssecurity governance sectionsec severity4

Merge request reports

Loading
Loading