diff --git a/api/ticket_status_api.php b/api/ticket_status_api.php index cce86f3..c905154 100644 --- a/api/ticket_status_api.php +++ b/api/ticket_status_api.php @@ -126,23 +126,6 @@ if ($workflowModel->transitionRequiresComment($currentStatus, $newStatus) && $co exit; } -// Post the comment first (per-key label) so a close-with-reason is one call. -if ($comment !== '') { - $commentModel = new CommentModel($conn); - $commentResult = $commentModel->addComment($ticketId, [ - 'user_name' => $keyName, - 'comment_text' => $comment, - 'markdown_enabled' => !empty($data['markdown_enabled']), - ], $createdBy); - if (empty($commentResult['success'])) { - error_log('ticket_status_api: addComment failed for ticket ' . $ticketId - . ': ' . ($commentResult['error'] ?? 'unknown')); - http_response_code(500); - echo json_encode(['success' => false, 'error' => 'Failed to add comment']); - exit; - } -} - // Apply the status change. updateTicket sets updated_by/updated_at and handles // closed_at (set on close, cleared on reopen) via its own SQL. $updateData = [ @@ -155,10 +138,33 @@ $updateData = [ 'priority' => (int)$ticket['priority'], ]; -$updateResult = $ticketModel->updateTicket($updateData, $createdBy); -if (empty($updateResult['success'])) { - error_log('ticket_status_api: updateTicket failed for ticket ' . $ticketId - . ': ' . ($updateResult['error'] ?? 'unknown')); +// Post the comment and apply the status change in one transaction, so a +// failure partway through can't leave a "reason" comment persisted with no +// matching status change (previously these were two independent writes with +// no shared rollback). +$conn->begin_transaction(); +try { + if ($comment !== '') { + $commentModel = new CommentModel($conn); + $commentResult = $commentModel->addComment($ticketId, [ + 'user_name' => $keyName, + 'comment_text' => $comment, + 'markdown_enabled' => !empty($data['markdown_enabled']), + ], $createdBy); + if (empty($commentResult['success'])) { + throw new Exception($commentResult['error'] ?? 'Failed to add comment'); + } + } + + $updateResult = $ticketModel->updateTicket($updateData, $createdBy); + if (empty($updateResult['success'])) { + throw new Exception($updateResult['error'] ?? 'Failed to update ticket status'); + } + + $conn->commit(); +} catch (Exception $e) { + $conn->rollback(); + error_log('ticket_status_api: transaction failed for ticket ' . $ticketId . ': ' . $e->getMessage()); http_response_code(500); echo json_encode(['success' => false, 'error' => 'Failed to update ticket status']); exit; diff --git a/api/update_ticket.php b/api/update_ticket.php index dded313..08c4c22 100644 --- a/api/update_ticket.php +++ b/api/update_ticket.php @@ -195,8 +195,8 @@ try { // Enforce requires_comment transitions server-side. if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) { - $comment = trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); - if ($comment === '') { + $statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); + if ($statusChangeComment === '') { return [ 'success' => false, 'error' => 'A comment is required for this status change', @@ -207,40 +207,67 @@ try { } } - // Update ticket with user tracking and optional optimistic locking - $expectedUpdatedAt = $data['expected_updated_at'] ?? null; - $result = $this->ticketModel->updateTicket($updateData, $this->userId, $expectedUpdatedAt); + // A comment accompanying a status change (required or optional) is + // persisted in the SAME transaction as the status update below, so + // a failure partway through can't leave an orphaned "reason" + // comment attached with no matching status change — the two + // previously ran as separate, non-transactional HTTP calls from + // the client (add_comment.php then update_ticket.php). + $statusChangeComment = $statusChangeComment ?? trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); - // Handle conflict case - if (!$result['success']) { - $response = [ - 'success' => false, - 'error' => $result['error'] ?? 'Failed to update ticket in database' - ]; - if (!empty($result['conflict'])) { + $result = null; + $this->conn->begin_transaction(); + try { + if ($statusChangeComment !== '' && $currentTicket['status'] !== $updateData['status']) { + $commentResult = $this->commentModel->addComment($id, [ + 'user_name' => $this->currentUser['display_name'] ?? $this->currentUser['username'] ?? 'User', + 'comment_text' => $statusChangeComment, + 'markdown_enabled' => !empty($data['markdown_enabled']), + ], $this->userId); + if (empty($commentResult['success'])) { + throw new Exception($commentResult['error'] ?? 'Failed to add comment'); + } + } + + // Update ticket with user tracking and optional optimistic locking + $expectedUpdatedAt = $data['expected_updated_at'] ?? null; + $result = $this->ticketModel->updateTicket($updateData, $this->userId, $expectedUpdatedAt); + if (!$result['success']) { + throw new Exception($result['error'] ?? 'Failed to update ticket in database'); + } + + // Handle visibility update if provided (already validated above) + if (isset($data['visibility'])) { + $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId); + if (!$visResult) { + throw new Exception('Failed to update ticket visibility'); + } + } + + $this->conn->commit(); + } catch (Exception $e) { + $this->conn->rollback(); + $response = ['success' => false, 'error' => $e->getMessage()]; + if (is_array($result) && !empty($result['conflict'])) { $response['conflict'] = true; $response['current_updated_at'] = $result['current_updated_at'] ?? null; } return $response; } - // Handle visibility update if provided (already validated above) - if (isset($data['visibility'])) { - $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId); - if ($visResult && $this->userId) { - $this->auditLog->log( - $this->userId, - 'update', - 'ticket', - (string)$id, - [ - 'field' => 'visibility', - 'from' => $currentTicket['visibility'] ?? 'public', - 'to' => $data['visibility'], - 'groups' => $visibilityGroups - ] - ); - } + if (isset($data['visibility']) && $this->userId) { + $this->auditLog->log( + $this->userId, + 'update', + 'ticket', + (string)$id, + [ + 'field' => 'visibility', + 'from' => $currentTicket['visibility'] ?? 'public', + 'to' => $data['visibility'], + 'groups' => $visibilityGroups + ] + ); } // Log ticket update to audit log — only the changed fields (delta) diff --git a/assets/js/base.js b/assets/js/base.js index 329f022..d64d633 100644 --- a/assets/js/base.js +++ b/assets/js/base.js @@ -2858,8 +2858,11 @@ TICKET STATUS CHANGE (comment-aware) lt.ticketStatus.submit(ticketId, newStatus, { comment? }) → Promise Posts /api/update_ticket.php. If the server rejects with - requires_comment, opens a comment modal, persists the comment via - /api/add_comment.php, then retries the update once WITH the comment. + requires_comment, opens a comment modal, then retries the update once + WITH the comment — update_ticket.php persists it in the same DB + transaction as the status change itself, so there's no separate + add_comment.php call that could leave an orphaned comment if the + status update then failed. Rejects with err.cancelled === true if the user cancels the modal. ================================================================ */ function _statusCommentModal(newStatus) { @@ -2922,9 +2925,9 @@ cancelErr.cancelled = true; throw cancelErr; } - // Persist the comment, then retry the status change with it included. - return api.post('/api/add_comment.php', { ticket_id: id, comment_text: comment }) - .then(() => api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment })); + // Retry with the comment included — update_ticket.php persists it + // transactionally with the status update itself. + return api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment }); }); }); }, diff --git a/assets/js/ticket.js b/assets/js/ticket.js index f40cab2..aae3d10 100644 --- a/assets/js/ticket.js +++ b/assets/js/ticket.js @@ -713,12 +713,13 @@ function updateTicketStatus() { return; } cleanup(true); - // Post comment first (persists it), then change status with the same - // comment included so the server's requires_comment check passes. - const ticketId = getTicketIdFromUrl(); - lt.api.post('/api/add_comment.php', { ticket_id: ticketId, comment_text: comment }) - .then(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)) - .catch(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)); + // The comment is sent as part of the status-change request itself + // (update_ticket.php persists it in the same DB transaction as the + // status update) rather than as a separate prior add_comment.php + // call — previously those were two independent, non-transactional + // writes, so a failure partway through could leave the "reason" + // comment persisted with no matching status change ever applied. + performStatusChange(statusSelect, selectedOption, newStatus, comment); }); // Focus textarea on open setTimeout(() => { const ta = document.getElementById(`${modalId}_comment`); if (ta) ta.focus(); }, 100); diff --git a/create_ticket_api.php b/create_ticket_api.php index a723104..c5f11b6 100644 --- a/create_ticket_api.php +++ b/create_ticket_api.php @@ -232,301 +232,353 @@ $priority = (int)$priority; $ticketHash = generateTicketHash($data); $auditLog = new AuditLogModel($conn); -// Look up any existing ticket with this hash (open OR closed) -$checkStmt = $conn->prepare("SELECT ticket_id, status, title, priority FROM tickets WHERE hash = ? ORDER BY created_at DESC LIMIT 1"); -$checkStmt->bind_param("s", $ticketHash); -$checkStmt->execute(); -$existing = $checkStmt->get_result()->fetch_assoc(); -$checkStmt->close(); +// Everything from here through either updating/reopening the matched ticket +// or inserting a brand-new one runs inside one transaction with a row lock +// on the hash lookup. Without this, two concurrent requests carrying the +// same dedup hash (e.g. overlapping monitoring runs) could both read the +// same pre-update snapshot and each independently apply an escalation. FOR +// UPDATE on this equality lookup against the unique-indexed hash column also +// takes a lock on the "gap" where no row currently exists, so two concurrent +// requests for a genuinely new hash are still safe from a duplicate row — +// but that gap lock is shared, not exclusive, so both can reach the INSERT +// below and deadlock with each other rather than one blocking cleanly on the +// other's row. See the retry loop and comment near the INSERT's catch block +// for how that case is handled. +// Retried once if the INSERT below deadlocks with another connection's +// concurrent insert into the same not-yet-existing hash (see comment +// above the INSERT's catch block) — the retry's own SELECT ... FOR UPDATE +// will then find the winner's already-committed row and take the +// update/escalate branch instead of erroring out. +$maxDedupAttempts = 2; +for ($dedupAttempt = 1; $dedupAttempt <= $maxDedupAttempts; $dedupAttempt++) { + $conn->begin_transaction(); -if ($existing) { - $existingId = $existing['ticket_id']; - $existingStatus = $existing['status']; - $existingTitle = $existing['title']; - $existingPriority = (int)$existing['priority']; - $newPriority = (int)$priority; + // Look up any existing ticket with this hash (open OR closed) + $checkStmt = $conn->prepare("SELECT ticket_id, status, title, priority FROM tickets WHERE hash = ? ORDER BY created_at DESC LIMIT 1 FOR UPDATE"); + $checkStmt->bind_param("s", $ticketHash); + $checkStmt->execute(); + $existing = $checkStmt->get_result()->fetch_assoc(); + $checkStmt->close(); - if ($existingStatus !== 'Closed') { - // Ticket is still active — update title, escalate priority, and refresh - // description with latest sensor data. - $changes = []; - $updateSql = "UPDATE tickets SET updated_at = NOW(), updated_by = ?"; - $bindTypes = "i"; - $bindVals = [$userId]; + if ($existing) { + $existingId = $existing['ticket_id']; + $existingStatus = $existing['status']; + $existingTitle = $existing['title']; + $existingPriority = (int)$existing['priority']; + $newPriority = (int)$priority; - if ($title !== $existingTitle) { - $updateSql .= ", title = ?"; - $bindTypes .= "s"; - $bindVals[] = $title; - $changes['title'] = ['from' => $existingTitle, 'to' => $title]; - } + if ($existingStatus !== 'Closed') { + // Ticket is still active — update title, escalate priority, and refresh + // description with latest sensor data. + $changes = []; + $updateSql = "UPDATE tickets SET updated_at = NOW(), updated_by = ?"; + $bindTypes = "i"; + $bindVals = [$userId]; - if ($newPriority < $existingPriority) { - $updateSql .= ", priority = ?"; - $bindTypes .= "i"; - $bindVals[] = $newPriority; - $changes['priority'] = ['from' => $existingPriority, 'to' => $newPriority]; - } - - // Always refresh the description so the ticket body shows current sensor data - if (!empty($description)) { - $updateSql .= ", description = ?"; - $bindTypes .= "s"; - $bindVals[] = $description; - $changes['description_refreshed'] = true; - } - - if (!empty($changes)) { - $updateSql .= " WHERE ticket_id = ?"; - $bindTypes .= "s"; - $bindVals[] = $existingId; - - $updStmt = $conn->prepare($updateSql); - $updStmt->bind_param($bindTypes, ...$bindVals); - $updStmt->execute(); - $updStmt->close(); - - // Only post a comment on priority escalation — title and description updates - // are silent (title changes like rising counters would spam a comment every run). - // Keep it short: the full sensor data is refreshed in the ticket description, - // so the comment just records the bump + a brief reason (no ASCII dump). - if (isset($changes['priority'])) { - $pLabels = [1 => 'P1 (Critical)', 2 => 'P2 (High)', 3 => 'P3 (Medium)', 4 => 'P4 (Low)', 5 => 'P5 (Minimal)']; - $fromP = (int)$changes['priority']['from']; - $toP = (int)$changes['priority']['to']; - $fromL = $pLabels[$fromP] ?? "P{$fromP}"; - $toL = $pLabels[$toP] ?? "P{$toP}"; - $commentText = "**hwmonDaemon raised priority {$fromL} → {$toL}.**\n\n" - . "The latest monitoring scan reported a more severe condition for this issue, " - . "so it now needs faster attention. Current sensor data is in the ticket description above."; - $commentStmt = $conn->prepare( - "INSERT INTO ticket_comments (ticket_id, user_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, 'hwmonDaemon', ?, 1)" - ); - $commentStmt->bind_param("sis", $existingId, $userId, $commentText); - $commentStmt->execute(); - $commentStmt->close(); + if ($title !== $existingTitle) { + $updateSql .= ", title = ?"; + $bindTypes .= "s"; + $bindVals[] = $title; + $changes['title'] = ['from' => $existingTitle, 'to' => $title]; } - $auditLog->log($userId, 'update', 'ticket', $existingId, array_merge( - array_diff_key($changes, ['description_refreshed' => true]), - ['reason' => 'auto-updated by hwmonDaemon (condition worsened)'] - )); - - // Only notify on priority escalation — title-only updates (e.g. rising - // Power_On_Hours counter) should not generate a Matrix ping every hour. - if (isset($changes['priority'])) { - require_once __DIR__ . '/helpers/NotificationHelper.php'; - NotificationHelper::sendTicketNotification($existingId, [ - 'title' => $title, - 'priority' => $changes['priority']['to'], - 'category' => $category, - 'type' => $type, - 'status' => $existingStatus, - ], 'automated'); + if ($newPriority < $existingPriority) { + $updateSql .= ", priority = ?"; + $bindTypes .= "i"; + $bindVals[] = $newPriority; + $changes['priority'] = ['from' => $existingPriority, 'to' => $newPriority]; } - // Ticket state (priority/title/description) changed — refresh dashboard stats. + // Always refresh the description so the ticket body shows current sensor data + if (!empty($description)) { + $updateSql .= ", description = ?"; + $bindTypes .= "s"; + $bindVals[] = $description; + $changes['description_refreshed'] = true; + } + + if (!empty($changes)) { + $updateSql .= " WHERE ticket_id = ?"; + $bindTypes .= "s"; + $bindVals[] = $existingId; + + $updStmt = $conn->prepare($updateSql); + $updStmt->bind_param($bindTypes, ...$bindVals); + $updStmt->execute(); + $updStmt->close(); + + // Only post a comment on priority escalation — title and description updates + // are silent (title changes like rising counters would spam a comment every run). + // Keep it short: the full sensor data is refreshed in the ticket description, + // so the comment just records the bump + a brief reason (no ASCII dump). + if (isset($changes['priority'])) { + $pLabels = [1 => 'P1 (Critical)', 2 => 'P2 (High)', 3 => 'P3 (Medium)', 4 => 'P4 (Low)', 5 => 'P5 (Minimal)']; + $fromP = (int)$changes['priority']['from']; + $toP = (int)$changes['priority']['to']; + $fromL = $pLabels[$fromP] ?? "P{$fromP}"; + $toL = $pLabels[$toP] ?? "P{$toP}"; + $commentText = "**hwmonDaemon raised priority {$fromL} → {$toL}.**\n\n" + . "The latest monitoring scan reported a more severe condition for this issue, " + . "so it now needs faster attention. Current sensor data is in the ticket description above."; + $commentStmt = $conn->prepare( + "INSERT INTO ticket_comments (ticket_id, user_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, 'hwmonDaemon', ?, 1)" + ); + $commentStmt->bind_param("sis", $existingId, $userId, $commentText); + $commentStmt->execute(); + $commentStmt->close(); + } + + $auditLog->log($userId, 'update', 'ticket', $existingId, array_merge( + array_diff_key($changes, ['description_refreshed' => true]), + ['reason' => 'auto-updated by hwmonDaemon (condition worsened)'] + )); + + // Only notify on priority escalation — title-only updates (e.g. rising + // Power_On_Hours counter) should not generate a Matrix ping every hour. + if (isset($changes['priority'])) { + require_once __DIR__ . '/helpers/NotificationHelper.php'; + NotificationHelper::sendTicketNotification($existingId, [ + 'title' => $title, + 'priority' => $changes['priority']['to'], + 'category' => $category, + 'type' => $type, + 'status' => $existingStatus, + ], 'automated'); + } + + // Ticket state (priority/title/description) changed — refresh dashboard stats. + (new StatsModel($conn))->invalidateCache(); + } + + $conn->commit(); + Database::close(); + echo json_encode([ + 'success' => true, + 'ticket_id' => $existingId, + 'message' => empty($changes) ? 'Duplicate — no change' : 'Existing ticket updated', + 'action' => empty($changes) ? 'deduplicated' : 'updated', + 'changes' => $changes, + ]); + exit; + } + + // Ticket was closed — reopen it and add a recurrence comment. Route + // through the Workflow Designer like every other status-write path in + // the app, rather than forcing status='Open' via raw SQL regardless of + // configured transition rules. + $workflowModel = new WorkflowModel($conn); + $reopenStatus = 'Open'; + if (!$workflowModel->isTransitionAllowed('Closed', 'Open', false)) { + // Direct Closed->Open isn't configured — fall back to any transition + // the Workflow Designer does allow from Closed that this unattended, + // non-admin automation can actually satisfy (no comment prompt, no + // admin elevation). If even that doesn't exist, leave the ticket + // Closed rather than force an unconfigured state. + $reopenStatus = null; + foreach ($workflowModel->getAllowedTransitions('Closed') as $transition) { + if (!$transition['requires_comment'] && !$transition['requires_admin']) { + $reopenStatus = $transition['to_status']; + break; + } + } + } + + if ($reopenStatus !== null) { + $ticketModel = new TicketModel($conn); + $ticketModel->updateTicket([ + 'ticket_id' => $existingId, + 'title' => $title, + 'description' => $description, + 'category' => $category, + 'type' => $type, + 'status' => $reopenStatus, + 'priority' => $priority, + ], $userId); + } else { + error_log("create_ticket_api: hwmonDaemon recurrence for ticket $existingId — " + . "no admin-free, comment-free transition from Closed is configured; leaving ticket Closed"); + } + + $commentText = "**Issue recurred — ticket reopened automatically.**\n\n" . + "hwmonDaemon detected this condition again. The ticket description reflects the " + . "original report; see this comment's timestamp for when the issue recurred."; + if ($reopenStatus === null) { + $commentText = "**Issue recurred, but the ticket could not be reopened automatically.**\n\n" + . "hwmonDaemon detected this condition again. No Workflow Designer transition from " + . "Closed is configured that this automation can perform unattended (no comment/admin " + . "requirement); the ticket remains Closed. Please review and reopen manually if appropriate."; + } + $commentStmt = $conn->prepare( + "INSERT INTO ticket_comments (ticket_id, user_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, 'hwmonDaemon', ?, 1)" + ); + $commentStmt->bind_param("sis", $existingId, $userId, $commentText); + $commentStmt->execute(); + $commentStmt->close(); + + if ($reopenStatus !== null) { + $auditLog->log($userId, 'update', 'ticket', $existingId, [ + 'status' => ['from' => 'Closed', 'to' => $reopenStatus], + 'reason' => 'auto-reopened by hwmonDaemon (issue recurred)', + ]); + + // Ticket reopened — refresh dashboard stats. (new StatsModel($conn))->invalidateCache(); + } else { + $auditLog->log($userId, 'update', 'ticket', $existingId, [ + 'reason' => 'hwmonDaemon recurrence detected but no valid reopen transition configured; ticket left Closed', + ]); } + $conn->commit(); Database::close(); + + if ($reopenStatus !== null) { + require_once __DIR__ . '/helpers/NotificationHelper.php'; + NotificationHelper::sendTicketNotification($existingId, [ + 'title' => $title, + 'priority' => $priority, + 'category' => $category, + 'type' => $type, + 'status' => $reopenStatus, + ], 'automated'); + } + echo json_encode([ - 'success' => true, - 'ticket_id' => $existingId, - 'message' => empty($changes) ? 'Duplicate — no change' : 'Existing ticket updated', - 'action' => empty($changes) ? 'deduplicated' : 'updated', - 'changes' => $changes, + 'success' => true, + 'ticket_id' => $existingId, + 'message' => $reopenStatus !== null + ? 'Existing closed ticket reopened' + : 'Recurrence noted; ticket left Closed (no valid workflow transition configured)', + 'action' => $reopenStatus !== null ? 'reopened' : 'recurrence_noted', ]); exit; } - // Ticket was closed — reopen it and add a recurrence comment. Route - // through the Workflow Designer like every other status-write path in - // the app, rather than forcing status='Open' via raw SQL regardless of - // configured transition rules. - $workflowModel = new WorkflowModel($conn); - $reopenStatus = 'Open'; - if (!$workflowModel->isTransitionAllowed('Closed', 'Open', false)) { - // Direct Closed->Open isn't configured — fall back to any transition - // the Workflow Designer does allow from Closed that this unattended, - // non-admin automation can actually satisfy (no comment prompt, no - // admin elevation). If even that doesn't exist, leave the ticket - // Closed rather than force an unconfigured state. - $reopenStatus = null; - foreach ($workflowModel->getAllowedTransitions('Closed') as $transition) { - if (!$transition['requires_comment'] && !$transition['requires_admin']) { - $reopenStatus = $transition['to_status']; - break; - } + // No existing ticket — create a new one. Still inside the transaction opened + // above, so a concurrent request for the same hash is blocked on its own + // SELECT ... FOR UPDATE until this one commits or rolls back (see comment + // there) rather than racing this INSERT. + // + // Note on FOR UPDATE over a not-yet-existing key: InnoDB's gap lock in that + // case is a shared lock, not exclusive — two concurrent transactions can + // both acquire it and both reach this INSERT. The conflict only surfaces + // when they each request the insert-intention lock for the same gap, + // which InnoDB resolves as a deadlock (error 1213), not by blocking one + // of the SELECTs. The outer loop above retries that case: the loser rolls + // back and re-runs its own SELECT ... FOR UPDATE, which by then finds the + // winner's committed row and takes the update/escalate branch instead. + // + // Generate a collision-safe unique ticket_id with a pre-check + retry loop (same + // approach as TicketModel::createTicket) so a ticket_id clash cannot happen. That + // way a 1062 on INSERT below can only be the unique_hash (dedup) key — and with + // the FOR UPDATE lock above, only in the unlikely case of a hash collision from + // two genuinely different reports, not the same-hash race this used to be. + $ticket_id = null; + $maxAttempts = 50; + $attempts = 0; + do { + try { + $candidateId = sprintf('%09d', random_int(100000000, 999999999)); + } catch (Exception $e) { + $candidateId = sprintf('%09d', mt_rand(100000000, 999999999)); } + + $idCheckStmt = $conn->prepare("SELECT ticket_id FROM tickets WHERE ticket_id = ? LIMIT 1"); + $idCheckStmt->bind_param("s", $candidateId); + $idCheckStmt->execute(); + $idExists = $idCheckStmt->get_result()->num_rows > 0; + $idCheckStmt->close(); + + if (!$idExists) { + $ticket_id = $candidateId; + } + $attempts++; + } while ($ticket_id === null && $attempts < $maxAttempts); + + if ($ticket_id === null) { + $conn->rollback(); + error_log('create_ticket_api: failed to generate a unique ticket_id after ' . $maxAttempts . ' attempts'); + http_response_code(500); + echo json_encode(['success' => false, 'error' => 'Internal server error']); + exit; } - if ($reopenStatus !== null) { - $ticketModel = new TicketModel($conn); - $ticketModel->updateTicket([ - 'ticket_id' => $existingId, - 'title' => $title, - 'description' => $description, - 'category' => $category, - 'type' => $type, - 'status' => $reopenStatus, - 'priority' => $priority, - ], $userId); - } else { - error_log("create_ticket_api: hwmonDaemon recurrence for ticket $existingId — " - . "no admin-free, comment-free transition from Closed is configured; leaving ticket Closed"); - } - - $commentText = "**Issue recurred — ticket reopened automatically.**\n\n" . - "hwmonDaemon detected this condition again. The ticket description reflects the " - . "original report; see this comment's timestamp for when the issue recurred."; - if ($reopenStatus === null) { - $commentText = "**Issue recurred, but the ticket could not be reopened automatically.**\n\n" - . "hwmonDaemon detected this condition again. No Workflow Designer transition from " - . "Closed is configured that this automation can perform unattended (no comment/admin " - . "requirement); the ticket remains Closed. Please review and reopen manually if appropriate."; - } - $commentStmt = $conn->prepare( - "INSERT INTO ticket_comments (ticket_id, user_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, 'hwmonDaemon', ?, 1)" + $insertStmt = $conn->prepare( + "INSERT INTO tickets (ticket_id, title, description, status, priority, category, type, hash, created_by) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)" + ); + $insertStmt->bind_param( + "ssssssssi", + $ticket_id, + $title, + $description, + $status, + $priority, + $category, + $type, + $ticketHash, + $userId ); - $commentStmt->bind_param("sis", $existingId, $userId, $commentText); - $commentStmt->execute(); - $commentStmt->close(); - if ($reopenStatus !== null) { - $auditLog->log($userId, 'update', 'ticket', $existingId, [ - 'status' => ['from' => 'Closed', 'to' => $reopenStatus], - 'reason' => 'auto-reopened by hwmonDaemon (issue recurred)', - ]); - - // Ticket reopened — refresh dashboard stats. - (new StatsModel($conn))->invalidateCache(); - } else { - $auditLog->log($userId, 'update', 'ticket', $existingId, [ - 'reason' => 'hwmonDaemon recurrence detected but no valid reopen transition configured; ticket left Closed', - ]); + try { + $inserted = $insertStmt->execute(); + } catch (mysqli_sql_exception $e) { + $insertStmt->close(); + $conn->rollback(); + if (in_array($e->getCode(), [1213, 1205], true) && $dedupAttempt < $maxDedupAttempts) { + // Deadlock (1213) or lock wait timeout (1205) from a concurrent + // insert into the same not-yet-existing hash gap — see the note + // above. Retry: the next iteration's own SELECT ... FOR UPDATE + // will find whichever side won and take the update/escalate path. + continue; + } + if ($e->getCode() === 1062) { + // Should be unreachable in the same-hash race this issue was filed + // for now that the SELECT above takes FOR UPDATE — kept as a + // defensive fallback in case of a genuine hash collision between two + // different reports. + echo json_encode(['success' => false, 'error' => 'Duplicate ticket']); + } else { + error_log('create_ticket_api: insert failed: ' . $e->getMessage()); + http_response_code(500); + echo json_encode(['success' => false, 'error' => 'Internal server error']); + } + exit; } + $insertStmt->close(); - Database::close(); - - if ($reopenStatus !== null) { - require_once __DIR__ . '/helpers/NotificationHelper.php'; - NotificationHelper::sendTicketNotification($existingId, [ + if ($inserted) { + $auditLog->logTicketCreate($userId, $ticket_id, [ 'title' => $title, 'priority' => $priority, 'category' => $category, 'type' => $type, - 'status' => $reopenStatus, + ]); + + // New ticket created — refresh dashboard stats. + (new StatsModel($conn))->invalidateCache(); + + $conn->commit(); + Database::close(); + + require_once __DIR__ . '/helpers/NotificationHelper.php'; + NotificationHelper::sendTicketNotification($ticket_id, [ + 'title' => $title, + 'priority' => $priority, + 'category' => $category, + 'type' => $type, + 'status' => $status, ], 'automated'); - } - echo json_encode([ - 'success' => true, - 'ticket_id' => $existingId, - 'message' => $reopenStatus !== null - ? 'Existing closed ticket reopened' - : 'Recurrence noted; ticket left Closed (no valid workflow transition configured)', - 'action' => $reopenStatus !== null ? 'reopened' : 'recurrence_noted', - ]); - exit; -} - -// No existing ticket — create a new one. -// Generate a collision-safe unique ticket_id with a pre-check + retry loop (same -// approach as TicketModel::createTicket) so a ticket_id clash cannot happen. That -// way a 1062 on INSERT below can only be the unique_hash (dedup) key racing, and -// is correctly reported as a duplicate rather than a dropped hardware alert. -$ticket_id = null; -$maxAttempts = 50; -$attempts = 0; -do { - try { - $candidateId = sprintf('%09d', random_int(100000000, 999999999)); - } catch (Exception $e) { - $candidateId = sprintf('%09d', mt_rand(100000000, 999999999)); - } - - $idCheckStmt = $conn->prepare("SELECT ticket_id FROM tickets WHERE ticket_id = ? LIMIT 1"); - $idCheckStmt->bind_param("s", $candidateId); - $idCheckStmt->execute(); - $idExists = $idCheckStmt->get_result()->num_rows > 0; - $idCheckStmt->close(); - - if (!$idExists) { - $ticket_id = $candidateId; - } - $attempts++; -} while ($ticket_id === null && $attempts < $maxAttempts); - -if ($ticket_id === null) { - error_log('create_ticket_api: failed to generate a unique ticket_id after ' . $maxAttempts . ' attempts'); - http_response_code(500); - echo json_encode(['success' => false, 'error' => 'Internal server error']); - exit; -} - -$insertStmt = $conn->prepare( - "INSERT INTO tickets (ticket_id, title, description, status, priority, category, type, hash, created_by) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)" -); -$insertStmt->bind_param( - "ssssssssi", - $ticket_id, - $title, - $description, - $status, - $priority, - $category, - $type, - $ticketHash, - $userId -); - -try { - $inserted = $insertStmt->execute(); -} catch (mysqli_sql_exception $e) { - $insertStmt->close(); - if ($e->getCode() === 1062) { - // Race condition: another node inserted the same hash between our SELECT and INSERT - echo json_encode(['success' => false, 'error' => 'Duplicate ticket']); + echo json_encode([ + 'success' => true, + 'ticket_id' => $ticket_id, + 'message' => 'Ticket created successfully', + ]); } else { - error_log('create_ticket_api: insert failed: ' . $e->getMessage()); + $conn->rollback(); + error_log('create_ticket_api: ticket insert reported failure: ' . $conn->error); http_response_code(500); echo json_encode(['success' => false, 'error' => 'Internal server error']); } - exit; -} -$insertStmt->close(); - -if ($inserted) { - $auditLog->logTicketCreate($userId, $ticket_id, [ - 'title' => $title, - 'priority' => $priority, - 'category' => $category, - 'type' => $type, - ]); - - // New ticket created — refresh dashboard stats. - (new StatsModel($conn))->invalidateCache(); - - Database::close(); - - require_once __DIR__ . '/helpers/NotificationHelper.php'; - NotificationHelper::sendTicketNotification($ticket_id, [ - 'title' => $title, - 'priority' => $priority, - 'category' => $category, - 'type' => $type, - 'status' => $status, - ], 'automated'); - - echo json_encode([ - 'success' => true, - 'ticket_id' => $ticket_id, - 'message' => 'Ticket created successfully', - ]); -} else { - error_log('create_ticket_api: ticket insert reported failure: ' . $conn->error); - http_response_code(500); - echo json_encode(['success' => false, 'error' => 'Internal server error']); } diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index a5d68ef..47dee2c 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -33,6 +33,23 @@ class BulkOperationsModel return $this->workflowModel; } + /** + * Re-fetch a ticket row inside the current transaction with a row lock + * (FOR UPDATE), so a concurrent transaction touching the same row blocks + * until this one commits or rolls back instead of both validating + * against the same stale snapshot. Must be called after + * begin_transaction() and before the row is written. + */ + private function lockTicketForUpdate(string $ticketId): ?array + { + $stmt = $this->conn->prepare("SELECT * FROM tickets WHERE ticket_id = ? FOR UPDATE"); + $stmt->bind_param('s', $ticketId); + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $stmt->close(); + return $row ?: null; + } + /** * The status a bulk operation is trying to move tickets into, or null for * operations that don't change status. @@ -183,13 +200,27 @@ class BulkOperationsModel $success = false; try { + // Re-fetch and row-lock the ticket inside the transaction for any + // operation that validates against or reads its current fields — + // the pre-transaction $ticketsById snapshot (loaded before + // begin_transaction()) can be stale by the time we get here if a + // concurrent request (a single-ticket edit, or another bulk op) + // changed the row in between. Validating a transition against a + // stale status, or writing back stale title/description/etc., + // could silently bypass Workflow Designer rules or clobber a + // concurrent edit. FOR UPDATE blocks a concurrent transaction + // from reading/writing this row until ours commits or rolls back. + $needsCurrentTicket = $targetStatus !== null || $operation['operation_type'] === 'bulk_priority'; + $currentTicket = $needsCurrentTicket + ? $this->lockTicketForUpdate($ticketId) + : ($ticketsById[$ticketId] ?? null); + // bulk_status / bulk_close enforce the same Workflow Designer // rules as the single-ticket path: a transition the designer // doesn't define is refused, and requires_comment is honoured // (checked up front, above). requires_admin is satisfied because // api/bulk_operation.php already gates the endpoint on admin. if ($targetStatus !== null) { - $currentTicket = $ticketsById[$ticketId] ?? null; if ($currentTicket && $currentTicket['status'] === $targetStatus) { // Already in the requested state — nothing to do, and // reporting a no-op as a failure would just confuse. @@ -211,8 +242,7 @@ class BulkOperationsModel switch ($operation['operation_type']) { case 'bulk_close': - // Get current ticket from pre-loaded batch - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId, @@ -264,7 +294,7 @@ class BulkOperationsModel case 'bulk_priority': if (isset($parameters['priority'])) { - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId, @@ -292,7 +322,7 @@ class BulkOperationsModel case 'bulk_status': if (isset($parameters['status'])) { - $currentTicket = $ticketsById[$ticketId] ?? null; + // $currentTicket is the fresh, row-locked read from above. if ($currentTicket) { $updateResult = $ticketModel->updateTicket([ 'ticket_id' => $ticketId,