From a844d7b428b917b8c4e9eb5405ef14a4519ea36e Mon Sep 17 00:00:00 2001
From: johnnyq <johnny@pittpc.com>
Date: Mon, 27 Jul 2026 17:35:39 -0400
Subject: [PATCH] Adds a shared flock guard used by all five cron entry points,
 keyed per script and per install. Replaces the mail queue's own lock file,
 which was not atomic and could be deleted out from under a long run. Bounds
 the SMTP conversation so an unresponsive server cannot hold the lock.
 Recovers rows left at Sending by a run that died, which nothing previously
 picked up.

---
 cron/certificate_refresher.php |  4 +++
 cron/cron.php                  | 15 ++--------
 cron/domain_refresher.php      |  4 +++
 cron/mail_queue.php            | 51 ++++++++++++++++++----------------
 cron/ticket_email_parser.php   |  4 +++
 includes/cron_lock.php         | 38 +++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 36 deletions(-)
 create mode 100644 includes/cron_lock.php

diff --git a/cron/certificate_refresher.php b/cron/certificate_refresher.php
index 4bdb04de..f8df97c5 100644
--- a/cron/certificate_refresher.php
+++ b/cron/certificate_refresher.php
@@ -8,6 +8,10 @@ if (php_sapi_name() !== 'cli') {
     die("This script must be run from the command line.\n");
 }
 
+// Prevent overlapping runs of this script
+$cron_lock_script = __FILE__;
+require_once "../includes/cron_lock.php";
+
 require_once "../config.php";
 
 // Set Timezone
diff --git a/cron/cron.php b/cron/cron.php
index 604d4dba..98b609f3 100644
--- a/cron/cron.php
+++ b/cron/cron.php
@@ -8,18 +8,9 @@ if (php_sapi_name() !== 'cli') {
     die("This script must be run from the command line.\n");
 }
 
-// Only one run at a time. Autopay charges cards, so an overlapping run (the previous
-// run still going when the next one fires) could bill the same invoice twice. The lock
-// file is named per install so separate ITFlow instances on one host don't block each
-// other, and the handle is held for the life of the process, released when it exits.
-$cron_lock_file = sys_get_temp_dir() . '/itflow_cron_' . md5(__DIR__) . '.lock';
-$cron_lock_handle = fopen($cron_lock_file, 'c');
-if ($cron_lock_handle === false) {
-    die("Cannot open the cron lock file at $cron_lock_file - check permissions and open_basedir.\n");
-}
-if (!flock($cron_lock_handle, LOCK_EX | LOCK_NB)) {
-    die("Cron is already running - exiting.\n");
-}
+// Prevent overlapping runs of this script
+$cron_lock_script = __FILE__;
+require_once "../includes/cron_lock.php";
 
 require_once "../config.php";
 
diff --git a/cron/domain_refresher.php b/cron/domain_refresher.php
index 0412b1e5..abaa7c0e 100644
--- a/cron/domain_refresher.php
+++ b/cron/domain_refresher.php
@@ -8,6 +8,10 @@ if (php_sapi_name() !== 'cli') {
     die("This script must be run from the command line.\n");
 }
 
+// Prevent overlapping runs of this script
+$cron_lock_script = __FILE__;
+require_once "../includes/cron_lock.php";
+
 require_once "../config.php";
 
 // Set Timezone
diff --git a/cron/mail_queue.php b/cron/mail_queue.php
index 180889c2..a3e443d5 100644
--- a/cron/mail_queue.php
+++ b/cron/mail_queue.php
@@ -7,6 +7,10 @@ if (php_sapi_name() !== 'cli') {
     die("This script must be run from the command line.\n");
 }
 
+// Prevent overlapping runs of this script
+$cron_lock_script = __FILE__;
+require_once "../includes/cron_lock.php";
+
 require_once "../config.php";
 require_once "../includes/inc_set_timezone.php";
 require_once "../functions.php";
@@ -81,25 +85,6 @@ if (empty($config_smtp_provider)) {
     exit(0);
 }
 
-/** =======================================================================
- *  Lock file
- * ======================================================================= */
-$temp_dir = sys_get_temp_dir();
-$lock_file_path = "{$temp_dir}/itflow_mail_queue_{$installation_id}.lock";
-
-if (file_exists($lock_file_path)) {
-    $file_age = time() - filemtime($lock_file_path);
-    if ($file_age > 600) {
-        unlink($lock_file_path);
-        logApp("Cron-Mail-Queue", "warning", "Cron Mail Queue detected a lock file was present but was over 10 minutes old so it removed it.");
-    } else {
-        logApp("Cron-Mail-Queue", "info", "Cron Mail Queue attempted to execute but was already executing so instead it terminated.");
-        exit("Script is already running. Exiting.");
-    }
-}
-
-file_put_contents($lock_file_path, "Locked");
-
 /** =======================================================================
  *  Mail OAuth helpers + sender function
  * ======================================================================= */
