Don't let a compensating payment call swallow the original error
Both placeOrder and cancelOrderItem call a compensating payment action (refund / re-charge) after catching a failure, then rethrow. If that compensating call itself threw, its error replaced the original one, hiding the actual root cause behind whatever the payment provider happened to fail with on the second attempt. Wrap each compensation in its own try/catch, log the compensation failure, and always rethrow the original error. Also fixes a stale comment left over from renaming createFirstStepForOrderItem to initializeOrderItemRecipe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
+23
-2
@@ -257,7 +257,7 @@ export const placeOrder = async (
|
|||||||
|
|
||||||
// Catch the "menu item has no Recipe Steps" data bug before charging
|
// Catch the "menu item has no Recipe Steps" data bug before charging
|
||||||
// anyone, rather than charging first and refunding once
|
// anyone, rather than charging first and refunding once
|
||||||
// createFirstStepForOrderItem discovers it mid-transaction.
|
// initializeOrderItemRecipe discovers it mid-transaction.
|
||||||
const itemIdsWithSteps = new Set(
|
const itemIdsWithSteps = new Set(
|
||||||
(
|
(
|
||||||
await db
|
await db
|
||||||
@@ -337,8 +337,17 @@ export const placeOrder = async (
|
|||||||
})
|
})
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
// The charge already happened; a failed transaction must not leave the
|
// The charge already happened; a failed transaction must not leave the
|
||||||
// customer paying for an Order that doesn't exist.
|
// customer paying for an Order that doesn't exist. If the compensating
|
||||||
|
// refund itself fails, the transaction failure is still the real cause —
|
||||||
|
// log the refund failure rather than letting it hide that error.
|
||||||
|
try {
|
||||||
await paymentProvider.refund(charge.chargeId, totalCents)
|
await paymentProvider.refund(charge.chargeId, totalCents)
|
||||||
|
} catch (refundError) {
|
||||||
|
console.error(
|
||||||
|
`Failed to refund charge ${charge.chargeId} after a failed order transaction:`,
|
||||||
|
refundError,
|
||||||
|
)
|
||||||
|
}
|
||||||
throw error
|
throw error
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -468,8 +477,20 @@ export const cancelOrderItem = async (
|
|||||||
await updateOrderCompleteIfEverySiblingIsDone(tx, orderItem.orderId)
|
await updateOrderCompleteIfEverySiblingIsDone(tx, orderItem.orderId)
|
||||||
})
|
})
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
// The refund already happened; an aborted cancellation must not leave
|
||||||
|
// the customer refunded for an item that's still cooking. If the
|
||||||
|
// compensating re-charge itself fails, the cancellation failure is
|
||||||
|
// still the real cause — log the re-charge failure rather than letting
|
||||||
|
// it hide that error.
|
||||||
if (order.paymentChargeId) {
|
if (order.paymentChargeId) {
|
||||||
|
try {
|
||||||
await paymentProvider.charge(orderItem.priceCents)
|
await paymentProvider.charge(orderItem.priceCents)
|
||||||
|
} catch (rechargeError) {
|
||||||
|
console.error(
|
||||||
|
`Failed to re-charge ${orderItem.priceCents} cents for order item ${orderItemId} after an aborted cancellation:`,
|
||||||
|
rechargeError,
|
||||||
|
)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
throw error
|
throw error
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import {
|
|||||||
describe,
|
describe,
|
||||||
expect,
|
expect,
|
||||||
it,
|
it,
|
||||||
|
vi,
|
||||||
} from 'vitest'
|
} from 'vitest'
|
||||||
|
|
||||||
import * as schema from '#/db/schema.ts'
|
import * as schema from '#/db/schema.ts'
|
||||||
@@ -159,6 +160,34 @@ describe('placing an order', () => {
|
|||||||
seed.menuItems.pizza.priceCents,
|
seed.menuItems.pizza.priceCents,
|
||||||
)
|
)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('surfaces the original transaction failure even if the compensating refund also fails', async () => {
|
||||||
|
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||||
|
|
||||||
|
const failingRefundPayment: typeof payment = {
|
||||||
|
...payment,
|
||||||
|
refund: async () => {
|
||||||
|
throw new Error('simulated payment provider outage')
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
const dbWithFailingTransaction = Object.create(
|
||||||
|
testDb.db,
|
||||||
|
) as typeof testDb.db
|
||||||
|
dbWithFailingTransaction.transaction = () => {
|
||||||
|
throw new Error('simulated DB failure after payment')
|
||||||
|
}
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
ordering.placeOrder(dbWithFailingTransaction, failingRefundPayment, {
|
||||||
|
items: [{ menuItemId: seed.menuItems.pizza.id }],
|
||||||
|
customer: { type: 'guest', name: 'Ada', contact: 'ada@example.com' },
|
||||||
|
}),
|
||||||
|
).rejects.toThrow('simulated DB failure after payment')
|
||||||
|
|
||||||
|
expect(consoleError).toHaveBeenCalled()
|
||||||
|
consoleError.mockRestore()
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe('cancelling an Order Item', () => {
|
describe('cancelling an Order Item', () => {
|
||||||
@@ -287,6 +316,42 @@ describe('cancelling an Order Item', () => {
|
|||||||
expect(rechargedAmountCents).toBe(seed.menuItems.pizza.priceCents)
|
expect(rechargedAmountCents).toBe(seed.menuItems.pizza.priceCents)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('surfaces the original cancellation failure even if the compensating re-charge also fails', async () => {
|
||||||
|
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||||
|
|
||||||
|
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
|
||||||
|
|
||||||
|
const failingRechargePayment: typeof payment = {
|
||||||
|
...payment,
|
||||||
|
charge: async () => {
|
||||||
|
throw new Error('simulated payment provider outage')
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
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,
|
||||||
|
failingRechargePayment,
|
||||||
|
itemId,
|
||||||
|
accessFor(order),
|
||||||
|
),
|
||||||
|
).rejects.toThrow('simulated DB failure after refund')
|
||||||
|
|
||||||
|
expect(consoleError).toHaveBeenCalled()
|
||||||
|
consoleError.mockRestore()
|
||||||
|
})
|
||||||
|
|
||||||
it('gives an accurate message for a double-cancel', async () => {
|
it('gives an accurate message for a double-cancel', async () => {
|
||||||
await placeGuestOrder([{ menuItemId: seed.menuItems.burger.id }]) // occupies the only grill
|
await placeGuestOrder([{ menuItemId: seed.menuItems.burger.id }]) // occupies the only grill
|
||||||
const order = await placeGuestOrder([
|
const order = await placeGuestOrder([
|
||||||
|
|||||||
Reference in New Issue
Block a user