Skip to content

Commit e1a0952

Browse files
committed
fix: review feedback
Signed-off-by: romanetar <roman_ag@hotmail.com>
1 parent c6e6473 commit e1a0952

6 files changed

Lines changed: 171 additions & 77 deletions

File tree

‎app/Services/Model/Imp/SummitOrderService.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -827,7 +827,7 @@ public function run(array $formerState): array
827827

828828
$this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $qty, $owner_email) {
829829
$promo_code->addUsage($owner_email, $qty);
830-
});
830+
}, 30);
831831

832832
});
833833
// mark a done
@@ -865,7 +865,7 @@ public function undo()
865865

866866
$this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $info, $owner_email) {
867867
$promo_code->removeUsage(intval($info['qty']), $owner_email);
868-
});
868+
}, 30);
869869

870870
});
871871
}
@@ -950,7 +950,7 @@ public function run(array $formerState): array
950950

951951
$this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $reservations) {
952952
$ticket_type->sell($reservations[$ticket_type->getId()]);
953-
});
953+
}, 30);
954954

955955
}
956956
});
@@ -967,7 +967,7 @@ public function undo()
967967
if (is_null($ticket_type)) return;
968968
$this->lock_service->lock('ticket_type.' . $ticket_type->getId() . '.sell.lock', function () use ($ticket_type, $qty) {
969969
$ticket_type->restore($qty);
970-
});
970+
}, 30);
971971
});
972972
}
973973
}
@@ -1536,7 +1536,7 @@ public function run(array $formerState): array
15361536
if (empty($promo_code_val)) throw new ValidationException("Promo code is required.");
15371537

15381538
$type_id = $ticket_dto['type_id'];
1539-
$order = $this->lock_service->lock('ticket_type.' . $type_id . 'promo_code.' . $promo_code_val . '.sell.lock',
1539+
$order = $this->lock_service->lock('ticket_type.' . $type_id . '.promo_code.' . $promo_code_val . '.sell.lock',
15401540
function () use ($promo_code_val, $type_id) {
15411541

15421542
$attendee_email = $this->owner->getEmail();
@@ -1658,7 +1658,7 @@ function () use ($promo_code_val, $type_id) {
16581658

16591659

16601660
return $order;
1661-
});
1661+
}, 30);
16621662
return ['order' => $order];
16631663
});
16641664
}

‎app/Services/Utils/ILockManagerService.php‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,20 +24,21 @@ interface ILockManagerService
2424
* @param string $name
2525
* @param int $lifetime
2626
* @throws UnacquiredLockException
27-
* @return mixed
27+
* @return string ownership token — must be passed to releaseLock
2828
*/
29-
public function acquireLock(string $name,int $lifetime = self::DefaultLifetime);
29+
public function acquireLock(string $name, int $lifetime = self::DefaultLifetime): string;
30+
3031
/**
31-
* @param string $name
32-
* @return mixed
32+
* @param string $name
33+
* @param string $token ownership token returned by acquireLock
3334
*/
34-
public function releaseLock(string $name);
35+
public function releaseLock(string $name, string $token): void;
3536

3637
/**
3738
* @param string $name
3839
* @param Closure $callback
3940
* @param int $lifetime
4041
* @return mixed
4142
*/
42-
public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime);
43+
public function lock(string $name, Closure $callback, int $lifetime = self::DefaultLifetime): mixed;
4344
}

