From fccbfbb1fe92256b9992ed418bef3c018375b056 Mon Sep 17 00:00:00 2001 From: romanetar Date: Thu, 10 Sep 2026 16:22:55 +0200 Subject: [PATCH 1/7] 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 715569b62f..8882210666 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 67c7f7d518..2687609bb7 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 811c8c5e55..48493e57d0 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 ea08bc8a4a..6c38366925 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/7] 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 d7340c63ae..5984aeabc4 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 48493e57d0..027912a772 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 0000000000..ef11489a34 --- /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" + ); + } } From c7f0507900293f54955ebb91c966f1869739c355 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 23 Sep 2026 15:15:43 +0200 Subject: [PATCH 3/7] test(badge-scan): cover the unique-violation handler in addBadgeScan The sequential POST tests only ever reach the pre-INSERT existence check, and the unique-index test bypasses the service, so the handler that is the actual idempotency guarantee ran in no test. Build the service with a mocked ISponsorUserInfoGrantRepository whose findExistingBadgeScan misses a pre-inserted winning scan, so the INSERT really hits SponsorBadgeScan_ScanDedupKey: - blinded once: the handler must resolve to the winner, one row left - blinded on the re-read too: the violation must be rethrown --- ...OAuth2SummitBadgeScanApiControllerTest.php | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index 861bedc681..a97c7f1175 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -14,19 +14,29 @@ use App\Models\Foundation\Main\IGroup; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; +use App\Models\Foundation\Summit\Repositories\ISponsorRepository; +use App\Models\Foundation\Summit\Repositories\ISummitAttendeeBadgeRepository; +use App\Services\Model\Imp\SponsorUserInfoGrantService; use App\Services\Model\ISponsorUserInfoGrantService; use App\Services\Utils\Exceptions\UnacquiredLockException; use App\Services\Utils\ILockManagerService; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Illuminate\Support\Facades\App; +use LaravelDoctrine\ORM\Facades\Registry; use Libs\ModelSerializers\AbstractSerializer; +use libs\utils\ITransactionService; +use Mockery; use models\main\Group; +use models\summit\ISponsorUserInfoGrantRepository; +use models\summit\ISummitAttendeeRepository; use models\summit\Sponsor; +use models\summit\SponsorBadgeScan; use models\summit\SummitAttendee; use models\summit\SummitAttendeeBadge; use models\summit\SummitAttendeeTicket; use models\summit\SummitLeadReportSetting; use models\summit\SummitOrder; +use models\utils\SilverstripeBaseModel; /** * Class OAuth2SummitBadgeScanApiControllerTest */ @@ -70,6 +80,7 @@ protected function setUp():void protected function tearDown():void { + Mockery::close(); self::clearSummitTestData(); parent::tearDown(); } @@ -1151,4 +1162,133 @@ public function testAddBadgeScanPopulatesTheScanDedupKey(){ "addBadgeScan must stamp the dedup key on the new scan, otherwise the UNIQUE index protects nothing" ); } + + /** + * Builds the real SponsorUserInfoGrantService, except that its + * findExistingBadgeScan answers null for the first $stubbed_calls calls + * and delegates to the real repository afterwards. That is how a + * sequential test reproduces the race the UNIQUE index exists for: the + * pre-INSERT existence check misses a row that is already committed, + * exactly as it does when the lock lapsed and a concurrent request won. + * @param int $stubbed_calls + * @param int $calls counts every findExistingBadgeScan call, by reference + * @return ISponsorUserInfoGrantService + */ + private function buildServiceWithBlindExistenceCheck(int $stubbed_calls, int &$calls): ISponsorUserInfoGrantService + { + $real_repository = App::make(ISponsorUserInfoGrantRepository::class); + $repository = Mockery::mock(ISponsorUserInfoGrantRepository::class); + $repository->shouldReceive('findExistingBadgeScan') + ->andReturnUsing(function(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date) use($real_repository, $stubbed_calls, &$calls){ + $calls++; + if($calls <= $stubbed_calls) return null; + return $real_repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + }); + + return new SponsorUserInfoGrantService( + $repository, + App::make(ISummitAttendeeRepository::class), + App::make(ISummitAttendeeBadgeRepository::class), + App::make(ISponsorRepository::class), + App::make(ITransactionService::class), + App::make(ILockManagerService::class) + ); + } + + /** + * Persists the scan that "won the race": same (sponsor, badge, scan_date) + * and therefore the same ScanDedupKey the service is about to INSERT. + * @param Sponsor $sponsor + * @param SummitAttendeeBadge $badge + * @param \DateTime $scan_date + * @return SponsorBadgeScan + */ + private function insertWinningScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): SponsorBadgeScan + { + $winner = new SponsorBadgeScan(); + $winner->setScanDate($scan_date); + $winner->setQRCode('dedup-race-winner'); + $winner->setUser(self::$member); + $winner->setBadge($badge); + $winner->setNotes(''); + $winner->setScanDedupKey(SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date)); + $sponsor->addUserInfoGrant($winner); + self::$em->persist($winner); + self::$em->flush(); + return $winner; + } + + /** + * The UniqueConstraintViolationException handler in addBadgeScanLocked is + * what actually guarantees one row per scan, and the sequential POST tests + * above never reach it (their second request stops at the existence check). + * Here the check is blinded once, so the INSERT really hits the index: the + * flush fails, the EntityManager is closed, and the handler has to re-read + * the winner in a fresh transaction and return it instead of failing or + * writing a second row. + */ + public function testAddBadgeScanResolvesAUniqueViolationToTheWinningScan(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $scan_date_epoch = 1572019200; + $scan_date = new \DateTime("@$scan_date_epoch"); + + $winner = $this->insertWinningScan($sponsor, $badge, $scan_date); + $winner_id = $winner->getId(); + $dedup_key = $winner->getScanDedupKey(); + + $calls = 0; + $service = $this->buildServiceWithBlindExistenceCheck(1, $calls); + + $scan = $service->addBadgeScan(self::$summit, self::$member, [ + 'qr_code' => $badge->generateQRCode(), + 'scan_date' => $scan_date_epoch, + 'sponsor_id' => $sponsor->getId(), + ]); + + $this->assertEquals(2, $calls, + "the existence check must run once before the INSERT and once more in the unique-violation handler"); + $this->assertEquals($winner_id, $scan->getId(), + "a unique violation on the dedup key must resolve to the scan that won the race"); + + // The failed flush closed the manager the test started with. + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $count = $em->getRepository(SponsorBadgeScan::class)->count(['scan_dedup_key' => $dedup_key]); + $this->assertEquals(1, $count, "the losing request must not leave a second row behind"); + } + + /** + * The handler only swallows a violation it can attribute to this scan's + * own tuple. If the re-read finds nothing, the violation came from some + * other constraint and must surface, not be turned into a bogus success. + * Simulated by keeping the existence check blind on the re-read too. + */ + public function testAddBadgeScanRethrowsAUniqueViolationItCannotResolve(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $scan_date_epoch = 1572019200; + + $this->insertWinningScan($sponsor, $badge, new \DateTime("@$scan_date_epoch")); + + $calls = 0; + $service = $this->buildServiceWithBlindExistenceCheck(PHP_INT_MAX, $calls); + + $threw = false; + try { + $service->addBadgeScan(self::$summit, self::$member, [ + 'qr_code' => $badge->generateQRCode(), + 'scan_date' => $scan_date_epoch, + 'sponsor_id' => $sponsor->getId(), + ]); + } catch (UniqueConstraintViolationException $ex) { + $threw = true; + } + + $this->assertEquals(2, $calls, + "the handler must have re-checked for the winner before giving up"); + $this->assertTrue($threw, + "a unique violation the handler cannot match to an existing scan must be rethrown"); + } } From b567ac6644ef1a7388af238fd38bc6079bb93e20 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 23 Sep 2026 15:17:07 +0200 Subject: [PATCH 4/7] fix(badge-scan): reload sponsor, badge and member on every transaction attempt On a reconnectable error DoctrineTransactionService discards the EntityManager and re-runs the closure against a fresh one. The closure reused the Sponsor, badge and Member resolved before the transaction, which the new manager does not know: the cascade from Sponsor::$user_info_grants never fired, the COMMIT succeeded empty and a transient scan came back as a 201 with id 0. The scanning app then marks the scan uploaded and never resends it. Capture ids instead and reload the entities at the top of each attempt (IMemberRepository is now injected for the member). The first attempt is served by the identity map. The test makes the first findExistingBadgeScan call throw a DeadlockException and asserts one persisted row with a real id. --- .../Model/Imp/SponsorUserInfoGrantService.php | 37 ++++++++- ...OAuth2SummitBadgeScanApiControllerTest.php | 75 ++++++++++++++++--- 2 files changed, 98 insertions(+), 14 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index 027912a772..e79d3bd433 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -22,6 +22,7 @@ use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; use models\exceptions\ValidationException; +use models\main\IMemberRepository; use models\main\Member; use models\summit\ISponsorUserInfoGrantRepository; use models\summit\ISummitAttendeeRepository; @@ -59,6 +60,11 @@ final class SponsorUserInfoGrantService */ private $sponsor_repository; + /** + * @var IMemberRepository + */ + private $member_repository; + /** * @var ILockManagerService */ @@ -69,6 +75,7 @@ final class SponsorUserInfoGrantService * @param ISummitAttendeeRepository $attendee_repository * @param ISummitAttendeeBadgeRepository $badge_repository * @param ISponsorRepository $sponsor_repository + * @param IMemberRepository $member_repository * @param ITransactionService $tx_service * @param ILockManagerService $lock_service */ @@ -78,6 +85,7 @@ public function __construct ISummitAttendeeRepository $attendee_repository, ISummitAttendeeBadgeRepository $badge_repository, ISponsorRepository $sponsor_repository, + IMemberRepository $member_repository, ITransactionService $tx_service, ILockManagerService $lock_service ) @@ -87,6 +95,7 @@ public function __construct $this->attendee_repository = $attendee_repository; $this->badge_repository = $badge_repository; $this->sponsor_repository = $sponsor_repository; + $this->member_repository = $member_repository; $this->lock_service = $lock_service; } @@ -306,6 +315,15 @@ public function addBadgeScan(Summit $summit, Member $current_member, array $data * is rejected by the index and the UniqueConstraintViolationException * handler below resolves the retry to the row that won the race. * + * The transaction closure captures ids, not the Sponsor/badge/Member + * resolved in addBadgeScan, and reloads them on every attempt. On a + * reconnectable error DoctrineTransactionService resets the registry to + * a fresh EntityManager and re-runs the closure; entities captured from + * the previous manager are unknown to it, so the cascade from + * Sponsor::$user_info_grants never fires, flush() commits an empty unit + * of work and a transient scan with id 0 is returned as if it had been + * saved. On the first attempt the reload is served by the identity map. + * * Rows predating that index keep a NULL ScanDedupKey and are covered by * the explicit existence check alone - see the column's own docblock. * @@ -340,10 +358,25 @@ private function addBadgeScanLocked( { $lock_name = sprintf('badge_scan.%d.%d.%d.lock', $sponsor->getId(), $badge->getId(), $scan_date_epoch); $dedup_key = SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date); + $sponsor_id = $sponsor->getId(); + $badge_id = $badge->getId(); + $member_id = $current_member->getId(); 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){ + return $this->lock_service->lock($lock_name, function() use($sponsor_id, $badge_id, $member_id, $scan_date, $dedup_key, $qr_code, $source, $data){ + return $this->tx_service->transaction(function() use($sponsor_id, $badge_id, $member_id, $scan_date, $dedup_key, $qr_code, $source, $data){ + // Reloaded on every attempt, see the docblock: a retried attempt + // runs against a fresh EntityManager. + $sponsor = $this->sponsor_repository->getById($sponsor_id); + if(!$sponsor instanceof Sponsor) + throw new EntityNotFoundException("Sponsor not found."); + $badge = $this->badge_repository->getById($badge_id); + if(!$badge instanceof SummitAttendeeBadge) + throw new EntityNotFoundException("badge not found."); + $current_member = $this->member_repository->getById($member_id); + if(!$current_member instanceof Member) + throw new EntityNotFoundException("Member not found."); + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); if(!is_null($existing)){ Log::warning( diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index a97c7f1175..1400856091 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -20,6 +20,8 @@ use App\Services\Model\ISponsorUserInfoGrantService; use App\Services\Utils\Exceptions\UnacquiredLockException; use App\Services\Utils\ILockManagerService; +use Doctrine\DBAL\Driver\PDO\Exception as PDODriverException; +use Doctrine\DBAL\Exception\DeadlockException; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Illuminate\Support\Facades\App; use LaravelDoctrine\ORM\Facades\Registry; @@ -27,6 +29,7 @@ use libs\utils\ITransactionService; use Mockery; use models\main\Group; +use models\main\IMemberRepository; use models\summit\ISponsorUserInfoGrantRepository; use models\summit\ISummitAttendeeRepository; use models\summit\Sponsor; @@ -1164,25 +1167,26 @@ public function testAddBadgeScanPopulatesTheScanDedupKey(){ } /** - * Builds the real SponsorUserInfoGrantService, except that its - * findExistingBadgeScan answers null for the first $stubbed_calls calls - * and delegates to the real repository afterwards. That is how a - * sequential test reproduces the race the UNIQUE index exists for: the - * pre-INSERT existence check misses a row that is already committed, + * Builds the real SponsorUserInfoGrantService, except that every + * findExistingBadgeScan call goes through $find($call_number, $real), + * where $real() runs the real repository query. Blinding that check is + * how a sequential test reproduces the race the UNIQUE index exists for: + * the pre-INSERT existence check misses a row that is already committed, * exactly as it does when the lock lapsed and a concurrent request won. - * @param int $stubbed_calls + * Throwing from it is how a test injects a failure into a given attempt + * of the transaction. + * @param \Closure $find * @param int $calls counts every findExistingBadgeScan call, by reference * @return ISponsorUserInfoGrantService */ - private function buildServiceWithBlindExistenceCheck(int $stubbed_calls, int &$calls): ISponsorUserInfoGrantService + private function buildServiceWithExistenceCheck(\Closure $find, int &$calls): ISponsorUserInfoGrantService { $real_repository = App::make(ISponsorUserInfoGrantRepository::class); $repository = Mockery::mock(ISponsorUserInfoGrantRepository::class); $repository->shouldReceive('findExistingBadgeScan') - ->andReturnUsing(function(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date) use($real_repository, $stubbed_calls, &$calls){ + ->andReturnUsing(function(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date) use($real_repository, $find, &$calls){ $calls++; - if($calls <= $stubbed_calls) return null; - return $real_repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + return $find($calls, fn() => $real_repository->findExistingBadgeScan($sponsor, $badge, $scan_date)); }); return new SponsorUserInfoGrantService( @@ -1190,6 +1194,7 @@ private function buildServiceWithBlindExistenceCheck(int $stubbed_calls, int &$c App::make(ISummitAttendeeRepository::class), App::make(ISummitAttendeeBadgeRepository::class), App::make(ISponsorRepository::class), + App::make(IMemberRepository::class), App::make(ITransactionService::class), App::make(ILockManagerService::class) ); @@ -1239,7 +1244,7 @@ public function testAddBadgeScanResolvesAUniqueViolationToTheWinningScan(){ $dedup_key = $winner->getScanDedupKey(); $calls = 0; - $service = $this->buildServiceWithBlindExistenceCheck(1, $calls); + $service = $this->buildServiceWithExistenceCheck(fn(int $n, \Closure $real) => $n === 1 ? null : $real(), $calls); $scan = $service->addBadgeScan(self::$summit, self::$member, [ 'qr_code' => $badge->generateQRCode(), @@ -1273,7 +1278,7 @@ public function testAddBadgeScanRethrowsAUniqueViolationItCannotResolve(){ $this->insertWinningScan($sponsor, $badge, new \DateTime("@$scan_date_epoch")); $calls = 0; - $service = $this->buildServiceWithBlindExistenceCheck(PHP_INT_MAX, $calls); + $service = $this->buildServiceWithExistenceCheck(fn() => null, $calls); $threw = false; try { @@ -1291,4 +1296,50 @@ public function testAddBadgeScanRethrowsAUniqueViolationItCannotResolve(){ $this->assertTrue($threw, "a unique violation the handler cannot match to an existing scan must be rethrown"); } + + /** + * A reconnectable error (deadlock, lock wait timeout, lost connection) + * makes DoctrineTransactionService discard the EntityManager and re-run + * the closure against a fresh one. If the closure reused the Sponsor, + * badge and Member resolved before the transaction, they would be unknown + * to that new manager: the cascade from the sponsor never fires, the + * COMMIT succeeds with nothing in it and a transient scan with id 0 comes + * back as a 201 - a scan the app then marks uploaded and never resends. + * The retried attempt must persist exactly one real row instead. + */ + public function testAddBadgeScanPersistsTheScanWhenTheFirstAttemptDeadlocks(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + $scan_date_epoch = 1572019200; + $sponsor_id = $sponsor->getId(); + $badge_id = $badge->getId(); + + $calls = 0; + $service = $this->buildServiceWithExistenceCheck(function(int $n, \Closure $real){ + if($n === 1) + throw new DeadlockException( + PDODriverException::new(new \PDOException('Deadlock found when trying to get lock; try restarting transaction', 1213)), + null + ); + return $real(); + }, $calls); + + $scan = $service->addBadgeScan(self::$summit, self::$member, [ + 'qr_code' => $badge->generateQRCode(), + 'scan_date' => $scan_date_epoch, + 'sponsor_id' => $sponsor_id, + ]); + + $this->assertEquals(2, $calls, "the deadlocked attempt must have been retried once"); + $this->assertGreaterThan(0, $scan->getId(), + "the retried attempt must return a persisted scan, not a transient one with id 0"); + + // The deadlock discarded the manager the test started with. + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $count = $em->getRepository(SponsorBadgeScan::class)->count([ + 'scan_dedup_key' => sprintf('%d:%d:%d', $sponsor_id, $badge_id, $scan_date_epoch), + ]); + $this->assertEquals(1, $count, "exactly one row must be committed by the retried attempt"); + } } From b5b05fd6f8a9cbf35d318f955dabfd134917b92e Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 23 Sep 2026 15:18:48 +0200 Subject: [PATCH 5/7] fix(badge-scan): apply a retry's notes and extra questions to the matched scan The app POSTs right after the scan, while the notes/questions form is still open. If that POST commits but fails on the client, the form submit goes out as a second POST carrying notes and extra_questions, which was matched to the existing scan and returned unchanged - the data was dropped and the app never resends it. Both the existence check and the unique-violation handler now go through mergeRetryIntoExistingScan, which applies them the way updateBadgeScan does: only non-empty notes (a late, emptier attempt cannot wipe newer data) and the full answer set when one is sent. The dedup key has no member in it, so a match recorded by another rep of the same sponsor is left untouched rather than overwritten. --- .../Model/Imp/SponsorUserInfoGrantService.php | 57 ++++++- ...OAuth2SummitBadgeScanApiControllerTest.php | 151 +++++++++++++++++- 2 files changed, 203 insertions(+), 5 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index e79d3bd433..3d8a30f0d2 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -388,7 +388,7 @@ private function addBadgeScanLocked( $existing->getId() ) ); - return $existing; + return $this->mergeRetryIntoExistingScan($existing, $member_id, $data); } $scan = new SponsorBadgeScan(); @@ -440,18 +440,69 @@ private function addBadgeScanLocked( ) ); - return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $ex){ + return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $member_id, $data, $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; + return $this->mergeRetryIntoExistingScan($existing, $member_id, $data); }); } } + /** + * A retry matched to an existing scan is not always a byte-identical + * resend. The scanning app POSTs as soon as the badge is scanned, while + * the notes/extra questions form is still open; if that first POST + * committed but failed on the client (timeout, dropped connection, 5xx), + * the scan keeps no server id and the form submit goes out as a second + * POST that carries the notes and answers. Returning the existing row + * unchanged would drop them silently, and the app would then mark the + * scan uploaded and never resend them - so they are applied here, the + * same way updateBadgeScan applies them. + * + * Only non-empty fields are applied, so an earlier, emptier attempt that + * arrives late cannot wipe data a later one already stored. The extra + * answers are replaced as a whole (hadCompletedExtraQuestions rebuilds + * the set), which is right because the app always sends its full set. + * + * The dedup key does not include the member, so the match may be a scan + * another rep of the same sponsor recorded in the same second. That is + * not a retry of this request, and its notes and answers are that rep's + * own: they are left untouched and the existing scan is returned as is. + * @param SponsorBadgeScan $existing + * @param int $member_id + * @param array $data + * @return SponsorBadgeScan + * @throws ValidationException + */ + private function mergeRetryIntoExistingScan(SponsorBadgeScan $existing, int $member_id, array $data): SponsorBadgeScan + { + if($existing->getUser()->getId() !== $member_id){ + Log::warning( + sprintf( + "SponsorUserInfoGrantService::addBadgeScan existing scan %s was recorded by member %s, not %s - not applying notes/extra questions to it.", + $existing->getId(), + $existing->getUser()->getId(), + $member_id + ) + ); + return $existing; + } + + $notes = trim($data['notes'] ?? ''); + if(!empty($notes)) + $existing->setNotes($notes); + + $extra_questions = $data['extra_questions'] ?? []; + if (count($extra_questions) && !$existing->hadCompletedExtraQuestions($extra_questions)) + throw new ValidationException("You neglected to fill in all mandatory questions for the badge scan."); + + return $existing; + } + /** * @param Summit $summit * @param Member $current_member diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index 1400856091..62319c7c7f 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -30,6 +30,7 @@ use Mockery; use models\main\Group; use models\main\IMemberRepository; +use models\main\Member; use models\summit\ISponsorUserInfoGrantRepository; use models\summit\ISummitAttendeeRepository; use models\summit\Sponsor; @@ -1206,14 +1207,15 @@ private function buildServiceWithExistenceCheck(\Closure $find, int &$calls): IS * @param Sponsor $sponsor * @param SummitAttendeeBadge $badge * @param \DateTime $scan_date + * @param Member|null $user defaults to self::$member * @return SponsorBadgeScan */ - private function insertWinningScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): SponsorBadgeScan + private function insertWinningScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date, ?Member $user = null): SponsorBadgeScan { $winner = new SponsorBadgeScan(); $winner->setScanDate($scan_date); $winner->setQRCode('dedup-race-winner'); - $winner->setUser(self::$member); + $winner->setUser($user ?? self::$member); $winner->setBadge($badge); $winner->setNotes(''); $winner->setScanDedupKey(SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date)); @@ -1342,4 +1344,149 @@ public function testAddBadgeScanPersistsTheScanWhenTheFirstAttemptDeadlocks(){ ]); $this->assertEquals(1, $count, "exactly one row must be committed by the retried attempt"); } + + /** + * Payload of a scan of the default attendee's badge for sponsors[0], + * plus whatever extra fields the test adds. + * @param array $extra + * @return array + */ + private function badgeScanPayload(array $extra = []): array + { + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + return array_merge([ + 'qr_code' => $attendee->getFirstTicket()->getBadge()->generateQRCode(), + 'scan_date' => 1572019200, + 'sponsor_id' => self::$summit->getSummitSponsors()[0]->getId(), + ], $extra); + } + + /** + * Reads a scan back from the database rather than the identity map. + * @param int $scan_id + * @return SponsorBadgeScan + */ + private function reloadScan(int $scan_id): SponsorBadgeScan + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $scan = $em->getRepository(SponsorBadgeScan::class)->find($scan_id); + $this->assertInstanceOf(SponsorBadgeScan::class, $scan); + return $scan; + } + + /** + * The app's first POST goes out right after the scan, before the notes + * form is submitted. When that POST commits but fails on the client, the + * form submit arrives as a second POST that carries the notes, and it is + * matched to the scan the first one created. Those notes must be stored, + * not dropped because the scan already existed. + */ + public function testAddBadgeScanRetryAppliesItsNotesToTheExistingScan(){ + $service = App::make(ISponsorUserInfoGrantService::class); + + $first = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $first_id = $first->getId(); + + $second = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'notes' => 'hot lead, follow up', + ])); + + $this->assertEquals($first_id, $second->getId(), "the retry must still resolve to the existing scan"); + $this->assertEquals('hot lead, follow up', $this->reloadScan($first_id)->getNotes(), + "notes carried by the retry must be persisted on the existing scan"); + } + + /** + * The reverse order: an earlier, emptier attempt that arrives after the + * one carrying the notes must not wipe them. + */ + public function testAddBadgeScanRetryWithoutNotesKeepsTheStoredNotes(){ + $service = App::make(ISponsorUserInfoGrantService::class); + + $first = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'notes' => 'hot lead, follow up', + ])); + $first_id = $first->getId(); + + $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + + $this->assertEquals('hot lead, follow up', $this->reloadScan($first_id)->getNotes(), + "a retry without notes must leave the stored notes alone"); + } + + /** + * Same as the notes case, for the extra question answers the form sends. + */ + public function testAddBadgeScanRetryAppliesItsExtraQuestionsToTheExistingScan(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $question = $sponsor->getExtraQuestions()[0]; + if (!$question instanceof SummitSponsorExtraQuestionType) self::fail(); + $question_id = $question->getId(); + + $service = App::make(ISponsorUserInfoGrantService::class); + + $first = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload()); + $first_id = $first->getId(); + $this->assertCount(0, $first->getExtraQuestionAnswers()); + + $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'extra_questions' => [ + ['question_id' => $question_id, 'answer' => 'None'], + ], + ])); + + $answers = $this->reloadScan($first_id)->getExtraQuestionAnswers(); + $this->assertCount(1, $answers, "answers carried by the retry must be persisted on the existing scan"); + $this->assertEquals($question_id, $answers->first()->getQuestionId()); + $this->assertEquals('None', $answers->first()->getValue()); + } + + /** + * The dedup key has no member in it, so a match can be a scan another + * rep of the same sponsor recorded in the same second. That is not a + * retry of this request, and it must not overwrite that rep's notes. + */ + public function testAddBadgeScanDoesNotApplyNotesToAnotherMembersScan(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + + $other = $this->insertWinningScan($sponsor, $badge, new \DateTime("@1572019200"), self::$member2); + $other_id = $other->getId(); + + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'notes' => 'hot lead, follow up', + ])); + + $this->assertEquals($other_id, $scan->getId()); + $this->assertEquals('', $this->reloadScan($other_id)->getNotes(), + "another member's scan must not take this request's notes"); + } + + /** + * The unique-violation handler resolves to the winning row through the + * same merge, so notes that lost the race are not dropped either. + */ + public function testAddBadgeScanUniqueViolationAppliesNotesToTheWinningScan(){ + $sponsor = self::$summit->getSummitSponsors()[0]; + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $badge = $attendee->getFirstTicket()->getBadge(); + + $winner = $this->insertWinningScan($sponsor, $badge, new \DateTime("@1572019200")); + $winner_id = $winner->getId(); + + $calls = 0; + $service = $this->buildServiceWithExistenceCheck(fn(int $n, \Closure $real) => $n === 1 ? null : $real(), $calls); + + $scan = $service->addBadgeScan(self::$summit, self::$member, $this->badgeScanPayload([ + 'notes' => 'hot lead, follow up', + ])); + + $this->assertEquals(2, $calls, "the INSERT must have hit the index and gone through the handler"); + $this->assertEquals($winner_id, $scan->getId()); + $this->assertEquals('hot lead, follow up', $this->reloadScan($winner_id)->getNotes(), + "notes of the request that lost the race must be persisted on the winning scan"); + } } From 6e4abdb8cc493b635cea602af2c56225e1e3f07d Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 6 Oct 2026 19:25:48 +0200 Subject: [PATCH 6/7] fix(badge-scan): make the scanning member part of the scan's dedup identity Two reps of the same sponsor scanning the same badge in the same second shared one scan, so the second rep's follow-up PUT overwrote the first rep's notes and answers. The dedup key, the existence lookup and the lock name now include the member, so each rep keeps their own row. The other-member branch in mergeRetryIntoExistingScan is unreachable and gone. --- .../Summit/Registration/SponsorBadgeScan.php | 10 +++-- .../ISponsorUserInfoGrantRepository.php | 3 +- ...DoctrineSponsorUserInfoGrantRepository.php | 8 +++- .../Model/Imp/SponsorUserInfoGrantService.php | 43 +++++++------------ ...OAuth2SummitBadgeScanApiControllerTest.php | 28 ++++++------ 5 files changed, 47 insertions(+), 45 deletions(-) diff --git a/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php b/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php index 5984aeabc4..1b4d09d209 100644 --- a/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php +++ b/app/Models/Foundation/Summit/Registration/SponsorBadgeScan.php @@ -93,7 +93,7 @@ class SponsorBadgeScan extends SponsorUserInfoGrant private $source; /** - * Denormalized "::" identity of the physical + * 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 @@ -201,14 +201,18 @@ public function setScanDate(\DateTime $scan_date): void * 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. + * The scanning member is part of the identity: a real retry always comes from + * the same member, whereas two reps of one sponsor scanning the same badge in + * the same second are two distinct scans, each with its own notes and answers. * @param Sponsor $sponsor * @param SummitAttendeeBadge $badge + * @param Member $member * @param \DateTime $scan_date * @return string */ - public static function buildDedupKey(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): string + public static function buildDedupKey(Sponsor $sponsor, SummitAttendeeBadge $badge, Member $member, \DateTime $scan_date): string { - return sprintf('%d:%d:%d', $sponsor->getId(), $badge->getId(), $scan_date->getTimestamp()); + return sprintf('%d:%d:%d:%d', $sponsor->getId(), $badge->getId(), $member->getId(), $scan_date->getTimestamp()); } /** diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php index 8882210666..0788b1261a 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use models\main\Member; use models\utils\IBaseRepository; /** * Interface ISponsorUserInfoGrantRepository @@ -27,5 +28,5 @@ interface ISponsorUserInfoGrantRepository extends IBaseRepository * @param \DateTime $scan_date * @return SponsorBadgeScan|null */ - public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): ?SponsorBadgeScan; + public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, Member $member, \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 2687609bb7..92608c4340 100644 --- a/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php @@ -13,6 +13,7 @@ **/ use App\Repositories\SilverStripeDoctrineRepository; use Doctrine\ORM\QueryBuilder; +use models\main\Member; use models\summit\ISponsorUserInfoGrantRepository; use models\summit\Presentation; use models\summit\Sponsor; @@ -130,15 +131,16 @@ protected function getBaseEntity() /** * 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. + * is meant for paged listing) for an exact (sponsor, badge, member, 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 Member $member * @param \DateTime $scan_date * @return SponsorBadgeScan|null */ - public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date): ?SponsorBadgeScan + public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, Member $member, \DateTime $scan_date): ?SponsorBadgeScan { $query = $this->getEntityManager() ->createQueryBuilder() @@ -146,9 +148,11 @@ public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $bad ->from(SponsorBadgeScan::class, "e") ->where("e.sponsor = :sponsor") ->andWhere("e.badge = :badge") + ->andWhere("e.user = :member") ->andWhere("e.scan_date = :scan_date") ->setParameter("sponsor", $sponsor) ->setParameter("badge", $badge) + ->setParameter("member", $member) ->setParameter("scan_date", $scan_date) ->setMaxResults(1); diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php index 3d8a30f0d2..82fc7865ec 100644 --- a/app/Services/Model/Imp/SponsorUserInfoGrantService.php +++ b/app/Services/Model/Imp/SponsorUserInfoGrantService.php @@ -276,13 +276,13 @@ public function addBadgeScan(Summit $summit, Member $current_member, array $data } // Phase 2: create the scan, guarded against a concurrent duplicate - // for this same (sponsor, badge, scan_date) - see addBadgeScanLocked. + // for this same (sponsor, badge, member, 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 + * for the given (sponsor, badge, member, 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 @@ -356,11 +356,11 @@ private function addBadgeScanLocked( array $data ): 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); $sponsor_id = $sponsor->getId(); $badge_id = $badge->getId(); $member_id = $current_member->getId(); + $lock_name = sprintf('badge_scan.%d.%d.%d.%d.lock', $sponsor_id, $badge_id, $member_id, $scan_date_epoch); + $dedup_key = SponsorBadgeScan::buildDedupKey($sponsor, $badge, $current_member, $scan_date); try { return $this->lock_service->lock($lock_name, function() use($sponsor_id, $badge_id, $member_id, $scan_date, $dedup_key, $qr_code, $source, $data){ @@ -377,7 +377,7 @@ private function addBadgeScanLocked( if(!$current_member instanceof Member) throw new EntityNotFoundException("Member not found."); - $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $current_member, $scan_date); if(!is_null($existing)){ Log::warning( sprintf( @@ -388,7 +388,7 @@ private function addBadgeScanLocked( $existing->getId() ) ); - return $this->mergeRetryIntoExistingScan($existing, $member_id, $data); + return $this->mergeRetryIntoExistingScan($existing, $data); } $scan = new SponsorBadgeScan(); @@ -440,14 +440,17 @@ private function addBadgeScanLocked( ) ); - return $this->tx_service->transaction(function() use($sponsor, $badge, $scan_date, $member_id, $data, $ex){ - $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $scan_date); + return $this->tx_service->transaction(function() use($sponsor_id, $badge_id, $member_id, $scan_date, $data, $ex){ + $sponsor = $this->sponsor_repository->getById($sponsor_id); + $badge = $this->badge_repository->getById($badge_id); + $member = $this->member_repository->getById($member_id); + $existing = $this->repository->findExistingBadgeScan($sponsor, $badge, $member, $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 $this->mergeRetryIntoExistingScan($existing, $member_id, $data); + return $this->mergeRetryIntoExistingScan($existing, $data); }); } } @@ -468,30 +471,16 @@ private function addBadgeScanLocked( * answers are replaced as a whole (hadCompletedExtraQuestions rebuilds * the set), which is right because the app always sends its full set. * - * The dedup key does not include the member, so the match may be a scan - * another rep of the same sponsor recorded in the same second. That is - * not a retry of this request, and its notes and answers are that rep's - * own: they are left untouched and the existing scan is returned as is. + * The dedup key includes the member, so a match is always a scan this same + * member recorded: two reps of one sponsor scanning the same badge in the + * same second get one scan each and never reach this method for the other's. * @param SponsorBadgeScan $existing - * @param int $member_id * @param array $data * @return SponsorBadgeScan * @throws ValidationException */ - private function mergeRetryIntoExistingScan(SponsorBadgeScan $existing, int $member_id, array $data): SponsorBadgeScan + private function mergeRetryIntoExistingScan(SponsorBadgeScan $existing, array $data): SponsorBadgeScan { - if($existing->getUser()->getId() !== $member_id){ - Log::warning( - sprintf( - "SponsorUserInfoGrantService::addBadgeScan existing scan %s was recorded by member %s, not %s - not applying notes/extra questions to it.", - $existing->getId(), - $existing->getUser()->getId(), - $member_id - ) - ); - return $existing; - } - $notes = trim($data['notes'] ?? ''); if(!empty($notes)) $existing->setNotes($notes); diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index 62319c7c7f..5bc2b86d32 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -352,7 +352,7 @@ public function testAddBadgeScanBlocksOnAConcurrentLockHolder(){ // $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_name = sprintf('badge_scan.%d.%d.%d.%d.lock', $sponsor->getId(), $badge->getId(), self::$member->getId(), $scan_date_epoch); $lock_service = App::make(ILockManagerService::class); $held_token = $lock_service->acquireLock($lock_name, 10); @@ -375,7 +375,7 @@ public function testAddBadgeScanBlocksOnAConcurrentLockHolder(){ } $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"); + "either it isn't locking on (sponsor, badge, member, 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); @@ -1092,7 +1092,7 @@ public function testScanDedupKeyUniqueIndexRejectsADuplicateRow(){ $badge = $attendee->getFirstTicket()->getBadge(); $scan_date = new \DateTime("@1572019200"); - $dedup_key = \models\summit\SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date); + $dedup_key = \models\summit\SponsorBadgeScan::buildDedupKey($sponsor, $badge, self::$member, $scan_date); $first = new \models\summit\SponsorBadgeScan(); $first->setScanDate($scan_date); @@ -1185,9 +1185,9 @@ private function buildServiceWithExistenceCheck(\Closure $find, int &$calls): IS $real_repository = App::make(ISponsorUserInfoGrantRepository::class); $repository = Mockery::mock(ISponsorUserInfoGrantRepository::class); $repository->shouldReceive('findExistingBadgeScan') - ->andReturnUsing(function(Sponsor $sponsor, SummitAttendeeBadge $badge, \DateTime $scan_date) use($real_repository, $find, &$calls){ + ->andReturnUsing(function(Sponsor $sponsor, SummitAttendeeBadge $badge, Member $member, \DateTime $scan_date) use($real_repository, $find, &$calls){ $calls++; - return $find($calls, fn() => $real_repository->findExistingBadgeScan($sponsor, $badge, $scan_date)); + return $find($calls, fn() => $real_repository->findExistingBadgeScan($sponsor, $badge, $member, $scan_date)); }); return new SponsorUserInfoGrantService( @@ -1215,10 +1215,11 @@ private function insertWinningScan(Sponsor $sponsor, SummitAttendeeBadge $badge, $winner = new SponsorBadgeScan(); $winner->setScanDate($scan_date); $winner->setQRCode('dedup-race-winner'); - $winner->setUser($user ?? self::$member); + $user = $user ?? self::$member; + $winner->setUser($user); $winner->setBadge($badge); $winner->setNotes(''); - $winner->setScanDedupKey(SponsorBadgeScan::buildDedupKey($sponsor, $badge, $scan_date)); + $winner->setScanDedupKey(SponsorBadgeScan::buildDedupKey($sponsor, $badge, $user, $scan_date)); $sponsor->addUserInfoGrant($winner); self::$em->persist($winner); self::$em->flush(); @@ -1443,11 +1444,11 @@ public function testAddBadgeScanRetryAppliesItsExtraQuestionsToTheExistingScan() } /** - * The dedup key has no member in it, so a match can be a scan another - * rep of the same sponsor recorded in the same second. That is not a - * retry of this request, and it must not overwrite that rep's notes. + * The dedup key includes the member, so another rep of the same sponsor + * scanning the same badge in the same second is not a retry: it gets its + * own scan, and the first rep's notes are left alone. */ - public function testAddBadgeScanDoesNotApplyNotesToAnotherMembersScan(){ + public function testAddBadgeScanCreatesASeparateScanPerMemberInTheSameSecond(){ $sponsor = self::$summit->getSummitSponsors()[0]; $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); $badge = $attendee->getFirstTicket()->getBadge(); @@ -1460,7 +1461,10 @@ public function testAddBadgeScanDoesNotApplyNotesToAnotherMembersScan(){ 'notes' => 'hot lead, follow up', ])); - $this->assertEquals($other_id, $scan->getId()); + $this->assertNotEquals($other_id, $scan->getId(), + "a scan by a different member in the same second must not collapse into the other member's scan"); + $this->assertEquals(self::$member->getId(), $scan->getUser()->getId()); + $this->assertEquals('hot lead, follow up', $this->reloadScan($scan->getId())->getNotes()); $this->assertEquals('', $this->reloadScan($other_id)->getNotes(), "another member's scan must not take this request's notes"); } From dbd2ffe9287ccbbe058edc131c4a3e17ac0669f5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 6 Oct 2026 20:26:11 +0200 Subject: [PATCH 7/7] test(badge-scan): expect the member id in the hardcoded dedup keys --- tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php index 5bc2b86d32..4f761eafb1 100644 --- a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php @@ -1161,7 +1161,7 @@ public function testAddBadgeScanPopulatesTheScanDedupKey(){ $this->assertNotNull($scan); $this->assertEquals( - sprintf('%d:%d:%d', $sponsor->getId(), $badge->getId(), $scan_date_epoch), + sprintf('%d:%d:%d:%d', $sponsor->getId(), $badge->getId(), self::$member->getId(), $scan_date_epoch), $scan->getScanDedupKey(), "addBadgeScan must stamp the dedup key on the new scan, otherwise the UNIQUE index protects nothing" ); @@ -1341,7 +1341,7 @@ public function testAddBadgeScanPersistsTheScanWhenTheFirstAttemptDeadlocks(){ // The deadlock discarded the manager the test started with. $em = Registry::getManager(SilverstripeBaseModel::EntityManager); $count = $em->getRepository(SponsorBadgeScan::class)->count([ - 'scan_dedup_key' => sprintf('%d:%d:%d', $sponsor_id, $badge_id, $scan_date_epoch), + 'scan_dedup_key' => sprintf('%d:%d:%d:%d', $sponsor_id, $badge_id, self::$member->getId(), $scan_date_epoch), ]); $this->assertEquals(1, $count, "exactly one row must be committed by the retried attempt"); }