diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 968bcf45406..c70b49a1709 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -84,6 +84,9 @@ module Jobs bcc_addresses: bcc_addresses, ) + # Idempotency check – if the EmailLog already exists, do not send again. + return if EmailLog.exists?(message_id: message.message_id) + begin Email::Sender.new(message, :group_smtp, recipient_user).send rescue Net::ReadTimeout => err @@ -95,6 +98,7 @@ module Jobs message: "Got SMTP read timeout when sending group SMTP email", env: args, ) + raise err # Re-raise the error so Sidekiq's retry mechanism kicks in. end # Create an incoming email record to avoid importing again from IMAP diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index b5ec592a74a..ae96cdb5dc5 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -191,12 +191,6 @@ RSpec.describe Jobs::GroupSmtpEmail do expect { Jobs.enqueue(:group_smtp_email, **args) }.not_to raise_error end - it "does not retry the job on SMTP read timeouts, because we can't be sure if the send actually failed or if ENTER . ENTER just timed out" do - Jobs.run_immediately! - Email::Sender.any_instance.expects(:send).raises(Net::ReadTimeout) - expect { Jobs.enqueue(:group_smtp_email, **args) }.not_to raise_error - end - context "when there are cc_addresses" do it "has the cc_addresses and cc_user_ids filled in correctly" do job.execute(args) @@ -322,4 +316,38 @@ RSpec.describe Jobs::GroupSmtpEmail do ).to eq(true) end end + + it "re-raises Net::ReadTimeout to trigger Sidekiq retries" do + allow_any_instance_of(Email::Sender).to receive(:send).and_raise( + Net::ReadTimeout.new("timeout"), + ) + expect { job.execute(args) }.to raise_error(Net::ReadTimeout) + end + + it "does not send email if an EmailLog with the same message_id already exists" do + message = Mail::Message.new(body: "hello", to: "myemail@example.invalid") + message.message_id = "unique_message_id" + GroupSmtpMailer + .expects(:send_mail) + .with( + group, + "test@test.com", + post, + cc_addresses: %w[otherguy@test.com cormac@lit.com], + bcc_addresses: [], + ) + .returns(message) + EmailLog.expects(:exists?).with(message_id: message.message_id).returns(true) + Email::Sender.any_instance.expects(:send).never + + job.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + it "retries the job on SMTP read timeouts when there are no duplicates" do + Jobs.run_immediately! + EmailLog.stubs(:exists?).with(message_id: anything).returns(false) + Email::Sender.any_instance.expects(:send).raises(Net::ReadTimeout) + expect { Jobs.enqueue(:group_smtp_email, **args) }.to raise_error(Net::ReadTimeout) + end end