diff --git a/docs/superpowers/evidence/2026-09-05-housekeeping-backend-review.md b/docs/superpowers/evidence/2026-09-05-housekeeping-backend-review.md new file mode 100644 index 00000000..57e5cf96 --- /dev/null +++ b/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. diff --git a/src/actions/admin-commerce-concurrency.test.ts b/src/actions/admin-commerce-concurrency.test.ts new file mode 100644 index 00000000..012df6db --- /dev/null +++ b/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) => { + 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$/); + }); +}); diff --git a/src/actions/admin-marketplace.ts b/src/actions/admin-marketplace.ts index 13639e18..45ffc988 100644 --- a/src/actions/admin-marketplace.ts +++ b/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 { 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"); } diff --git a/src/actions/admin-vouchers.ts b/src/actions/admin-vouchers.ts index 64021bf3..7deae16d 100644 --- a/src/actions/admin-vouchers.ts +++ b/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) { diff --git a/src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.test.ts b/src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.test.ts new file mode 100644 index 00000000..7c87856c --- /dev/null +++ b/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 } }); + }); +}); diff --git a/src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.ts b/src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.ts index 66260be3..c143148b 100644 --- a/src/features/housekeeping/domains/economy/services/mutation-runtime-commerce.ts +++ b/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 = {}; 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))