From d205a9577ae145fac12a0110c3260ab7e38c80d1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:41:41 -0400 Subject: [PATCH 1/3] Fix TOCTOU race in bulk operations by row-locking tickets (#34) processBulkOperation() validated each ticket's status/priority transition against a pre-transaction snapshot (ticketsById) instead of re-reading inside the transaction, so two concurrent bulk operations touching the same ticket could both pass validation against stale data and one transition could silently clobber the other. Add lockTicketForUpdate(), which re-fetches a ticket via SELECT ... FOR UPDATE, and use it wherever the loop needs current status/priority, removing the three redundant re-reads from the stale snapshot in the bulk_close/bulk_priority/ bulk_status branches. Verified against real MariaDB with two concurrent OS processes: the second process blocked ~1.2s on the first's held row lock, then correctly observed the first's committed status and rejected an otherwise-stale- data-permitted invalid transition. Also regression-tested normal bulk_close/bulk_priority operation on real tickets. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- models/BulkOperationsModel.php | 40 +++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) 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, From 310dcd0840b7b487e5ec2272a7f3fa78f8e0eb9c Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:41:48 -0400 Subject: [PATCH 2/3] Persist status-change comments transactionally with the update (#37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment accompanying a status change (required or user-supplied) was posted via a separate, independent HTTP call/write (add_comment.php, or a second add_comment call in lt.ticketStatus.submit()'s requires_comment retry path) before the status update itself. A failure partway through — or the client never issuing the second call — could leave a "reason" comment persisted with no matching status change, or vice versa, with no rollback tying the two together. api/update_ticket.php and api/ticket_status_api.php now post the comment and apply the status update inside one transaction, rolling back both on any failure. assets/js/ticket.js and lt.ticketStatus.submit() in assets/js/base.js no longer make a separate add_comment.php call; they pass the comment directly to update_ticket.php, which persists it server-side alongside the status change. Verified against real MariaDB by extracting the live ApiTicketController and the ticket_status_api.php transaction logic and running them directly: a forced optimistic-lock conflict correctly rolled back both the comment and the status change, and a successful call persisted exactly one comment alongside the status change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- api/ticket_status_api.php | 48 ++++++++++++---------- api/update_ticket.php | 85 ++++++++++++++++++++++++++------------- assets/js/base.js | 13 +++--- assets/js/ticket.js | 13 +++--- 4 files changed, 98 insertions(+), 61 deletions(-) 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); From 3778a599c37c32f57ce98f3b3f0ff8a25e8d9958 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 21:41:57 -0400 Subject: [PATCH 3/3] Serialize dedup-hash lookups in create_ticket_api.php (#35) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concurrent hwmonDaemon reports carrying the same dedup hash could both read the same pre-update ticket snapshot and each independently apply a priority escalation (losing one), or both attempt to INSERT a new ticket for a hash that didn't exist yet and have the loser's request dropped with a "Duplicate ticket" error instead of falling through to the update/escalate path. Wrap the hash lookup through the update-or-insert in one transaction, with the lookup taking SELECT ... FOR UPDATE. For an existing row this serializes the read-modify-write so a second request observes the first's committed state. For a not-yet-existing hash, InnoDB's gap lock there is shared rather than exclusive, so two concurrent inserts can both reach the INSERT and deadlock (1213) instead of one blocking cleanly on the other's row; retry the whole lookup once on that deadlock (or a lock-wait-timeout, 1205) so the retry's own SELECT finds the winner's committed row and takes the update path instead of erroring. Verified against real MariaDB with two concurrent OS processes for both scenarios: (1) same existing active ticket — the second process blocked ~1.1s on the first's held row lock, then correctly escalated from the first's committed priority rather than a stale value; (2) same not-yet-existing hash — reproduced the 1213 deadlock deterministically across 5/5 runs with the original code, then confirmed the retry resolves it every time (5/5), leaving exactly one ticket row created and no dropped/erroring request. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV --- create_ticket_api.php | 578 +++++++++++++++++++++++------------------- 1 file changed, 315 insertions(+), 263 deletions(-) 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']); }