‎app/Services/Utils/LockManagerService.php‎

Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,6 @@ final class LockManagerService implements ILockManagerService {
3131
*/
3232
private $cache_service;
3333

34-
/** @var array<string,string> lock-name → per-call ownership token */
35-
private array $tokens = [];
36-
3734
/**
3835
* LockManagerService constructor.
3936
* @param ICacheService $cache_service
@@ -45,19 +42,18 @@ public function __construct(ICacheService $cache_service){
4542
/**
4643
* @param string $name
4744
* @param int $lifetime
48-
* @return LockManagerService
45+
* @return string ownership token — pass to releaseLock
4946
* @throws UnacquiredLockException
5047
*/
51-
public function acquireLock(string $name, int $lifetime = 3600):LockManagerService
48+
public function acquireLock(string $name, int $lifetime = 3600): string
5249
{
5350
Log::debug(sprintf("LockManagerService::acquireLock name %s lifetime %s", $name, $lifetime));
5451
$token = bin2hex(random_bytes(16));
5552
$attempt = 0;
5653
do {
5754
$success = $this->cache_service->addSingleValue($name, $token, $lifetime);
5855
if ($success) {
59-
$this->tokens[$name] = $token;
60-
return $this;
56+
return $token;
6157
}
6258
$wait_interval = (int)(self::BackOffBaseInterval * (self::BackOffMultiplier ** $attempt));
6359
Log::debug(sprintf("LockManagerService::acquireLock name %s retrying in %s µs (attempt %s)", $name, $wait_interval, $attempt));
@@ -72,36 +68,30 @@ public function acquireLock(string $name, int $lifetime = 3600):LockManagerServi
7268

7369
/**
7470
* @param string $name
75-
* @return $this
71+
* @param string $token ownership token returned by acquireLock
7672
*/
77-
public function releaseLock(string $name):LockManagerService
73+
public function releaseLock(string $name, string $token): void
7874
{
7975
Log::debug(sprintf("LockManagerService::releaseLock name %s", $name));
80-
if (!isset($this->tokens[$name])) {
81-
return $this;
82-
}
83-
$this->cache_service->deleteIfValueMatches($name, $this->tokens[$name]);
84-
unset($this->tokens[$name]);
85-
return $this;
76+
$this->cache_service->deleteIfValueMatches($name, $token);
8677
}
8778

8879
/**
8980
* @param string $name
9081
* @param Closure $callback
9182
* @param int $lifetime
92-
* @return null
83+
* @return mixed
9384
* @throws UnacquiredLockException
9485
* @throws Exception
9586
*/
96-
public function lock(string $name, Closure $callback, int $lifetime = 3600)
87+
public function lock(string $name, Closure $callback, int $lifetime = 3600): mixed
9788
{
98-
$result = null;
99-
$acquired = false;
89+
$token = null;
90+
$result = null;
10091
Log::debug(sprintf("LockManagerService::lock name %s lifetime %s", $name, $lifetime));
10192

10293
try {
103-
$this->acquireLock($name, $lifetime);
104-
$acquired = true;
94+
$token = $this->acquireLock($name, $lifetime);
10595
Log::debug(sprintf("LockManagerService::lock name %s calling callback", $name));
10696
$result = $callback($this);
10797
}
@@ -114,8 +104,8 @@ public function lock(string $name, Closure $callback, int $lifetime = 3600)
114104
throw $ex;
115105
}
116106
finally {
117-
if ($acquired) {
118-
$this->releaseLock($name);
107+
if ($token !== null) {
108+
$this->releaseLock($name, $token);
119109
}
120110
}
121111
return $result;

‎app/Services/Utils/RedisCacheService.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,7 @@ public function storeHash($name, array $values, $ttl = 0)
239239
public function incCounter($counter_name, $ttl = 0)
240240
{
241241
return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) {
242-
if ($conn->set($counter_name, 1, ['NX' => true]) !== null) {
243-
if ($ttl > 0) $conn->expire($counter_name, (int)$ttl);
242+
if ($conn->set($counter_name, 1, ['EX' => (int)$ttl, 'NX' => true]) !== null) {
244243
return 1;
245244
}
246245
return (int)$conn->incr($counter_name);
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
<?php namespace Tests\Integration;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use Illuminate\Support\Facades\Redis;
16+
use PHPUnit\Framework\Attributes\Group;
17+
use services\utils\RedisCacheService;
18+
use Tests\CreatesApplication;
19+
use Tests\TestCase;
20+
21+
/**
22+
* Integration tests for RedisCacheService::addSingleValue.
23+
*
24+
* These tests require a live Redis instance and verify two properties that
25+
* mocks cannot exercise:
26+
*
27+
* 1. Driver compatibility — the variadic SET NX EX form works with the
28+
* configured Predis/PhpRedis driver. If the driver is switched to
29+
* PhpRedis, set() returns false on an NX-miss (not null), which would
30+
* silently break the `!== null` check; this test catches that regression.
31+
*
32+
* 2. Atomicity — key and TTL are written in a single command; there is no
33+
* window where the key exists without a TTL. Verified by reading TTL
34+
* immediately after addSingleValue returns.
35+
*
36+
*/
37+
#[Group("integration")]
38+
final class RedisCacheServiceAddSingleValueTest extends TestCase
39+
{
40+
use CreatesApplication;
41+
42+
private const TEST_KEY = 'test:add_single_value:lock';
43+
private const TTL = 30;
44+
45+
private RedisCacheService $service;
46+
private mixed $redis;
47+
48+
protected function setUp(): void
49+
{
50+
parent::setUp();
51+
$this->redis = Redis::connection();
52+
$this->service = new RedisCacheService();
53+
// Start clean regardless of any leftover from a previous failed run.
54+
$this->redis->del(self::TEST_KEY);
55+
}
56+
57+
protected function tearDown(): void
58+
{
59+
$this->redis->del(self::TEST_KEY);
60+
parent::tearDown();
61+
}
62+
63+
/**
64+
* First call must succeed and leave a TTL on the key.
65+
* Second call on the same key must return false (NX semantics).
66+
*/
67+
public function testAddSingleValueSetsKeyWithTtlAndNxSemanticsHold(): void
68+
{
69+
$token = bin2hex(random_bytes(16));
70+
71+
$acquired = $this->service->addSingleValue(self::TEST_KEY, $token, self::TTL);
72+
$this->assertTrue($acquired, 'first addSingleValue must return true');
73+
74+
// Atomicity: TTL must already be set — no gap between key write and expire.
75+
$ttl = (int)$this->redis->ttl(self::TEST_KEY);
76+
$this->assertGreaterThanOrEqual(1, $ttl, 'key must have a positive TTL immediately after addSingleValue');
77+
$this->assertLessThanOrEqual(self::TTL, $ttl, 'TTL must not exceed the requested lifetime');
78+
79+
// NX semantics: a second call while the key still exists must fail.
80+
$again = $this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL);
81+
$this->assertFalse($again, 'addSingleValue must return false when key already exists (NX)');
82+
}
83+
84+
/**
85+
* After the key is deleted the lock can be re-acquired, confirming the
86+
* return-value contract holds across both the true and false branches.
87+
*/
88+
public function testAddSingleValueReturnsTrueAfterKeyIsDeleted(): void
89+
{
90+
$token = bin2hex(random_bytes(16));
91+
92+
$this->assertTrue($this->service->addSingleValue(self::TEST_KEY, $token, self::TTL));
93+
$this->redis->del(self::TEST_KEY);
94+
$this->assertTrue(
95+
$this->service->addSingleValue(self::TEST_KEY, bin2hex(random_bytes(16)), self::TTL),
96+
'addSingleValue must return true once the key has been removed'
97+
);
98+
}
99+
}

0 commit comments

Comments
 (0)