Merge development into main: concurrency/atomicity fixes (#34, #35, #37)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV

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'],
 ];
 
+// 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'])) {
-    error_log('ticket_status_api: updateTicket failed for ticket ' . $ticketId
+        throw new Exception($updateResult['error'] ?? 'Failed to update ticket status');
-        . ': ' . ($updateResult['error'] ?? 'unknown'));
+    }
+
+    $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;

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'] ?? ''));
+                    $statusChangeComment = trim((string)($data['comment'] ?? $data['comment_text'] ?? ''));
-                    if ($comment === '') {
+                    if ($statusChangeComment === '') {
                         return [
                             'success' => false,
                             'error' => 'A comment is required for this status change',
@@ -207,27 +207,55 @@ try {
                 }
             }
 
+            // 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'] ?? ''));
+
+            $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);
-
-            // Handle conflict case
                 if (!$result['success']) {
-                $response = [
+                    throw new Exception($result['error'] ?? 'Failed to update ticket in database');
-                    'success' => false,
+                }
-                    'error' => $result['error'] ?? 'Failed to update ticket in database'
+
-                ];
+                // Handle visibility update if provided (already validated above)
-                if (!empty($result['conflict'])) {
+                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']) && $this->userId) {
-            if (isset($data['visibility'])) {
-                $visResult = $this->ticketModel->updateVisibility($id, $data['visibility'], $visibilityGroups, $this->userId);
-                if ($visResult && $this->userId) {
                 $this->auditLog->log(
                     $this->userId,
                     'update',
@@ -241,7 +269,6 @@ try {
                     ]
                 );
             }
-            }
 
             // Log ticket update to audit log — only the changed fields (delta)
             if ($this->userId) {

assets/js/base.js

@@ -2858,8 +2858,11 @@
      TICKET STATUS CHANGE (comment-aware)
      lt.ticketStatus.submit(ticketId, newStatus, { comment? }) → Promise<data>
      Posts /api/update_ticket.php. If the server rejects with
-     requires_comment, opens a comment modal, persists the comment via
+     requires_comment, opens a comment modal, then retries the update once
-     /api/add_comment.php, then retries the update once WITH the comment.
+     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.
+          // Retry with the comment included — update_ticket.php persists it
-          return api.post('/api/add_comment.php', { ticket_id: id, comment_text: comment })
+          // transactionally with the status update itself.
-            .then(() => api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment }));
+          return api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment });
         });
       });
     },

assets/js/ticket.js

@@ -713,12 +713,13 @@ function updateTicketStatus() {
                 return;
             }
             cleanup(true);
-            // Post comment first (persists it), then change status with the same
+            // The comment is sent as part of the status-change request itself
-            // comment included so the server's requires_comment check passes.
+            // (update_ticket.php persists it in the same DB transaction as the
-            const ticketId = getTicketIdFromUrl();
+            // status update) rather than as a separate prior add_comment.php
-            lt.api.post('/api/add_comment.php', { ticket_id: ticketId, comment_text: comment })
+            // call — previously those were two independent, non-transactional
-            .then(() => performStatusChange(statusSelect, selectedOption, newStatus, comment))
+            // writes, so a failure partway through could leave the "reason"
-            .catch(() => performStatusChange(statusSelect, selectedOption, newStatus, comment));
+            // 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);

create_ticket_api.php

@@ -232,8 +232,29 @@ $priority = (int)$priority;
 $ticketHash = generateTicketHash($data);
 $auditLog   = new AuditLogModel($conn);
 
+// 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();
+
     // 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 = $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();
@@ -329,6 +350,7 @@ if ($existing) {
                 (new StatsModel($conn))->invalidateCache();
             }
 
+            $conn->commit();
             Database::close();
             echo json_encode([
                 'success'            => true,
@@ -407,6 +429,7 @@ if ($existing) {
             ]);
         }
 
+        $conn->commit();
         Database::close();
 
         if ($reopenStatus !== null) {
@@ -431,11 +454,25 @@ if ($existing) {
         exit;
     }
 
-// No existing ticket — create a new one.
+    // 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 racing, and
+    // way a 1062 on INSERT below can only be the unique_hash (dedup) key — and with
-// is correctly reported as a duplicate rather than a dropped hardware alert.
+    // 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;
@@ -459,6 +496,7 @@ do {
     } 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']);
@@ -486,8 +524,19 @@ 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) {
-        // Race condition: another node inserted the same hash between our SELECT and INSERT
+            // 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());
@@ -509,6 +558,7 @@ if ($inserted) {
         // New ticket created — refresh dashboard stats.
         (new StatsModel($conn))->invalidateCache();
 
+        $conn->commit();
         Database::close();
 
         require_once __DIR__ . '/helpers/NotificationHelper.php';
@@ -526,7 +576,9 @@ if ($inserted) {
             'message'    => 'Ticket created successfully',
         ]);
     } else {
+        $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']);
     }
+}

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 is the fresh, row-locked read from above.
-                            $currentTicket = $ticketsById[$ticketId] ?? null;
                             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,