creating a dashboard raced itself into a 500
"Could not save that change — ws-layout-platform was not written — Internal Server Error", on a dashboard that had in fact been created. `upsertDashboard` was update-then-insert, which is a read-then-write race, and creating a dashboard walks straight into it: the client sends `workspaces` (the list) and `ws-layout-<id>` (the layout) as two SEPARATE PATCHes, and both call this. Both found no row, both INSERTed, and the loser hit the primary key. The surviving row and the failing insert were stamped one millisecond apart in the log, which is what gave it away — this was never a name collision. One statement now, `ON CONFLICT DO UPDATE`. `set` carries only the fields the caller passed, so whichever request lands second writes its own column and leaves the other's alone. The two orderings become equivalent instead of one of them fatal. Proved against the real table rather than reasoned about: two plain inserts, concurrent fulfilled, rejected <- the reported 500 two upserts, concurrent fulfilled, fulfilled The `where` on the DO UPDATE is not decoration. `dashboards.id` is a GLOBAL primary key fed by a slug, so two accounts naming a dashboard the same thing collide, and an unguarded upsert would let the second silently take over the first's row. Verified on a scratch table that a non-owner's upsert is DROPPED rather than applied or thrown — the row is left untouched. This machine has one account, so that path could not be exercised end to end; the SQL semantics were. That is still the wrong data model — the real fix is a composite key — but it now fails by refusing rather than by handing someone another account's dashboard. tsgo clean, 840 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -86,22 +86,47 @@ function buildDashboardSet(data: UpsertDashboardData, now: Date): Record<string,
|
||||
return set;
|
||||
}
|
||||
|
||||
/**
|
||||
* Create or update a dashboard, in ONE statement.
|
||||
*
|
||||
* This was update-then-insert, which is a read-then-write race, and creating a dashboard walks straight
|
||||
* into it: the client sends `workspaces` (the list) and `ws-layout-<id>` (the layout) as two separate
|
||||
* PATCHes. Both call this, both found no row, both INSERTed, and the loser hit the primary key. Observed
|
||||
* 2026-08-15 — the surviving row and the failing insert were stamped ONE MILLISECOND apart, and the user
|
||||
* saw "Internal Server Error" for a dashboard that had in fact been created.
|
||||
*
|
||||
* `ON CONFLICT DO UPDATE` makes the two orderings equivalent instead of one of them fatal. `set` only
|
||||
* carries the fields the caller actually passed, so whichever request lands second updates its own column
|
||||
* and leaves the other's alone.
|
||||
*
|
||||
* The `where` is not decoration. `dashboards.id` is a GLOBAL primary key fed by a slug, so two accounts
|
||||
* naming a dashboard the same thing collide — and without this, the second one's write would silently
|
||||
* take over the first one's row. With it, the update matches nothing and the write is dropped. That is
|
||||
* still wrong, but it is wrong in the direction of not handing someone another account's dashboard, and
|
||||
* the real fix is a composite key (see TODO.md).
|
||||
*/
|
||||
export async function upsertDashboard(userId: number, id: string, data: UpsertDashboardData): Promise<void> {
|
||||
const now = new Date();
|
||||
if (await updateDashboard(userId, id, data)) return;
|
||||
|
||||
await db.insert(dashboards).values({
|
||||
id,
|
||||
userId,
|
||||
name: data.name ?? id,
|
||||
config: data.config ?? {},
|
||||
layout: data.layout ?? null,
|
||||
terminals: data.terminals ?? {},
|
||||
hostTerminals: data.hostTerminals ?? {},
|
||||
sortOrder: data.sortOrder ?? 0,
|
||||
createdAt: now,
|
||||
updatedAt: now,
|
||||
});
|
||||
await db
|
||||
.insert(dashboards)
|
||||
.values({
|
||||
id,
|
||||
userId,
|
||||
name: data.name ?? id,
|
||||
config: data.config ?? {},
|
||||
layout: data.layout ?? null,
|
||||
terminals: data.terminals ?? {},
|
||||
hostTerminals: data.hostTerminals ?? {},
|
||||
sortOrder: data.sortOrder ?? 0,
|
||||
createdAt: now,
|
||||
updatedAt: now,
|
||||
})
|
||||
.onConflictDoUpdate({
|
||||
target: dashboards.id,
|
||||
set: buildDashboardSet(data, now),
|
||||
where: eq(dashboards.userId, userId),
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user