fix: prevent duplicate StripeConnectedAccount on concurrent requests

[amolmjoshi93]

Replace non-atomic check-then-create with find_or_create_by! and add rescue for ActiveRecord::RecordNotUnique to handle race conditions when users double-click the connect Stripe button.

Fixes PG::UniqueViolation on index_stripe_connected_accounts_on_company_id

Closes #2029

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of Stripe account connection by ensuring existing accounts are properly reused and handling concurrent connection attempts gracefully.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes fix a race condition in the payment settings controller where concurrent requests could create duplicate StripeConnectedAccount records. The controller now uses find_or_create_by! with error handling for ActiveRecord::RecordNotUnique exceptions, reloading and re-rendering on conflicts. Tests are added to validate existing account retrieval and concurrent request handling.

Changes

Cohort / File(s)	Summary
Payment Settings Controller app/controllers/internal_api/v1/payment_settings_controller.rb	Replaces conditional StripeConnectedAccount creation with find_or_create_by! using current_company. Adds rescue handler for ActiveRecord::RecordNotUnique that reloads the account and re-renders. Removes previous nil-check logic.
Payment Settings Tests spec/requests/internal_api/v1/payment_settings/connect_stripe_spec.rb	Adds three new test cases: one validating existing account retrieval without duplication, and two testing race condition handling when concurrent creates trigger uniqueness violations. Tests mock Stripe API (Account.create, Account.retrieve, AccountLink.create) interactions.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Controller
    participant Database
    
    Client->>Controller: POST /stripe/connect (Request 1)
    Client->>Controller: POST /stripe/connect (Request 2)
    activate Controller
    Controller->>Database: find_or_create_by!(company_id)
    activate Database
    Database-->>Controller: Creates account (Request 1 wins)
    deactivate Database
    Controller->>Client: Render account (Request 1)
    deactivate Controller
    
    activate Controller
    Controller->>Database: find_or_create_by!(company_id)
    activate Database
    Database-->>Controller: RecordNotUnique exception (Request 2)
    deactivate Database
    rect rgb(220, 240, 255)
        note right of Controller: Race condition caught
        Controller->>Database: reload stripe_connected_account
        Database-->>Controller: Fetches existing account
        Controller->>Client: Render account (Request 2)
    end
    deactivate Controller

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Two requests hopped in quick succession,
Creating chaos in one connection!
With find_or_create_by, we catch the race,
Reload with grace, and show the right face.
Unique constraints now stay intact,
A patched bug—that's a fact! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: preventing duplicate StripeConnectedAccount records on concurrent requests, which is the core objective of the PR.
Linked Issues check	✅ Passed	The PR implementation fully addresses issue #2029 by replacing conditional creation with find_or_create_by!, adding ActiveRecord::RecordNotUnique rescue handling, and adding tests for both normal and race-condition scenarios.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the duplicate StripeConnectedAccount issue. The controller modification, rescue handler, and new tests are all within scope.

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

app/controllers/internal_api/v1/payment_settings_controller.rb (1)

15-20: LGTM! Race condition handling is sound.

The use of find_or_create_by! with a rescue for ActiveRecord::RecordNotUnique correctly addresses the race condition described in issue #2029. The rescue block properly reloads the company association to retrieve the account created by the concurrent request.

Optional defensive improvement: Consider adding a nil guard in the rescue block in case stripe_connected_account is unexpectedly nil (though this should be impossible after a uniqueness violation):

🔎 Suggested defensive nil handling

  rescue ActiveRecord::RecordNotUnique
-   render :connect_stripe, locals: { stripe_connected_account: current_company.reload.stripe_connected_account }
+   account = current_company.reload.stripe_connected_account
+   raise "Expected stripe_connected_account to exist after RecordNotUnique" if account.nil?
+   render :connect_stripe, locals: { stripe_connected_account: account }

spec/requests/internal_api/v1/payment_settings/connect_stripe_spec.rb (1)

47-61: Improve race condition test verification and cleanup.

The test correctly simulates the race condition by stubbing find_or_create_by! to raise ActiveRecord::RecordNotUnique, but there are a few areas for improvement:

1. Line 51: The class-level stub on StripeConnectedAccount could leak to other tests. Consider using instance-level stubbing or ensuring proper cleanup.

2. Lines 48-49: The Stripe::Account.create mock appears unnecessary since the rescue path doesn't create a new Stripe account—it reloads the existing one.

3. Lines 59-60: The test only verifies that an accountLink key exists, but doesn't verify that the correct existing account is being used after the rescue.

🔎 Suggested improvements

  it "handles race condition when duplicate record is created concurrently" do
-   allow(Stripe::Account).to receive(:create)
-     .and_return(OpenStruct.new({ id: "acct_existing456" }))
    existing_account = create(:stripe_connected_account, company:)
    allow(StripeConnectedAccount).to receive(:find_or_create_by!).and_raise(ActiveRecord::RecordNotUnique)
-   allow(Stripe::Account).to receive(:retrieve)
+   allow(Stripe::Account).to receive(:retrieve).with(existing_account.account_id)
      .and_return(OpenStruct.new({ details_submitted: false }))
    allow(Stripe::AccountLink).to receive(:create)
      .and_return(OpenStruct.new({ url: "https://connect.stripe.com/setup/s/recovered" }))

    send_request :post, internal_api_v1_payments_settings_stripe_connect_path, headers: auth_headers(user)

    expect(response).to have_http_status(:ok)
-   expect(json_response).to have_key("accountLink")
+   expect(json_response["accountLink"]).to eq("https://connect.stripe.com/setup/s/recovered")
+   expect(Stripe::Account).to have_received(:retrieve).with(existing_account.account_id)
  end

This ensures:

• The mock for Stripe::Account.create is removed (not needed in rescue path)
• The Stripe::Account.retrieve is called with the correct existing account ID
• The response returns the expected URL
• Verification that the correct account was retrieved after the race

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bc587a0 and 78a4dac.

📒 Files selected for processing (2)

• app/controllers/internal_api/v1/payment_settings_controller.rb
• spec/requests/internal_api/v1/payment_settings/connect_stripe_spec.rb

🧰 Additional context used

🧬 Code graph analysis (1)

app/controllers/internal_api/v1/payment_settings_controller.rb (4)

app/controllers/concerns/current_company_concern.rb (1)

• current_company (10-14)

app/services/payment_providers/create_stripe_provider_service.rb (1)

• stripe_connected_account (27-29)

app/controllers/internal_api/v1/payments/providers_controller.rb (1)

• stripe_connected_account (31-33)

app/controllers/payment_settings_controller.rb (1)

• stripe_connected_account (16-18)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Running tests

🔇 Additional comments (1)

spec/requests/internal_api/v1/payment_settings/connect_stripe_spec.rb (1)

30-45: Good test coverage for the existing account scenario.

This test properly verifies that when a StripeConnectedAccount already exists, the controller returns it without creating a duplicate. The expectation that the count doesn't change is a strong assertion.