fix(housekeeping): serialize commerce state edits

docs/superpowers/evidence/2026-09-05-housekeeping-backend-review.md

@@ -0,0 +1,38 @@
+# Housekeeping backend review checkpoint
+
+This is an incremental backend review, not a completion or deployment claim.
+The current UI route matrix cannot establish operation-level parity.
+
+## Delivered blocks
+
+### Audit reasons and external outcomes (555bc75f)
+
+- Content and Economy preserve normalized reasons through the real service and production audit adapters.
+- Hotel preserves reasons through command, service and audit.
+- Successful Hotel RCON execution with unavailable completion auditing returns partial completion with external completed; it does not emit a false RCON failure.
+- Regressions were observed failing before implementation.
+- Full pre-push suite: 1,877 passed, 5 skipped. TypeScript and scoped Biome passed. Remote CI check passed.
+- Independent scoped review: no Critical or Important findings.
+
+### Commerce editing concurrency
+
+- ASE marketplace cancellation reads and checks the listing under a transaction-held row lock; inactive listings return CONFLICT.
+- Legacy marketplace cancellation includes the row lock, update and staff activity in one transaction. Audit exceptions propagate for rollback.
+- ASE and legacy voucher edits lock the record and reject caps below recorded usage. Legacy edits reject missing vouchers rather than reporting a successful no-op.
+- Four ASE and two legacy regressions failed before fixes; the expanded focused suite has 14 passing tests.
+- Tests execute real Drizzle SQL generation against controlled transport responses. They do not simulate MariaDB locking or prove live multi-connection behavior.
+- Independent scoped review: no Critical or Important findings.
+
+## Open backend work
+
+| Area | Evidence / required follow-up |
+| --- | --- |
+| Voucher redemption | `src/actions/voucher.ts` reads eligibility, inserts used-row, delivers currency, then updates usage in separate operations. No atomic cap reservation. A failed reward can leave a consumed voucher. |
+| Currency delivery | `src/lib/services/send-currency.ts` uses RCON followed by database fallback; socket dispatch is not emulator acknowledgment. Do not invent exactly-once guarantees or blindly replay increments. |
+| Reason enforcement | Reason propagation is fixed for the named paths, but operation-level required-reason policies and denied/failure auditing still need a complete cross-entrypoint inventory. |
+| Functional parity | Compare each query and mutation in Content, Economy, Hotel, People, System and Operations with retained legacy API/actions. A registered handler is not proof of complete functionality. |
+| Commerce audit completeness | Review full before/after snapshots, voucher code-edit parity, and canonical audit coverage of legacy voucher actions. |
+| Validation and references | Review bounded numeric/string inputs, missing targets, foreign references, bulk all-or-nothing behavior and duplicate conflicts per operation. |
+| Live acceptance | Local DB and RCON refuse connections. This does not prevent source implementation; it prevents live integration claims. |
+
+Keep PR 53 draft. Preserve legacy pages, local untracked files, production routing and the existing database contents.

src/actions/admin-commerce-concurrency.test.ts

