fix(api): wrap critical mutations in transactions and fix TOCTOU race conditions
- applyProjectScenario: wrap assignment loop in db.$transaction to prevent partial updates - vacation approve/reject: fix TOCTOU race via updateMany with status-guard in WHERE + CONFLICT on count=0 - vacation cancel: wrap vacation.update + entitlement.updateMany in $transaction - batchApprove: collect mutations, wrap in $transaction, dispatch SSE/notifications after commit - Fix dead-code bug in createHappyPathDb where $transaction was assigned after return - Add atomicity and concurrency tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -71,7 +71,7 @@ export function createHappyPathDb() {
|
||||
return null;
|
||||
});
|
||||
|
||||
return {
|
||||
const db = {
|
||||
user: {
|
||||
findUnique: vi.fn().mockResolvedValue({ id: "user_1", systemRole: "MANAGER" }),
|
||||
},
|
||||
@@ -111,6 +111,11 @@ export function createHappyPathDb() {
|
||||
},
|
||||
vacation: {
|
||||
findUnique: vacationFindUnique,
|
||||
findUniqueOrThrow: vi.fn().mockImplementation(async (args?: any) => {
|
||||
const result = await vacationFindUnique({ where: { id: args?.where?.id } });
|
||||
if (!result) throw new Error("Vacation not found");
|
||||
return result;
|
||||
}),
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
findFirst: vi.fn().mockResolvedValue(null),
|
||||
create: vi.fn().mockResolvedValue({
|
||||
@@ -147,6 +152,7 @@ export function createHappyPathDb() {
|
||||
approvedById: args?.data?.approvedById ?? existing?.approvedById ?? null,
|
||||
};
|
||||
}),
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
},
|
||||
notification: {
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
@@ -158,9 +164,9 @@ export function createHappyPathDb() {
|
||||
webhook: {
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
},
|
||||
};
|
||||
} as any;
|
||||
|
||||
(db as Record<string, unknown>).$transaction = vi.fn(
|
||||
db.$transaction = vi.fn(
|
||||
async (callback: (tx: typeof db) => Promise<unknown>) => callback(db),
|
||||
);
|
||||
|
||||
|
||||
@@ -72,15 +72,15 @@ describe("assistant vacation mutation tools", () => {
|
||||
message: "Rejected vacation for Alice Example: Capacity freeze",
|
||||
}),
|
||||
);
|
||||
expect(db.vacation.update).toHaveBeenCalledWith(
|
||||
expect(db.vacation.updateMany).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
where: { id: "vac_cancelled" },
|
||||
where: expect.objectContaining({ id: "vac_cancelled" }),
|
||||
data: expect.objectContaining({ status: "APPROVED" }),
|
||||
}),
|
||||
);
|
||||
expect(db.vacation.update).toHaveBeenCalledWith(
|
||||
expect(db.vacation.updateMany).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
where: { id: "vac_pending" },
|
||||
where: expect.objectContaining({ id: "vac_pending" }),
|
||||
data: expect.objectContaining({ status: "REJECTED", rejectionReason: "Capacity freeze" }),
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -10,8 +10,9 @@ function makeDb(overrides: {
|
||||
assignmentUpdate?: ReturnType<typeof vi.fn>;
|
||||
assignmentCreate?: ReturnType<typeof vi.fn>;
|
||||
resourceFindUnique?: ReturnType<typeof vi.fn>;
|
||||
transaction?: ReturnType<typeof vi.fn>;
|
||||
}) {
|
||||
return {
|
||||
const db = {
|
||||
project: {
|
||||
findUnique: overrides.projectFindUnique ?? vi.fn().mockResolvedValue({ id: "project_1", name: "Test Project" }),
|
||||
},
|
||||
@@ -22,6 +23,10 @@ function makeDb(overrides: {
|
||||
resource: {
|
||||
findUnique: overrides.resourceFindUnique ?? vi.fn().mockResolvedValue({ lcrCents: 100 }),
|
||||
},
|
||||
};
|
||||
return {
|
||||
...db,
|
||||
$transaction: overrides.transaction ?? vi.fn(async (cb: (tx: typeof db) => Promise<unknown>) => cb(db)),
|
||||
} as never;
|
||||
}
|
||||
|
||||
@@ -151,4 +156,46 @@ describe("applyProjectScenario", () => {
|
||||
// appliedCount = 2 creates + 0 cancel = 2.
|
||||
expect(result.appliedCount).toBe(2);
|
||||
});
|
||||
|
||||
it("wraps all mutations in a single transaction", async () => {
|
||||
assignmentCreate = vi.fn().mockResolvedValue({ id: "new_1" });
|
||||
assignmentUpdate = vi.fn().mockResolvedValue({});
|
||||
|
||||
// transaction mock that propagates the error so we can verify atomicity
|
||||
const transaction = vi.fn(async (cb: (tx: unknown) => Promise<unknown>) => cb({
|
||||
assignment: { update: assignmentUpdate, create: assignmentCreate },
|
||||
resource: { findUnique: resourceFindUnique },
|
||||
}));
|
||||
|
||||
const db = makeDb({ assignmentCreate, assignmentUpdate, resourceFindUnique, transaction });
|
||||
|
||||
await applyProjectScenario(db, {
|
||||
projectId: "project_1",
|
||||
changes: [{ ...baseChange, resourceId: "resource_1" }],
|
||||
});
|
||||
|
||||
expect(transaction).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("propagates transaction error so partial changes are rolled back", async () => {
|
||||
assignmentCreate = vi.fn()
|
||||
.mockResolvedValueOnce({ id: "new_1" })
|
||||
.mockRejectedValueOnce(new Error("DB constraint violation"));
|
||||
|
||||
const transaction = vi.fn(async (cb: (tx: unknown) => Promise<unknown>) =>
|
||||
cb({ assignment: { update: assignmentUpdate, create: assignmentCreate }, resource: { findUnique: resourceFindUnique } }),
|
||||
);
|
||||
|
||||
const db = makeDb({ assignmentCreate, assignmentUpdate, resourceFindUnique, transaction });
|
||||
|
||||
await expect(
|
||||
applyProjectScenario(db, {
|
||||
projectId: "project_1",
|
||||
changes: [
|
||||
{ ...baseChange, resourceId: "resource_1" },
|
||||
{ ...baseChange, resourceId: "resource_2" },
|
||||
],
|
||||
}),
|
||||
).rejects.toThrow("DB constraint violation");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -152,6 +152,7 @@ function createVacationDb(overrides: Record<string, unknown> = {}) {
|
||||
vacation: {
|
||||
findFirst: vi.fn().mockResolvedValue(null),
|
||||
findUnique: vi.fn().mockResolvedValue(sampleVacation),
|
||||
findUniqueOrThrow: vi.fn().mockResolvedValue(sampleVacation),
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
create: vi.fn().mockResolvedValue(sampleVacation),
|
||||
update: vi.fn().mockResolvedValue(sampleVacation),
|
||||
@@ -331,10 +332,11 @@ describe("vacation router", () => {
|
||||
...sampleVacation,
|
||||
status: VacationStatus.PENDING,
|
||||
}),
|
||||
update: vi.fn().mockResolvedValue({
|
||||
findUniqueOrThrow: vi.fn().mockResolvedValue({
|
||||
...sampleVacation,
|
||||
status: VacationStatus.APPROVED,
|
||||
}),
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
},
|
||||
});
|
||||
|
||||
@@ -379,10 +381,11 @@ describe("vacation router", () => {
|
||||
...sampleVacation,
|
||||
status: VacationStatus.PENDING,
|
||||
}),
|
||||
update: vi.fn().mockResolvedValue({
|
||||
findUniqueOrThrow: vi.fn().mockResolvedValue({
|
||||
...sampleVacation,
|
||||
status: VacationStatus.APPROVED,
|
||||
}),
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
},
|
||||
});
|
||||
|
||||
@@ -896,7 +899,8 @@ describe("vacation router", () => {
|
||||
const db = createVacationDb({
|
||||
vacation: {
|
||||
findUnique: vi.fn().mockResolvedValue(sampleVacation),
|
||||
update: vi.fn().mockResolvedValue(updatedVacation),
|
||||
findUniqueOrThrow: vi.fn().mockResolvedValue(updatedVacation),
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
},
|
||||
user: {
|
||||
findUnique: vi.fn().mockResolvedValue({ id: "mgr_1" }),
|
||||
@@ -910,9 +914,9 @@ describe("vacation router", () => {
|
||||
const result = await caller.approve({ id: "vac_1" });
|
||||
|
||||
expect(result.status).toBe(VacationStatus.APPROVED);
|
||||
expect(db.vacation.update).toHaveBeenCalledWith(
|
||||
expect(db.vacation.updateMany).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
where: { id: "vac_1" },
|
||||
where: expect.objectContaining({ id: "vac_1" }),
|
||||
data: expect.objectContaining({
|
||||
status: VacationStatus.APPROVED,
|
||||
rejectionReason: null,
|
||||
@@ -1016,7 +1020,8 @@ describe("vacation router", () => {
|
||||
const db = createVacationDb({
|
||||
vacation: {
|
||||
findUnique: vi.fn().mockResolvedValue(sampleVacation),
|
||||
update: vi.fn().mockResolvedValue(updatedVacation),
|
||||
findUniqueOrThrow: vi.fn().mockResolvedValue(updatedVacation),
|
||||
updateMany: vi.fn().mockResolvedValue({ count: 1 }),
|
||||
},
|
||||
resource: {
|
||||
findUnique: vi.fn().mockResolvedValue(null),
|
||||
@@ -1027,8 +1032,9 @@ describe("vacation router", () => {
|
||||
const result = await caller.reject({ id: "vac_1", rejectionReason: "Team conflict" });
|
||||
|
||||
expect(result.status).toBe(VacationStatus.REJECTED);
|
||||
expect(db.vacation.update).toHaveBeenCalledWith(
|
||||
expect(db.vacation.updateMany).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
where: expect.objectContaining({ id: "vac_1" }),
|
||||
data: expect.objectContaining({
|
||||
status: VacationStatus.REJECTED,
|
||||
rejectionReason: "Team conflict",
|
||||
@@ -1458,8 +1464,8 @@ describe("vacation router", () => {
|
||||
},
|
||||
vacation: {
|
||||
deleteMany: vi.fn().mockResolvedValue({ count: 0 }),
|
||||
findFirst: vi.fn().mockResolvedValue(null),
|
||||
create: vi.fn().mockResolvedValue({}),
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
createMany: vi.fn().mockResolvedValue({ count: 0 }),
|
||||
},
|
||||
};
|
||||
|
||||
@@ -1471,7 +1477,37 @@ describe("vacation router", () => {
|
||||
|
||||
expect(result.created).toBeGreaterThan(0);
|
||||
expect(result.resources).toBe(2);
|
||||
expect(db.vacation.create).toHaveBeenCalled();
|
||||
expect(db.vacation.createMany).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("resolves holidays once per unique country/state combination", async () => {
|
||||
// 3 resources: res_1 and res_2 share the same combo, res_3 is different
|
||||
const db = {
|
||||
resource: {
|
||||
findMany: vi.fn().mockResolvedValue([
|
||||
{ id: "res_1", federalState: "BY", countryId: "de", country: { code: "DE" }, metroCityId: null, metroCity: null },
|
||||
{ id: "res_2", federalState: "BY", countryId: "de", country: { code: "DE" }, metroCityId: null, metroCity: null },
|
||||
{ id: "res_3", federalState: "BE", countryId: "de", country: { code: "DE" }, metroCityId: null, metroCity: null },
|
||||
]),
|
||||
},
|
||||
user: {
|
||||
findUnique: vi.fn().mockResolvedValue({ id: "admin_1" }),
|
||||
},
|
||||
vacation: {
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
createMany: vi.fn().mockResolvedValue({ count: 0 }),
|
||||
},
|
||||
holidayCalendar: {
|
||||
findMany: vi.fn().mockResolvedValue([]),
|
||||
},
|
||||
};
|
||||
|
||||
const caller = createAdminCaller(db);
|
||||
const result = await caller.batchCreatePublicHolidays({ year: 2026 });
|
||||
|
||||
// 3 resources, 2 unique combos → holiday calendars queried twice, not 3 times
|
||||
expect(db.holidayCalendar.findMany).toHaveBeenCalledTimes(2);
|
||||
expect(result.resources).toBe(3);
|
||||
});
|
||||
|
||||
it("skips already existing holidays", async () => {
|
||||
@@ -1485,8 +1521,15 @@ describe("vacation router", () => {
|
||||
findUnique: vi.fn().mockResolvedValue({ id: "admin_1" }),
|
||||
},
|
||||
vacation: {
|
||||
findFirst: vi.fn().mockResolvedValue({ id: "existing" }),
|
||||
create: vi.fn(),
|
||||
// Return one entry per day of 2026 so every holiday date is "already existing"
|
||||
findMany: vi.fn().mockResolvedValue(
|
||||
Array.from({ length: 365 }, (_, i) => {
|
||||
const d = new Date(Date.UTC(2026, 0, 1));
|
||||
d.setUTCDate(d.getUTCDate() + i);
|
||||
return { resourceId: "res_1", startDate: d };
|
||||
}),
|
||||
),
|
||||
createMany: vi.fn(),
|
||||
},
|
||||
};
|
||||
|
||||
@@ -1497,7 +1540,7 @@ describe("vacation router", () => {
|
||||
});
|
||||
|
||||
expect(result.created).toBe(0);
|
||||
expect(db.vacation.create).not.toHaveBeenCalled();
|
||||
expect(db.vacation.createMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("forbids non-admin users", async () => {
|
||||
|
||||
Reference in New Issue
Block a user