Skip to content

fix(lock): implement Redlock single-instance pattern in LockManagerService #537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
smarcet merged 10 commits into from
Aug 31, 2026
Merged

fix(lock): implement Redlock single-instance pattern in LockManagerService #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3f6f8b0
fix(lock): implement Redlock single-instance pattern in LockManagerSe…
romanetar May 4, 2026
face963
fix: review feedback
romanetar Jun 15, 2026
3edd45a
fix: PR review feedback
romanetar Aug 3, 2026
ab4a59a
fix(orders): capture ticket_dto in AutoAssignPrePaidTicketTask closure
romanetar Aug 10, 2026
46ea9b3
fix(cache): detect SET...NX miss consistently across Predis/PhpRedis
romanetar Aug 10, 2026
03c5e8f
test(lock): assert deleteIfValueMatches receives the exact acquired t…
romanetar Aug 10, 2026
59f19fc
fix(test): remove redundant CreatesApplication trait usage
romanetar Aug 17, 2026
3c49aa0
fix(sponsor-users): serialize addSponsorUser/addSponsorUserToGroup pe…
romanetar Aug 17, 2026
2d598eb
fix(ci): run Repositories and Services suites by path so PHPUnit exec…
smarcet Aug 31, 2026
290357f
fix(sponsor-users): raise pair-lock lifetime above the IDP call timeout
smarcet Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .github/workflows/push.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,17 @@ jobs:
- { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" }
- { name: "AuditEventTypesTest", filter: "--filter AuditEventTypesTest" }
- { name: "GuzzleTracingTest", filter: "--filter GuzzleTracingTest" }
- { name: "Repositories", filter: "tests/Repositories/" }
- { name: "Services", filter: "tests/Unit/Services/" }
- { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" }
# Named by path because no job in this matrix runs the tests/ root, only its
# subdirectories - a file added there runs nowhere unless it is listed here.
- { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php tests/PresentationSubmissionReopenedEmailTest.php" }
- { name: "Repositories", filter: "tests/Repositories/" }
- { name: "Services", filter: "tests/Unit/Services/" }
- { name: "Integration", filter: "tests/Integration/" }
# Runs the same suite under PhpRedis - addSingleValue/incCounter's
# SET...NX miss sentinel differs between drivers (null vs false),
# so a driver-agnostic bug there only shows up here.
- { name: "IntegrationPhpRedis", filter: "tests/Integration/", redis_client: "phpredis" }
env:
OTEL_SERVICE_ENABLED: false
APP_ENV: testing
Expand Down Expand Up @@ -187,6 +192,8 @@ jobs:
COMPOSER_AUTH: '{"github-oauth": {"github.com": "${{ secrets.COMPOSER_AUTH_TOKEN }}"} }'

- name: Run ${{ matrix.suite.name }}
env:
REDIS_CLIENT: ${{ matrix.suite.redis_client || 'predis' }}
run: |
./update_doctrine.sh
php artisan db:create_initial_db --schema=config
Expand Down
10 changes: 10 additions & 0 deletions Libs/Utils/ICacheService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@ public function setSingleValue($key, $value, $ttl = 0);
*/
public function addSingleValue($key, $value, $ttl = 0);

/**
* Atomically compare-and-delete: DEL the key only when its current value
* equals $expectedValue. Implementations MUST use an atomic operation
* (Lua EVAL or equivalent) — never a separate GET + conditional DEL.
* @param string $key
* @param string $expectedValue
* @return bool true iff the key existed, matched, and was deleted
*/
public function deleteIfValueMatches(string $key, string $expectedValue): bool;

/**
* Set time to live to a given key
* @param $key
Expand Down
205 changes: 135 additions & 70 deletions app/Services/Model/Imp/SponsorUserSyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
use App\Services\Model\AbstractService;
use App\Services\Model\IMemberService;
use App\Services\Model\ISponsorUserSyncService;
use App\Services\Utils\Exceptions\UnacquiredLockException;
use App\Services\Utils\ILockManagerService;
use Illuminate\Support\Facades\Log;
use LaravelDoctrine\ORM\Facades\Registry;
use libs\utils\ITransactionService;
Expand Down Expand Up @@ -53,6 +55,21 @@ final class SponsorUserSyncService

private ISponsorRepository $sponsor_repository;

private ILockManagerService $lock_service;

/**
* Lifetime, in seconds, for the sponsor_user.*.ext_*.lock held around
* addSponsorUser / addSponsorUserToGroup. Both paths resolve the member
* INSIDE the lock, which for a brand-new user makes a synchronous IDP
* call (registerExternalUserById) bounded by curl.timeout (60s default) -
* so the lifetime must exceed that, or a slow IDP dissolves the lock right
* as the competing MQ job's first retry lands (RetryBackoff starts at 30s).
* It must also stay below the job's LAST retry (+150s with tries=3 and
* backoff '30,120'), so a worker killed mid-callback leaves a lock that
* expires before the final retry rather than burning it too.
*/
private const SponsorUserLockLifetime = 120;

/**
* SponsorUserSyncService constructor.
* @param ISummitRepository $summit_repository
Expand All @@ -63,6 +80,7 @@ final class SponsorUserSyncService
* @param IExternalUserApi $external_user_api
* @param ISponsorRepository $sponsor_repository
* @param ITransactionService $tx_service
* @param ILockManagerService $lock_service
*/
public function __construct
(
Expand All @@ -73,7 +91,8 @@ public function __construct
IMemberService $member_service,
IExternalUserApi $external_user_api,
ISponsorRepository $sponsor_repository,
ITransactionService $tx_service
ITransactionService $tx_service,
ILockManagerService $lock_service
)
{
parent::__construct($tx_service);
Expand All @@ -84,6 +103,24 @@ public function __construct
$this->member_service = $member_service;
$this->external_user_api = $external_user_api;
$this->sponsor_repository = $sponsor_repository;
$this->lock_service = $lock_service;
}

/**
* Lock key guarding a (sponsor, external user) pair against the two MQ
* consumers (AddSponsorMemberMQJob / UpdateSponsorMemberGroupsMQJob) that
* can both observe a missing Sponsor_Users row and both insert one.
* Built from ids already present on both entry points' signatures - it
* must NOT depend on the local MemberID, since resolving that is part of
* what the lock protects.
*
* @param int $sponsor_id
* @param int $user_id external (IDP) user id
* @return string
*/
private function sponsorUserLockKey(int $sponsor_id, int $user_id): string
{
return "sponsor_user.{$sponsor_id}.ext_{$user_id}.lock";
}

/**
Expand Down Expand Up @@ -214,26 +251,40 @@ public function validateParams(int $summit_id, int $user_id): array

/**
* @inheritDoc
* @throws UnacquiredLockException propagated on purpose - same as any
* other failure here, it must burn one of the MQ job's retries rather
* than be swallowed (see the note below).
*/
public function addSponsorUser(int $summit_id, int $sponsor_id, int $user_id): void
{
// Do NOT swallow failures here: the MQ job (tries = 3) needs the
// exception to apply its retry / failed_jobs machinery. A swallowed
// failure loses the membership event silently.
// failure loses the membership event silently. This also applies to
// UnacquiredLockException from the lock below.
Log::debug(
"SponsorUserSyncService::addSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}");

list($summit, $member) = $this->validateParams($summit_id, $user_id);
// summit_sponsor_service->addSponsorUser already opens and commits its
// own transaction, so no transaction is needed at this level: by the
// time the lock callback returns, the insert is committed and visible.
$this->lock_service->lock(
$this->sponsorUserLockKey($sponsor_id, $user_id),
function () use ($summit_id, $sponsor_id, $user_id) {

Log::debug(
"SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}");
list($summit, $member) = $this->validateParams($summit_id, $user_id);

Log::debug(
"SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}");

$member = $this->ensureSponsorGroupMembership($member, $user_id);
$member = $this->ensureSponsorGroupMembership($member, $user_id);

$this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId());
$this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId());

Log::info(
"SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}");
Log::info(
"SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}");
},
self::SponsorUserLockLifetime
);
}

/**
Expand Down Expand Up @@ -297,6 +348,8 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id

/**
* @inheritDoc
* @throws UnacquiredLockException propagated on purpose, same as any
* other failure in this path (see addSponsorUser).
*/
public function addSponsorUserToGroup(int $user_id, string $group_slug, int $sponsor_id, int $summit_id): void
{
Expand All @@ -318,67 +371,79 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo
"Sponsor {$sponsor_id} does not belong to summit {$summit_id}.");
}

// Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below.
// registerExternalUserById opens its own transaction and dispatches NewMember /
// MemberDataUpdatedExternally right after it. Those jobs are pushed immediately:
// afterCommit only defers dispatch for Eloquent-managed transactions, and this
// service uses the Doctrine DBAL connection directly. Keeping the registration
// outside guarantees the Member row is committed before any job references its id.
$member_id = $this->resolveMember($user_id)->getId();

$this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) {

// Re-load inside the transaction: the tx service may have reset the entity
// manager, which would leave an entity resolved outside it detached.
$member = $this->member_repository->getById($member_id);
if (!$member instanceof Member) {
throw new EntityNotFoundException("Member with id {$member_id} not found");
}

// Grant the global group FIRST: Sponsor::addUser (reached through the
// eager-create path below) validates the member already belongs to a
// sponsor group, and for a brand-new sponsor user this very event is
// what delivers that group.
if (!$member->belongsToGroup($group_slug)) {
$group = $this->group_repository->getBySlug($group_slug);
if (is_null($group)) {
throw new EntityNotFoundException("Group {$group_slug} not found");
}
$member->add2Group($group);
}

// Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair.
// If the row does not exist yet (MQ ordering race: group event arrived before membership
// event), create it eagerly so the permission is never silently dropped.
if ($member->addSponsorPermission($sponsor_id, $group_slug) === 0) {
Log::warning(
"SponsorUserSyncService::addSponsorUserToGroup no Sponsor_Users row found for " .
"member {$member->getId()} / sponsor {$sponsor_id} — creating it eagerly");

$summit = $this->summit_repository->getById($summit_id);
if (!$summit instanceof Summit) {
throw new EntityNotFoundException("Summit {$summit_id} not found");
}

$this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId());

// Flush the UoW so the INSERT is visible to the raw SQL retry
// on the same connection within the active transaction.
Registry::getManager(SilverstripeBaseModel::EntityManager)->flush();

// Retry now that the row exists.
$retryResult = $member->addSponsorPermission($sponsor_id, $group_slug);
if ($retryResult === 0) {
throw new \RuntimeException(
"Failed to write permission after eager Sponsor_Users creation " .
"for member {$member->getId()} / sponsor {$sponsor_id}"
);
}
}

