From 493df5f25d8125b3f409f6daad091ece3319472a Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Fri, 15 May 2026 14:58:49 +0000 Subject: [PATCH] batch3 T2 review fixes: movePane sentinel + count race + PATCH atomicity - Move sentinel from -1 to -100 (outside the negate range) so moves from position 0 no longer collide with negated row at -1 - Pull count check + position validation inside sql.begin in POST so two concurrent inserts can't both pass the max-5 guard - Wrap movePane + state UPDATE in a single transaction in PATCH so partial failures roll back consistently Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/server/src/routes/panes.ts | 114 +++++++++++++++++--------------- 1 file changed, 62 insertions(+), 52 deletions(-) diff --git a/apps/server/src/routes/panes.ts b/apps/server/src/routes/panes.ts index 9bb04ac..43cc30e 100644 --- a/apps/server/src/routes/panes.ts +++ b/apps/server/src/routes/panes.ts @@ -1,4 +1,5 @@ import type { FastifyInstance } from 'fastify'; +import type { TransactionSql } from 'postgres'; import type { Sql } from '../db.js'; import type { Pane, PaneCreateRequest, PaneUpdateRequest } from '../types/api.js'; @@ -6,29 +7,28 @@ const VALID_KINDS = new Set(['chat', 'file_browser']); const MAX_PANES = 5; async function movePane( - sql: Sql, + tx: TransactionSql, paneId: string, sid: string, oldPos: number, newPos: number ): Promise { if (oldPos === newPos) return; - await sql.begin(async (tx) => { - // Move target pane to temporary negative slot to avoid collision - await tx`UPDATE session_panes SET position = -1 WHERE id = ${paneId}`; - if (newPos > oldPos) { - await tx`UPDATE session_panes SET position = -position - WHERE session_id = ${sid} AND position > ${oldPos} AND position <= ${newPos}`; - await tx`UPDATE session_panes SET position = -position - 1 - WHERE session_id = ${sid} AND position < 0 AND id != ${paneId}`; - } else { - await tx`UPDATE session_panes SET position = -position - 2 - WHERE session_id = ${sid} AND position >= ${newPos} AND position < ${oldPos}`; - await tx`UPDATE session_panes SET position = -position - 1 - WHERE session_id = ${sid} AND position < -1`; - } - await tx`UPDATE session_panes SET position = ${newPos} WHERE id = ${paneId}`; - }); + // Move target pane to a sentinel well outside the negate range [-MAX_PANES, -1] + // so it never collides with negated rows during the shift steps. + await tx`UPDATE session_panes SET position = -100 WHERE id = ${paneId}`; + if (newPos > oldPos) { + await tx`UPDATE session_panes SET position = -position + WHERE session_id = ${sid} AND position > ${oldPos} AND position <= ${newPos}`; + await tx`UPDATE session_panes SET position = -position - 1 + WHERE session_id = ${sid} AND position < 0 AND id != ${paneId}`; + } else { + await tx`UPDATE session_panes SET position = -position - 2 + WHERE session_id = ${sid} AND position >= ${newPos} AND position < ${oldPos}`; + await tx`UPDATE session_panes SET position = -position - 1 + WHERE session_id = ${sid} AND position < 0 AND id != ${paneId}`; + } + await tx`UPDATE session_panes SET position = ${newPos} WHERE id = ${paneId}`; } export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void { @@ -70,32 +70,26 @@ export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void { } const sid = req.params.id; - - const countRows = await sql<{ n: number }[]>` - SELECT COUNT(*)::int AS n FROM session_panes WHERE session_id = ${sid} - `; - const count = countRows[0]?.n ?? 0; - - if (count >= MAX_PANES) { - reply.code(400); - return { error: `session already has ${MAX_PANES} panes (maximum)` }; - } - - // Determine insert position - let insertPos: number; - if (position === undefined || position === null) { - insertPos = count; - } else { - if (position < 0 || position > count) { - reply.code(400); - return { error: `position must be between 0 and ${count} (existing count)` }; - } - insertPos = position; - } - const state = {}; + let insertError: string | null = null; const inserted = await sql.begin(async (tx) => { + const countResult = await tx<{ n: number }[]>` + SELECT COUNT(*)::int AS n FROM session_panes WHERE session_id = ${sid} + `; + const n = countResult[0]!.n; + if (n >= MAX_PANES) { + throw new Error('MAX_PANES_EXCEEDED'); + } + let insertPos: number; + if (position === undefined || position === null) { + insertPos = n; + } else { + if (position < 0 || position > n) { + throw new Error('OUT_OF_BOUNDS'); + } + insertPos = position; + } await tx`UPDATE session_panes SET position = -position - 1 WHERE session_id = ${sid} AND position >= ${insertPos}`; const [row] = await tx` @@ -106,8 +100,24 @@ export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void { await tx`UPDATE session_panes SET position = -position WHERE session_id = ${sid} AND position < 0`; return row; + }).catch((err: Error) => { + insertError = err.message; + return null; }); + if (insertError === 'MAX_PANES_EXCEEDED') { + reply.code(400); + return { error: `session already has ${MAX_PANES} panes (maximum)` }; + } + if (insertError === 'OUT_OF_BOUNDS') { + reply.code(400); + return { error: `position out of bounds` }; + } + if (insertError) { + reply.code(500); + return { error: 'internal error' }; + } + reply.code(201); return inserted as Pane; } @@ -149,18 +159,18 @@ export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void { } } - // Apply position change if needed - if (position !== undefined && position !== oldPos) { - await movePane(sql, req.params.id, sid, oldPos, position); - } - - // Apply state change if provided - if (state !== undefined) { - await sql` - UPDATE session_panes SET state = ${JSON.stringify(state)}::jsonb - WHERE id = ${req.params.id} - `; - } + // Apply position and/or state changes atomically + await sql.begin(async (tx) => { + if (position !== undefined && position !== oldPos) { + await movePane(tx, req.params.id, sid, oldPos, position); + } + if (state !== undefined) { + await tx` + UPDATE session_panes SET state = ${JSON.stringify(state)}::jsonb + WHERE id = ${req.params.id} + `; + } + }); const [updated] = await sql` SELECT id, session_id, position, kind, state, created_at