Skip to content

Commit ec9f730

Browse files
caseylockerclaude
caseylocker
and
claude
committed
fix(promo-codes): close reservation counter leak on cancel + fix undo retry
Two bugs found by Codex review of the TOCTOU fix: 1. restoreTicketsPromoCodes (cancel/refund path) called removeUsage() on the promo code but never decremented the per-member reservation counter (SummitPromoCodeMemberReservation.QtyUsed). After a legitimate cancellation, discovery showed quota available but checkout rejected on the stale counter. Fix: add ?Member $owner parameter, decrement the reservation row after successful removeUsage() in the same try block. Non-ValidationException errors from the reservation path propagate and roll back the transaction. All 4 call sites updated. 2. PreProcessReservationTask::undo() set $undone=true before the loop, making partial failures unrecoverable — failed entries were cleared from $reserved unconditionally. Fix: build a $remaining list of failed entries, set $reserved=$remaining and $undone=empty($remaining) so a retry re-processes only the failed codes. New test file RestorePathReservationTest (6 cases) exercises the real restoreTicketsPromoCodes path via reflection: successful decrement, guest order skip, missing reservation row skip, over-decrement clamp, and non-ValidationException propagation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent 999caa1 commit ec9f730

2 files changed

Lines changed: 269 additions & 11 deletions

File tree

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

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,17 +1236,19 @@ protected function getTicketType(int $type_id): SummitTicketType
12361236
public function undo()
12371237
{
12381238
if ($this->undone) return;
1239-
$this->undone = true;
12401239

12411240
if (empty($this->reserved)
12421241
|| is_null($this->owner)
12431242
|| is_null($this->promo_code_repository)
12441243
|| is_null($this->member_reservation_repository)
12451244
|| is_null($this->tx_service)
12461245
) {
1246+
$this->undone = true;
12471247
return;
12481248
}
12491249

1250+
$remaining = [];
1251+
12501252
foreach ($this->reserved as $entry) {
12511253
$code_value = $entry['code'];
12521254
$qty = intval($entry['qty']);
@@ -1263,16 +1265,16 @@ public function undo()
12631265
$reservation->decrement($qty);
12641266
});
12651267
} catch (\Throwable $ex) {
1266-
// Undo is best-effort; log and continue so the remaining
1267-
// reservations for other codes in this saga still get released.
12681268
Log::warning(sprintf(
12691269
"PreProcessReservationTask::undo failed to release %s × %s: %s",
12701270
$code_value, $qty, $ex->getMessage()
12711271
));
1272+
$remaining[] = $entry;
12721273
}
12731274
}
12741275

1275-
$this->reserved = [];
1276+
$this->reserved = $remaining;
1277+
$this->undone = empty($remaining);
12761278
}
12771279
}
12781280

@@ -2772,7 +2774,7 @@ public function cancel(Summit $summit, string $order_hash): SummitOrder
27722774

27732775
list($tickets_to_return, $promo_codes_to_return) = $order->calculateTicketsAndPromoCodesToReturn();
27742776

2775-
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return);
2777+
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return, $order->getOwner());
27762778

27772779
$order->setCancelled();
27782780

@@ -2782,11 +2784,12 @@ public function cancel(Summit $summit, string $order_hash): SummitOrder
27822784

27832785
/**
27842786
* @param Summit $summit
2785-
* @param $tickets_to_return
2786-
* @param $promo_codes_to_return
2787+
* @param array $tickets_to_return
2788+
* @param array $promo_codes_to_return
2789+
* @param Member|null $owner
27872790
* @return void
27882791
*/
2789-
private function restoreTicketsPromoCodes(Summit $summit, $tickets_to_return, $promo_codes_to_return): void
2792+
private function restoreTicketsPromoCodes(Summit $summit, array $tickets_to_return, array $promo_codes_to_return, ?Member $owner): void
27902793
{
27912794

27922795
// restore tickets and promo-codes
@@ -2822,6 +2825,14 @@ private function restoreTicketsPromoCodes(Summit $summit, $tickets_to_return, $p
28222825
Log::debug(sprintf("SummitOrderService::restoreTicketsPromoCodes compensating promo code %s on %s usages", $code, $qty));
28232826
try {
28242827
$promo_code->removeUsage($qty, $value["owner_email"]);
2828+
2829+
if (!is_null($owner)) {
2830+
$reservation = $this->member_reservation_repository
2831+
->getByPromoCodeAndMember($promo_code, $owner);
2832+
if (!is_null($reservation)) {
2833+
$reservation->decrement($qty);
2834+
}
2835+
}
28252836
} catch (ValidationException $ex) {
28262837
Log::warning($ex);
28272838
}
@@ -2945,7 +2956,7 @@ public function confirmOrdersOlderThanNMinutes(int $minutes, int $max = 100): vo
29452956

29462957
list($tickets_to_return, $promo_codes_to_return) = $order->calculateTicketsAndPromoCodesToReturn();
29472958

2948-
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return);
2959+
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return, $order->getOwner());
29492960

