From fccbfbb1fe92256b9992ed418bef3c018375b056 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 10 Sep 2026 16:22:55 +0200 Subject: [PATCH 1/2] fix: make addBadgeScan idempotent against a client retry of the same scan SUP-86b9fp53j: scanbadgeapp's SyncService retries an upload whenever its own 4s client-side timeout elapses, with no guarantee the original request didn't already reach the server and commit. Two such requests racing addBadgeScan could both find no existing SponsorBadgeScan for the same (sponsor, badge, scan_date) and both INSERT, producing two server-side rows for one physical scan - the duplicate reported in the ticket. This closes the server side of that bug; the client-side races (periodic sync vs. the upload button, vs. individual retry) were already fixed in scanbadgeapp. A retry always carries the exact same scan_date as the original attempt (the client never changes a scan's captured timestamp between attempts), so exact (sponsor, badge, scan_date) equality is enough to recognize a retry without a fuzzy time-window heuristic - a genuinely later re-scan of the same badge gets a new scan_date and is never collapsed. addBadgeScan now resolves the ticket/badge/sponsor first (read-only, so it runs outside any transaction), then creates the scan under ILockManagerService (Redis-backed; already used elsewhere in this codebase, e.g. SummitOrderService) keyed by that same tuple. The lock wraps the whole transaction, held past its COMMIT rather than released as soon as the row is attached in-memory - releasing any earlier would let a concurrent request's existence check run, and find nothing, before the first request's INSERT is actually durable. A plain check-then-insert alone doesn't close this under READ_COMMITTED, the isolation level ITransactionService::transaction defaults to. ISponsorUserInfoGrantRepository::findExistingBadgeScan queries SponsorBadgeScan directly for the exact match. UnacquiredLockException is left to propagate rather than wrapped in a ValidationException: the scanning app treats a non-4xx failure as transient and retries on its own, which is the right outcome for lock contention - a ValidationException would mark it a permanent client error instead. Tests added to OAuth2SummitBadgeScanApiControllerTest: two identical POSTs produce one row and the same response id; a different scan_date is not deduplicated; and - since PHPUnit calls are sequential and a plain existence check alone would pass the first two - a dedicated test that holds the exact lock name externally and confirms addBadgeScan fails to acquire it, proving the lock itself is what's being exercised, not just the end result of the happy path. Co-Authored-By: Claude Sonnet 5 --- .../ISponsorUserInfoGrantRepository.php | 11 +- ...DoctrineSponsorUserInfoGrantRepository.php | 32 +- .../Model/Imp/SponsorUserInfoGrantService.php | 319 ++++++++++++------ ...OAuth2SummitBadgeScanApiControllerTest.php | 197 +++++++++++ 4 files changed, 446 insertions(+), 113 deletions(-) diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php index 715569b62..888221066 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php @@ -18,5 +18,14 @@ */ interface ISponsorUserInfoGrantRepository extends IBaseRepository { - + /** + * Looks up a previously persisted SponsorBadgeScan for the exact same + * (sponsor, badge, scan_date) tuple, used to make SponsorUserInfoGrantService::addBadgeScan + * idempotent against a client retry of the same scan (SUP-86b9fp53j). + * @param Sponsor $sponsor + * @param SummitAttendeeBadge $badge + * @param \DateTime $scan_date + * @return SponsorBadgeScan|null + */ + public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): ?SponsorBadgeScan; } \ No newline at end of file diff --git a/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php b/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php index 67c7f7d51..2687609bb 100644 --- a/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php @@ -15,8 +15,10 @@ use Doctrine\ORM\QueryBuilder; use models\summit\ISponsorUserInfoGrantRepository; use models\summit\Presentation; +use models\summit\Sponsor; use models\summit\SponsorBadgeScan; use models\summit\SponsorUserInfoGrant; +use models\summit\SummitAttendeeBadge; use models\summit\SummitEvent; use utils\DoctrineFilterMapping; use utils\DoctrineInstanceOfFilterMapping; @@ -124,4 +126,32 @@ protected function getBaseEntity() { return SponsorUserInfoGrant::class; } -} \ No newline at end of file + + /** + * Queries SponsorBadgeScan directly (not the generic filter/order pipeline + * above, which matches against the whole SponsorUserInfoGrant hierarchy and + * is meant for paged listing) for an exact (sponsor, badge, scan_date) match. + * Doctrine resolves the SponsorUserInfoGrant/SponsorBadgeScan joined-table + * inheritance transparently, so no manual join is needed here. + * @param Sponsor $sponsor + * @param SummitAttendeeBadge $badge + * @param \DateTime $scan_date + * @return SponsorBadgeScan|null + */ + public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): ?SponsorBadgeScan + { + $query = $this->getEntityManager() + ->createQueryBuilder() + ->select("e") + ->from(SponsorBadgeScan::class, "e") + ->where("e.sponsor = :sponsor") + ->andWhere("e.badge = :badge") + ->andWhere("e.scan_date = :scan_date") + ->setParameter("sponsor", $sponsor) + ->setParameter("badge", $badge) + ->setParameter("scan_date", $scan_date) + ->setMaxResults(1); + + return $query->getQuery()->getOneOrNullResult(); + } +} diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index 811c8c5e5..48493e57d 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -16,7 +16,7 @@ use App\Models\Foundation\Summit\Repositories\ISummitAttendeeBadgeRepository; use App\Services\Model\AbstractService; use App\Services\Model\ISponsorUserInfoGrantService; -use App\Utils\AES; +use App\Services\Utils\ILockManagerService; use Illuminate\Support\Facades\Log; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -58,12 +58,18 @@ final class SponsorUserInfoGrantService */ private $sponsor_repository; + /** + * @var ILockManagerService + */ + private $lock_service; + /** * @param ISponsorUserInfoGrantRepository $repository * @param ISummitAttendeeRepository $attendee_repository * @param ISummitAttendeeBadgeRepository $badge_repository * @param ISponsorRepository $sponsor_repository * @param ITransactionService $tx_service + * @param ILockManagerService $lock_service */ public function __construct ( @@ -71,7 +77,8 @@ public function __construct ISummitAttendeeRepository $attendee_repository, ISummitAttendeeBadgeRepository $badge_repository, ISponsorRepository $sponsor_repository, - ITransactionService $tx_service + ITransactionService $tx_service, + ILockManagerService $lock_service ) { parent::__construct($tx_service); @@ -79,6 +86,7 @@ public function __construct $this->attendee_repository = $attendee_repository; $this->badge_repository = $badge_repository; $this->sponsor_repository = $sponsor_repository; + $this->lock_service = $lock_service; } /** @@ -112,6 +120,17 @@ public function addGrant(Summit $summit, int $sponsor_id, Member $current_member }); } + /** + * Redis TTL (seconds) for the per-(sponsor, badge, scan_date) dedup lock + * below - a crash-safety ceiling (a process that dies mid-critical-section + * without releasing must not wedge that scan forever), not the + * acquire-contention timeout, which ILockManagerService governs on its + * own (LockManagerService::MaxRetries with backoff). Matches the + * lifetime SummitOrderService already uses for its own short-lived + * per-entity locks. + */ + private const BADGE_SCAN_LOCK_LIFETIME_SECONDS = 30; + /** * @param Summit $summit * @param Member $current_member @@ -121,137 +140,215 @@ public function addGrant(Summit $summit, int $sponsor_id, Member $current_member */ public function addBadgeScan(Summit $summit, Member $current_member, array $data): SponsorBadgeScan { - return $this->tx_service->transaction(function() use($summit, $current_member, $data){ - $raw_qr_code = $data['qr_code'] ?? null; - $raw_attendee_email = $data['attendee_email'] ?? null; - if(empty($raw_qr_code) && empty($raw_attendee_email)) - throw new ValidationException("Missing required parameters (qr_code or attendee_email)."); - $ticket_number = null; - $qr_code = null; - $source = null; - if(!empty($raw_qr_code)) { - $qr_code = SummitAttendeeBadge::decodeQRCodeFor($summit, $raw_qr_code); - $fields = SummitAttendeeBadge::parseQRCode($qr_code); - $prefix = $fields['prefix']; - if($summit->getBadgeQRPrefix() != $prefix) - throw new ValidationException + // Phase 1: parse the request and resolve the ticket/badge/sponsor it + // refers to. Entirely read-only (nothing is persisted here), so it + // runs outside any transaction - which is what lets the dedup lock + // below be acquired, keyed by (sponsor, badge, scan_date), BEFORE + // the transaction that actually creates the scan ever opens. + $raw_qr_code = $data['qr_code'] ?? null; + $raw_attendee_email = $data['attendee_email'] ?? null; + if(empty($raw_qr_code) && empty($raw_attendee_email)) + throw new ValidationException("Missing required parameters (qr_code or attendee_email)."); + $ticket_number = null; + $qr_code = null; + $source = null; + if(!empty($raw_qr_code)) { + $qr_code = SummitAttendeeBadge::decodeQRCodeFor($summit, $raw_qr_code); + $fields = SummitAttendeeBadge::parseQRCode($qr_code); + $prefix = $fields['prefix']; + if($summit->getBadgeQRPrefix() != $prefix) + throw new ValidationException + ( + sprintf ( - sprintf - ( - "%s qr code is not valid for summit %s.", - $qr_code, - $summit->getId() - ) - ); - $ticket_number = $fields['ticket_number']; - $source = SponsorBadgeScan::Source_QR; + "%s qr code is not valid for summit %s.", + $qr_code, + $summit->getId() + ) + ); + $ticket_number = $fields['ticket_number']; + $source = SponsorBadgeScan::Source_QR; + } + else if(!empty($raw_attendee_email)) { + $attendee = $this->attendee_repository->getBySummitAndEmail($summit, trim($raw_attendee_email)); + if(is_null($attendee)){ + throw new EntityNotFoundException("Attendee not found."); } - else if(!empty($raw_attendee_email)) { - $attendee = $this->attendee_repository->getBySummitAndEmail($summit, trim($raw_attendee_email)); - if(is_null($attendee)){ - throw new EntityNotFoundException("Attendee not found."); - } - $ticket = null; - foreach ($attendee->getTickets() as $t) { - if ($t->isActive() && $t->hasBadge()) { $ticket = $t; break; } - } - - if(is_null($ticket)){ - throw new EntityNotFoundException("Ticket not found."); - } - $ticket_number = $ticket->getNumber(); - $badge = $ticket->getBadge(); - // generate QR code on-demand if missing - $qr_code = $badge->generateQRCode(); - $qr_code = base64_encode($qr_code); - // normalize qr code - $qr_code = SummitAttendeeBadge::decodeQRCodeFor($summit, $qr_code); - $source = SponsorBadgeScan::Source_Attendee_Email; + $ticket = null; + foreach ($attendee->getTickets() as $t) { + if ($t->isActive() && $t->hasBadge()) { $ticket = $t; break; } } - $scan_date_epoch = intval($data['scan_date']); - $scan_date = new \DateTime("@$scan_date_epoch"); - $begin_date = $summit->getBeginDate(); - $end_date = $summit->getEndDate(); - - /* - if(!($scan_date >= $begin_date && $scan_date <= $end_date)) - throw new ValidationException("scan_date does not belong to summit period."); - */ - if(empty($ticket_number)){ - throw new ValidationException("Ticket not found."); + if(is_null($ticket)){ + throw new EntityNotFoundException("Ticket not found."); } + $ticket_number = $ticket->getNumber(); + $badge = $ticket->getBadge(); + // generate QR code on-demand if missing + $qr_code = $badge->generateQRCode(); + $qr_code = base64_encode($qr_code); + // normalize qr code + $qr_code = SummitAttendeeBadge::decodeQRCodeFor($summit, $qr_code); + $source = SponsorBadgeScan::Source_Attendee_Email; + } + + $scan_date_epoch = intval($data['scan_date']); + $scan_date = new \DateTime("@$scan_date_epoch"); + + /* + $begin_date = $summit->getBeginDate(); + $end_date = $summit->getEndDate(); + + if(!($scan_date >= $begin_date && $scan_date <= $end_date)) + throw new ValidationException("scan_date does not belong to summit period."); + */ + if(empty($ticket_number)){ + throw new ValidationException("Ticket not found."); + } + + $badge = $this->badge_repository->getBadgeByTicketNumber($ticket_number); + + if(is_null($badge)) + throw new EntityNotFoundException("badge not found."); + + // if we are and admin / show admin , then we need to provide the sponsor id + if($current_member->isAuthzFor($summit)){ + + Log::debug("SponsorUserInfoGrantService::addBadgeScan current member is an admin"); + + if (empty($data['sponsor_id'])) + throw new ValidationException("sponsor_id is required when current member is an admin."); + $sponsor_id = intval($data['sponsor_id']); + $sponsor = $this->sponsor_repository->getById($sponsor_id); + if(!$sponsor instanceof Sponsor){ + throw new EntityNotFoundException("Sponsor not found."); + } + if($sponsor->getSummitId() !== $summit->getId()){ + throw new ValidationException("Sponsor does not belong to this summit."); + } + Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (admin provided).", $sponsor->getId())); + } + else { + $member_sponsors = $current_member->getAccessibleSponsorsBySummit($summit); - $badge = $this->badge_repository->getBadgeByTicketNumber($ticket_number); - - if(is_null($badge)) - throw new EntityNotFoundException("badge not found."); - - // if we are and admin / show admin , then we need to provide the sponsor id - if($current_member->isAuthzFor($summit)){ + if ($member_sponsors->isEmpty()) + throw new ValidationException("Current member does not have badge scan permissions for any sponsor of this summit."); - Log::debug("SponsorUserInfoGrantService::addBadgeScan current member is an admin"); + if ($member_sponsors->count() === 1) { + $sponsor = $member_sponsors->first(); + Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (first).", $sponsor->getId())); + } else { + Log::debug("SponsorUserInfoGrantService::addBadgeScan current member is associated to multiple sponsors."); if (empty($data['sponsor_id'])) - throw new ValidationException("sponsor_id is required when current member is an admin."); - $sponsor_id = intval($data['sponsor_id']); - $sponsor = $this->sponsor_repository->getById($sponsor_id); - if(!$sponsor instanceof Sponsor){ - throw new EntityNotFoundException("Sponsor not found."); - } - if($sponsor->getSummitId() !== $summit->getId()){ - throw new ValidationException("Sponsor does not belong to this summit."); - } - Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (admin provided).", $sponsor->getId())); - } - else { - $member_sponsors = $current_member->getAccessibleSponsorsBySummit($summit); + throw new ValidationException("sponsor_id is required when the member belongs to multiple sponsors."); - if ($member_sponsors->isEmpty()) - throw new ValidationException("Current member does not have badge scan permissions for any sponsor of this summit."); - - if ($member_sponsors->count() === 1) { - $sponsor = $member_sponsors->first(); - Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (first).", $sponsor->getId())); - } else { - Log::debug("SponsorUserInfoGrantService::addBadgeScan current member is associated to multiple sponsors."); + $sponsor_id = intval($data['sponsor_id']); + $sponsor = $member_sponsors->filter(fn($s) => $s->getId() === $sponsor_id)->first(); - if (empty($data['sponsor_id'])) - throw new ValidationException("sponsor_id is required when the member belongs to multiple sponsors."); + if ($sponsor === false) + throw new ValidationException("Current member does not belong to the selected summit sponsor."); - $sponsor_id = intval($data['sponsor_id']); - $sponsor = $member_sponsors->filter(fn($s) => $s->getId() === $sponsor_id)->first(); + Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (multiple).", $sponsor->getId())); - if ($sponsor === false) - throw new ValidationException("Current member does not belong to the selected summit sponsor."); + } + } - Log::debug(sprintf("SponsorUserInfoGrantService::addBadgeScan selected sponsor %s (multiple).", $sponsor->getId())); + // Phase 2: create the scan, guarded against a concurrent duplicate + // for this same (sponsor, badge, scan_date) - see addBadgeScanLocked. + return $this->addBadgeScanLocked($sponsor, $badge, $scan_date, $scan_date_epoch, $qr_code, $source, $current_member, $data); + } + /** + * Creates (or, on a retry of the same scan, returns) the SponsorBadgeScan + * for the given (sponsor, badge, scan_date). SUP-86b9fp53j: the scanning + * app retries an upload whenever its own client-side timeout elapses, + * with no guarantee the original request didn't already reach this far + * and commit - two such requests reading "no existing scan yet" before + * either INSERTs is exactly how one physical badge scan ended up as two + * rows. A plain existence check right before the INSERT doesn't close + * that window under READ_COMMITTED (the isolation level + * ITransactionService::transaction defaults to): two concurrent + * transactions can both run the check before either commits. + * + * ILockManagerService (Redis-backed; see SummitOrderService for other + * callers) keyed by the same tuple serializes them - and, critically, + * the lock wraps the whole transaction() call below, held past its + * COMMIT rather than released as soon as the row is attached in-memory: + * releasing any earlier would let a second request's existence check + * run (and find nothing) before the first request's INSERT is actually + * durable and visible to it. + * + * A lock the retries inside ILockManagerService::acquireLock can't get + * throws UnacquiredLockException, deliberately left to propagate (same + * as SponsorUserSyncService's own lock usage) rather than turned into a + * ValidationException: the scanning app's SyncService treats a non-4xx + * failure as transient and retries the scan on its own, which is the + * right outcome for lock contention - a ValidationException would mark + * it a permanent client error instead and stop retrying it. + * @param Sponsor $sponsor + * @param SummitAttendeeBadge $badge + * @param \DateTime $scan_date + * @param int $scan_date_epoch + * @param string $qr_code + * @param string $source + * @param Member $current_member + * @param array $data + * @return SponsorBadgeScan + * @throws \Exception + */ + private function addBadgeScanLocked( + Sponsor $sponsor, + SummitAttendeeBadge $badge, + \DateTime $scan_date, + int $scan_date_epoch, + string $qr_code, + string $source, + Member $current_member, + array $data + ): SponsorBadgeScan + { + $lock_name = sprintf('badge_scan.%d.%d.%d.lock', $sponsor->getId(), $badge->getId(), $scan_date_epoch); + + return $this->lock_service->lock($lock_name, function() use($sponsor, $badge, $scan_date, $qr_code, $source, $current_member, $data){ + return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $qr_code, $source, $current_member, $data){ + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + if(!is_null($existing)){ + Log::warning( + sprintf( + "SponsorUserInfoGrantService::addBadgeScan duplicate scan detected for sponsor %s badge %s scan_date %s - returning existing scan %s", + $sponsor->getId(), + $badge->getId(), + $scan_date->getTimestamp(), + $existing->getId() + ) + ); + return $existing; } - } - $scan = new SponsorBadgeScan(); - $scan->setScanDate($scan_date); - $scan->setQRCode($qr_code); - $scan->setUser($current_member); - $scan->setBadge($badge); - $scan->setSource($source); - $scan->setNotes(isset($data['notes'])? trim($data['notes']): ""); + $scan = new SponsorBadgeScan(); + $scan->setScanDate($scan_date); + $scan->setQRCode($qr_code); + $scan->setUser($current_member); + $scan->setBadge($badge); + $scan->setSource($source); + $scan->setNotes(isset($data['notes'])? trim($data['notes']): ""); - $sponsor->addUserInfoGrant($scan); + $sponsor->addUserInfoGrant($scan); - // extra questions - $extra_questions = $data['extra_questions'] ?? []; + // extra questions + $extra_questions = $data['extra_questions'] ?? []; - if (count($extra_questions)) { - $res = $scan->hadCompletedExtraQuestions($extra_questions); - if (!$res) { - throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + if (count($extra_questions)) { + $res = $scan->hadCompletedExtraQuestions($extra_questions); + if (!$res) { + throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + } } - } - return $scan; - }); + return $scan; + }); + }, self::BADGE_SCAN_LOCK_LIFETIME_SECONDS); } /** diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index ea08bc8a4..6c3836692 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -14,6 +14,10 @@ use App\Models\Foundation\Main\IGroup; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; +use App\Services\Model\ISponsorUserInfoGrantService; +use App\Services\Utils\Exceptions\UnacquiredLockException; +use App\Services\Utils\ILockManagerService; +use Illuminate\Support\Facades\App; use Libs\ModelSerializers\AbstractSerializer; use models\main\Group; use models\summit\Sponsor; @@ -170,6 +174,199 @@ public function testAddBadgeScanWithOneSponsorPerMember(){ return $scan; } + /** + * SUP-86b9fp53j: the scanning app retries an upload whenever its own + * client-side timeout elapses, with no guarantee the original request + * didn't already reach the server and commit - the retry carries the + * exact same qr_code/scan_date/sponsor_id as the first attempt, since + * the app never changes a scan's captured timestamp between attempts. + * Two POSTs of that identical payload must produce exactly one + * SponsorBadgeScan, with the second response returning the same one + * the first created (not a validation error, and not a second row). + */ + public function testAddBadgeScanIsIdempotentOnRetry(){ + self::$member->clearGroups(); + self::$member->add2Group($this->sponsor_group); + self::$em->persist(self::$member); + self::$em->flush(); + + $sponsor = self::$summit->getSummitSponsors()[0]; + $sponsor->addUser(self::$member); + self::$em->persist($sponsor); + self::$em->flush(); + + $params = [ + 'id' => self::$summit->getId(), + ]; + + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + + // Generated once and reused across both requests: a real retry + // resends the exact same body, it doesn't re-derive the QR code. + $data = [ + 'qr_code' => $badge->generateQRCode(), + 'scan_date' => 1572019200, + 'sponsor_id' => $sponsor->getId(), + ]; + $body = json_encode($data); + + $first_response = $this->action( + "POST", + "OAuth2SummitBadgeScanApiController@add", + $params, + [], + [], + [], + $this->getAuthHeaders(), + $body + ); + + $this->assertResponseStatus(201); + $first_scan = json_decode($first_response->getContent()); + $this->assertTrue(!is_null($first_scan)); + + $second_response = $this->action( + "POST", + "OAuth2SummitBadgeScanApiController@add", + $params, + [], + [], + [], + $this->getAuthHeaders(), + $body + ); + + $this->assertResponseStatus(201); + $second_scan = json_decode($second_response->getContent()); + $this->assertTrue(!is_null($second_scan)); + + $this->assertEquals($first_scan->id, $second_scan->id, + "a retry of the identical scan must return the same entity, not create a new one"); + + $count = self::$em->getRepository(\models\summit\SponsorBadgeScan::class) + ->count(['sponsor' => $sponsor, 'badge' => $badge]); + $this->assertEquals(1, $count, + "exactly one SponsorBadgeScan row must exist for this sponsor+badge+scan_date, not two"); + } + + /** + * A different scan_date for the same sponsor+badge must NOT be + * deduplicated - it's a genuine second scan (e.g. the sponsor scanned + * this attendee again later), not a retry of the same attempt. + */ + public function testAddBadgeScanWithDifferentScanDateIsNotDeduplicated(){ + self::$member->clearGroups(); + self::$member->add2Group($this->sponsor_group); + self::$em->persist(self::$member); + self::$em->flush(); + + $sponsor = self::$summit->getSummitSponsors()[0]; + $sponsor->addUser(self::$member); + self::$em->persist($sponsor); + self::$em->flush(); + + $params = [ + 'id' => self::$summit->getId(), + ]; + + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $qr_code = $badge->generateQRCode(); + + $first_response = $this->action( + "POST", + "OAuth2SummitBadgeScanApiController@add", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['qr_code' => $qr_code, 'scan_date' => 1572019200, 'sponsor_id' => $sponsor->getId()]) + ); + $this->assertResponseStatus(201); + $first_scan = json_decode($first_response->getContent()); + + $second_response = $this->action( + "POST", + "OAuth2SummitBadgeScanApiController@add", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['qr_code' => $qr_code, 'scan_date' => 1572019260, 'sponsor_id' => $sponsor->getId()]) + ); + $this->assertResponseStatus(201); + $second_scan = json_decode($second_response->getContent()); + + $this->assertNotEquals($first_scan->id, $second_scan->id, + "a genuinely later scan of the same badge must not be collapsed into the earlier one"); + } + + /** + * The two tests above prove the end result (one row survives two + * identical POSTs), but a plain "check then insert" with no locking at + * all would pass them too, since PHPUnit calls are strictly sequential - + * they never actually overlap two in-flight requests. This test proves + * the lock itself is what SponsorUserInfoGrantService::addBadgeScan + * acquires: holding the exact lock name it should use externally, then + * calling the real service directly (bypassing HTTP, so the thrown + * exception type is visible), and asserting it fails to acquire the + * lock and gives up - the concurrency-closing mechanism this fix + * actually depends on for a genuine race, not just the happy path. + */ + public function testAddBadgeScanBlocksOnAConcurrentLockHolder(){ + self::$member->clearGroups(); + self::$member->add2Group($this->sponsor_group); + self::$em->persist(self::$member); + self::$em->flush(); + + $sponsor = self::$summit->getSummitSponsors()[0]; + $sponsor->addUser(self::$member); + self::$em->persist($sponsor); + self::$em->flush(); + + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $qr_code = $badge->generateQRCode(); + $scan_date_epoch = 1572019200; + + // Same construction as SponsorUserInfoGrantService::addBadgeScanLocked's + // $lock_name - deliberately duplicated (not called via a shared + // constant) so this test also catches a future change to that + // format silently no longer matching what's held here. + $lock_name = sprintf('badge_scan.%d.%d.%d.lock', $sponsor->getId(), $badge->getId(), $scan_date_epoch); + + $lock_service = App::make(ILockManagerService::class); + $held_token = $lock_service->acquireLock($lock_name, 10); + + $data = [ + 'qr_code' => $qr_code, + 'scan_date' => $scan_date_epoch, + 'sponsor_id' => $sponsor->getId(), + ]; + + $service = App::make(ISponsorUserInfoGrantService::class); + + $threw = false; + try { + $service->addBadgeScan(self::$summit, self::$member, $data); + } catch (UnacquiredLockException $ex) { + $threw = true; + } finally { + $lock_service->releaseLock($lock_name, $held_token); + } + $this->assertTrue($threw, + "addBadgeScan must fail to acquire a lock already held under the exact name this test holds - ". + "either it isn't locking on (sponsor, badge, scan_date) at all, or the name format drifted"); + + // With the external holder gone, the same call now succeeds. + $scan = $service->addBadgeScan(self::$summit, self::$member, $data); + $this->assertNotNull($scan); + $this->assertEquals($sponsor->getId(), $scan->getSponsor()->getId()); + } + public function testAddBadgeScanByAttendeeEmail(){ self::$member->clearGroups(); self::$member->add2Group($this->sponsor_group); From 27f953bc8d493c44f9d92904fc5e35b06b4e0a79 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 10 Sep 2026 18:26:42 +0200 Subject: [PATCH 2/2] fix(badge-scan): enforce scan idempotency with a unique index, not the lock TTL The Redis dedup lock added in fccbfbb1f cannot guarantee one row per scan: it has a TTL, no renewal and no fencing token, so it can lapse while the transaction it wraps is still running -- DoctrineTransactionService retries a root transaction up to MaxRetries = 10 on reconnectable errors with no backoff bounding the wall clock, and LockManagerService::releaseLock only logs 'lock was not held by this token at release time' when that happens. A concurrent retry can then acquire the same lock name, run findExistingBadgeScan, see nothing committed yet and INSERT a duplicate -- exactly the race the fix exists to close. SponsorUserSyncService hit this same wall with the structurally identical pattern and raised its lifetime from 30 to 120 in 290357fd0; raising a TTL only moves the boundary. Moves the invariant to where it can actually hold: a UNIQUE index over the new SponsorBadgeScan.ScanDedupKey column ("::"), with UniqueConstraintViolationException resolved to the row that won the race. Caught outside the transaction on purpose -- a failed flush leaves the EntityManager closed and the connection rollback-only, so the winning row can only be re-read in a fresh transaction (same placement as SummitService::addEventToMemberSchedule). A violation that does not resolve to our tuple is rethrown rather than swallowed. The lock stays as an optimization that keeps the common case from doing wasted work, and its 30s TTL stays short on purpose: acquireLock only waits ~0.7s before giving up, so a long-lived orphan would turn every retry of that one scan into a failure. The column has to be denormalized onto SponsorBadgeScan rather than indexed over the tuple itself: SponsorUserInfoGrant/SponsorBadgeScan is a JOINED pair with SponsorID on the parent table and BadgeID/ScanDate on the child, and a UNIQUE index cannot span both. The migration deliberately does not backfill and does not delete anything. The column is nullable and existing rows keep NULL; MySQL permits unlimited NULLs in a UNIQUE index, so the duplicates this bug already produced neither block the CREATE nor have to be reconciled here (deleting scan rows would cascade into SponsorBadgeScanExtraQuestionAnswer -- real lead-gen data). Those rows stay covered by the explicit findExistingBadgeScan check; only rows created from now on carry a key, and those are exactly the ones a retry can race against. Tests: the UNIQUE index actually rejects a second row carrying the same key, and addBadgeScan stamps the key -- without the latter every new row would go in NULL and the protection would silently be gone. Co-Authored-By: Claude Opus 5 (1M context) --- .../Summit/Registration/SponsorBadgeScan.php | 58 +++++++ .../Model/Imp/SponsorUserInfoGrantService.php | 160 +++++++++++++----- .../model/Version20260910181020.php | 84 +++++++++ ...OAuth2SummitBadgeScanApiControllerTest.php | 90 ++++++++++ 4 files changed, 346 insertions(+), 46 deletions(-) create mode 100644 database/migrations/model/Version20260910181020.php diff --git a/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php b/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php index d7340c63a..5984aeabc 100644 --- a/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php +++ b/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php @@ -92,6 +92,32 @@ class SponsorBadgeScan extends SponsorUserInfoGrant #[ORM\Column(name: 'Source', type: 'string', options: ['default' => self::Source_QR])] private $source; + /** + * Denormalized "::" identity of the physical + * scan this row represents, carrying a UNIQUE index (see the migration that + * adds SponsorBadgeScan_ScanDedupKey). That index - not the Redis dedup lock + * in SponsorUserInfoGrantService::addBadgeScanLocked - is what actually makes + * addBadgeScan idempotent: a lock with a TTL and no renewal cannot guarantee + * mutual exclusion (it can expire mid-transaction, and LockManagerService + * only logs the mismatch at release time), so the database has to be the + * authority on "one row per scan". + * + * The column has to live here rather than being an index over the tuple + * itself: SponsorUserInfoGrant/SponsorBadgeScan is a JOINED inheritance pair + * with SponsorID on the parent table and BadgeID/ScanDate on this one, and a + * UNIQUE index cannot span both tables. + * + * Nullable on purpose, and rows created before that migration keep NULL: + * MySQL allows any number of NULLs in a UNIQUE index, so pre-existing + * duplicates (which this bug already produced in production) neither block + * the index creation nor need deleting. Those historical rows stay covered by + * the explicit findExistingBadgeScan() check, which matches on the real + * columns; every new row gets a key and is covered by the index too. + * @var string|null + */ + #[ORM\Column(name: 'ScanDedupKey', type: 'string', nullable: true)] + private $scan_dedup_key; + /** * @var SponsorBadgeScanExtraQuestionAnswer[] */ @@ -169,6 +195,38 @@ public function setScanDate(\DateTime $scan_date): void $this->scan_date = $scan_date; } + /** + * Builds the value for the ScanDedupKey UNIQUE index from the tuple that + * identifies one physical scan. Uses the scan_date's epoch so the key is + * insensitive to how the DateTime was constructed (timezone, sub-second + * precision the DATETIME column would drop anyway) - the scanning app + * sends the timestamp as epoch seconds and resends it unchanged on a retry. + * @param Sponsor $sponsor + * @param SummitAttendeeBadge $badge + * @param \DateTime $scan_date + * @return string + */ + public static function buildDedupKey(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): string + { + return sprintf('%d:%d:%d', $sponsor->getId(), $badge->getId(), $scan_date->getTimestamp()); + } + + /** + * @return string|null + */ + public function getScanDedupKey(): ?string + { + return $this->scan_dedup_key; + } + + /** + * @param string $scan_dedup_key + */ + public function setScanDedupKey(string $scan_dedup_key): void + { + $this->scan_dedup_key = $scan_dedup_key; + } + public function getAttendeeFirstName():?string{ $attendee = $this->getBadge()->getTicket()->getOwner(); return $attendee->hasMember() ? $attendee->getMember()->getFirstName() : $attendee->getFirstName(); diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index 48493e57d..027912a77 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -17,6 +17,7 @@ use App\Services\Model\AbstractService; use App\Services\Model\ISponsorUserInfoGrantService; use App\Services\Utils\ILockManagerService; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Illuminate\Support\Facades\Log; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -125,9 +126,20 @@ public function addGrant(Summit $summit, int $sponsor_id, Member $current_member * below - a crash-safety ceiling (a process that dies mid-critical-section * without releasing must not wedge that scan forever), not the * acquire-contention timeout, which ILockManagerService governs on its - * own (LockManagerService::MaxRetries with backoff). Matches the - * lifetime SummitOrderService already uses for its own short-lived - * per-entity locks. + * own (LockManagerService::MaxRetries with backoff). + * + * This value is deliberately NOT a correctness parameter, and is kept + * short so an orphaned lock frees that scan quickly (acquireLock only + * waits ~0.7s before giving up, so a long-lived orphan would turn every + * retry of that one scan into a failure). It can't be one: the lock has + * no renewal and no fencing token, so it can expire while the transaction + * it wraps is still running - DoctrineTransactionService retries a root + * transaction up to MaxRetries = 10 on reconnectable errors with no + * backoff bounding the wall clock, and LockManagerService::releaseLock + * only logs 'lock was not held by this token at release time' when the + * TTL already lapsed. The SponsorBadgeScan.ScanDedupKey UNIQUE index is + * what actually guarantees one row per scan; the lock just keeps the + * common case from doing wasted work. */ private const BADGE_SCAN_LOCK_LIFETIME_SECONDS = 30; @@ -271,13 +283,31 @@ public function addBadgeScan(Summit $summit, Member $current_member, array $data * ITransactionService::transaction defaults to): two concurrent * transactions can both run the check before either commits. * - * ILockManagerService (Redis-backed; see SummitOrderService for other - * callers) keyed by the same tuple serializes them - and, critically, - * the lock wraps the whole transaction() call below, held past its - * COMMIT rather than released as soon as the row is attached in-memory: - * releasing any earlier would let a second request's existence check - * run (and find nothing) before the first request's INSERT is actually - * durable and visible to it. + * The invariant is enforced in two layers, and only the second one is + * authoritative: + * + * 1. ILockManagerService (Redis-backed; see SponsorUserSyncService and + * SummitOrderService for other callers) keyed by the same tuple + * serializes the common case, with the lock wrapping the whole + * transaction() call below and held past its COMMIT rather than + * released as soon as the row is attached in-memory - releasing any + * earlier would let a second request's existence check run (and find + * nothing) before the first request's INSERT is durable. This is an + * optimization: it keeps a retry from doing wasted work and from + * provoking the exception path below. + * + * 2. The SponsorBadgeScan.ScanDedupKey UNIQUE index is what actually + * guarantees one row per scan. The lock cannot: it has a TTL, no + * renewal and no fencing token, so it can lapse while the transaction + * it wraps is still running (DoctrineTransactionService retries a root + * transaction up to MaxRetries = 10 on reconnectable errors, with no + * backoff bounding the wall clock) and LockManagerService::releaseLock + * merely logs that it was no longer held. When that happens the INSERT + * is rejected by the index and the UniqueConstraintViolationException + * handler below resolves the retry to the row that won the race. + * + * Rows predating that index keep a NULL ScanDedupKey and are covered by + * the explicit existence check alone - see the column's own docblock. * * A lock the retries inside ILockManagerService::acquireLock can't get * throws UnacquiredLockException, deliberately left to propagate (same @@ -309,46 +339,84 @@ private function addBadgeScanLocked( ): SponsorBadgeScan { $lock_name = sprintf('badge_scan.%d.%d.%d.lock', $sponsor->getId(), $badge->getId(), $scan_date_epoch); + $dedup_key = SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date); + + try { + return $this->lock_service->lock($lock_name, function() use($sponsor, $badge, $scan_date, $dedup_key, $qr_code, $source, $current_member, $data){ + return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $dedup_key, $qr_code, $source, $current_member, $data){ + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + if(!is_null($existing)){ + Log::warning( + sprintf( + "SponsorUserInfoGrantService::addBadgeScan duplicate scan detected for sponsor %s badge %s scan_date %s - returning existing scan %s", + $sponsor->getId(), + $badge->getId(), + $scan_date->getTimestamp(), + $existing->getId() + ) + ); + return $existing; + } - return $this->lock_service->lock($lock_name, function() use($sponsor, $badge, $scan_date, $qr_code, $source, $current_member, $data){ - return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $qr_code, $source, $current_member, $data){ - $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); - if(!is_null($existing)){ - Log::warning( - sprintf( - "SponsorUserInfoGrantService::addBadgeScan duplicate scan detected for sponsor %s badge %s scan_date %s - returning existing scan %s", - $sponsor->getId(), - $badge->getId(), - $scan_date->getTimestamp(), - $existing->getId() - ) - ); - return $existing; - } - - $scan = new SponsorBadgeScan(); - $scan->setScanDate($scan_date); - $scan->setQRCode($qr_code); - $scan->setUser($current_member); - $scan->setBadge($badge); - $scan->setSource($source); - $scan->setNotes(isset($data['notes'])? trim($data['notes']): ""); - - $sponsor->addUserInfoGrant($scan); - - // extra questions - $extra_questions = $data['extra_questions'] ?? []; - - if (count($extra_questions)) { - $res = $scan->hadCompletedExtraQuestions($extra_questions); - if (!$res) { - throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + $scan = new SponsorBadgeScan(); + $scan->setScanDate($scan_date); + $scan->setQRCode($qr_code); + $scan->setUser($current_member); + $scan->setBadge($badge); + $scan->setSource($source); + $scan->setNotes(isset($data['notes'])? trim($data['notes']): ""); + // Populates the column carrying the UNIQUE index, which is what + // actually rejects a duplicate if the lock above failed to serialize + // this request - the check right above only closes the window it can see. + $scan->setScanDedupKey($dedup_key); + + $sponsor->addUserInfoGrant($scan); + + // extra questions + $extra_questions = $data['extra_questions'] ?? []; + + if (count($extra_questions)) { + $res = $scan->hadCompletedExtraQuestions($extra_questions); + if (!$res) { + throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + } } - } - return $scan; + return $scan; + }); + }, self::BADGE_SCAN_LOCK_LIFETIME_SECONDS); + } + catch(UniqueConstraintViolationException $ex){ + // SponsorBadgeScan_ScanDedupKey rejected the INSERT: another request for + // this same physical scan committed first, so the existence check above + // ran before that row was visible - either because the dedup lock's TTL + // lapsed mid-transaction (it has no renewal, and releaseLock only logs + // the mismatch) or because the two requests never contended on it at all. + // Either way the retry is satisfied by returning the row that won. + // + // Caught out here rather than inside the closures on purpose: a failed + // flush leaves the EntityManager closed and the connection rollback-only, + // so the winning row can only be re-read in a fresh transaction. Same + // placement as SummitService::addEventToMemberSchedule's own handling. + // DoctrineTransactionService::shouldReconnect() does not treat this as + // reconnectable, so it reaches us instead of being retried. + Log::warning( + sprintf( + "SponsorUserInfoGrantService::addBadgeScan unique violation on dedup key %s - a concurrent request won the race, resolving to the committed scan.", + $dedup_key + ) + ); + + return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $ex){ + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + if(is_null($existing)){ + // Not our tuple - some other unique index on the scan or its answers + // rejected the write, and swallowing that would hide a real failure. + throw $ex; + } + return $existing; }); - }, self::BADGE_SCAN_LOCK_LIFETIME_SECONDS); + } } /** diff --git a/database/migrations/model/Version20260910181020.php b/database/migrations/model/Version20260910181020.php new file mode 100644 index 000000000..ef11489a3 --- /dev/null +++ b/database/migrations/model/Version20260910181020.php @@ -0,0 +1,84 @@ +addSql(<<addSql(<<addSql(<<addSql(<<assertResponseStatus(200); $this->assertNotEmpty($content); } + + /** + * The dedup lock cannot guarantee one row per scan on its own - it has a + * TTL, no renewal and no fencing token, so it can lapse while the + * transaction it wraps is still running. The SponsorBadgeScan.ScanDedupKey + * UNIQUE index is what does, so this asserts the index is actually there + * and rejecting: two rows carrying the same key must not both persist, + * whatever the service layer above happens to do. + */ + public function testScanDedupKeyUniqueIndexRejectsADuplicateRow(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $scan_date = new \DateTime("@1572019200"); + + $dedup_key = \models\summit\SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date); + + $first = new \models\summit\SponsorBadgeScan(); + $first->setScanDate($scan_date); + $first->setQRCode('dedup-index-test'); + $first->setUser(self::$member); + $first->setBadge($badge); + $first->setNotes(''); + $first->setScanDedupKey($dedup_key); + $sponsor->addUserInfoGrant($first); + self::$em->persist($first); + self::$em->flush(); + + // Byte-identical key, which is the whole point: a second physical row for + // one scan is what the production bug produced and what the index forbids. + $second = new \models\summit\SponsorBadgeScan(); + $second->setScanDate($scan_date); + $second->setQRCode('dedup-index-test'); + $second->setUser(self::$member); + $second->setBadge($badge); + $second->setNotes(''); + $second->setScanDedupKey($dedup_key); + $sponsor->addUserInfoGrant($second); + self::$em->persist($second); + + $threw = false; + try { + self::$em->flush(); + } catch (UniqueConstraintViolationException $ex) { + $threw = true; + } + + $this->assertTrue($threw, + "SponsorBadgeScan_ScanDedupKey must reject a second row with the same dedup key - ". + "without that index the lock's TTL is the only thing preventing duplicates, which it cannot be"); + } + + /** + * The index above only protects rows that actually carry a key, so this + * asserts the service populates it: a scan created through the real + * addBadgeScan path must come out with the ScanDedupKey its + * (sponsor, badge, scan_date) tuple implies. If this regressed, every new + * row would go in with NULL - which MySQL allows without limit in a UNIQUE + * index - and the duplicate protection would silently be gone. + */ + public function testAddBadgeScanPopulatesTheScanDedupKey(){ + self::$member->clearGroups(); + self::$member->add2Group($this->sponsor_group); + self::$em->persist(self::$member); + self::$em->flush(); + + $sponsor = self::$summit->getSummitSponsors()[0]; + $sponsor->addUser(self::$member); + self::$em->persist($sponsor); + self::$em->flush(); + + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $scan_date_epoch = 1572019200; + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, [ + 'qr_code' => $badge->generateQRCode(), + 'scan_date' => $scan_date_epoch, + 'sponsor_id' => $sponsor->getId(), + ]); + + $this->assertNotNull($scan); + $this->assertEquals( + sprintf('%d:%d:%d', $sponsor->getId(), $badge->getId(), $scan_date_epoch), + $scan->getScanDedupKey(), + "addBadgeScan must stamp the dedup key on the new scan, otherwise the UNIQUE index protects nothing" + ); + } }