@@ -248,6 +233,9 @@ function sendQueueEmail(
     $mail->isSMTP();
     $mail->Host = $host;
     $mail->Port = $port;
+    // Bound the SMTP conversation. Without this an unresponsive mail server can
+    // hold the cron lock open indefinitely and stall the whole queue.
+    $mail->Timeout = 30;
 
     $enc = strtolower($encryption);
     if ($enc === '' || $enc === 'none') {
@@ -301,6 +289,26 @@ function sendQueueEmail(
     return true;
 }
 
+/** =======================================================================
+ *  RECOVER: status = 1 (Sending) left behind by a run that died
+ *
+ *  Nothing else in the codebase ever selects status 1, so without this a row
+ *  claimed by a run that was killed mid-send stays 'Sending' forever and is never
+ *  delivered. The cron lock above guarantees no other run of this script is in
+ *  progress, so any row still sitting at status 1 is by definition orphaned and
+ *  safe to reclaim. It is moved to failed rather than queued so it inherits the
+ *  retry pass's 30 minute backoff and attempt cap instead of retrying instantly.
+ *
+ *  This can re-send a message that did go out but died before being marked sent.
+ *  That trade is deliberate: a duplicate is recoverable, an invoice that silently
+ *  never arrives is not.
+ * ======================================================================= */
+mysqli_query($mysqli, "UPDATE email_queue SET email_status = 2, email_failed_at = NOW(), email_attempts = email_attempts + 1 WHERE email_status = 1");
+$orphaned_emails = mysqli_affected_rows($mysqli);
+if ($orphaned_emails > 0) {
+    logApp("Cron-Mail-Queue", "warning", "Recovered $orphaned_emails email(s) left in a sending state by a previous run - queued for retry.");
+}
+
 /** =======================================================================
  *  SEND: status = 0 (Queued)
  * ======================================================================= */
@@ -464,8 +472,3 @@ if (mysqli_num_rows($sql_failed_queue) > 0) {
         }
     }
 }
-
-/** =======================================================================
- *  Unlock
- * ======================================================================= */
-unlink($lock_file_path);
diff --git a/cron/ticket_email_parser.php b/cron/ticket_email_parser.php
index 17a2ae1a..119a5bf5 100644
--- a/cron/ticket_email_parser.php
+++ b/cron/ticket_email_parser.php
@@ -15,6 +15,10 @@ if (php_sapi_name() !== 'cli') {
     die("This script must be run from the command line.\n");
 }
 
+// Prevent overlapping runs of this script
+$cron_lock_script = __FILE__;
+require_once "../includes/cron_lock.php";
+
 // Autoload (Webklex & any composer deps)
 require_once "../libs/vendor/autoload.php";
 
diff --git a/includes/cron_lock.php b/includes/cron_lock.php
new file mode 100644
index 00000000..e77c1f3e
--- /dev/null
+++ b/includes/cron_lock.php
@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * ITFlow - Single-run guard for cron entry points
+ *
+ * Required by each cron script immediately after its CLI check and before config.php,
+ * with the caller setting $cron_lock_script = __FILE__ first.
+ *
+ * Cron work is not safe to run twice at once: autopay charges cards, the mail queue
+ * sends client email, the parser creates tickets. If a run is still going when the next
+ * one fires, those actions can happen twice. The lock name is derived from the calling
+ * script's full path, so it is unique per script and per install - separate ITFlow
+ * instances on one host never block each other, and neither do two different cron
+ * scripts belonging to the same install.
+ *
+ * flock is used rather than a lock file whose presence is checked, because checking for
+ * a file and then creating it is not atomic - two runs starting together can both find
+ * it absent. flock is also released by the kernel however the process exits, so a killed
+ * run leaves nothing stale behind and no age heuristic is needed to clean up after it.
+ *
+ * Contention is not logged: mail_queue and the email parser run every minute, so hitting
+ * a run that is still going is expected, not a fault worth reporting. A lock file that
+ * cannot be opened at all is a real misconfiguration and does report loudly.
+ *
+ * The handle is deliberately left open: the lock is held for the life of the process.
+ */
+
+$cron_lock_file = sys_get_temp_dir() . '/itflow_cron_' . md5($cron_lock_script) . '.lock';
+$cron_lock_handle = fopen($cron_lock_file, 'c');
+if ($cron_lock_handle === false) {
+    die("Cannot open the cron lock file at $cron_lock_file - check permissions and open_basedir.\n");
+}
+if (!flock($cron_lock_handle, LOCK_EX | LOCK_NB)) {
+    // Exit silently. On a per-minute schedule, finding a previous run still going is
+    // normal operation rather than an error, and anything written to stdout here would
+    // be mailed to the crontab owner every single minute for the length of that run.
+    exit(0);
+}