Refund before cancelling, not after, with compensation on failure

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 <noreply@anthropic.com>

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()
+        .update(schema.orderItems)
-        .from(schema.orderItems)
+        .set({ status: 'cancelled' })
-        .where(eq(schema.orderItems.id, orderItemId)),
+        .where(eq(schema.orderItems.id, orderItemId))
-    )
-    if (!orderItem) throw new Error('Order item not found')
 
-    const order = firstRow(
+      await updateOrderCompleteIfEverySiblingIsDone(tx, orderItem.orderId)
-      await tx
+    })
-        .select()
+  } catch (error) {
-        .from(schema.orders)
+    if (order.paymentChargeId) {
-        .where(eq(schema.orders.id, orderItem.orderId)),
+      await paymentProvider.charge(orderItem.priceCents)
-    )
-    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 error
-      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,
-    )
   }
 }
 

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([