From b5deac468e512bd0773a3e6034e6e8a1a63c7ae3 Mon Sep 17 00:00:00 2001
From: Pranav <pranav@chatwoot.com>
Date: Thu, 20 Mar 2025 18:24:28 -0700
Subject: [PATCH] fix: Fix duplicate contact inbox race condition (#11139)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This PR addresses a race condition in the contact inbox model caused by
duplicate `source_id` values linked to different contacts.

The issue typically occurs when an agent updates a contact’s email or
phone number or when two contacts are merged. In these scenarios, the
`source_id`, which is intended to uniquely identify the contact in a
session, may still be associated with the old contact inbox.

To solve this, we check if there’s already a ContactInbox with the same
source_id but linked to another contact. If we find one, we update that
old record by changing its source_id to a random value. This breaks the
wrong connection and prevents issues, while still keeping the old data
safe.

However, this is only a temporary fix. The main issue is with the way
the contact inbox model is designed. Right now, it’s being used to track
sessions, but that may not be necessary for non-live chat channels. In
the long run, we should consider redesigning this part of the system to
avoid such problems.
---
 app/builders/contact_inbox_builder.rb       | 35 +++++++++++++++++++
 app/models/inbox.rb                         |  4 +++
 spec/builders/contact_inbox_builder_spec.rb | 37 +++++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/app/builders/contact_inbox_builder.rb b/app/builders/contact_inbox_builder.rb
index 8fcd2b158..ffa45db2e 100644
--- a/app/builders/contact_inbox_builder.rb
+++ b/app/builders/contact_inbox_builder.rb
@@ -64,5 +64,40 @@ class ContactInboxBuilder
       inbox_id: @inbox.id,
       source_id: @source_id
     )
+  rescue ActiveRecord::RecordNotUnique
+    Rails.logger.info("[ContactInboxBuilder] RecordNotUnique #{@source_id} #{@contact.id} #{@inbox.id}")
+    update_old_contact_inbox
+    retry
+  end
+
+  def update_old_contact_inbox
+    # The race condition occurs when there’s a contact inbox with the
+    # same source ID but linked to a different contact. This can happen
+    # if the agent updates the contact’s email or phone number, or
+    # if the contact is merged with another.
+    #
+    # We update the old contact inbox source_id to a random value to
+    # avoid disrupting the current flow. However, the root cause of
+    # this issue is a flaw in the contact inbox model design.
+    # Contact inbox is essentially tracking a session and is not
+    # needed for non-live chat channels.
+    raise ActiveRecord::RecordNotUnique unless allowed_channels?
+
+    contact_inbox = ::ContactInbox.find_by(inbox_id: @inbox.id, source_id: @source_id)
+    return if contact_inbox.blank?
+
+    contact_inbox.update!(source_id: new_source_id)
+  end
+
+  def new_source_id
+    if @inbox.whatsapp? || @inbox.sms? || @inbox.twilio?
+      "whatsapp:#{@source_id}#{rand(100)}"
+    else
+      "#{rand(10)}#{@source_id}"
+    end
+  end
+
+  def allowed_channels?
+    @inbox.email? || @inbox.sms? || @inbox.twilio? || @inbox.whatsapp?
   end
 end
diff --git a/app/models/inbox.rb b/app/models/inbox.rb
index 2c08172ff..508675858 100644
--- a/app/models/inbox.rb
+++ b/app/models/inbox.rb
@@ -98,6 +98,10 @@ class Inbox < ApplicationRecord
     update_account_cache
   end
 
+  def sms?
+    channel_type == 'Channel::Sms'
+  end
+
   def facebook?
     channel_type == 'Channel::FacebookPage'
   end
diff --git a/spec/builders/contact_inbox_builder_spec.rb b/spec/builders/contact_inbox_builder_spec.rb
index 1dda58063..f0e3deb31 100644
--- a/spec/builders/contact_inbox_builder_spec.rb
+++ b/spec/builders/contact_inbox_builder_spec.rb
@@ -330,5 +330,42 @@ describe ContactInboxBuilder do
         expect(contact_inbox.source_id).not_to be_nil
       end
     end
+
+    context 'when there is a race condition' do
+      let(:account) { create(:account) }
+      let(:contact) { create(:contact, account: account) }
+      let(:contact2) { create(:contact, account: account) }
+      let(:channel) { create(:channel_email, account: account) }
+      let(:channel_api) { create(:channel_api, account: account) }
+      let(:source_id) { 'source_123' }
+
+      it 'handles RecordNotUnique error by updating source_id and retrying' do
+        existing_contact_inbox = create(:contact_inbox, contact: contact2, inbox: channel.inbox, source_id: source_id)
+
+        described_class.new(
+          contact: contact,
+          inbox: channel.inbox,
+          source_id: source_id
+        ).perform
+
+        expect(ContactInbox.last.source_id).to eq(source_id)
+        expect(ContactInbox.last.contact_id).to eq(contact.id)
+        expect(ContactInbox.last.inbox_id).to eq(channel.inbox.id)
+        expect(existing_contact_inbox.reload.source_id).to include(source_id)
+        expect(existing_contact_inbox.reload.source_id).not_to eq(source_id)
+      end
+
+      it 'does not update source_id for channels other than email or phone number' do
+        create(:contact_inbox, contact: contact2, inbox: channel_api.inbox, source_id: source_id)
+
+        expect do
+          described_class.new(
+            contact: contact,
+            inbox: channel_api.inbox,
+            source_id: source_id
+          ).perform
+        end.to raise_error(ActiveRecord::RecordNotUnique)
+      end
+    end
   end
 end