Keep expiring orders behind ones left to the webhook

1 of 1 check failed

The cron asked for the 50 oldest pending orders on every run. An order whose payment went
through but whose webhook had not arrived stays pending and stays oldest, so once there were 50
of them the run never reached the abandoned checkouts behind them, and their stock stayed held.

expireOrders now pages through the expired orders by (created_at, id), up to 200 a run.

Justin Torre committed 17 hours ago

8b37e66deea2fcad2af6b857a033202873340443

Parent 94db762 Browse files History

Changed files

3 files changed72 additions14 deletionsagainst 94db762

Changed.rebasely/kb/concepts/checkout.md+4−2
Changedsrc/expiry.ts+31−11
Changedtest/expiry.test.ts+37−1

Changed.rebasely/kb/concepts/checkout.md+4−2

View file

Changedsrc/expiry.ts+31−11

View file

Changedtest/expiry.test.ts+37−1

View file

Checks

1 of 1 check failed

Status	Check	Reported by	Description	Details
Failed	duragit/ci/test	Rebasely Actions	out of memory	Run

Runs

Status	Workflow	Event	Started
Failed	ci	Push	2 h ago

		@@ -9,7 +9,7 @@
		- src/payments.ts
		- src/expiry.ts
		- migrations/0003_orders.sql
		-reviewed: 2026-10-09
		+reviewed: 2026-10-15
		---
		# Checkout

		@@ -33,12 +33,14 @@

		## Unpaid orders

		-An order holds its stock until it is paid or cancelled, so unpaid ones expire ([[0009-cancel-unpaid-orders-after-30-minutes]]). Every five minutes the cron trigger runs `expireOrders` (`src/expiry.ts`) over up to 50 orders that have been `pending_payment` for more than 30 minutes:
		+An order holds its stock until it is paid or cancelled, so unpaid ones expire ([[0009-cancel-unpaid-orders-after-30-minutes]]). Every five minutes the cron trigger runs `expireOrders` (`src/expiry.ts`) over the orders that have been `pending_payment` for more than 30 minutes, oldest first. For each one:

		1. Cancel the PaymentIntent at Stripe. If Stripe refuses because the intent succeeded or a payment is processing, the payment won the race: leave the order to the webhook.
		2. Otherwise cancel the order, which puts the stock back and sends `order.cancelled`.

		Cancelling the intent first means a shopper cannot pay for an order after it has been cancelled.
		+
		+An order left to the webhook is still `pending_payment` on the next run, and still the oldest. So the run reads the expired orders 50 at a time by `(created_at, id)` and moves past each page, rather than asking for the 50 oldest again. A run looks at no more than 200 orders (`MAX_PER_RUN`), which keeps its Stripe calls bounded; the next run starts from the oldest again. Without the cursor, a webhook outage that left 50 paid orders pending would stop every abandoned checkout behind them from giving its stock back.

		## Order states

		@@ -8,23 +8,43 @@

		export const PAYMENT_WINDOW_MS = 30 * 60 * 1000;
		const BATCH = 50;
		+// One run looks at no more than this many orders, so its calls to Stripe stay bounded. The next
		+// run starts again from the oldest.
		+export const MAX_PER_RUN = 200;

		export type ExpiryReport = { cancelled: string[]; left_to_webhook: string[] };

		+type Expired = { id: string; payment_intent_id: string \| null; created_at: number };
		+
		export async function expireOrders(db: D1Database, events: Queue<OrderEvent>, payments: Payments, now: number): Promise<ExpiryReport> {
		- const { results } = await db
		- .prepare("SELECT id, payment_intent_id FROM orders WHERE status = 'pending_payment' AND created_at < ? ORDER BY created_at LIMIT ?")
		- .bind(now - PAYMENT_WINDOW_MS, BATCH)
		- .all<{ id: string; payment_intent_id: string \| null }>();
		const report: ExpiryReport = { cancelled: [], left_to_webhook: [] };
		- for (const o of results) {
		- // The intent succeeded or is processing: the payment won the race, and the webhook will mark
		- // the order paid. Leave it alone.
		- if (o.payment_intent_id && !(await payments.cancelIntent(o.payment_intent_id))) {
		- report.left_to_webhook.push(o.id);
		- continue;
		+ // Orders left to the webhook stay pending_payment, so a query that always starts at the oldest
		+ // order would keep returning them and never reach the ones behind. The run pages through the
		+ // expired orders by (created_at, id) instead, past every order it has already looked at.
		+ let after = null as Expired \| null;
		+ let seen = 0;
		+ while (seen < MAX_PER_RUN) {
		+ const { results } = await db
		+ .prepare(
		+ `SELECT id, payment_intent_id, created_at FROM orders
		+ WHERE status = 'pending_payment' AND created_at < ?1
		+ AND (?2 IS NULL OR (created_at, id) > (?2, ?3))
		+ ORDER BY created_at, id LIMIT ?4`,
		+ )
		+ .bind(now - PAYMENT_WINDOW_MS, after?.created_at ?? null, after?.id ?? null, Math.min(BATCH, MAX_PER_RUN - seen))
		+ .all<Expired>();
		+ for (const o of results) {
		+ // The intent succeeded or is processing: the payment won the race, and the webhook will
		+ // mark the order paid. Leave it alone.
		+ if (o.payment_intent_id && !(await payments.cancelIntent(o.payment_intent_id))) {
		+ report.left_to_webhook.push(o.id);
		+ continue;
		+ }
		+ if ((await cancelOrder(db, events, o.id, now)).changed) report.cancelled.push(o.id);
		}
		- if ((await cancelOrder(db, events, o.id, now)).changed) report.cancelled.push(o.id);
		+ seen += results.length;
		+ after = results.at(-1) ?? null;
		+ if (results.length < BATCH) break;
		}
		return report;
		}

		@@ -1,6 +1,6 @@
		import assert from "node:assert/strict";
		import { test } from "node:test";
		-import { expireOrders, PAYMENT_WINDOW_MS } from "../src/expiry.ts";
		+import { expireOrders, MAX_PER_RUN, PAYMENT_WINDOW_MS } from "../src/expiry.ts";
		import { markPaid } from "../src/orders.ts";
		import { NOW, setup } from "./helpers/app.ts";

		@@ -49,4 +49,40 @@
		await markPaid(s.env.DB, s.events, s.order.id, `pi_${s.order.id}`, NOW);
		assert.deepEqual(await s.expire(NOW + 2 * PAYMENT_WINDOW_MS), { cancelled: [], left_to_webhook: [] });
		assert.deepEqual(s.payments.cancelled, []);
		+});
		+
		+// Regression: the query used to take the 50 oldest pending orders every run. Orders whose payment
		+// had gone through but whose webhook had not arrived stayed pending and stayed oldest, so once
		+// there were 50 of them, no abandoned order behind them was ever cancelled.
		+test("orders left to the webhook do not stop the run from reaching newer abandoned orders", async () => {
		+ const s = setup();
		+ await s.addProduct("wool-socks", { stock: 60 });
		+ for (let i = 0; i < 51; i++) {
		+ const cartId = (await s.call("POST", "/carts")).body.cart.id;
		+ await s.call("PUT", `/carts/${cartId}/items/wool-socks`, { quantity: 1 });
		+ await s.call("POST", `/carts/${cartId}/checkout`, CHECKOUT);
		+ }
		+ const { results } = await s.env.DB.prepare("SELECT id FROM orders ORDER BY created_at, id").all<{ id: string }>();
		+ const paid = results.slice(0, 50).map((o) => o.id);
		+ const abandoned = results[50]!.id;
		+ for (const id of paid) s.payments.uncancellable.add(`pi_${id}`);
		+
		+ const report = await expireOrders(s.env.DB, s.events, s.payments, NOW + PAYMENT_WINDOW_MS + 1);
		+ assert.deepEqual(report.left_to_webhook, paid);
		+ assert.deepEqual(report.cancelled, [abandoned]);
		+ assert.equal((await s.call("GET", "/products/wool-socks")).body.product.stock, 10);
		+});
		+
		+test("one run looks at no more than MAX_PER_RUN orders", async () => {
		+ const s = setup();
		+ await s.addProduct("wool-socks", { stock: MAX_PER_RUN + 5 });
		+ for (let i = 0; i < MAX_PER_RUN + 5; i++) {
		+ const cartId = (await s.call("POST", "/carts")).body.cart.id;
		+ await s.call("PUT", `/carts/${cartId}/items/wool-socks`, { quantity: 1 });
		+ await s.call("POST", `/carts/${cartId}/checkout`, CHECKOUT);
		+ }
		+ const first = await expireOrders(s.env.DB, s.events, s.payments, NOW + PAYMENT_WINDOW_MS + 1);
		+ assert.equal(first.cancelled.length, MAX_PER_RUN);
		+ const second = await expireOrders(s.env.DB, s.events, s.payments, NOW + PAYMENT_WINDOW_MS + 1);
		+ assert.equal(second.cancelled.length, 5);
		});