feat: include support mailbox cases

- Build conversation without persisting in NewConversationStrategy
- Wrap conversation persistence with message creation in single transaction
- Extract persist_conversation_if_needed method for clarity
- Update tests to reflect build-not-create behavior
- Add edge case handling from old SupportMailbox

app/mailboxes/reply_mailbox.rb

@@ -7,9 +7,15 @@ class ReplyMailbox < ApplicationMailbox
     # Return early if no conversation was found (e.g., notification emails, suspended accounts)
     return unless @conversation
 
-    decorate_mail
-    create_message
-    add_attachments_to_message
+    # Wrap everything in a transaction to ensure atomicity
+    # This prevents orphan conversations if message/attachment creation fails
+    # and ensures idempotency on job retry (conversation won't be duplicated)
+    ActiveRecord::Base.transaction do
+      persist_conversation_if_needed
+      decorate_mail
+      create_message
+      add_attachments_to_message
+    end
   end
 
   private
@@ -20,6 +26,15 @@ class ReplyMailbox < ApplicationMailbox
     Rails.logger.info "Email #{mail.message_id} rejected - no conversation found" unless @conversation
   end
 
+  def persist_conversation_if_needed
+    # Save the conversation if it's a new record (from NewConversationStrategy)
+    # We persist here instead of in the strategy to maintain transaction integrity
+    return unless @conversation.new_record?
+
+    @conversation.save!
+    Rails.logger.info "Created new conversation #{@conversation.id} for email #{mail.message_id}"
+  end
+
   def decorate_mail
     @processed_mail = MailPresenter.new(mail, @conversation.account)
   end

app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb

@@ -4,8 +4,12 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
 
   attr_accessor :processed_mail, :account, :inbox, :contact, :contact_inbox, :conversation
 
-  # This strategy always succeeds by creating a new conversation if none was found.
-  # It should be used as the last strategy in the chain to ensure emails are never dropped.
+  # This strategy prepares a new conversation but doesn't persist it yet.
+  # Why we don't use create! here:
+  # - Avoids orphan conversations if message/attachment creation fails later
+  # - Prevents duplicate conversations on job retry (no idempotency issue)
+  # - Follows the pattern from old SupportMailbox where everything was in one transaction
+  # The actual persistence happens in ReplyMailbox within a transaction that includes message creation.
   def find
     channel = EmailChannelFinder.new(mail).perform
     return nil unless channel # No valid channel found
@@ -21,12 +25,11 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
     existing_conversation = find_conversation_by_in_reply_to
     return existing_conversation if existing_conversation
 
-    ActiveRecord::Base.transaction do
-      find_or_create_contact
-      create_conversation
-    end
+    # Prepare contact (persisted) but not conversation
+    find_or_create_contact
 
-    @conversation
+    # Build conversation without saving - ReplyMailbox will handle persistence
+    build_conversation
   end
 
   private
@@ -48,8 +51,9 @@ class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::
     @processed_mail.sender_name || @processed_mail.from.first.split('@').first
   end
 
-  def create_conversation
-    @conversation = ::Conversation.create!(
+  def build_conversation
+    # Build but don't persist - ReplyMailbox will save in transaction with message
+    @conversation = ::Conversation.new(
       account_id: @account.id,
       inbox_id: @inbox.id,
       contact_id: @contact.id,

spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb

@@ -15,16 +15,17 @@ RSpec.describe Mailbox::ConversationFinderStrategies::NewConversationStrategy do
   describe '#find' do
     context 'when channel is found' do
       context 'with new contact' do
-        it 'creates a new conversation with new contact' do
+        it 'builds a new conversation with new contact' do
           strategy = described_class.new(mail)
 
           expect do
             conversation = strategy.find
             expect(conversation).to be_a(Conversation)
+            expect(conversation.new_record?).to be(true) # Not persisted yet
             expect(conversation.inbox).to eq(email_channel.inbox)
             expect(conversation.account).to eq(account)
-          end.to change(Conversation, :count).by(1)
-                                             .and change(Contact, :count).by(1)
+          end.to change(Conversation, :count).by(0) # No conversation created yet
+                                             .and change(Contact, :count).by(1) # Contact is created
                                                                          .and change(ContactInbox, :count).by(1)
         end
 
@@ -53,14 +54,15 @@ RSpec.describe Mailbox::ConversationFinderStrategies::NewConversationStrategy do
           create(:contact_inbox, contact: existing_contact, inbox: email_channel.inbox)
         end
 
-        it 'creates conversation with existing contact' do
+        it 'builds conversation with existing contact' do
           strategy = described_class.new(mail)
 
           expect do
             conversation = strategy.find
             expect(conversation).to be_a(Conversation)
+            expect(conversation.new_record?).to be(true) # Not persisted yet
             expect(conversation.contact).to eq(existing_contact)
-          end.to change(Conversation, :count).by(1)
+          end.to change(Conversation, :count).by(0) # No conversation created yet
                                              .and not_change(Contact, :count)
             .and not_change(ContactInbox, :count)
         end
@@ -142,19 +144,17 @@ RSpec.describe Mailbox::ConversationFinderStrategies::NewConversationStrategy do
 
     context 'when conversation creation fails' do
       before do
-        # Make conversation creation fail
-        allow(Conversation).to receive(:create!).and_raise(ActiveRecord::RecordInvalid)
+        # Make conversation build fail with invalid attributes
+        allow(Conversation).to receive(:new).and_return(Conversation.new)
       end
 
-      it 'rolls back the transaction' do
+      it 'returns invalid conversation object' do
         strategy = described_class.new(mail)
 
-        expect do
-          strategy.find
-        end.to raise_error(ActiveRecord::RecordInvalid)
-          .and not_change(Conversation, :count)
-          .and not_change(Contact, :count)
-          .and not_change(ContactInbox, :count)
+        conversation = strategy.find
+        expect(conversation).to be_a(Conversation)
+        expect(conversation.new_record?).to be(true)
+        expect(conversation.valid?).to be(false)
       end
     end
   end