29502961
$order->setCancelled();
29512962
}
@@ -3066,7 +3077,7 @@ public function processOrder2Revoke(int $order_id): void
30663077

30673078
list($tickets_to_return, $promo_codes_to_return) = $order->calculateTicketsAndPromoCodesToReturn();
30683079

3069-
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return);
3080+
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return, $order->getOwner());
30703081

30713082
$order->setCancelled();
30723083

@@ -3266,7 +3277,7 @@ public function deleteOrder(Summit $summit, int $order_id)
32663277
$ticket->setCancelled();
32673278
}
32683279

3269-
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return);
3280+
$this->restoreTicketsPromoCodes($summit, $tickets_to_return, $promo_codes_to_return, $order->getOwner());
32703281

32713282
$summit->removeOrder($order);
32723283

‎tests/Unit/Services/RestorePathReservationTest.php‎

Lines changed: 247 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,247 @@
1+
<?php namespace Tests\Unit\Services;
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 App\Models\Foundation\Summit\Registration\IBuildDefaultPaymentGatewayProfileStrategy;
16+
use App\Models\Foundation\Summit\Repositories\ISummitAttendeeBadgePrintRuleRepository;
17+
use App\Models\Foundation\Summit\Repositories\ISummitAttendeeBadgeRepository;
18+
use App\Models\Foundation\Summit\Repositories\ISummitOrderRepository;
19+
use App\Models\Foundation\Summit\Repositories\ISummitRefundRequestRepository;
20+
use App\Services\FileSystem\IFileDownloadStrategy;
21+
use App\Services\FileSystem\IFileUploadStrategy;
22+
use App\Services\Model\ICompanyService;
23+
use App\Services\Model\IMemberService;
24+
use App\Services\Model\Strategies\TicketFinder\ITicketFinderStrategyFactory;
25+
use App\Services\Model\SummitOrderService;
26+
use App\Services\Utils\ILockManagerService;
27+
use libs\utils\ITransactionService;
28+
use models\main\ICompanyRepository;
29+
use models\main\IMemberRepository;
30+
use models\main\ITagRepository;
31+
use models\main\Member;
32+
use models\summit\ISummitAttendeeRepository;
33+
use models\summit\ISummitAttendeeTicketRepository;
34+
use models\summit\ISummitPromoCodeMemberReservationRepository;
35+
use models\summit\ISummitRegistrationPromoCodeRepository;
36+
use models\summit\ISummitRepository;
37+
use models\summit\ISummitTicketTypeRepository;
38+
use models\summit\Summit;
39+
use models\summit\SummitPromoCodeMemberReservation;
40+
use models\summit\SummitRegistrationPromoCode;
41+
use Mockery;
42+
use PHPUnit\Framework\TestCase;
43+
44+
/**
45+
* Verifies that restoreTicketsPromoCodes (the cancel/refund path)
46+
* decrements SummitPromoCodeMemberReservation.QtyUsed alongside the
47+
* existing promo-code removeUsage() call.
48+
*
49+
* Uses reflection on the private method — building a full cancel()
50+
* fixture would require mocking the order repository, payment gateway,
51+
* and order state machine, which is outside the scope of this fix.
52+
*
53+
* @package Tests\Unit\Services
54+
*/
55+
class RestorePathReservationTest extends TestCase
56+
{
57+
protected function setUp(): void
58+
{
59+
parent::setUp();
60+
\Illuminate\Support\Facades\Facade::clearResolvedInstances();
61+
$container = new \Illuminate\Container\Container();
62+
$container->instance('app', $container);
63+
$container->instance('log', new class {
64+
public function __call($name, $args) { /* swallow */ }
65+
});
66+
\Illuminate\Support\Facades\Facade::setFacadeApplication($container);
67+
}
68+
69+
protected function tearDown(): void
70+
{
71+
\Illuminate\Support\Facades\Facade::clearResolvedInstances();
72+
\Illuminate\Support\Facades\Facade::setFacadeApplication(null);
73+
Mockery::close();
74+
parent::tearDown();
75+
}
76+
77+
private function buildService(
78+
ISummitRegistrationPromoCodeRepository $promoRepo,
79+
ISummitPromoCodeMemberReservationRepository $reservationRepo
80+
): SummitOrderService {
81+
return new SummitOrderService(
82+
Mockery::mock(ISummitTicketTypeRepository::class),
83+
Mockery::mock(IMemberRepository::class),
84+
$promoRepo,
85+
$reservationRepo,
86+
Mockery::mock(ISummitAttendeeRepository::class),
87+
Mockery::mock(ISummitOrderRepository::class),
88+
Mockery::mock(ISummitAttendeeTicketRepository::class),
89+
Mockery::mock(ISummitAttendeeBadgeRepository::class),
90+
Mockery::mock(ISummitRepository::class),
91+
Mockery::mock(ISummitAttendeeBadgePrintRuleRepository::class),
92+
Mockery::mock(IMemberService::class),
93+
Mockery::mock(IBuildDefaultPaymentGatewayProfileStrategy::class),
94+
Mockery::mock(IFileUploadStrategy::class),
95+
Mockery::mock(IFileDownloadStrategy::class),
96+
Mockery::mock(ICompanyRepository::class),
97+
Mockery::mock(ITagRepository::class),
98+
Mockery::mock(ISummitRefundRequestRepository::class),
99+
Mockery::mock(ICompanyService::class),
100+
Mockery::mock(ITicketFinderStrategyFactory::class),
101+
Mockery::mock(ITransactionService::class),
102+
Mockery::mock(ILockManagerService::class)
103+
);
104+
}
105+
106+
private function invokeRestoreTicketsPromoCodes(
107+
SummitOrderService $service,
108+
Summit $summit,
109+
array $tickets_to_return,
110+
array $promo_codes_to_return,
111+
?Member $owner
112+
): void {
113+
$method = new \ReflectionMethod($service, 'restoreTicketsPromoCodes');
114+
$method->setAccessible(true);
115+
$method->invoke($service, $summit, $tickets_to_return, $promo_codes_to_return, $owner);
116+
}
117+
118+
/**
119+
* @dataProvider successfulDecrementProvider
120+
*/
121+
public function testCancelDecrementsReservationRow(int $priorQty, int $returnQty, int $expectedQty): void
122+
{
123+
$summit = Mockery::mock(Summit::class);
124+
$summit->shouldReceive('getId')->andReturn(1);
125+
126+
$owner = Mockery::mock(Member::class);
127+
128+
$promoCode = Mockery::mock(SummitRegistrationPromoCode::class);
129+
$promoCode->shouldReceive('removeUsage')->with($returnQty, '[email protected]')->once();
130+
131+
$reservation = new SummitPromoCodeMemberReservation($promoCode, $owner, $priorQty);
132+
133+
$promoRepo = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
134+
$promoRepo->shouldReceive('getByValueExclusiveLock')
135+
->with($summit, 'TESTCODE')
136+
->andReturn($promoCode);
137+
138+
$reservationRepo = Mockery::mock(ISummitPromoCodeMemberReservationRepository::class);
139+
$reservationRepo->shouldReceive('getByPromoCodeAndMember')
140+
->with($promoCode, $owner)
141+
->andReturn($reservation);
142+
143+
$service = $this->buildService($promoRepo, $reservationRepo);
144+
145+
$this->invokeRestoreTicketsPromoCodes($service, $summit, [], [
146+
'TESTCODE' => ['qty' => $returnQty, 'owner_email' => '[email protected]']
147+
], $owner);
148+
149+
$this->assertSame($expectedQty, $reservation->getQtyUsed());
150+
}
151+
152+
/**
153+
* @return array
154+
*/
155+
public static function successfulDecrementProvider(): array
156+
{
157+
return [
158+
'single ticket cancel' => [3, 1, 2],
159+
'full cancel' => [2, 2, 0],
160+
'over-decrement clamps to zero' => [1, 5, 0],
161+
];
162+
}
163+
164+
public function testCancelWithNullOwnerSkipsReservationDecrement(): void
165+
{
166+
$summit = Mockery::mock(Summit::class);
167+
$summit->shouldReceive('getId')->andReturn(1);
168+
169+
$promoCode = Mockery::mock(SummitRegistrationPromoCode::class);
170+
$promoCode->shouldReceive('removeUsage')->with(1, '[email protected]')->once();
171+
172+
$promoRepo = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
173+
$promoRepo->shouldReceive('getByValueExclusiveLock')
174+
->with($summit, 'TESTCODE')
175+
->andReturn($promoCode);
176+
177+
$reservationRepo = Mockery::mock(ISummitPromoCodeMemberReservationRepository::class);
178+
$reservationRepo->shouldNotReceive('getByPromoCodeAndMember');
179+
180+
$service = $this->buildService($promoRepo, $reservationRepo);
181+
182+
$this->invokeRestoreTicketsPromoCodes($service, $summit, [], [
183+
'TESTCODE' => ['qty' => 1, 'owner_email' => '[email protected]']
184+
], null);
185+
186+
$this->assertTrue(true, 'No reservation decrement attempted for guest order');
187+
}
188+
189+
public function testCancelWithMissingReservationRowSkipsSilently(): void
190+
{
191+
$summit = Mockery::mock(Summit::class);
192+
$summit->shouldReceive('getId')->andReturn(1);
193+
194+
$owner = Mockery::mock(Member::class);
195+
196+
$promoCode = Mockery::mock(SummitRegistrationPromoCode::class);
197+
$promoCode->shouldReceive('removeUsage')->with(1, '[email protected]')->once();
198+
199+
$promoRepo = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
200+
$promoRepo->shouldReceive('getByValueExclusiveLock')
201+
->with($summit, 'TESTCODE')
202+
->andReturn($promoCode);
203+
204+
$reservationRepo = Mockery::mock(ISummitPromoCodeMemberReservationRepository::class);
205+
$reservationRepo->shouldReceive('getByPromoCodeAndMember')
206+
->with($promoCode, $owner)
207+
->andReturn(null);
208+
209+
$service = $this->buildService($promoRepo, $reservationRepo);
210+
211+
$this->invokeRestoreTicketsPromoCodes($service, $summit, [], [
212+
'TESTCODE' => ['qty' => 1, 'owner_email' => '[email protected]']
213+
], $owner);
214+
215+
$this->assertTrue(true, 'No error when reservation row does not exist');
216+
}
217+
218+
public function testReservationDecrementExceptionPropagates(): void
219+
{
220+
$summit = Mockery::mock(Summit::class);
221+
$summit->shouldReceive('getId')->andReturn(1);
222+
223+
$owner = Mockery::mock(Member::class);
224+
225+
$promoCode = Mockery::mock(SummitRegistrationPromoCode::class);
226+
$promoCode->shouldReceive('removeUsage')->with(1, '[email protected]')->once();
227+
228+
$promoRepo = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
229+
$promoRepo->shouldReceive('getByValueExclusiveLock')
230+
->with($summit, 'TESTCODE')
231+
->andReturn($promoCode);
232+
233+
$reservationRepo = Mockery::mock(ISummitPromoCodeMemberReservationRepository::class);
234+
$reservationRepo->shouldReceive('getByPromoCodeAndMember')
235+
->with($promoCode, $owner)
236+
->andThrow(new \RuntimeException('Doctrine connection lost'));
237+
238+
$service = $this->buildService($promoRepo, $reservationRepo);
239+
240+
$this->expectException(\RuntimeException::class);
241+
$this->expectExceptionMessage('Doctrine connection lost');
242+
243+
$this->invokeRestoreTicketsPromoCodes($service, $summit, [], [
244+
'TESTCODE' => ['qty' => 1, 'owner_email' => '[email protected]']
245+
], $owner);
246+
}
247+
}

0 commit comments

Comments
 (0)