From c058d04c0551f70906da7198408271fc9486524b Mon Sep 17 00:00:00 2001 From: colemanw Date: Thu, 6 Aug 2026 16:44:34 -0400 Subject: [PATCH] CiviMail - Find and fix any missing recipients It has been reported that some recipients may be silently skipped during large CiviMail dispatches. The "why" is unclear, it may be due to deadlocks or concurrency issues. This adds a stopgap to ensure these irregularities are caught, fixed, and hopefully people will report them and help us track down the root cause. See https://lab.civicrm.org/dev/core/-/work_items/6678 --- CRM/Mailing/BAO/MailingJob.php | 73 ++++++++++++++ .../CRM/Mailing/BAO/MailingJobTest.php | 97 +++++++++++++++++++ 2 files changed, 170 insertions(+) diff --git a/CRM/Mailing/BAO/MailingJob.php b/CRM/Mailing/BAO/MailingJob.php index c52b84721480..0857c950ef27 100644 --- a/CRM/Mailing/BAO/MailingJob.php +++ b/CRM/Mailing/BAO/MailingJob.php @@ -248,6 +248,10 @@ public static function runJobs_post($mode = NULL) { $anyChildLeft = CRM_Core_DAO::singleValueQuery($child_job_sql, $params); + if (!$anyChildLeft) { + $anyChildLeft = self::queueMissingRecipients((int) $job->mailing_id, (int) $job->id); + } + // all of the child jobs are complete, update // the parent job as well as the mailing status if (!$anyChildLeft) { @@ -274,6 +278,75 @@ public static function runJobs_post($mode = NULL) { } } + /** + * Stopgap to find and enqueue any missing mailing recipients. + * + * This shouldn't be needed, but it's been reported that the initial mailing job sometimes misses recipients. + * This searches for any missing recipients and creates a child mailing job for them. + * + * @see https://lab.civicrm.org/dev/core/-/work_items/6678 + * + * @param int $mailingID + * @param int $parentJobID + * + * @return bool True if a new child job was created for unqueued recipients, false otherwise. + */ + private static function queueMissingRecipients(int $mailingID, int $parentJobID): bool { + $sql = " + SELECT mr.email_id, mr.contact_id, mr.phone_id + FROM civicrm_mailing_recipients mr + LEFT JOIN civicrm_mailing_event_queue meq + ON meq.mailing_id = mr.mailing_id + AND meq.is_test = 0 + AND ( + (mr.email_id IS NOT NULL AND mr.email_id = meq.email_id) + OR (mr.phone_id IS NOT NULL AND mr.phone_id = meq.phone_id) + ) + INNER JOIN civicrm_contact cc ON cc.id = mr.contact_id + WHERE mr.mailing_id = %1 + AND meq.id IS NULL + AND (mr.email_id > 0 OR mr.phone_id > 0) + AND cc.is_opt_out = 0 + "; + + $unread = CRM_Core_DAO::executeQuery($sql, [1 => [$mailingID, 'Integer']])->fetchAll(); + if (count($unread) === 0) { + return FALSE; + } + + $maxOffset = (int) CRM_Core_DAO::singleValueQuery(" + SELECT MAX(job_offset) FROM civicrm_mailing_job WHERE mailing_id = %1 AND job_type = 'child' + ", [1 => [$mailingID, 'Integer']]); + + $newJob = MailingJob::create(FALSE)->setValues([ + 'mailing_id' => $mailingID, + 'job_type' => 'child', + 'parent_id' => $parentJobID, + 'job_offset' => $maxOffset + 1000, + 'job_limit' => count($unread), + 'status' => 'Scheduled', + ])->execute()->first(); + + $records = array_map(fn($row) => [ + 'job_id' => $newJob['id'], + 'mailing_id' => $mailingID, + 'is_test' => FALSE, + 'email_id' => $row['email_id'] ? (int) $row['email_id'] : NULL, + 'contact_id' => (int) $row['contact_id'], + 'phone_id' => $row['phone_id'] ? (int) $row['phone_id'] : NULL, + ], $unread); + + CRM_Mailing_Event_BAO_MailingEventQueue::writeRecords($records); + + Civi::log()->notice("MAILING RECIPIENT IRREGULARITY FIXED: Mailing #{mailing_id} missed {count} recipient(s) in its initial queue. Additional job #{job_id} has been created to ensure the missing recipient(s) receive the mailing. Please report this on https://lab.civicrm.org/dev/core/-/work_items/6678 to help us track down why these irregularities happen in the first place.", [ + 'mailing_id' => $mailingID, + 'count' => count($records), + 'job_id' => $newJob['id'], + ]); + + return TRUE; + } + /** * before we run jobs, we need to split the jobs * diff --git a/tests/phpunit/CRM/Mailing/BAO/MailingJobTest.php b/tests/phpunit/CRM/Mailing/BAO/MailingJobTest.php index 85e9b1f1bb72..626a7417d6ad 100644 --- a/tests/phpunit/CRM/Mailing/BAO/MailingJobTest.php +++ b/tests/phpunit/CRM/Mailing/BAO/MailingJobTest.php @@ -16,6 +16,7 @@ * @group headless */ class CRM_Mailing_BAO_MailingJobTest extends CiviUnitTestCase { + use \Civi\Test\Api4TestTrait; /** * Tests CRM_Mailing_BAO_MailingJob::isTemporaryError() method. @@ -40,4 +41,100 @@ public function testIsTemporaryError(): void { } } + /** + * Tests CRM_Mailing_BAO_MailingJob::queueMissingRecipients() stopgap. + */ + public function testQueueMissingRecipients(): void { + $contact = $this->createTestRecord('Contact', [ + 'contact_type' => 'Individual', + 'first_name' => 'Reconcile', + 'last_name' => 'TestUser', + ]); + $contactID = $contact['id']; + + $email = $this->createTestRecord('Email', [ + 'contact_id' => $contactID, + 'email' => 'reconcile_test@example.org', + 'is_primary' => 1, + ]); + $emailID = $email['id']; + + // Create a dummy mailing + $mailing = $this->createTestRecord('Mailing', [ + 'name' => 'Reconciliation Test Mailing', + 'subject' => 'Test Subject', + 'body_text' => 'Test Body', + 'status' => 'Running', + ]); + $mailingID = $mailing['id']; + + // Create parent job + $parentJob = $this->createTestRecord('MailingJob', [ + 'mailing_id' => $mailingID, + 'status' => 'Running', + 'is_test' => 0, + 'job_type' => NULL, + ]); + $parentJobID = $parentJob['id']; + + // Create completed child job + $childJob = $this->createTestRecord('MailingJob', [ + 'mailing_id' => $mailingID, + 'parent_id' => $parentJobID, + 'status' => 'Complete', + 'is_test' => 0, + 'job_type' => 'child', + 'job_offset' => 0, + 'job_limit' => 10, + ]); + + // Insert a recipient record into civicrm_mailing_recipients without queueing in civicrm_mailing_event_queue + CRM_Core_DAO::executeQuery(" + INSERT INTO civicrm_mailing_recipients (mailing_id, contact_id, email_id) + VALUES (%1, %2, %3) + ", [ + 1 => [$mailingID, 'Integer'], + 2 => [$contactID, 'Integer'], + 3 => [$emailID, 'Integer'], + ]); + + // Initial check: recipient is not queued + $reconciled = Invasive::call(['CRM_Mailing_BAO_MailingJob', 'queueMissingRecipients'], [$mailingID, $parentJobID]); + $this->assertTrue($reconciled, 'queueMissingRecipients should return true when unqueued recipients are found'); + + // Verify a new child job was created + $childJobs = \Civi\Api4\MailingJob::get(FALSE) + ->addWhere('parent_id', '=', $parentJobID) + ->addWhere('job_type', '=', 'child') + ->execute(); + $this->assertCount(2, $childJobs, 'A new child job should be created for unqueued recipients'); + + // Get the new child job ID + $newJob = NULL; + foreach ($childJobs as $job) { + if ($job['id'] !== $childJob['id']) { + $newJob = $job; + break; + } + } + $this->assertNotNull($newJob, 'New child job found'); + $this->assertEquals('Scheduled', $newJob['status']); + + // Verify recipient was added to civicrm_mailing_event_queue for the new job + $queuedCount = CRM_Core_DAO::singleValueQuery(" + SELECT COUNT(*) + FROM civicrm_mailing_event_queue + WHERE job_id = %1 AND email_id = %2 AND contact_id = %3 + ", [ + 1 => [$newJob['id'], 'Integer'], + 2 => [$emailID, 'Integer'], + 3 => [$contactID, 'Integer'], + ]); + $this->assertEquals(1, $queuedCount, 'Unqueued recipient should now be present in event queue'); + + // Second check: no more unqueued recipients + $reconciledAgain = Invasive::call(['CRM_Mailing_BAO_MailingJob', 'queueMissingRecipients'], [$mailingID, $parentJobID]); + $this->assertFalse($reconciledAgain, 'queueMissingRecipients should return false when all recipients are queued'); + } + }