@@ -0,0 +1,149 @@
+import { drizzle } from "drizzle-orm/mysql-proxy";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+const state = vi.hoisted(() => ({
+	rows: [] as unknown[][],
+	statements: [] as Array<{ sql: string; inTransaction: boolean }>,
+	inTransaction: false,
+	connection: undefined as unknown,
+	activity: vi.fn(),
+}));
+vi.mock("@/lib/admin/guard", () => ({
+	requirePermission: async () => ({ id: 42 }),
+	requirePermissionRateLimited: async () => ({ id: 42 }),
+}));
+vi.mock("next/cache", () => ({ revalidatePath: vi.fn() }));
+vi.mock("@/lib/permissions", async () => import("@/lib/permission-slugs"));
+vi.mock("@/lib/auth", () => ({ auth: vi.fn() }));
+vi.mock("@/lib/services/staff-activity", () => ({
+	logStaffActivity: state.activity,
+	logStaffActivityInTransaction: state.activity,
+}));
+vi.mock("@/lib/db", async () => ({
+	...(await import("@/db/schema")),
+	db: {
+		select: (...args: unknown[]) =>
+			Reflect.apply(
+				(state.connection as { select: (...args: unknown[]) => unknown })
+					.select,
+				state.connection,
+				args,
+			),
+		update: (...args: unknown[]) =>
+			Reflect.apply(
+				(state.connection as { update: (...args: unknown[]) => unknown })
+					.update,
+				state.connection,
+				args,
+			),
+		transaction: async (run: (tx: unknown) => Promise<unknown>) => {
+			state.inTransaction = true;
+			try {
+				return await run(state.connection);
+			} finally {
+				state.inTransaction = false;
+			}
+		},
+	},
+}));
+
+import { cancelMarketplaceListing } from "./admin-marketplace";
+import { updateVoucher } from "./admin-vouchers";
+
+beforeEach(() => {
+	state.rows = [];
+	state.statements = [];
+	state.inTransaction = false;
+	state.activity.mockReset();
+	state.connection = drizzle(async (sql, _params, method) => {
+		state.statements.push({ sql, inTransaction: state.inTransaction });
+		return {
+			rows: method === "all" ? state.rows : [{ affectedRows: 1, insertId: 0 }],
+		};
+	});
+});
+
+describe("Legacy commerce concurrency", () => {
+	it.each(["1.5", "Infinity", "-1", "0"])(
+		"rejects invalid listing id %s before database access",
+		async (id) => {
+			const input = new FormData();
+			input.set("id", id);
+			await cancelMarketplaceListing(input);
+			expect(state.statements).toEqual([]);
+		},
+	);
+
+	it("does not update or audit a listing already sold", async () => {
+		state.rows = [[7, 2, 42, 99, 50]];
+		const input = new FormData();
+		input.set("id", "7");
+		await cancelMarketplaceListing(input);
+		expect(state.statements).toHaveLength(1);
+		expect(state.activity).not.toHaveBeenCalled();
+	});
+
+	it("propagates cancellation audit failures out of the transaction", async () => {
+		state.rows = [[7, 1, 42, 99, 50]];
+		state.activity.mockRejectedValueOnce(new Error("audit unavailable"));
+		const input = new FormData();
+		input.set("id", "7");
+		await expect(cancelMarketplaceListing(input)).rejects.toThrow(
+			"audit unavailable",
+		);
+	});
+
+	it("reports a missing voucher instead of a successful no-op", async () => {
+		const result = await updateVoucher({
+			id: "7",
+			code: "PROMO",
+			amount: 50,
+			maxUses: 5,
+		});
+		expect(result).toMatchObject({ ok: false, error: "Voucher not found" });
+		expect(state.statements).toHaveLength(1);
+	});
+
+	it("accepts a voucher cap equal to usage and commits the update under lock", async () => {
+		state.rows = [[6]];
+		const result = await updateVoucher({
+			id: "7",
+			code: "PROMO",
+			amount: 50,
+			maxUses: 6,
+		});
+		expect(result.ok).toBe(true);
+		expect(state.statements).toHaveLength(2);
+		expect(state.statements.every((statement) => statement.inTransaction)).toBe(
+			true,
+		);
+		expect(state.statements[1]?.sql).not.toContain("`use_count` =");
+	});
+	it("cancels marketplace listings under a transaction-held row lock", async () => {
+		state.rows = [[7, 1, 42, 99, 50]];
+		const input = new FormData();
+		input.set("id", "7");
+		await cancelMarketplaceListing(input);
+		expect(state.statements[0]).toMatchObject({ inTransaction: true });
+		expect(state.statements[0]?.sql).toMatch(/for update$/);
+		expect(state.statements[1]).toMatchObject({ inTransaction: true });
+		expect(state.activity).toHaveBeenCalledWith(
+			expect.objectContaining({ action: "marketplace_cancel" }),
+			state.connection,
+		);
+	});
+
+	it("does not reduce a voucher cap below recorded usage", async () => {
+		state.rows = [[6]];
+		const result = await updateVoucher({
+			id: "7",
+			code: "PROMO",
+			amount: 50,
+			maxUses: 5,
+		});
+		expect(result.ok).toBe(false);
+		expect(state.statements).toHaveLength(1);
+		expect(state.statements[0]).toMatchObject({ inTransaction: true });
+		expect(state.statements[0]?.sql).toMatch(/for update$/);
+	});
+});

src/actions/admin-marketplace.ts

@@ -5,7 +5,7 @@ import { revalidatePath } from "next/cache";
 import { requirePermissionRateLimited } from "@/lib/admin/guard";
 import { db, MarketplaceItems } from "@/lib/db";
 import { PERMS } from "@/lib/permissions";
