diff --git a/CHANGELOG.md b/CHANGELOG.md index 9421d480..645381b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,11 @@ This file documents all notable changes made to ITFlow. the command line. The last error a job hit is kept until it is dismissed, rather than disappearing behind the next success, and the page says plainly when the crontab entry itself is missing — the dispatcher records a heartbeat every minute whether or not any job was due. +- **The nightly run is safe to repeat.** Late fees and overdue invoice reminders now apply at + most once per invoice per day, and a card that declined an autopay charge is not retried + until the next day — so a Run Now after the scheduled pass, or a misconfigured schedule, no + longer stacks fees or re-emails clients. Nightly Tasks itself only accepts the daily + schedule in Settings > Cron. - **Ticket SLAs (optional).** SLAs define a response target and an optional resolution target, and are assigned per client and priority, with a global default and an explicit "no SLA" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7a05e09e..7ba93dd1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -97,7 +97,8 @@ Because the jobs share one PHP process, job code has three rules: 1. **Never `exit()` or `die()`.** It ends the whole cycle and every job after it. Use `cronJobStop($message, $exit_code)` instead: it exits when the script was run directly and unwinds back to the dispatcher when it wasn't, so both paths behave as they always have. 2. **Never declare a function or class another job might declare.** Two jobs each declaring the same helper is a fatal `Cannot redeclare` the moment they share a process. Shared helpers belong in `functions/`. -3. **Set what you read.** One global scope and one set of `require_once` includes are shared across the cycle — a job's own `require_once "../config.php"` is a no-op if an earlier job already loaded it, and any variable an earlier job left behind is still there. Do not rely on the state a fresh process would have given you. +3. **Be safe to run twice in one day.** The dispatcher's lock stops overlap, but nothing stops a repeat: an admin presses Run Now after the scheduled pass, or a schedule is misconfigured. Work selected by a date match (`... = CURDATE()`) fires again on every run of that day unless something records that it happened — nightly's late fees and overdue reminders guard on the history rows they write. A job whose work cannot be made repeat-safe declares `'interval_safe' => false` in `includes/cron_jobs.php`, which locks it to the daily schedule in Settings > Cron and in the dispatcher. +4. **Set what you read.** One global scope and one set of `require_once` includes are shared across the cycle — a job's own `require_once "../config.php"` is a no-op if an earlier job already loaded it, and any variable an earlier job left behind is still there. Do not rely on the state a fresh process would have given you. Every script in `cron/` still runs standalone (`php cron/mail_queue.php`) and still takes its own lock when it does, so anything can be run by hand for testing. diff --git a/admin/modals/cron/cron_edit.php b/admin/modals/cron/cron_edit.php index 12e9d41f..d2cc429b 100644 --- a/admin/modals/cron/cron_edit.php +++ b/admin/modals/cron/cron_edit.php @@ -9,6 +9,7 @@ $row = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT * FROM cron_jobs WHERE c $registry = cronJobRegistryByName(); $job = $registry[$row['cron_job_name']] ?? null; +$cron_job_interval_safe = ($job['interval_safe'] ?? true); $cron_job_name = escapeHtml($row['cron_job_name']); $cron_job_label = escapeHtml($job['label'] ?? $row['cron_job_name']); @@ -50,10 +51,15 @@ ob_start(); + + This job's work repeats if it runs twice in one day, so it only runs on the daily schedule. +
diff --git a/admin/post/cron.php b/admin/post/cron.php index 4d2a9090..62cf2b6b 100644 --- a/admin/post/cron.php +++ b/admin/post/cron.php @@ -28,6 +28,12 @@ if (isset($_POST['edit_cron_job'])) { $enabled = isset($_POST['enabled']) ? 1 : 0; $schedule = $_POST['schedule'] === 'Daily' ? 'Daily' : 'Interval'; + // A job the registry marks interval-unsafe only accepts the daily schedule. The form + // does not offer anything else, so anything else arriving here is a crafted request + if (($registry[$row['cron_job_name']]['interval_safe'] ?? true) === false) { + $schedule = 'Daily'; + } + // A job cannot be asked to run more than once a minute (the dispatcher only wakes that // often) and anything over a day belongs on the daily schedule instead. $interval_minutes = min(1440, max(1, intval($_POST['interval_minutes']))); diff --git a/cron/cron.php b/cron/cron.php index 0bae1c82..b41e9e56 100644 --- a/cron/cron.php +++ b/cron/cron.php @@ -104,6 +104,13 @@ function cronJobClaim($mysqli, array $job): bool // start a second or two late, and an exact n-minute comparison would then find the // job 'not due yet' and skip every other cycle. $interval = max(1, intval($row['cron_job_interval_minutes'])); + + // A job the registry marks interval-unsafe never runs more often than daily, + // whatever its row says - a leftover row from before the schedule was locked + // must not revive the every-minute failure. See includes/cron_jobs.php. + if (($job['interval_safe'] ?? true) === false) { + $interval = max($interval, 1440); + } $threshold = date('Y-m-d H:i:s', time() - (($interval * 60) - 30)); $scheduled = true; } diff --git a/cron/nightly_tasks.php b/cron/nightly_tasks.php index 82fd8c53..64533f2d 100644 --- a/cron/nightly_tasks.php +++ b/cron/nightly_tasks.php @@ -99,6 +99,29 @@ if ($config_enable_cron == 0) { cronJobStop("Cron: is not enabled -- Quitting.."); } +/* + * Whether cron has already recorded doing something to an invoice today, judged by the + * history rows this script writes. The overdue and autopay queries below are day-matched - + * they select the same invoices on every run of a given day - so each action that emails a + * client or changes money checks here first. This is what makes a second run in one day + * (Run Now after the 3am pass) safe. Named for this script per the shared-process rule in + * CONTRIBUTING. + */ +function cronInvoiceHistoryToday(int $invoice_id, string $description_prefix): bool +{ + global $mysqli; + + $description_prefix = escapeSql($description_prefix); + + $sql = mysqli_query($mysqli, "SELECT history_id FROM history + WHERE history_invoice_id = $invoice_id + AND history_description LIKE '$description_prefix%' + AND history_created_at >= CURDATE() + LIMIT 1"); + + return mysqli_num_rows($sql) > 0; +} + /* * ############################################################################################################### * STARTUP ACTIONS @@ -554,8 +577,9 @@ if ($config_send_invoice_reminders == 1) { continue; } - // Late Charges - if ($config_invoice_late_fee_enable == 1 && $day > 1) { + // Late Charges - at most one per invoice per day, or a second run of this + // script stacks another fee on the already-inflated balance + if ($config_invoice_late_fee_enable == 1 && $day > 1 && !cronInvoiceHistoryToday($invoice_id, 'Cron applied a late fee')) { $todays_date = date('Y-m-d'); $late_fee_amount = ($invoice_balance * $config_invoice_late_fee_percent) / 100; @@ -578,6 +602,12 @@ if ($config_send_invoice_reminders == 1) { appNotify("Invoice Overdue", "Invoice $invoice_prefix$invoice_number for $client_name with a balance of " . numfmt_format_currency($currency_format, $invoice_balance, $invoice_currency_code) . " is overdue by $day days", "/agent/invoice.php?invoice_id=$invoice_id", $client_id); + // One client email per invoice per day - the 3am run and a Run Now the same + // afternoon must not both mail them + if (cronInvoiceHistoryToday($invoice_id, 'Cron Emailed Overdue Invoice')) { + continue; + } + $subject = "Overdue Invoice $invoice_prefix$invoice_number"; // Only show the paid line if a payment has actually been applied @@ -815,6 +845,12 @@ while ($row = mysqli_fetch_assoc($sql_recurring_payments)) { $contact_name = escapeSql($row['contact_name']); $contact_email = escapeSql($row['contact_email']); + // A card that already declined today is not retried - the day-matched selection above + // would otherwise re-attempt the same charge on every extra run of this script + if (cronInvoiceHistoryToday($invoice_id, 'Stripe autopay failed')) { + continue; + } + // Only attempt autopay if a saved payment method is set if ($recurring_payment_saved_payment_id) { // Get the saved payment method and provider details diff --git a/includes/cron_jobs.php b/includes/cron_jobs.php index a349edc1..57e67827 100644 --- a/includes/cron_jobs.php +++ b/includes/cron_jobs.php @@ -22,6 +22,11 @@ * * That shared use is why this sits here rather than in cron/includes/ with the lock, which * only cron loads: the admin pages would otherwise be reaching into the cron directory. + * + * 'interval_safe' => false marks a job whose work repeats if the day repeats - nightly's + * late fees and overdue reminders fire again on a second run of the same day. Settings > + * Cron only offers the daily schedule for such a job, and the dispatcher refuses to run + * one on an interval whatever its row says. */ function cronJobRegistry(): array @@ -66,6 +71,7 @@ function cronJobRegistry(): array 'description' => 'The daily run: recurring invoices and tickets, overdue reminders, autopay, late fees, clean-up, update check.', 'schedule' => 'Daily', 'daily_at' => '03:00', + 'interval_safe' => false, ], [ 'name' => 'certificate_refresher',