refactor: take the three risky rows back to the plan, without the bugs they invited
All three were deviations I had argued for. Reversing them as asked, each in the shape the plan wanted and with the failure it would otherwise cause closed. consume now catches the unique-constraint violation, as specified, instead of relying only on a read-before-insert. The read stays for the ordinary path, but it never closed the race — only the unique key does. What made the catch dangerous is that Doctrine closes the EntityManager on a constraint violation and the rest of the request dies with it, so the catch resets the registry. Without that, "already consumed" would surface as an unrelated 500. A test inserts the ledger row from a second connection and then asks the service to consume: it returns true, the manager is still open, and exactly one session is taken. Cancellation is one transaction now: status, capacity release, credit refund, penalty and the timeline row commit together. An appointment marked cancelled whose capacity was never released is the worst of both — the patient has no appointment and nobody can take the slot. Notification stays outside the commit, because an SMS cannot be rolled back and must not sit inside something that can. A test with an SMS provider that always throws proves the cancellation still commits. The ledger's running balance is computed in the UI from the rows on screen. The server still sends its own and remains the reference; the point of computing it here is that the column now reflects the rows the user is actually looking at, so a truncated list shows up as a mismatch rather than as a number nobody can check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -55,22 +55,38 @@ final class CancellationService
|
||||
|
||||
$penalty = $this->calculator->calculate($appointment, $status, $now);
|
||||
|
||||
$appointment->transitionTo($status);
|
||||
$this->em->flush();
|
||||
/**
|
||||
* همهٔ نوشتنهای دیتابیس در **یک** تراکنش.
|
||||
*
|
||||
* وضعیت نوبت، آزادسازی ظرفیت، بازگشت اعتبار، جریمه و ردیف تایملاین یک واقعهاند:
|
||||
* نوبتی که «لغو» شده ولی ظرفیتش آزاد نشده، بدترین حالت ممکن است — هم بیمار نوبت
|
||||
* ندارد هم کسی نمیتواند آن وقت را بگیرد.
|
||||
*
|
||||
* اطلاعرسانی **بیرون** این بلوک است و بعد از commit اجرا میشود: پیامک قابل
|
||||
* برگرداندن نیست، پس نباید داخل چیزی باشد که ممکن است برگردد.
|
||||
*/
|
||||
[$released, $charged] = $this->em->wrapInTransaction(
|
||||
function () use ($appointment, $status, $penalty, $actor, $reason): array {
|
||||
$appointment->transitionTo($status);
|
||||
$this->em->flush();
|
||||
|
||||
// آزادسازی منابع و بازگشت اعتبار پکیج و جلسهٔ دوره — همه در `cancel` بوکینگ.
|
||||
$released = $this->booking->cancel($appointment);
|
||||
// آزادسازی منابع و بازگشت اعتبار پکیج و جلسهٔ دوره — همه در `cancel` بوکینگ.
|
||||
$released = $this->booking->cancel($appointment);
|
||||
|
||||
if (!$penalty->creditRefundable) {
|
||||
$this->revokeRefundedCredit($appointment);
|
||||
}
|
||||
if (!$penalty->creditRefundable) {
|
||||
$this->revokeRefundedCredit($appointment);
|
||||
}
|
||||
|
||||
$charged = $this->chargePenalty($appointment, $penalty, $actor);
|
||||
$charged = $this->chargePenalty($appointment, $penalty, $actor);
|
||||
|
||||
$this->recordTimelineEntry($appointment, $actor, $reason);
|
||||
|
||||
return [$released, $charged];
|
||||
},
|
||||
);
|
||||
|
||||
$notified = $this->waitlist->notifyForFreedSlot($appointment);
|
||||
|
||||
$this->recordTimelineEntry($appointment, $actor, $reason);
|
||||
|
||||
return [
|
||||
'appointment_uuid' => $appointment->getUuid(),
|
||||
'status' => $appointment->getStatus(),
|
||||
|
||||
@@ -10,8 +10,10 @@ use App\Package\Entity\SessionCreditLedger;
|
||||
use App\Package\Repository\SessionCreditLedgerRepository;
|
||||
use App\Shared\Event\DomainEventPublisher;
|
||||
use App\Shared\Event\DomainEvents;
|
||||
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
|
||||
use Doctrine\ORM\EntityManagerInterface;
|
||||
use Doctrine\DBAL\LockMode;
|
||||
use Doctrine\Persistence\ManagerRegistry;
|
||||
|
||||
/**
|
||||
* تنها نویسندهٔ دفتر اعتبار.
|
||||
@@ -25,6 +27,7 @@ final class CreditLedgerService
|
||||
public function __construct(
|
||||
private readonly SessionCreditLedgerRepository $ledger,
|
||||
private readonly DomainEventPublisher $events,
|
||||
private readonly ManagerRegistry $registry,
|
||||
private readonly EntityManagerInterface $em,
|
||||
) {}
|
||||
|
||||
@@ -66,6 +69,23 @@ final class CreditLedgerService
|
||||
* ناچیز و سادگیاش برنده است.
|
||||
*/
|
||||
public function consume(PatientPackage $package, Appointment $appointment, ?ServiceItem $service = null): bool
|
||||
{
|
||||
try {
|
||||
return $this->consumeOnce($package, $appointment, $service);
|
||||
} catch (UniqueConstraintViolationException) {
|
||||
// دو درخواست همزمان برای یک نوبت: کلید یکتا دومی را رد کرد و همین درست
|
||||
// است — یک جلسه خورده شده.
|
||||
//
|
||||
// ولی Doctrine روی نقض کلید **خودِ EntityManager را میبندد**، و مدیرِ بسته
|
||||
// بقیهٔ همین request را هم میسوزاند. بازنشانی رجیستری تنها راه زنده ماندن
|
||||
// است؛ بدون آن، «مصرف تکراری» به یک خطای ۵۰۰ بیربط تبدیل میشد.
|
||||
$this->registry->resetManager();
|
||||
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
private function consumeOnce(PatientPackage $package, Appointment $appointment, ?ServiceItem $service): bool
|
||||
{
|
||||
// قفل بدون تراکنش معنا ندارد؛ خواندن و نوشتن باید در یک واحد اتمی باشند
|
||||
// وگرنه دو درخواست همزمان هر دو ماندهٔ ۱ را میبینند.
|
||||
@@ -76,10 +96,11 @@ final class CreditLedgerService
|
||||
return false;
|
||||
}
|
||||
|
||||
// همین نوبت قبلاً مصرف کرده؟ `confirm` idempotent است و اجرای دومش نباید
|
||||
// جلسهٔ دوم بخورد. بررسی **پیش از** درج است نه گرفتنِ استثنا: نقض کلید
|
||||
// یکتا در Doctrine خودِ EntityManager را میبندد و بقیهٔ همان request را
|
||||
// هم میسوزاند. کلید یکتا آخرین خط دفاع میماند، نه مسیر عادی.
|
||||
// `confirm` idempotent است و اجرای دومش نباید جلسهٔ دوم بخورد.
|
||||
//
|
||||
// بررسی پیش از درج **تنها** تکیهگاه نیست: بین این خواندن و آن نوشتن هنوز
|
||||
// یک پنجرهٔ رقابت هست و تنها چیزی که واقعاً میبندد کلید یکتاست. پس هر دو
|
||||
// را داریم — بررسی برای مسیر عادی، و `catch` برای رقابت واقعی.
|
||||
if ($this->ledger->findForAppointment($appointment, SessionCreditLedger::KIND_CONSUME) !== null) {
|
||||
return true;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user