From 506319ce4f473913764746389fb23d4003a9e95f Mon Sep 17 00:00:00 2001 From: lukas Date: Wed, 9 Sep 2026 20:01:55 +0200 Subject: [PATCH] Refund before cancelling, not after, with compensation on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cancelOrderItem committed the cancellation (freeing the Machine slot, marking the item cancelled) before calling the payment provider's refund — so a refund failure (gateway timeout, provider outage) left the item cancelled with no refund ever issued, and no record that one was owed. placeOrder already avoids the equivalent problem by charging before its DB transaction and refunding if the transaction then fails; apply the same pattern here for consistency. Refund now happens first; the DB transaction's atomic queued-status check (added for the earlier cancellation race) doubles as the signal that the refund needs to be undone if this transaction loses the race or otherwise fails, so the catch block re-charges to compensate. Co-Authored-By: Claude Sonnet 5 --- src/domain/ordering.ts | 129 ++++++++++++++++++++++------------------- tests/ordering.test.ts | 38 ++++++++++++ 2 files changed, 107 insertions(+), 60 deletions(-) diff --git a/src/domain/ordering.ts b/src/domain/ordering.ts index f6db50b..e353ba3 100644 --- a/src/domain/ordering.ts +++ b/src/domain/ordering.ts @@ -400,69 +400,78 @@ export const cancelOrderItem = async ( orderItemId: number, access: OrderAccess, ) => { - const result = await db.transaction(async (tx) => { - const orderItem = firstRow( + const orderItem = firstRow( + await db + .select() + .from(schema.orderItems) + .where(eq(schema.orderItems.id, orderItemId)), + ) + if (!orderItem) throw new Error('Order item not found') + + const order = firstRow( + await db + .select() + .from(schema.orders) + .where(eq(schema.orders.id, orderItem.orderId)), + ) + if (!order) throw new Error('Order not found') + assertOrderAccess(order, access) + + if (orderItem.status === 'cancelled') { + throw new OrderItemNotCancellableError('it has already been cancelled') + } + if (orderItem.status === 'ready' || orderItem.status === 'picked_up') { + throw new OrderItemNotCancellableError('it is already done cooking') + } + + const currentStep = await getCurrentStepInstance(db, orderItemId) + if (!currentStep || currentStep.status !== 'queued') { + throw new OrderItemNotCancellableError('it has already started cooking') + } + + // Refund before touching any state, mirroring placeOrder's charge-first + // pattern: if the refund itself fails, nothing about this cancellation is + // ever recorded, exactly like a declined charge leaves no Order behind. + if (order.paymentChargeId) { + await paymentProvider.refund(order.paymentChargeId, orderItem.priceCents) + } + + try { + await db.transaction(async (tx) => { + // Re-check status = 'queued' as part of the delete itself, atomically: + // a Machine may have claimed this Step (assignQueuedStepsForTag), or a + // concurrent cancelOrderItem call may have already deleted it, in the + // time since the read above (which can include the refund call, an + // external network round trip). If so, zero rows match and the catch + // below re-charges to undo the refund we already issued. + const deletedSteps = await tx + .delete(schema.preparationStepInstances) + .where( + and( + eq(schema.preparationStepInstances.id, currentStep.id), + eq(schema.preparationStepInstances.status, 'queued'), + ), + ) + .returning({ id: schema.preparationStepInstances.id }) + + if (deletedSteps.length === 0) { + throw new OrderItemNotCancellableError( + 'it has already started cooking', + ) + } + await tx - .select() - .from(schema.orderItems) - .where(eq(schema.orderItems.id, orderItemId)), - ) - if (!orderItem) throw new Error('Order item not found') + .update(schema.orderItems) + .set({ status: 'cancelled' }) + .where(eq(schema.orderItems.id, orderItemId)) - const order = firstRow( - await tx - .select() - .from(schema.orders) - .where(eq(schema.orders.id, orderItem.orderId)), - ) - if (!order) throw new Error('Order not found') - assertOrderAccess(order, access) - - if (orderItem.status === 'cancelled') { - throw new OrderItemNotCancellableError('it has already been cancelled') + await updateOrderCompleteIfEverySiblingIsDone(tx, orderItem.orderId) + }) + } catch (error) { + if (order.paymentChargeId) { + await paymentProvider.charge(orderItem.priceCents) } - if (orderItem.status === 'ready' || orderItem.status === 'picked_up') { - throw new OrderItemNotCancellableError('it is already done cooking') - } - - const currentStep = await getCurrentStepInstance(tx, orderItemId) - if (!currentStep || currentStep.status !== 'queued') { - throw new OrderItemNotCancellableError('it has already started cooking') - } - - // Re-check status = 'queued' as part of the delete itself, atomically: - // a Machine may have claimed this Step (assignQueuedStepsForTag) between - // the read above and this statement. If so, zero rows match and we must - // not refund a Step that's already cooking. - const deletedSteps = await tx - .delete(schema.preparationStepInstances) - .where( - and( - eq(schema.preparationStepInstances.id, currentStep.id), - eq(schema.preparationStepInstances.status, 'queued'), - ), - ) - .returning({ id: schema.preparationStepInstances.id }) - - if (deletedSteps.length === 0) { - throw new OrderItemNotCancellableError('it has already started cooking') - } - - await tx - .update(schema.orderItems) - .set({ status: 'cancelled' }) - .where(eq(schema.orderItems.id, orderItemId)) - - await updateOrderCompleteIfEverySiblingIsDone(tx, orderItem.orderId) - - return { order, orderItem } - }) - - if (result.order.paymentChargeId) { - await paymentProvider.refund( - result.order.paymentChargeId, - result.orderItem.priceCents, - ) + throw error } } diff --git a/tests/ordering.test.ts b/tests/ordering.test.ts index 3401dd1..1eca15b 100644 --- a/tests/ordering.test.ts +++ b/tests/ordering.test.ts @@ -249,6 +249,44 @@ describe('cancelling an Order Item', () => { ).rejects.toThrow(ordering.ForbiddenError) }) + it('recharges the customer if the DB transaction fails after the refund has already gone through', async () => { + await placeGuestOrder([{ menuItemId: seed.menuItems.pizza.id }]) // occupies the only oven + const order = await placeGuestOrder([ + { menuItemId: seed.menuItems.pizza.id }, + ]) + const itemId = (await getStatus(order)).items[0].id + + let rechargedAmountCents: number | undefined + const spyingPayment: typeof payment = { + ...payment, + charge: async (amountCents) => { + rechargedAmountCents = amountCents + return payment.charge(amountCents) + }, + } + + const dbWithFailingTransaction = Object.create( + testDb.db, + ) as typeof testDb.db + dbWithFailingTransaction.transaction = () => { + throw new Error('simulated DB failure after refund') + } + + await expect( + ordering.cancelOrderItem( + dbWithFailingTransaction, + spyingPayment, + itemId, + accessFor(order), + ), + ).rejects.toThrow('simulated DB failure after refund') + + expect(payment.refundsFor(order.paymentChargeId!)).toBe( + seed.menuItems.pizza.priceCents, + ) + expect(rechargedAmountCents).toBe(seed.menuItems.pizza.priceCents) + }) + it('gives an accurate message for a double-cancel', async () => { await placeGuestOrder([{ menuItemId: seed.menuItems.burger.id }]) // occupies the only grill const order = await placeGuestOrder([