fix: atomic alias reassignment in upsert_aliases_for_model

[henrypark133]

Problem

upsert_aliases_for_model previously used a DELETE-then-INSERT pattern inside a transaction:

1. DELETE all existing aliases for the model
2. INSERT the new alias list

This created a visibility gap: if another model held an alias being reassigned, a concurrent reader between the DELETE and INSERT steps could observe the alias momentarily missing or pointing to the wrong model.

Solution

Replace with a merge (UPSERT-then-delete-stale) approach within a single transaction:

1. UPSERT each new alias — INSERT ... ON CONFLICT (alias) DO UPDATE SET model_id = EXCLUDED.model_id atomically reassigns the alias with no visibility gap.
2. DELETE stale aliases — remove any aliases previously associated with this model that are no longer in the new list.
3. SELECT and return the final alias set.

This ensures alias ownership transitions are atomic: there is never a moment when a valid alias is unowned.

Edge Case

Empty alias list: the UPSERT loop is a no-op and the DELETE removes all aliases for the model, which is the correct behaviour.

Files Changed

• crates/database/src/repositories/model_alias.rs

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello @henrypark133, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the reliability and consistency of model alias management. By transitioning to an atomic upsert-then-delete mechanism, it resolves potential race conditions and visibility issues that could arise during alias reassignments, ensuring that alias states are always accurate and immediately reflected across the system.

Highlights

• Atomic Alias Reassignment: The upsert_aliases_for_model function was refactored to use an atomic UPSERT-then-DELETE strategy instead of the previous DELETE-then-INSERT pattern. This change eliminates visibility gaps and ensures that alias ownership transitions are atomic, preventing aliases from momentarily disappearing or pointing to the wrong model during concurrent operations.
• Improved Data Consistency: The new approach leverages INSERT ... ON CONFLICT (alias) DO UPDATE SET to atomically reassign aliases, followed by a targeted DELETE for stale aliases. This guarantees that a valid alias is never unowned and maintains data consistency.
• Edge Case Handling: The implementation now explicitly handles the edge case of an empty alias list, correctly performing a simple DELETE of all aliases associated with the model.

Changelog

• crates/database/src/repositories/model_alias.rs
  • Refactored the upsert_aliases_for_model function to implement a merge (UPSERT-then-delete-stale) approach for alias management.
  • Updated the documentation comments for upsert_aliases_for_model to reflect the new atomic merge strategy and its benefits.
  • Introduced INSERT ... ON CONFLICT (alias_name) DO UPDATE SET logic to atomically reassign aliases.
  • Added a DELETE statement to remove aliases that are no longer associated with the canonical model.
  • Modified the function to explicitly query and return the final set of aliases for the model after the transaction.

Activity

• No human activity has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

PR Review: atomic alias reassignment

The core fix is correct — replacing DELETE-then-INSERT with UPSERT-then-delete-stale properly handles cross-model alias reassignment without unique constraint violations. The schema confirms alias_name has a UNIQUE constraint and updated_at has DEFAULT NOW() with an update trigger, so the insert path is safe.

Issues Found

1. Deadlock risk from inconsistent lock ordering (⚠️ worth fixing)

The UPSERT loop acquires row-level locks in the order callers pass aliases. Two concurrent calls sharing overlapping aliases but in different order will deadlock:

• Tx1: UPSERT ["foo", "bar"] → locks foo, waits on bar
• Tx2: UPSERT ["bar", "foo"] → locks bar, waits on foo

PostgreSQL detects and aborts one, and retry_db! will retry — so it's not a correctness bug, but it adds unnecessary churn. Fix by sorting before the loop:

let mut sorted_aliases: Vec<&String> = alias_names.iter().collect();
sorted_aliases.sort();
for alias_name in &sorted_aliases {
    // UPSERT...
}

2. N+1 queries in the UPSERT loop (minor perf concern)

One round-trip per alias. For typical small alias lists this is fine, but a single batch UPSERT using unnest() would be more efficient and naturally avoids the lock-ordering issue:

INSERT INTO model_aliases (alias_name, canonical_model_id, is_active)
SELECT unnest(::text[]), , true
ON CONFLICT (alias_name) DO UPDATE SET
    canonical_model_id = EXCLUDED.canonical_model_id,
    is_active = true

3. Misleading description of the original bug

The PR description says the old approach created a "visibility gap" for concurrent readers. This isn't accurate — both approaches use a transaction, so READ COMMITTED readers only see committed states. The real bug was that cross-model alias reassignment would fail with a unique constraint violation (no ON CONFLICT handling on the old INSERT). Worth correcting the description for future reference, though it doesn't affect the code.

4. The is_empty() branch is unnecessary

With an empty alias list, alias_name != ALL('{}'::text[]) is vacuously true in PostgreSQL — the DELETE in the non-empty branch would remove all aliases anyway. The special case adds complexity without correctness benefit (though it may avoid an empty-array ToSql edge case with tokio-postgres, which would be worth a comment if intentional).

Summary

The logic is sound and the fix is a genuine improvement. Items 3 and 4 are cosmetic. Item 1 (lock ordering) is the only one worth fixing before merge to avoid unnecessary retry churn in high-concurrency scenarios.

⚠️ Recommend addressing the lock-ordering issue before merge.

[gemini-code-assist]

Code Review

This pull request correctly addresses a potential race condition in upsert_aliases_for_model by switching from a DELETE-then-INSERT pattern to an UPSERT-then-DELETE approach. This ensures alias reassignments are atomic. My review includes a suggestion to further improve the implementation by simplifying the logic and using a single bulk UPSERT query, which will be more performant, especially with a large number of aliases.

[henrypark133]

Thanks for the thorough review! Here's how each item was addressed:

1. Deadlock risk from inconsistent lock ordering — Fixed ✅
Replaced the per-alias UPSERT loop with a single batch INSERT ... SELECT unnest($1::text[]). PostgreSQL acquires all row locks in one statement with consistent internal ordering, eliminating the deadlock scenario entirely.

2. N+1 queries — Fixed ✅
Same batch approach resolves this — now a single round-trip regardless of alias count.

3. Misleading description of the original bug — Acknowledged
Fair point. The real issue was unique constraint violations on cross-model alias reassignment (no ON CONFLICT on the old INSERT), not a visibility gap within a transaction. Will keep this in mind for the PR description.

4. is_empty() branch — Kept intentionally
You're right that != ALL('{}'::text[]) is vacuously true in PostgreSQL, but keeping the explicit branch avoids a needless UPSERT with an empty unnest array and makes the intent clear. It also sidesteps any edge cases with empty array serialization in tokio-postgres.