Serialize dedup-hash lookups in create_ticket_api.php (#35)

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV

create_ticket_api.php

@@ -232,14 +232,35 @@ $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) {
+    // 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 ($existing) {
         $existingId       = $existing['ticket_id'];
         $existingStatus   = $existing['status'];
         $existingTitle    = $existing['title'];
@@ -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) {
@@ -429,17 +452,31 @@ if ($existing) {
             '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 {
+    // 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) {
@@ -456,20 +493,21 @@ do {
             $ticket_id = $candidateId;
         }
         $attempts++;
-} while ($ticket_id === null && $attempts < $maxAttempts);
+    } while ($ticket_id === null && $attempts < $maxAttempts);
 
-if ($ticket_id === null) {
+    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;
-}
+    }
 
-$insertStmt = $conn->prepare(
+    $insertStmt = $conn->prepare(
         "INSERT INTO tickets (ticket_id, title, description, status, priority, category, type, hash, created_by)
          VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"
-);
-$insertStmt->bind_param(
+    );
+    $insertStmt->bind_param(
         "ssssssssi",
         $ticket_id,
         $title,
@@ -480,14 +518,25 @@ $insertStmt->bind_param(
         $type,
         $ticketHash,
         $userId
-);
+    );
 
-try {
+    try {
         $inserted = $insertStmt->execute();
-} catch (mysqli_sql_exception $e) {
+    } 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());
@@ -495,10 +544,10 @@ try {
             echo json_encode(['success' => false, 'error' => 'Internal server error']);
         }
         exit;
-}
-$insertStmt->close();
+    }
+    $insertStmt->close();
 
-if ($inserted) {
+    if ($inserted) {
         $auditLog->logTicketCreate($userId, $ticket_id, [
             'title'    => $title,
             'priority' => $priority,
@@ -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';
@@ -525,8 +575,10 @@ if ($inserted) {
             'ticket_id'  => $ticket_id,
             'message'    => 'Ticket created successfully',
         ]);
-} else {
+    } 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']);
+    }
 }