From 86fa57d8bf08321d315e03cce8cee137563ece42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 9 Oct 2026 17:42:00 +0200 Subject: [PATCH] fix(dashboard): queue concurrent layout writes behind a row lock Parallel PUTs to /api/dashboard/chart-overlay-prefs from one account ran in Serializable transactions, which aborted all but one with a serialization failure and answered 500. Lock the user row with FOR UPDATE instead, so each writer waits for the previous commit and reads it. The layout reset on DELETE /api/dashboard/widgets takes the same lock. --- .../dashboard/chart-overlay-prefs/route.ts | 58 ++++--- .../dashboard/widgets/__tests__/route.test.ts | 19 +- src/app/api/dashboard/widgets/route.ts | 51 +++--- ...ashboard-chart-overlay-concurrency.test.ts | 164 ++++++++++++++++++ 4 files changed, 234 insertions(+), 58 deletions(-) create mode 100644 tests/integration/dashboard-chart-overlay-concurrency.test.ts diff --git a/src/app/api/dashboard/chart-overlay-prefs/route.ts b/src/app/api/dashboard/chart-overlay-prefs/route.ts index daa87e2ad..35338d159 100644 --- a/src/app/api/dashboard/chart-overlay-prefs/route.ts +++ b/src/app/api/dashboard/chart-overlay-prefs/route.ts @@ -97,39 +97,41 @@ export const PUT = apiHandler(async (request: NextRequest) => { return returnAllZodIssues(parsed.error, 422); } - // Read-modify-write inside a Serializable transaction so two - // concurrent toggles (e.g. user opens two tabs and flips overlays - // on different charts) can't drop one another's update by reading - // the same layout snapshot and clobbering each other on write. + // Read-modify-write under a row lock, so two concurrent toggles (two + // tabs, two charts flipped in quick succession) can neither drop one + // another's update nor fail. `FOR UPDATE` queues every writer for this + // account behind the one holding the row, and the read happens through + // the same `tx`, after the lock, so it sees the previous writer's commit. + // A Serializable transaction used to guard this instead: it kept the + // updates from clobbering each other by aborting all but one of them + // with a serialization failure, which surfaced as a 500. // Resolver normalises legacy / missing fields, so layouts saved // before v1.4.18 pick up the new field with default-empty prefs // without a one-off migration. - await prisma.$transaction( - async (tx) => { - const row = await tx.user.findUnique({ - where: { id: user.id }, - select: { dashboardWidgetsJson: true }, - }); + await prisma.$transaction(async (tx) => { + await tx.$queryRaw`SELECT id FROM users WHERE id = ${user.id} FOR UPDATE`; + const row = await tx.user.findUnique({ + where: { id: user.id }, + select: { dashboardWidgetsJson: true }, + }); - const current = resolveDashboardLayout(row?.dashboardWidgetsJson); - const next: DashboardLayout = { - ...current, - chartOverlayPrefs: { - ...(current.chartOverlayPrefs ?? {}), - [parsed.data.chartKey]: parsed.data.prefs, - }, - }; - const normalized = serializeDashboardLayout(next); + const current = resolveDashboardLayout(row?.dashboardWidgetsJson); + const next: DashboardLayout = { + ...current, + chartOverlayPrefs: { + ...(current.chartOverlayPrefs ?? {}), + [parsed.data.chartKey]: parsed.data.prefs, + }, + }; + const normalized = serializeDashboardLayout(next); - await tx.user.update({ - where: { id: user.id }, - data: { - dashboardWidgetsJson: toJson(normalized), - }, - }); - }, - { isolationLevel: "Serializable" }, - ); + await tx.user.update({ + where: { id: user.id }, + data: { + dashboardWidgetsJson: toJson(normalized), + }, + }); + }); // The partial-update path mutates the same `User.dashboardWidgetsJson` // blob the `/api/dashboard/widgets` GET reads from. Bust the cache so diff --git a/src/app/api/dashboard/widgets/__tests__/route.test.ts b/src/app/api/dashboard/widgets/__tests__/route.test.ts index 0e9ce687f..fc9bc3e33 100644 --- a/src/app/api/dashboard/widgets/__tests__/route.test.ts +++ b/src/app/api/dashboard/widgets/__tests__/route.test.ts @@ -11,6 +11,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { NextRequest } from "next/server"; +const transactionLock = vi.hoisted(() => vi.fn(async () => [])); const transactionUser = vi.hoisted(() => ({ findUnique: vi.fn(), update: vi.fn(), @@ -123,8 +124,12 @@ beforeEach(() => { // test below the dead-switch block assumes. vi.mocked(isModuleEnabled).mockResolvedValue(true as never); vi.mocked(prisma.$transaction).mockImplementation((async ( - callback: (tx: { user: typeof transactionUser }) => Promise, - ) => callback({ user: transactionUser })) as never); + callback: (tx: { + user: typeof transactionUser; + $queryRaw: typeof transactionLock; + }) => Promise, + ) => + callback({ user: transactionUser, $queryRaw: transactionLock })) as never); }); describe("PUT /api/dashboard/widgets — 422 multi-issue envelope (v1.4.42 W2)", () => { @@ -942,9 +947,13 @@ describe("DELETE /api/dashboard/widgets — web-owned reset scope", () => { expect(res.status).toBe(200); expect(prisma.$transaction).toHaveBeenCalledOnce(); - expect(vi.mocked(prisma.$transaction).mock.calls[0]?.[1]).toEqual({ - isolationLevel: "Serializable", - }); + // A row lock, not Serializable isolation, which failed concurrent + // writers with a serialization error. + expect(vi.mocked(prisma.$transaction).mock.calls[0]?.[1]).toBeUndefined(); + expect(transactionLock).toHaveBeenCalledOnce(); + expect( + (transactionLock.mock.calls[0] as unknown[] | undefined)?.[0], + ).toEqual(expect.arrayContaining([expect.stringContaining("FOR UPDATE")])); expect(prisma.user.findUnique).not.toHaveBeenCalled(); expect(prisma.user.update).not.toHaveBeenCalled(); expect(transactionUser.findUnique).toHaveBeenCalledOnce(); diff --git a/src/app/api/dashboard/widgets/route.ts b/src/app/api/dashboard/widgets/route.ts index 688355b0e..d4d3675b1 100644 --- a/src/app/api/dashboard/widgets/route.ts +++ b/src/app/api/dashboard/widgets/route.ts @@ -494,32 +494,33 @@ export const DELETE = apiHandler(async () => { // dashboard to defaults is the loudest possible version of redecorating it. const { user } = await requireAuth(); - const { normalized, updatedAt } = await prisma.$transaction( - async (tx) => { - const existing = await tx.user.findUnique({ - where: { id: user.id }, - select: { dashboardWidgetsJson: true }, - }); - const existingLayout = resolveDashboardLayout( - existing?.dashboardWidgetsJson, - ); - const normalized = serializeDashboardLayout({ - ...existingLayout, - widgets: DEFAULT_DASHBOARD_LAYOUT.widgets, - enabledHeroItemKinds: DEFAULT_DASHBOARD_LAYOUT.enabledHeroItemKinds, - // A reset brings a hidden top card back. - todayCardVisible: DEFAULT_DASHBOARD_LAYOUT.todayCardVisible, - }); + // Under a row lock rather than Serializable isolation: a reset racing a + // chart-overlay toggle on the same account waits for it instead of + // failing with a serialization error (a 500). + const { normalized, updatedAt } = await prisma.$transaction(async (tx) => { + await tx.$queryRaw`SELECT id FROM users WHERE id = ${user.id} FOR UPDATE`; + const existing = await tx.user.findUnique({ + where: { id: user.id }, + select: { dashboardWidgetsJson: true }, + }); + const existingLayout = resolveDashboardLayout( + existing?.dashboardWidgetsJson, + ); + const normalized = serializeDashboardLayout({ + ...existingLayout, + widgets: DEFAULT_DASHBOARD_LAYOUT.widgets, + enabledHeroItemKinds: DEFAULT_DASHBOARD_LAYOUT.enabledHeroItemKinds, + // A reset brings a hidden top card back. + todayCardVisible: DEFAULT_DASHBOARD_LAYOUT.todayCardVisible, + }); - const updated = await tx.user.update({ - where: { id: user.id }, - data: { dashboardWidgetsJson: toJson(normalized) }, - select: { updatedAt: true }, - }); - return { normalized, updatedAt: updated.updatedAt.toISOString() }; - }, - { isolationLevel: "Serializable" }, - ); + const updated = await tx.user.update({ + where: { id: user.id }, + data: { dashboardWidgetsJson: toJson(normalized) }, + select: { updatedAt: true }, + }); + return { normalized, updatedAt: updated.updatedAt.toISOString() }; + }); annotate({ action: { name: "dashboard.widgets.reset" } }); diff --git a/tests/integration/dashboard-chart-overlay-concurrency.test.ts b/tests/integration/dashboard-chart-overlay-concurrency.test.ts new file mode 100644 index 000000000..7ccf4b471 --- /dev/null +++ b/tests/integration/dashboard-chart-overlay-concurrency.test.ts @@ -0,0 +1,164 @@ +/** + * Concurrent writes to one account's dashboard layout blob. + * + * `PUT /api/dashboard/chart-overlay-prefs` read-modifies-writes + * `User.dashboardWidgetsJson`. It used to do so in a Serializable + * transaction, which kept parallel toggles from clobbering one another by + * aborting all but one with a serialization failure, and every aborted one + * answered 500. A row lock queues them instead: every request succeeds, and + * the stored layout holds every toggle, because each writer reads after the + * previous one committed. A reset (`DELETE /api/dashboard/widgets`) racing + * the toggles takes the same lock and keeps the per-chart preferences. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +process.env.ENCRYPTION_KEY ??= + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + +import { cookieJar, headerJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; +import { CHART_OVERLAY_KEYS } from "@/lib/dashboard-layout"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + cookieJar.clear(); + headerJar.clear(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +async function seedUser(): Promise<{ userId: string }> { + const prisma = getPrismaClient(); + const user = await prisma.user.create({ + data: { + username: "chart-overlay-race-user", + email: "chart-overlay-race@example.test", + role: "USER", + }, + }); + const session = await prisma.session.create({ + data: { + userId: user.id, + expiresAt: new Date(Date.now() + 60_000), + }, + }); + cookieJar.set("healthlog_session", session.id); + return { userId: user.id }; +} + +type Handler = (req: Request) => Promise; + +function put(chartKey: string, showTrendArrow: boolean) { + return new Request("http://localhost/api/dashboard/chart-overlay-prefs", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + chartKey, + prefs: { + showTrendIndicator: true, + showTrendArrow, + showTargetRange: false, + comparisonBaseline: "none", + }, + }), + }); +} + +async function storedPrefs(userId: string) { + const row = await getPrismaClient().user.findUniqueOrThrow({ + where: { id: userId }, + select: { dashboardWidgetsJson: true }, + }); + const layout = row.dashboardWidgetsJson as { + chartOverlayPrefs?: Record; + } | null; + return layout?.chartOverlayPrefs ?? {}; +} + +describe("concurrent chart-overlay writes from one account", () => { + it("answers 200 to every parallel PUT and keeps every chart's toggle", async () => { + const { userId } = await seedUser(); + const { PUT } = + await import("@/app/api/dashboard/chart-overlay-prefs/route"); + const keys = CHART_OVERLAY_KEYS.slice(0, 8); + expect(keys.length).toBe(8); + + const responses = await Promise.all( + keys.map((key) => (PUT as Handler)(put(key, true))), + ); + expect(responses.map((res) => res.status)).toEqual(keys.map(() => 200)); + + const prefs = await storedPrefs(userId); + for (const key of keys) { + expect(prefs[key]?.showTrendArrow, key).toBe(true); + } + }); + + it("settles parallel writes to the same chart on one of the written values", async () => { + const { userId } = await seedUser(); + const { PUT } = + await import("@/app/api/dashboard/chart-overlay-prefs/route"); + const responses = await Promise.all( + Array.from({ length: 10 }, (_, i) => + (PUT as Handler)(put("weight", i % 2 === 0)), + ), + ); + expect(responses.map((res) => res.status)).toEqual( + Array.from({ length: 10 }, () => 200), + ); + const prefs = await storedPrefs(userId); + expect(typeof prefs.weight?.showTrendArrow).toBe("boolean"); + }); + + it("keeps every toggle when a layout reset races them", async () => { + const { userId } = await seedUser(); + const { PUT } = + await import("@/app/api/dashboard/chart-overlay-prefs/route"); + const { DELETE } = await import("@/app/api/dashboard/widgets/route"); + const keys = CHART_OVERLAY_KEYS.slice(0, 6); + + const responses = await Promise.all([ + ...keys.map((key) => (PUT as Handler)(put(key, true))), + (DELETE as Handler)( + new Request("http://localhost/api/dashboard/widgets", { + method: "DELETE", + }), + ), + ]); + expect(responses.map((res) => res.status)).toEqual( + responses.map(() => 200), + ); + + const prefs = await storedPrefs(userId); + for (const key of keys) { + expect(prefs[key]?.showTrendArrow, key).toBe(true); + } + }); +});