Log::info(
"SponsorUserSyncService::addSponsorUserToGroup member {$member->getId()} added to group {$group_slug} via sponsor {$sponsor_id}");
});
// Lock held OUTSIDE the transaction below: releasing it before commit
// would let a waiting AddSponsorMemberMQJob/UpdateSponsorMemberGroupsMQJob
// observe the row as still-missing and insert its own duplicate.
$this->lock_service->lock(
$this->sponsorUserLockKey($sponsor_id, $user_id),
function () use ($user_id, $group_slug, $sponsor_id, $summit_id) {

// Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below,
// but INSIDE the lock - resolving the local MemberID is part of what the lock
// protects. registerExternalUserById opens its own transaction and dispatches
// NewMember / MemberDataUpdatedExternally right after it. Those jobs are pushed
// immediately: afterCommit only defers dispatch for Eloquent-managed transactions,
// and this service uses the Doctrine DBAL connection directly. Keeping the
// registration outside the transaction guarantees the Member row is committed
// before any job references its id.
$member_id = $this->resolveMember($user_id)->getId();

$this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) {

// Re-load inside the transaction: the tx service may have reset the entity
// manager, which would leave an entity resolved outside it detached.
$member = $this->member_repository->getById($member_id);
if (!$member instanceof Member) {
throw new EntityNotFoundException("Member with id {$member_id} not found");
}

// Grant the global group FIRST: Sponsor::addUser (reached through the
// eager-create path below) validates the member already belongs to a
// sponsor group, and for a brand-new sponsor user this very event is
// what delivers that group.
if (!$member->belongsToGroup($group_slug)) {
$group = $this->group_repository->getBySlug($group_slug);
if (is_null($group)) {
throw new EntityNotFoundException("Group {$group_slug} not found");
}
$member->add2Group($group);
}

// Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair.
// If the row does not exist yet (MQ ordering race: group event arrived before membership
// event), create it eagerly so the permission is never silently dropped.
if ($member->addSponsorPermission($sponsor_id, $group_slug) === 0) {
Log::warning(
"SponsorUserSyncService::addSponsorUserToGroup no Sponsor_Users row found for " .
"member {$member->getId()} / sponsor {$sponsor_id} — creating it eagerly");

$summit = $this->summit_repository->getById($summit_id);
if (!$summit instanceof Summit) {
throw new EntityNotFoundException("Summit {$summit_id} not found");
}

$this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId());

// Flush the UoW so the INSERT is visible to the raw SQL retry
// on the same connection within the active transaction.
Registry::getManager(SilverstripeBaseModel::EntityManager)->flush();

// Retry now that the row exists.
$retryResult = $member->addSponsorPermission($sponsor_id, $group_slug);
if ($retryResult === 0) {
throw new \RuntimeException(
"Failed to write permission after eager Sponsor_Users creation " .
"for member {$member->getId()} / sponsor {$sponsor_id}"
);
}
}

Log::info(
"SponsorUserSyncService::addSponsorUserToGroup member {$member->getId()} added to group {$group_slug} via sponsor {$sponsor_id}");
});
},
self::SponsorUserLockLifetime
);
}

/**
Expand Down
Loading
Loading