From 91624d5f07808c95773814ab26a6490dd06e741f Mon Sep 17 00:00:00 2001 From: toanhoang Date: Tue, 4 Aug 2026 06:01:34 -0400 Subject: [PATCH] fix: close MaintenanceRecord race condition + restrict targetStatus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createMaintenanceRecord()'s AssetItem status guard pinned status:{not:"retired"} instead of status:"in_stock", so two concurrent opens (or an open racing a concurrent createAssignment()) could both pass the guard, producing two open MaintenanceRecords or an orphaned active Assignment. Guard now pins "in_stock" exactly, mirroring createAssignment()'s own atomic guard. closeMaintenanceRecord()'s targetStatus accepted any of the 4 AssetItemStatus values, including "assigned" — which sets AssetItem.status without creating a corresponding Assignment row, desyncing Assignment (source of truth for who holds what) from AssetItem.status with no way back. Restricted to in_stock|retired, the only two outcomes a closed repair can actually resolve to. --- src/lib/maintenance-records.ts | 53 +++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/lib/maintenance-records.ts b/src/lib/maintenance-records.ts index eb0e73a..1181963 100644 --- a/src/lib/maintenance-records.ts +++ b/src/lib/maintenance-records.ts @@ -108,9 +108,11 @@ export function validateCreateMaintenanceRecordInput(input: { return { ok: true, value: result.data }; } -// Closed set of the 4 AssetItemStatus enum values — reused verbatim (not -// re-derived) as the z.enum() literal list backing targetStatus below. -const assetItemStatusValues = ["in_stock", "assigned", "repair", "retired"] as const; +// Closed set of the 2 AssetItemStatus values a maintenance-record close is +// allowed to land on — deliberately NOT the full 4-value AssetItemStatus +// enum (see closeMaintenanceRecordSchema's targetStatus comment below for +// why "assigned"/"repair" are excluded). +const closeableTargetStatusValues = ["in_stock", "retired"] as const; const closeMaintenanceRecordSchema = z.object({ endDate: dateString("endDate is required and must be a valid date string"), @@ -125,13 +127,26 @@ const closeMaintenanceRecordSchema = z.object({ // Required — deliberately NOT defaulted to "in_stock", per docs/ // asset-license-management.md's "MaintenanceRecord" section: "không tự // động mặc định về in_stock, vì sửa không phải lúc nào cũng thành công." - // Any of the 4 AssetItemStatus values is accepted at the Zod layer (the - // doc's examples are in_stock/retired, but nothing in the spec restricts - // the set further) — see feature-implementer report's "Assumed" section. - targetStatus: z.enum(assetItemStatusValues, { + // Restricted to "in_stock" | "retired" only (NOT the full 4-value + // AssetItemStatus enum) — those are the only two outcomes a repair can + // actually resolve to (fixed → back to stock, unfixable → write off). + // "assigned" is deliberately rejected: setting AssetItem.status to + // "assigned" here would NOT create a corresponding Assignment row, so + // Assignment (the source of truth for "who currently holds what") and + // AssetItem.status would go out of sync with no way back — createAssignment() + // only accepts a "in_stock" AssetItem, and there is no standalone + // AssetItem-status-edit endpoint to undo it by hand. "repair" is + // rejected too: it would leave the record's own AssetItem "in repair" + // despite this exact call being the one that closes out its maintenance + // record — a closed record whose item is still marked "repair" is a + // contradiction, and it would also make the item wrongly ineligible for + // a new maintenance record's own "no other open record" precondition to + // ever apply cleanly (a new one could still be opened as a *different* + // record row while the old one sits closed but the item is still + // "repair"). + targetStatus: z.enum(closeableTargetStatusValues, { errorMap: () => ({ - message: - "targetStatus is required and must be one of 'in_stock', 'assigned', 'repair', 'retired'", + message: "targetStatus is required and must be one of 'in_stock', 'retired'", }), }), }); @@ -234,8 +249,12 @@ export type CreateMaintenanceRecordResult = * * Race-condition note: mirrors src/lib/assignments.ts's createAssignment() * pattern — the AssetItem.status flip uses a WHERE-guarded `updateMany` - * (`where: { id: assetItemId, status: { not: "retired" } }`) rather than a - * plain read-then-write. That `updateMany`'s `count` is the actual + * (`where: { id: assetItemId, status: "in_stock" }`) rather than a + * plain read-then-write. Pinning the exact required prior value (not + * excluding "retired") matters: an excluding guard would still match an + * item another concurrent transaction just flipped to "assigned" or + * "repair", letting two concurrent opens (or an open racing an Assignment + * create) both win. That `updateMany`'s `count` is the actual * race-closing gate: two concurrent opens racing on the same AssetItem * serialize on Postgres's row lock for that UPDATE, the first to commit * wins, and the second's WHERE clause no longer matches (status is no @@ -281,14 +300,20 @@ export async function createMaintenanceRecord( ); } - // Atomic race-closing gate — see this function's doc comment. + // Atomic race-closing gate — see this function's doc comment. Pins the + // exact prior value ("in_stock") rather than excluding a value + // ("not retired"), mirroring createAssignment()'s own status guard + // (src/lib/assignments.ts) — the excluding form would still match an + // AssetItem another concurrent transaction just flipped to "assigned" + // or "repair" (neither equals "retired"), letting two opens (or an + // open racing an Assignment create) both win. const statusUpdate = await tx.assetItem.updateMany({ - where: { id: input.assetItemId, status: { not: "retired" } }, + where: { id: input.assetItemId, status: "in_stock" }, data: { status: "repair" }, }); if (statusUpdate.count === 0) { throw new MaintenanceRecordBusinessError( - "assetItemId refers to an AssetItem that is retired (lost a race to a concurrent retire)" + "assetItemId refers to an AssetItem that is not in_stock (lost a race to a concurrent status change)" ); }