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) <noreply@anthropic.com>
This commit is contained in:
2026-05-15 14:58:49 +00:00
parent 2bc626a40a
commit 493df5f25d

View File

@@ -1,4 +1,5 @@
import type { FastifyInstance } from 'fastify'; import type { FastifyInstance } from 'fastify';
import type { TransactionSql } from 'postgres';
import type { Sql } from '../db.js'; import type { Sql } from '../db.js';
import type { Pane, PaneCreateRequest, PaneUpdateRequest } from '../types/api.js'; import type { Pane, PaneCreateRequest, PaneUpdateRequest } from '../types/api.js';
@@ -6,16 +7,16 @@ const VALID_KINDS = new Set(['chat', 'file_browser']);
const MAX_PANES = 5; const MAX_PANES = 5;
async function movePane( async function movePane(
sql: Sql, tx: TransactionSql,
paneId: string, paneId: string,
sid: string, sid: string,
oldPos: number, oldPos: number,
newPos: number newPos: number
): Promise<void> { ): Promise<void> {
if (oldPos === newPos) return; if (oldPos === newPos) return;
await sql.begin(async (tx) => { // Move target pane to a sentinel well outside the negate range [-MAX_PANES, -1]
// Move target pane to temporary negative slot to avoid collision // so it never collides with negated rows during the shift steps.
await tx`UPDATE session_panes SET position = -1 WHERE id = ${paneId}`; await tx`UPDATE session_panes SET position = -100 WHERE id = ${paneId}`;
if (newPos > oldPos) { if (newPos > oldPos) {
await tx`UPDATE session_panes SET position = -position await tx`UPDATE session_panes SET position = -position
WHERE session_id = ${sid} AND position > ${oldPos} AND position <= ${newPos}`; WHERE session_id = ${sid} AND position > ${oldPos} AND position <= ${newPos}`;
@@ -25,10 +26,9 @@ async function movePane(
await tx`UPDATE session_panes SET position = -position - 2 await tx`UPDATE session_panes SET position = -position - 2
WHERE session_id = ${sid} AND position >= ${newPos} AND position < ${oldPos}`; WHERE session_id = ${sid} AND position >= ${newPos} AND position < ${oldPos}`;
await tx`UPDATE session_panes SET position = -position - 1 await tx`UPDATE session_panes SET position = -position - 1
WHERE session_id = ${sid} AND position < -1`; WHERE session_id = ${sid} AND position < 0 AND id != ${paneId}`;
} }
await tx`UPDATE session_panes SET position = ${newPos} WHERE id = ${paneId}`; await tx`UPDATE session_panes SET position = ${newPos} WHERE id = ${paneId}`;
});
} }
export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void { 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 sid = req.params.id;
const state = {};
const countRows = await sql<{ n: number }[]>` 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} SELECT COUNT(*)::int AS n FROM session_panes WHERE session_id = ${sid}
`; `;
const count = countRows[0]?.n ?? 0; const n = countResult[0]!.n;
if (n >= MAX_PANES) {
if (count >= MAX_PANES) { throw new Error('MAX_PANES_EXCEEDED');
reply.code(400);
return { error: `session already has ${MAX_PANES} panes (maximum)` };
} }
// Determine insert position
let insertPos: number; let insertPos: number;
if (position === undefined || position === null) { if (position === undefined || position === null) {
insertPos = count; insertPos = n;
} else { } else {
if (position < 0 || position > count) { if (position < 0 || position > n) {
reply.code(400); throw new Error('OUT_OF_BOUNDS');
return { error: `position must be between 0 and ${count} (existing count)` };
} }
insertPos = position; insertPos = position;
} }
const state = {};
const inserted = await sql.begin(async (tx) => {
await tx`UPDATE session_panes SET position = -position - 1 await tx`UPDATE session_panes SET position = -position - 1
WHERE session_id = ${sid} AND position >= ${insertPos}`; WHERE session_id = ${sid} AND position >= ${insertPos}`;
const [row] = await tx<Pane[]>` const [row] = await tx<Pane[]>`
@@ -106,8 +100,24 @@ export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void {
await tx`UPDATE session_panes SET position = -position await tx`UPDATE session_panes SET position = -position
WHERE session_id = ${sid} AND position < 0`; WHERE session_id = ${sid} AND position < 0`;
return row; 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); reply.code(201);
return inserted as Pane; return inserted as Pane;
} }
@@ -149,18 +159,18 @@ export function registerPaneRoutes(app: FastifyInstance, sql: Sql): void {
} }
} }
// Apply position change if needed // Apply position and/or state changes atomically
await sql.begin(async (tx) => {
if (position !== undefined && position !== oldPos) { if (position !== undefined && position !== oldPos) {
await movePane(sql, req.params.id, sid, oldPos, position); await movePane(tx, req.params.id, sid, oldPos, position);
} }
// Apply state change if provided
if (state !== undefined) { if (state !== undefined) {
await sql` await tx`
UPDATE session_panes SET state = ${JSON.stringify(state)}::jsonb UPDATE session_panes SET state = ${JSON.stringify(state)}::jsonb
WHERE id = ${req.params.id} WHERE id = ${req.params.id}
`; `;
} }
});
const [updated] = await sql<Pane[]>` const [updated] = await sql<Pane[]>`
SELECT id, session_id, position, kind, state, created_at SELECT id, session_id, position, kind, state, created_at