-import { logStaffActivity } from "@/lib/services/staff-activity";
+import { logStaffActivityInTransaction } from "@/lib/services/staff-activity";
 
 /** Cancel an active marketplace listing (state 1 → 0). */
 export async function cancelMarketplaceListing(
@@ -13,32 +13,38 @@ export async function cancelMarketplaceListing(
 ): Promise<void> {
 	const staff = await requirePermissionRateLimited(PERMS.SHOP_EDIT);
 	const id = Number(formData.get("id"));
-	if (!(id > 0)) return;
+	if (!Number.isSafeInteger(id) || id <= 0) return;
 
-	const [listing] = await db
-		.select({
-			id: MarketplaceItems.id,
-			state: MarketplaceItems.state,
-			userId: MarketplaceItems.userId,
-			itemId: MarketplaceItems.itemId,
-			price: MarketplaceItems.price,
-		})
-		.from(MarketplaceItems)
-		.where(eq(MarketplaceItems.id, id))
-		.limit(1);
-	if (listing?.state !== 1) return;
+	await db.transaction(async (transaction) => {
+		const [listing] = await transaction
+			.select({
+				id: MarketplaceItems.id,
+				state: MarketplaceItems.state,
+				userId: MarketplaceItems.userId,
+				itemId: MarketplaceItems.itemId,
+				price: MarketplaceItems.price,
+			})
+			.from(MarketplaceItems)
+			.where(eq(MarketplaceItems.id, id))
+			.limit(1)
+			.for("update");
+		if (listing?.state !== 1) return;
 
-	await db
-		.update(MarketplaceItems)
-		.set({ state: 0 })
-		.where(eq(MarketplaceItems.id, id));
+		await transaction
+			.update(MarketplaceItems)
+			.set({ state: 0 })
+			.where(eq(MarketplaceItems.id, id));
 
-	await logStaffActivity({
-		staffId: staff.id,
-		action: "marketplace_cancel",
-		description: `Cancelled marketplace listing #${id} (item ${listing.itemId}, user ${listing.userId}, price ${listing.price})`,
-		targetType: "marketplace",
-		targetId: id,
+		await logStaffActivityInTransaction(
+			{
+				staffId: staff.id,
+				action: "marketplace_cancel",
+				description: `Cancelled marketplace listing #${id} (item ${listing.itemId}, user ${listing.userId}, price ${listing.price})`,
+				targetType: "marketplace",
+				targetId: id,
+			},
+			transaction,
+		);
 	});
 	revalidatePath("/admin/marketplace");
 }

src/actions/admin-vouchers.ts

@@ -119,16 +119,30 @@ export async function updateVoucher(input: {
 	}
 
 	try {
-		await db
-			.update(WebsiteShopVouchers)
-			.set({
-				code,
-				amount: Math.floor(amount),
-				maxUses,
-				expiresAt,
-				updatedAt: new Date(),
-			})
-			.where(eq(WebsiteShopVouchers.id, id));
+		const result = await db.transaction(async (transaction) => {
+			const [existing] = await transaction
+				.select({ useCount: WebsiteShopVouchers.useCount })
+				.from(WebsiteShopVouchers)
+				.where(eq(WebsiteShopVouchers.id, id))
+				.limit(1)
+				.for("update");
+			if (!existing) return actionError("Voucher not found");
+			if (maxUses < existing.useCount) {
+				return actionError("Maximum uses cannot be lower than recorded usage");
+			}
+			await transaction
+				.update(WebsiteShopVouchers)
+				.set({
+					code,
+					amount: Math.floor(amount),
+					maxUses,
+					expiresAt,
+					updatedAt: new Date(),
+				})
+				.where(eq(WebsiteShopVouchers.id, id));
+			return actionOk();
+		});
+		if (!result.ok) return result;
 		revalidatePath("/admin/vouchers");
 		return actionOk();
 	} catch (error) {

src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.test.ts

@@ -0,0 +1,87 @@
+import { drizzle } from "drizzle-orm/mysql-proxy";
+import { describe, expect, it, vi } from "vitest";
+import { executeEconomyDatabaseMutation } from "./mutation-runtime-commerce";
+
+vi.mock("@/lib/db", async () => ({ ...(await import("@/db/schema")), db: {} }));
+
+const context = {
+	capability: {
+		actor: { id: 42, username: "operator", rank: 7 },
+		isSuperAdmin: false,
+		has: () => true,
+		hasAny: () => true,
+		hasAll: () => true,
+	},
+	correlationId: "commerce-concurrency",
+	legacy: false,
+};
+
+function connection(rows: unknown[][]) {
+	const statements: Array<{ sql: string; params: unknown[] }> = [];
+	const database = drizzle(async (sql, params, method) => {
+		statements.push({ sql, params });
+		return {
+			rows: method === "all" ? rows : [{ affectedRows: 1, insertId: 0 }],
+		};
+	});
+	return { database, statements };
+}
+
+describe("Economy commerce concurrency", () => {
+	it("locks an active marketplace listing before cancelling it", async () => {
+		const { database, statements } = connection([[7, 1, 42, 99, 50]]);
+		const snapshot = await executeEconomyDatabaseMutation(
+			"marketplace.cancel",
+			{ id: 7 },
+			context,
+			database,
+		);
+		expect(statements[0]?.sql).toMatch(/for update$/);
+		expect(statements[0]?.params).toEqual([7, 1]);
+		expect(statements[1]?.sql).toMatch(/^update/);
+		expect(snapshot).toMatchObject({
+			before: { state: 1 },
+			after: { state: 0 },
+		});
+	});
+
+	it("reports a conflict without writing when a listing is already sold", async () => {
+		const { database, statements } = connection([[7, 2, 42, 99, 50]]);
+		await expect(
+			executeEconomyDatabaseMutation(
+				"marketplace.cancel",
+				{ id: 7 },
+				context,
+				database,
+			),
+		).rejects.toMatchObject({ code: "CONFLICT" });
+		expect(statements).toHaveLength(1);
+	});
+
+	it("locks voucher state before rejecting a cap below recorded usage", async () => {
+		const { database, statements } = connection([["7", 50, 10, 6]]);
+		await expect(
+			executeEconomyDatabaseMutation(
+				"voucher.change",
+				{ action: "update", id: "7", maxUses: 5 },
+				context,
+				database,
+			),
+		).rejects.toMatchObject({ code: "CONFLICT" });
+		expect(statements[0]?.sql).toMatch(/for update$/);
+		expect(statements).toHaveLength(1);
+	});
+
+	it("accepts a cap equal to recorded usage without resetting the counter", async () => {
+		const { database, statements } = connection([["7", 50, 10, 6]]);
+		const snapshot = await executeEconomyDatabaseMutation(
+			"voucher.change",
+			{ action: "update", id: "7", maxUses: 6 },
+			context,
+			database,
+		);
+		expect(statements[0]?.sql).toMatch(/for update$/);
+		expect(statements[1]?.sql).not.toContain("`use_count` =");
+		expect(snapshot).toMatchObject({ after: { maxUses: 6, useCount: 6 } });
+	});
+});

src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.ts

@@ -30,6 +30,7 @@ import type {
 	EconomyMutationOperation,
 	EconomyMutationSnapshot,
 } from "./mutations";
+import { EconomyMutationFailure } from "./mutations";
 
 type EconomyDatabase = typeof db;
 
@@ -153,9 +154,15 @@ async function marketplaceCancel(
 		})
 		.from(MarketplaceItems)
 		.where(eq(MarketplaceItems.id, id))
-		.limit(1);
+		.limit(1)
+		.for("update");
 	if (!listing) throw notFound();
-	if (listing.state !== 1) throw validation({ id: ["listing is not active"] });
+	if (listing.state !== 1) {
+		throw new EconomyMutationFailure(
+			"CONFLICT",
+			"errors.housekeeping.conflict",
+		);
+	}
 	await connection
 		.update(MarketplaceItems)
 		.set({ state: 0 })
@@ -216,7 +223,8 @@ async function voucherChange(
 		})
 		.from(WebsiteShopVouchers)
 		.where(eq(WebsiteShopVouchers.id, id))
-		.limit(1);
+		.limit(1)
+		.for("update");
 	if (!existing) throw notFound();
 	const before = {
 		id: id.toString(),
@@ -233,6 +241,12 @@ async function voucherChange(
 	const values: Partial<typeof WebsiteShopVouchers.$inferInsert> = {};
 	if (hasOwn(data, "amount")) values.amount = positiveInteger(data.amount);
 	if (hasOwn(data, "maxUses")) values.maxUses = positiveInteger(data.maxUses);
+	if (values.maxUses !== undefined && values.maxUses < existing.useCount) {
+		throw new EconomyMutationFailure(
+			"CONFLICT",
+			"errors.housekeeping.conflict",
+		);
+	}
 	if (hasOwn(data, "expiresAt")) {
 		const parsed = data.expiresAt
 			? new Date(text(data.expiresAt, 64, true))