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,