fix: failed to create new record due to index conflicts

[hanakannzashi]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @hanakannzashi, 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 resolves a potential issue in the user ban system where attempting to apply a new ban could lead to unique index conflicts if a previous ban of the same type had expired but was not explicitly marked as revoked. The change introduces a proactive step to automatically revoke such expired bans, ensuring that new bans can be applied without encountering database constraints and maintaining the integrity of the ban records.

Highlights

• Ban Management Logic: Implemented a pre-insertion check in the user_repository to automatically revoke expired user bans of the same type before applying a new ban. This prevents unique index conflicts that could arise from expired but unrevoked bans.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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. ↩

[gemini-code-assist]

Code Review

The pull request modifies the insert_user_ban function in crates/database/src/repositories/user_repository.rs. It adds a new step to revoke any expired user bans for a specific user and ban type before inserting a new ban. This pre-emptive update sets the revoked_at timestamp for active, expired bans to NOW() to prevent unique index conflicts, as the unique index only considers bans where revoked_at IS NULL. There were no review comments provided for these changes.

[claude]

Code Review - PR #88: fix: revoke ban

⚠️ CRITICAL ISSUES FOUND

1. Race Condition Between UPDATE and INSERT

Severity: High | Category: Production Safety

The fix introduces a race condition in multi-instance deployments. Two database operations without a transaction means in concurrent scenarios both threads can pass the UPDATE, then both try to INSERT causing unique constraint violations.

Fix: Wrap operations in a transaction using client.transaction().await

2. Bypass of Business Logic Check

Severity: Medium | Category: Logic & Functionality

The fix conflicts with the existing check in ban_user_for_duration (user_service.rs:133-140) which prevents creating bans when ANY active ban exists. The repository-level fix now allows creating new bans even when there's an active non-expired ban of a different type.

Questions for clarification:

1. Should users have multiple active bans of different ban_type simultaneously?
2. Is the service-layer check meant to prevent ALL bans, or just duplicate ban_types?
3. Is the revoke-expired-bans logic better placed in has_active_ban() as a cleanup operation?

Recommendation

1. MUST FIX: Add transaction boundary to prevent race conditions
2. SHOULD CLARIFY: Align service-layer and repository-layer logic about concurrent bans

⚠️ Issues Found - Requires Changes

[Copilot]

Pull request overview

This PR fixes a bug in the user ban creation logic where attempting to create a new ban would fail due to unique index conflicts when expired bans exist but haven't been explicitly revoked.

Key Changes:

• Added automatic revocation of expired bans before inserting a new ban to prevent unique constraint violations

Comments suppressed due to low confidence (1)

crates/database/src/repositories/user_repository.rs:368

• The two separate database operations (UPDATE followed by INSERT) create a potential race condition. Between these operations, another concurrent call to create_user_ban could insert a ban for the same user_id and ban_type, leading to the same unique constraint violation this fix is trying to prevent. Consider wrapping both operations in a database transaction to ensure atomicity.

        client
            .execute(
                "UPDATE user_bans
                 SET revoked_at = NOW()
                 WHERE user_id = $1
                   AND ban_type = $2
                   AND revoked_at IS NULL
                   AND expires_at IS NOT NULL
                   AND expires_at <= NOW()",
                &[&user_id, &ban_type.as_str()],
            )
            .await?;

        // Now insert the new ban
        client
            .execute(
                "INSERT INTO user_bans (user_id, reason, ban_type, expires_at)
                 VALUES ($1, $2, $3, $4)",
                &[&user_id, &reason, &ban_type.as_str(), &expires_at],
            )
            .await?;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[think-in-universe]

@claude review

[claude]

Critical Issues Found

1. Race Condition in Service Layer (HIGH PRIORITY)

Location: crates/services/src/user/service.rs:133-154

The service layer has a TOCTOU (time-of-check to time-of-use) race condition:

// Between this check...
if self.user_repository.has_active_ban(user_id).await? {
    return Ok(());
}

// ...and this insert, another thread could create a ban
self.user_repository
    .create_user_ban(user_id, ban_type, reason, expires_at)
    .await

Impact: In concurrent scenarios (multiple requests for the same user), both could pass the check and attempt to insert, causing one to fail with a duplicate key error despite the new transaction-based fix.

Fix: Move the active ban check inside the repository transaction, or handle the unique constraint error gracefully in the service layer.

2. Inconsistent Ban Type Handling (MEDIUM PRIORITY)

Location: crates/services/src/user/service.rs:133 vs crates/database/src/repositories/user_repository.rs:344-356

• has_active_ban() checks for ANY active ban (regardless of ban_type)
• The repository's revoke logic only affects the specific ban_type being inserted
• Service layer will skip creating a new ban if ANY active ban exists, even if it's a different type

Impact: Inconsistent behavior where:

• Active Manual ban → Cannot create NearBalanceLow ban (blocked at service layer)
• Expired Manual ban → Can create NearBalanceLow ban (repository revokes it)

Fix: Either make has_active_ban() check by ban_type, or clarify the business rule about whether multiple ban types can coexist.

3. Missing Error Context (LOW PRIORITY)

Location: crates/database/src/repositories/user_repository.rs:368-369

The transaction operations use ? without context. If the transaction fails, it's unclear whether it was the UPDATE or INSERT that failed.

Suggestion:

transaction
    .execute(...)
    .await
    .context("Failed to revoke expired bans")?;

transaction
    .execute(...)
    .await
    .context("Failed to insert new ban")?;

Positive Notes

• ✅ Transaction ensures atomicity of revoke + insert
• ✅ Tests comprehensively cover the edge cases
• ✅ The fix addresses the immediate issue of expired bans causing conflicts

⚠️ CRITICAL ISSUES FOUND - Please address race condition and ban type inconsistency before merging.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.