From 7e1351bde2a7f3172c36d42f4bee7ff2accb0ede Mon Sep 17 00:00:00 2001 From: toanhoang Date: Tue, 4 Aug 2026 06:04:23 -0400 Subject: [PATCH] fix: isolate Prisma Client per test file, re-enable fileParallelism Root-cause fix for cross-file DB pollution in the vitest suite, which surfaced as intermittent FK-violation failures once the suite hit 12 test files. tests/setup.ts previously mutated the global process.env.DATABASE_URL and relied on src/lib/prisma.ts's singleton picking it up, which is racy across files sharing a worker process (process.env isn't reset between files even though Vitest resets each file's module registry). fileParallelism: false only masked this. Now each test file's own tests/setup.ts creates its own throwaway Postgres database and vi.mock()s "@/lib/prisma" with a dedicated PrismaClient constructed via the explicit datasourceUrl option, so no mutable state is shared between files and fileParallelism can go back to its default (true). Verified with 4 consecutive full `npm test` runs (327/327 passing each time) and no leftover throwaway databases after each run. --- CLAUDE.md | 4 +-- tests/setup.ts | 81 ++++++++++++++++++++++++++++++++++++------------ vitest.config.ts | 20 +++++------- 3 files changed, 71 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2e6ff5f..c8178dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,10 +88,10 @@ Backend CRUD xong cho toàn bộ entity chốt tới nay: `Request` (chỉ state - Phân cấp tài sản (2026-08-03): `AssetCategory` (`valueGroup` high/low_value) → `AssetCategoryAttributeDef` (thuộc tính động theo category, unique `(categoryId,key)`) → `AssetModel` (`specs` JSONB, validate động qua Zod build runtime từ AttributeDef). `AssetItem.model` (string) retrofit sang `AssetItem.modelId` (FK). - `Assignment` xong + code-reviewer gate PASS (2026-08-03): CRUD chuẩn + `transferAssignment()` (đóng Assignment cũ + tạo mới cùng 1 transaction, cố tình **không** đụng `AssetItem.status`/`License.seatsUsed` vì resource không thực sự rảnh giữa chừng khi transfer) — fix 1 gap HIGH code-reviewer phát hiện (trước đó phải gọi 2 request rời, có race window + không rollback). `PATCH /api/assignments/[id]` dispatch theo body: có `employeeId` → transfer, có `returnedAt` → return, cả hai → 400. CONFIRMED qua test-runner độc lập: 284/284 test, tsc/lint sạch. - UI cơ bản đã có (nhánh `worktree-ui-basic`, đã merge vào `feat/assignment`, đã push): shared nav + fetch hook + table/select/label/textarea primitives, list+create cho mọi entity, list+create+return cho `Assignment`. Chưa có UI cho transfer (chỉ có API). +- Test-infra: fix gốc rễ race condition đã note ở lần cập nhật trước (2026-08-04) — bug lộ ra thật khi lên tới file test thứ 12 (`tests/maintenance-records.test.ts`), gây `PrismaClientKnownRequestError: Foreign key constraint violated` không ổn định ở `beforeEach` của 2 file khác dù chạy riêng lẻ vẫn xanh (root cause: `process.env.DATABASE_URL` bị nhiều file test ghi đè lên cùng 1 biến toàn cục, PrismaClient của `src/lib/prisma.ts` singleton rebind sai DB khi file này chạy đè lên timing của file khác). Fix thật: `tests/setup.ts` giờ dùng `vi.mock("@/lib/prisma", async () => {...})` — mỗi file test tự tạo DB tạm riêng + 1 `PrismaClient` riêng (qua option `datasourceUrl`, không đụng `process.env` toàn cục nữa) — không còn state nào chia sẻ giữa các file. `fileParallelism: false` đã bỏ khỏi `vitest.config.ts` (về default `true`). Verify: 4 lần `npm test` liên tiếp qua Docker, cả 4 lần đều 327/327 xanh, không leftover `test_*` database nào sau khi chạy xong. ## Việc còn dở -- `fileParallelism: false` (fix race condition test-infra, xem git blame `vitest.config.ts`) đúng nhưng "cùn" — chặn song song toàn bộ suite thay vì fix gốc rễ (`tests/setup.ts` ghi đè `process.env.DATABASE_URL` toàn cục). Đáng cân nhắc hơn nay đã 11 file test (~40s) — refactor mỗi file tự có Prisma Client riêng rồi bật lại parallelism, chưa gấp. - Fix timing side-channel ở `verifyUserCredentials`: trả về sớm khi email không tồn tại (bỏ qua bcrypt compare) → thời gian phản hồi khác nhau giữa "email sai" vs "password sai", có thể dò được email tồn tại. Fix: luôn chạy `bcrypt.compare()` kể cả khi không tìm thấy user (so với 1 hash giả cố định). - Cải thiện `scripts/create-user.ts` để password không lộ qua CLI arg (đọc từ stdin/prompt thay vì `--password`) — chấp nhận được vì chỉ IT admin tự chạy tay, không phải self-registration công khai. - Bump `vitest` lên 4.x (fix advisory critical GHSA-5xrq-8626-4rwp — không khai thác được vì project không chạy `vitest --ui`) — không khẩn cấp. @@ -108,4 +108,4 @@ Backend CRUD xong cho toàn bộ entity chốt tới nay: `Request` (chỉ state - Auth/phân quyền (ai được approve request, ai được xác nhận nhận hàng/bàn giao) — vẫn flat, chưa có role. --- -Cập nhật lần cuối: 2026-08-03 (Assignment transfer fix + code-reviewer gate PASS, UI cơ bản merge từ worktree-ui-basic, dọn gọn "Trạng thái hiện tại"/"Việc còn dở") +Cập nhật lần cuối: 2026-08-04 (fix root-cause race condition test-infra — per-file PrismaClient qua `vi.mock`, bật lại `fileParallelism`) diff --git a/tests/setup.ts b/tests/setup.ts index 42b6432..32cc0ff 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -1,18 +1,46 @@ // Creates a throwaway Postgres database per test-file run against the -// docker-compose `db` service, points DATABASE_URL at it so tests never -// touch a shared dev database, and drops it afterwards. Mirrors the old +// docker-compose `db` service, and drops it afterwards. Mirrors the old // sqlite temp-file-per-run isolation, adapted to Postgres (CREATE/DROP // DATABASE instead of a throwaway file). // // Creating/dropping the throwaway database itself stays raw `pg` (test // infra, not the app's data-access layer) — but applying the schema to the -// fresh database, and tearing down the pooled connection afterwards, now -// goes through Prisma instead of the old db.ts. +// fresh database goes through `prisma migrate deploy`, and the actual +// per-file PrismaClient used by both test bodies and app code (route +// handlers etc., all of which import `prisma` from "@/lib/prisma") is a +// throwaway PrismaClient instance created here, wired up explicitly. +// +// Root-cause note (2026-08-04): this used to work by mutating the global +// `process.env.DATABASE_URL` in a `beforeAll` and relying on the app's +// `src/lib/prisma.ts` singleton to pick that up. That's unsafe the moment +// two test files' setup/query timing can interleave within the same +// worker process — `process.env` is shared across all test files that +// happen to run in the same worker (Vitest's `test.isolate` resets each +// file's *module registry*, not `process.env`), so one file's mutation can +// leak into another's PrismaClient construction/reconnect. That forced +// `fileParallelism: false` as a blunt workaround (see git blame on +// vitest.config.ts), and even that stopped being sufficient once the +// worker pool got reused across enough sequential files (12 test files — +// see git history around this comment for the concrete FK-violation +// failures that motivated this rewrite). +// +// Fix: never touch `process.env.DATABASE_URL` at all. Instead, `vi.mock` +// "@/lib/prisma" (the same module id every test file and every app module +// imports `prisma` from) with a factory that creates this file's own +// throwaway database and returns a dedicated `PrismaClient` pointed at it +// via the explicit `datasourceUrl` constructor option (Prisma 5.2+) — +// never via the ambient env var. `vi.mock` factories are cached per +// isolated test-file module registry, so every import of "@/lib/prisma" +// within a given test file (the test file itself, and any route handler / +// lib module it pulls in) resolves to that one file-scoped client, while +// two different test files never share any mutable global state. That +// makes `fileParallelism: true` (the default, restored in +// vitest.config.ts) safe again. import { randomUUID } from "node:crypto"; import { execSync } from "node:child_process"; import { Client } from "pg"; -import { afterAll, beforeAll } from "vitest"; -import { prisma } from "@/lib/prisma"; +import { PrismaClient } from "@prisma/client"; +import { afterAll, vi } from "vitest"; const baseUrl = new URL( process.env.DATABASE_URL || @@ -26,6 +54,12 @@ function adminUrl(): string { return url.toString(); } +function testDbUrl(): string { + const url = new URL(baseUrl.toString()); + url.pathname = `/${testDbName}`; + return url.toString(); +} + async function withAdminClient(fn: (client: Client) => Promise): Promise { const client = new Client({ connectionString: adminUrl() }); await client.connect(); @@ -36,27 +70,36 @@ async function withAdminClient(fn: (client: Client) => Promise): Promise { - await withAdminClient((client) => client.query(`CREATE DATABASE ${testDbName}`)); +// Set by the "@/lib/prisma" mock factory below once it has created this +// file's throwaway database and client, so afterAll can tear both down. +// Populated synchronously-in-effect before any test body can run, since +// the mock factory is awaited as part of resolving the first import of +// "@/lib/prisma" (which every test file and every route/lib module under +// test does at the top of its own module graph, ahead of any test body). +let testPrisma: PrismaClient | undefined; - const testUrl = new URL(baseUrl.toString()); - testUrl.pathname = `/${testDbName}`; - process.env.DATABASE_URL = testUrl.toString(); +vi.mock("@/lib/prisma", async () => { + await withAdminClient((client) => client.query(`CREATE DATABASE ${testDbName}`)); // Apply the Prisma migration history to the fresh throwaway database. // `migrate deploy` replays the same real migrations production uses // (prisma/migrations), rather than a schema-diffing shortcut like // `db push` — keeping this test setup honest about what actually runs - // outside tests too. - execSync("npx prisma migrate deploy", { stdio: "inherit" }); -}, 30_000); + // outside tests too. Pass the target database explicitly via the + // subprocess's own `env` rather than mutating this process's + // `process.env` — the whole point of this rewrite is to avoid any + // shared mutable global between test files. + execSync("npx prisma migrate deploy", { + stdio: "inherit", + env: { ...process.env, DATABASE_URL: testDbUrl() }, + }); + + testPrisma = new PrismaClient({ datasourceUrl: testDbUrl() }); + return { prisma: testPrisma }; +}); afterAll(async () => { - // Disconnect the app's Prisma Client singleton first so the admin - // connection is free to drop the database (DROP DATABASE ... WITH - // (FORCE) would also disconnect it, but closing cleanly first avoids - // relying on that). - await prisma.$disconnect(); + await testPrisma?.$disconnect(); await withAdminClient((client) => client.query(`DROP DATABASE IF EXISTS ${testDbName} WITH (FORCE)`) ); diff --git a/vitest.config.ts b/vitest.config.ts index 0c6032f..c7de832 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -20,18 +20,12 @@ export default defineConfig({ // second worktree existed. include: ["tests/**/*.test.ts"], setupFiles: ["./tests/setup.ts"], - // Each test file spins up its own throwaway Postgres database by - // mutating process.env.DATABASE_URL in tests/setup.ts's beforeAll (see - // that file). That per-file DB switch is not safe under Vitest's default - // parallel file execution: two files' beforeAll/query timing can - // interleave such that both end up issuing real queries against the - // same throwaway DB (confirmed empirically once Request gained a real - // FK to Employee — tests/employees.test.ts and tests/requests.test.ts - // started intermittently deleting/reading each other's rows, causing - // spurious FK-violation and row-count failures that disappear when run - // sequentially). Disabling file parallelism trades a bit of wall-clock - // test time for correctness — each file's throwaway DB is now genuinely - // private for the duration of that file's run. - fileParallelism: false, + // Each test file gets its own throwaway Postgres database and its own + // dedicated PrismaClient, wired up via a `vi.mock("@/lib/prisma", ...)` + // factory in tests/setup.ts (not by mutating the ambient + // process.env.DATABASE_URL, which used to be shared/racy across files — + // see the long comment at the top of tests/setup.ts for the history). + // With no mutable state shared between files, parallel file execution + // (Vitest's default) is safe again. }, });