FIX: POP3 polling shouldn't stop after exception or old email (#12742)

app/jobs/scheduled/poll_mailbox.rb

@@ -39,9 +39,11 @@ module Jobs
       pop3.start(SiteSetting.pop3_polling_username, SiteSetting.pop3_polling_password) do |pop|
         pop.each_mail do |p|
           mail_string = p.pop
-          break if mail_too_old?(mail_string)
+          next if mail_too_old?(mail_string)
           process_popmail(mail_string)
           p.delete if SiteSetting.pop3_polling_delete_from_server?
+        rescue => e
+          Discourse.handle_job_exception(e, error_context(@args, "Failed to process incoming email."))
         end
       end
     rescue Net::OpenTimeout => e

spec/jobs/poll_mailbox_spec.rb

@@ -23,6 +23,23 @@ describe Jobs::PollMailbox do
   end
 
   describe ".poll_pop3" do
+    # the date is dynamic here because there is a 1 week cutoff for
+    # the pop3 polling
+    let(:example_email) do
+      email = <<~EMAIL
+        Return-Path: <one@foo.com>
+        From: One <one@foo.com>
+        To: team@bar.com
+        Subject: Testing email
+        Date: #{1.day.ago.strftime("%a, %d %b %Y")} 03:12:43 +0100
+        Message-ID: <34@foo.bar.mail>
+        Mime-Version: 1.0
+        Content-Type: text/plain
+        Content-Transfer-Encoding: 7bit
+
+        This is an email example.
+      EMAIL
+    end
 
     context "pop errors" do
       before do
@@ -55,6 +72,34 @@ describe Jobs::PollMailbox do
         expect(AdminDashboardData.problem_message_check(i18n_key))
           .to eq(I18n.t(i18n_key, base_path: Discourse.base_path))
       end
+
+      it "logs an error when pop fails and continues with next message" do
+        mail1 = Net::POPMail.new(1, nil, nil, nil)
+        mail2 = Net::POPMail.new(2, nil, nil, nil)
+        mail3 = Net::POPMail.new(3, nil, nil, nil)
+        mail4 = Net::POPMail.new(4, nil, nil, nil)
+
+        Net::POP3.any_instance.stubs(:start).yields(Net::POP3.new(nil, nil))
+        Net::POP3.any_instance.stubs(:mails).returns([mail1, mail2, mail3, mail4])
+
+        mail1.expects(:pop).raises(Net::POPError).once
+        mail1.expects(:delete).never
+
+        mail2.expects(:pop).returns(example_email).once
+        mail2.expects(:delete).raises(Net::POPError).once
+
+        mail3.expects(:pop).returns(example_email).once
+        mail3.expects(:delete).never
+
+        mail4.expects(:pop).returns(example_email).once
+        mail4.expects(:delete).returns(example_email).once
+
+        SiteSetting.pop3_polling_delete_from_server = true
+
+        poller.expects(:mail_too_old?).returns(false).then.raises(RuntimeError).then.returns(false).times(3)
+        poller.expects(:process_popmail).times(2)
+        poller.poll_pop3
+      end
     end
 
     it "calls enable_ssl when the setting is enabled" do
@@ -74,23 +119,6 @@ describe Jobs::PollMailbox do
     context "has emails" do
       let(:oldmail) { file_from_fixtures("old_destination.eml", "emails").read }
 
-      # the date is dynamic here because there is a 1 week cutoff for
-      # the pop3 polling
-      let(:example_email) do
-        email = <<~EMAIL
-        Return-Path: <one@foo.com>
-        From: One <one@foo.com>
-        To: team@bar.com
-        Subject: Testing email
-        Date: #{1.day.ago.strftime("%a, %d %b %Y")} 03:12:43 +0100
-        Message-ID: <34@foo.bar.mail>
-        Mime-Version: 1.0
-        Content-Type: text/plain
-        Content-Transfer-Encoding: 7bit
-
-        This is an email example.
-        EMAIL
-      end
       before do
         mail1 = Net::POPMail.new(1, nil, nil, nil)
         mail2 = Net::POPMail.new(2, nil, nil, nil)
@@ -122,6 +150,12 @@ describe Jobs::PollMailbox do
         SiteSetting.pop3_polling_delete_from_server = false
         poller.poll_pop3
       end
+
+      it "does not stop after an old email" do
+        SiteSetting.pop3_polling_delete_from_server = false
+        poller.expects(:mail_too_old?).returns(false, true, false, false).times(4)
+        poller.poll_pop3
+      end
     end
   end