fix(lock): implement Redlock single-instance pattern in LockManagerService #537
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -267,6 +267,7 @@ private function buildPrePaidSaga(Member $owner, Summit $summit, array $payload) | |
| $this->member_repository, | ||
| $this->attendee_repository, | ||
| $this->ticket_type_repository, | ||
| $this->promo_code_repository, | ||
| $this->tx_service, | ||
| $this->lock_service | ||
| )); | ||
|
|
@@ -830,7 +831,7 @@ public function run(array $formerState): array | |
|
|
||
| $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $qty, $owner_email) { | ||
| $promo_code->addUsage($owner_email, $qty); | ||
| }, 30); | ||
|
|
||
| }); | ||
| // mark a done | ||
|
|
@@ -868,7 +869,7 @@ public function undo() | |
|
|
||
| $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $info, $owner_email) { | ||
| $promo_code->removeUsage(intval($info['qty']), $owner_email); | ||
| }, 30); | ||
|
|
||
| }); | ||
| } | ||
|
|
@@ -953,7 +954,7 @@ public function run(array $formerState): array | |
|
|
||
| $this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $reservations) { | ||
| $ticket_type->sell($reservations[$ticket_type->getId()]); | ||
| }, 30); | ||
|
|
||
| } | ||
| }); | ||
|
|
@@ -970,7 +971,7 @@ public function undo() | |
| if (is_null($ticket_type)) return; | ||
| $this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $qty) { | ||
| $ticket_type->restore($qty); | ||
| }, 30); | ||
| }); | ||
| } | ||
| } | ||
|
|
@@ -1477,6 +1478,11 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask | |
| */ | ||
| private $ticket_type_repository; | ||
|
|
||
| /** | ||
| * @var ISummitRegistrationPromoCodeRepository | ||
| */ | ||
| private $promo_code_repository; | ||
|
|
||
| /** | ||
| * @var ILockManagerService | ||
| */ | ||
|
|
@@ -1490,19 +1496,21 @@ final class AutoAssignPrePaidTicketTask extends AbstractTask | |
| * @param IMemberRepository $member_repository | ||
| * @param ISummitAttendeeRepository $attendee_repository | ||
| * @param ISummitTicketTypeRepository $ticket_type_repository | ||
| * @param ISummitRegistrationPromoCodeRepository $promo_code_repository | ||
| * @param ITransactionService $tx_service | ||
| * @param ILockManagerService $lock_service | ||
| */ | ||
| public function __construct | ||
| ( | ||
| ?Member $owner, | ||
| Summit $summit, | ||
| array $payload, | ||
| IMemberRepository $member_repository, | ||
| ISummitAttendeeRepository $attendee_repository, | ||
| ISummitTicketTypeRepository $ticket_type_repository, | ||
| ISummitRegistrationPromoCodeRepository $promo_code_repository, | ||
| ITransactionService $tx_service, | ||
| ILockManagerService $lock_service | ||
| ) | ||
| { | ||
| $this->tx_service = $tx_service; | ||
|
|
@@ -1513,6 +1521,7 @@ public function __construct | |
| $this->member_repository = $member_repository; | ||
| $this->attendee_repository = $attendee_repository; | ||
| $this->ticket_type_repository = $ticket_type_repository; | ||
| $this->promo_code_repository = $promo_code_repository; | ||
| } | ||
|
|
||
| public function run(array $formerState): array | ||
|
|
@@ -1539,7 +1548,7 @@ public function run(array $formerState): array | |
| if (empty($promo_code_val)) throw new ValidationException("Promo code is required."); | ||
|
|
||
| $type_id = $ticket_dto['type_id']; | ||
| $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock', | ||
|
Collaborator
There was a problem hiding this comment. @romanetar The key fix is correct, but note it splits the lock namespace between fleets during a rolling deploy: old pods lock No code change needed — suggest a line in the release notes: deploy while prepaid-assignment traffic is idle, or drain old workers before starting the new fleet.
Collaborator
Author
There was a problem hiding this comment. Good flag, and confirmed against No code change from this thread, but worth noting: the DB pessimistic lock added for your other comment on this same call site ( Will add the release-note line about draining old workers / deploying during idle prepaid-assignment traffic anyway, since the underlying old-pod unconditional-release bug can still stomp on other locks in the system during the transition, not just this one. |
||
| function () use ($promo_code_val, $type_id) { | ||
|
Collaborator
There was a problem hiding this comment. @romanetar Missing The closure at this line ( I confirmed this against the branch directly: a regression test that submits Suggested fix: $order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock',
- function () use ($promo_code_val, $type_id) {
+ function () use ($promo_code_val, $type_id, $ticket_dto) {I verified this one-line change is sufficient — with it applied, the regression test below passes (attendee gets the ticket-level company/first/last name), and fails without it ( public function testAutoAssignPrePaidTicketUsesTicketLevelAttendeeData() {
// Fixture registration window starts tomorrow (see InsertSummitTestData);
// open it now so this test isn't blocked by an unrelated precondition.
self::$summit->setRegistrationBeginDate(new \DateTime('-1 day'));
self::$summit->setRegistrationEndDate(new \DateTime('+30 days'));
self::$em->persist(self::$summit);
self::$em->flush();
// Build a dedicated unassigned, paid, offline ticket so this test does not
// depend on the base fixture's order #0 (already-assigned attendee / online
// payment method, neither of which qualifies for prepaid pickup).
$owner = self::$defaultMember;
$order = new SummitOrder();
$order->setSummit(self::$summit);
$order->setOwner($owner);
$order->setPaymentMethodOffline();
$order->generateNumber();
$ticket = new SummitAttendeeTicket();
$ticket->setTicketType(self::$default_ticket_type);
$order->addTicket($ticket);
$ticket->activate();
$ticket->generateNumber();
$ticket->generateQRCode();
self::$summit->addOrder($order);
self::$em->persist($order);
self::$em->flush();
$order->setPaid();
self::$default_prepaid_discount_code->clearTickets();
self::$default_prepaid_discount_code->addTicket($ticket);
self::$em->persist(self::$default_prepaid_discount_code);
self::$em->persist($order);
self::$em->flush();
$service = App::make(ISummitOrderService::class);
$payload = [
"owner_email" => $owner->getEmail(),
"owner_first_name" => $owner->getFirstName(),
"owner_last_name" => $owner->getLastName(),
"owner_company" => $owner->getCompany(),
"tickets" => [
[
"type_id" => self::$default_ticket_type->getId(),
"promo_code" => self::$default_prepaid_discount_code->getCode(),
// Attendee is a different person than the order owner -
// this is the scenario AutoAssignPrePaidTicketTask exists for.
"attendee_company" => "Attendee Co",
"attendee_first_name" => "Jane",
"attendee_last_name" => "Doe",
],
]
];
$result_order = $service->reserve($owner, self::$summit, $payload);
$attendee = $result_order->getTickets()->first()->getOwner();
$this->assertEquals("Attendee Co", $attendee->getCompanyName());
$this->assertEquals("Jane", $attendee->getFirstName());
$this->assertEquals("Doe", $attendee->getSurname());
}It doesn't reuse the base fixture's order #0 or touch the three pre-existing |
||
|
|
||
| $attendee_email = $this->owner->getEmail(); | ||
|
|
@@ -1558,7 +1567,7 @@ function () use ($promo_code_val, $type_id) { | |
| if (empty($attendee_last_name)) | ||
| $attendee_last_name = $this->payload['owner_last_name'] ?? $this->owner->getLastName(); | ||
|
|
||
| $promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_val); | ||
| if (!PromoCodesUtils::isPrePaidPromoCode($promo_code)) | ||
| throw new EntityNotFoundException("Promo code is not found."); | ||
|
|
||
|
|
@@ -1661,7 +1670,7 @@ function () use ($promo_code_val, $type_id) { | |
|
|
||
|
|
||
| return $order; | ||
| }, 30); | ||
|
Collaborator
There was a problem hiding this comment. @romanetar This 30 s lifetime makes TTL expiry a silent loss of mutual exclusion, and this call site is the only one of the four where the Redis lock is the sole guard of the invariant — the fix should be a DB pessimistic lock, matching the sibling tasks. The other three sites are already backed by row locks — Suggested fix — same pattern as Independently,
Collaborator
Author
There was a problem hiding this comment. Verified both points against the code and applied fixes for both (will push shortly). DB pessimistic lock for
Ran |
||
| return ['order' => $order]; | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,9 +239,10 @@ public function storeHash($name, array $values, $ttl = 0) | |
| public function incCounter($counter_name, $ttl = 0) | ||
| { | ||
| return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) { | ||
| if ($ttl > 0) { | ||
| if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1; | ||
| } else { | ||
| if ($conn->set($counter_name, 1, 'NX') !== null) return 1; | ||
| } | ||
| return (int)$conn->incr($counter_name); | ||
| }, 0); | ||
|
|
@@ -306,12 +307,11 @@ public function setSingleValue($key, $value, $ttl = 0) | |
| public function addSingleValue($key, $value, $ttl = 0) | ||
| { | ||
| return $this->retryOnConnectionError(function ($conn) use ($key, $value, $ttl) { | ||
| if ($ttl > 0) { | ||
| return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null; | ||
|
Collaborator
There was a problem hiding this comment. @romanetar
Concretely: This isn't a new observation — it's the first point of an existing, still-open review comment on this same line (driver-compatibility, "If the driver is ever switched to PhpRedis, Verified directly: One mitigating note: I checked Suggested fix — treat both driver's failure sentinels the same way, consistent for - if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1;
+ if ($this->setNxSucceeded($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX'))) return 1;
...
- if ($conn->set($counter_name, 1, 'NX') !== null) return 1;
+ if ($this->setNxSucceeded($conn->set($counter_name, 1, 'NX'))) return 1;
...
- return $conn->set($key, $value, 'EX', (int)$ttl, 'NX') !== null;
+ return $this->setNxSucceeded($conn->set($key, $value, 'EX', (int)$ttl, 'NX'));
...
- return $conn->set($key, $value, 'NX') !== null;
+ return $this->setNxSucceeded($conn->set($key, $value, 'NX'));with a small private helper |
||
| } | ||
| return $conn->set($key, $value, 'NX') !== null; | ||
| }, false); | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Missing integration test: atomic All tests for this path mock
Recommend a
Collaborator
There was a problem hiding this comment. Reopening — this thread has two distinct asks and only one was addressed. Point 2 (atomicity / TTL-in-one-command integration coverage) was fixed and is covered by the I verified point 1 is a real, currently-live bug in the code as it stands today: under @romanetar — see that comment for the fix; leaving this one open until it's addressed there. |
||
| public function setKeyExpiration($key, $ttl) | ||
|
|
@@ -331,7 +331,21 @@ public function ttl($key) | |
| return (int)$conn->ttl($key); | ||
| }, 0); | ||
| } | ||
|
|
||
| public function deleteIfValueMatches(string $key, string $expectedValue): bool | ||
| { | ||
| $lua = <<<'LUA' | ||
| if redis.call('get', KEYS[1]) == ARGV[1] then | ||
| return redis.call('del', KEYS[1]) | ||
| else | ||
| return 0 | ||
| end | ||
| LUA; | ||
| return $this->retryOnConnectionError(function ($conn) use ($lua, $key, $expectedValue) { | ||
| return (int)$conn->eval($lua, 1, $key, $expectedValue) === 1; | ||
| }, false); | ||
| } | ||
|
|
||
| /** | ||
| * @param string $cache_region_key | ||
| * @return void | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romanetar this is a CI coverage regression. These entries changed from PHPUnit path arguments to
--filter tests/..., but PHPUnit--filtermatches test/class names, not directories. As a result, the Services and Repositories jobs now execute zero tests while still exiting 0.I reproduced it with:
vendor/bin/phpunit --filter tests/Unit/Services/ --do-not-cache-resultvendor/bin/phpunit --filter tests/Repositories/ --do-not-cache-resultBoth print
No tests executed!and exit 0. Please keeptests/Unit/Services/andtests/Repositories/as path arguments, and addtests/Integration/as a path argument too.