diff --git a/app/mailboxes/reply_mailbox.rb b/app/mailboxes/reply_mailbox.rb index f4fcb547f..6e766f83e 100644 --- a/app/mailboxes/reply_mailbox.rb +++ b/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 diff --git a/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb b/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb index c1c6b256e..f4f31bf90 100644 --- a/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb +++ b/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, diff --git a/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb b/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb index f790c97d9..a3f1ef7f3 100644 --- a/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb +++ b/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