mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-07 04:12:04 +00:00
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.
This commit is contained in:
1 parent
09f84ef418
commit
86fa57d8bf
4 files changed
+234
-58
No files matched your search
@@ -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
|
||||
|
||||
@@ -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<unknown>,
|
||||
) => callback({ user: transactionUser })) as never);
|
||||
callback: (tx: {
|
||||
user: typeof transactionUser;
|
||||
$queryRaw: typeof transactionLock;
|
||||
}) => Promise<unknown>,
|
||||
) =>
|
||||
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();
|
||||
|
||||
@@ -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" } });
|
||||
|
||||
|
||||
@@ -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<Response>;
|
||||
|
||||
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<string, { showTrendArrow: boolean }>;
|
||||
} | 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);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user