Harden recurring cron, bulk delete, error handling; fix semgrep CI
Continued fixes from the multi-agent review: - recurring tickets cron: now that the parse error is fixed the job runs, exposing two latent bugs. (1) next_run_at was only advanced after the full success path, so any failure (e.g. a NULL created_by passed to the non-nullable assignTicket() $assignedBy -> TypeError) left it in the past and re-created a duplicate ticket every cron cycle. Added an atomic claimForRun() (conditional UPDATE gated on still-due) called BEFORE creation, which also prevents overlapping runs from double-creating. (2) The cron used a raw mysqli with no utf8mb4, corrupting non-ASCII content; it now uses Database::getConnection(). Also guard the assignment so created_by NULL falls back to the assignee. - bulk delete: attachment files were unlinked inside the DB transaction, so an atomic-mode rollback restored rows but the files were already gone. deleteTicket() can now defer file removal to the caller, and BulkOperationsModel deletes files only after a successful commit. - UserModel: back-tick the `groups` column (reserved word on MySQL 8.0.2+). - create_ticket_api.php: stop leaking raw DB/exception messages to callers; log server-side and return a generic error. (Also includes a pre-existing working-tree tweak that adds title to the manual-ticket dedupe hash.) - CI: semgrep install failed under PEP 668; add --break-system-packages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -94,6 +94,10 @@ class BulkOperationsModel
|
||||
// Start transaction for data consistency
|
||||
$this->conn->begin_transaction();
|
||||
|
||||
// Attachment files for deleted tickets are removed only AFTER a successful
|
||||
// commit, so a rollback can't leave tickets with their files already gone.
|
||||
$filesToDelete = [];
|
||||
|
||||
try {
|
||||
foreach ($ticketIds as $ticketId) {
|
||||
$ticketId = trim($ticketId);
|
||||
@@ -200,7 +204,7 @@ class BulkOperationsModel
|
||||
break;
|
||||
|
||||
case 'bulk_delete':
|
||||
$success = $ticketModel->deleteTicket($ticketId);
|
||||
$success = $ticketModel->deleteTicket($ticketId, $filesToDelete);
|
||||
if ($success) {
|
||||
$auditLogModel->log(
|
||||
$operation['performed_by'],
|
||||
@@ -249,6 +253,16 @@ class BulkOperationsModel
|
||||
|
||||
// Commit the transaction
|
||||
$this->conn->commit();
|
||||
|
||||
// Now that the DB delete is durable, remove the physical files. Files
|
||||
// are deleted first; directory entries (no trailing filename) last.
|
||||
foreach ($filesToDelete as $path) {
|
||||
if (is_dir($path)) {
|
||||
@rmdir($path); // only succeeds if empty
|
||||
} elseif (file_exists($path)) {
|
||||
@unlink($path);
|
||||
}
|
||||
}
|
||||
} catch (Exception $e) {
|
||||
// Rollback on any unexpected error
|
||||
$this->conn->rollback();
|
||||
|
||||
Reference in New Issue
Block a user