fix(api): make v2 table PATCH all-or-nothing and name the lock in every 423

Greptile P1: PATCH applied locks, rename and move as three sequential
transactions, so a folder rejected mid-request left the earlier writes
persisted while the response reported failure — and the schema-changed
signal was skipped, leaving open clients on stale state. Every rejectable
condition now runs before the first write, and the signal fires whenever
anything did land.

Cursor: v2TableLockError dropped the lock kind, so async import, column
run, enrichment and table mutations returned a bare LOCKED. A table has
four independent locks, so the caller could not tell which to clear.

Author: TheodoreSpeaks

apps/sim/app/api/v2/tables/[tableId]/import-async/route.test.ts

@@ -19,7 +19,6 @@ const {
   mockAssertRowInsert,
   mockAssertRowDelete,
   mockGateError,
-  TableLockedError,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockResolveWorkspaceScope: vi.fn(),
@@ -30,7 +29,6 @@ const {
   mockAssertRowInsert: vi.fn(),
   mockAssertRowDelete: vi.fn(),
   mockGateError: vi.fn(),
-  TableLockedError: class TableLockedError extends Error {},
 }))
 
 vi.mock('@/app/api/v1/middleware', () => ({
@@ -43,25 +41,18 @@ vi.mock('@/app/api/table/utils', async (importOriginal) => ({
   checkAccess: mockCheckAccess,
 }))
 
-vi.mock('@/app/api/v2/tables/utils', async (importOriginal) => ({
-  ...(await importOriginal<Record<string, unknown>>()),
-  v2TableLockError: (error: unknown) =>
-    error instanceof TableLockedError
-      ? new Response(JSON.stringify({ error: { code: 'LOCKED', message: error.message } }), {
-          status: 423,
-        })
-      : null,
-}))
-
 vi.mock('@/lib/table/jobs/service', () => ({
   markTableJobRunning: mockMarkTableJobRunning,
   releaseJobClaim: mockReleaseJobClaim,
 }))
-vi.mock('@/lib/table/mutation-locks', () => ({
+// Only the assert helpers are stubbed — `TableLockedError` stays real so the
+// route's `v2TableLockError` recognizes it by `instanceof` and reports the lock
+// kind, exactly as it would in production.
+vi.mock('@/lib/table/mutation-locks', async (importOriginal) => ({
+  ...(await importOriginal<Record<string, unknown>>()),
   assertRowInsert: mockAssertRowInsert,
   assertRowDelete: mockAssertRowDelete,
   assertSchemaMutable: vi.fn(),
-  TableLockedError,
 }))
 vi.mock('@/lib/table/import-runner', () => ({ runTableImport: vi.fn() }))
 vi.mock('@/lib/core/utils/background', () => ({ runDetached: mockRunDetached }))
@@ -71,6 +62,7 @@ vi.mock('@/lib/users/queries', () => ({
 }))
 vi.mock('@/app/api/v2/lib/gate', () => ({ v2ApiGateError: mockGateError }))
 
+import { TableLockedError } from '@/lib/table/mutation-locks'
 import { POST } from '@/app/api/v2/tables/[tableId]/import-async/route'
 
 const TABLE = { id: 'table-1', workspaceId: 'ws-1', schema: { columns: [] }, archivedAt: null }
@@ -134,15 +126,20 @@ describe('POST /api/v2/tables/[tableId]/import-async', () => {
     expect(mockMarkTableJobRunning).not.toHaveBeenCalled()
   })
 
-  it('asserts the insert lock BEFORE claiming the slot, so a locked table never holds it', async () => {
+  it('asserts the insert lock BEFORE claiming the slot, and names the lock in the 423', async () => {
     mockAssertRowInsert.mockImplementation(() => {
-      throw new TableLockedError('Inserts are locked for this table')
+      throw new TableLockedError('insert')
     })
 
     const res = await callPost(BODY)
 
     expect(res.status).toBe(423)
     expect(mockMarkTableJobRunning).not.toHaveBeenCalled()
+    // A table has four independent locks, so "LOCKED" alone doesn't tell the
+    // caller which one to clear.
+    const body = await res.json()
+    expect(body.error.code).toBe('LOCKED')
+    expect(body.error.details).toEqual({ lock: 'insert' })
   })
 
   it('asserts the delete lock too when the mode replaces rows', async () => {

apps/sim/app/api/v2/tables/[tableId]/route.test.ts

@@ -22,6 +22,7 @@ const {
   mockFindActiveFolder,
   mockIsFeatureEnabled,
   mockGateError,
+  mockSignalSchemaChanged,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockResolveWorkspaceScope: vi.fn(),
@@ -35,6 +36,7 @@ const {
   mockFindActiveFolder: vi.fn(),
   mockIsFeatureEnabled: vi.fn(),
   mockGateError: vi.fn(),
+  mockSignalSchemaChanged: vi.fn(),
 }))
 
 vi.mock('@sim/audit', () => ({
@@ -63,7 +65,9 @@ vi.mock('@/lib/table', () => ({
   buildIdByName: vi.fn(),
 }))
 
-vi.mock('@/lib/table/events', () => ({ signalTableSchemaChanged: vi.fn() }))
+vi.mock('@/lib/table/events', () => ({
+  signalTableSchemaChanged: mockSignalSchemaChanged,
+}))
 vi.mock('@/lib/folders/queries', () => ({ findActiveFolder: mockFindActiveFolder }))
 vi.mock('@/lib/core/config/feature-flags', () => ({ isFeatureEnabled: mockIsFeatureEnabled }))
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
@@ -220,6 +224,55 @@ describe('PATCH /api/v2/tables/[tableId]', () => {
     expect(mockPerformMoveTableToFolder).not.toHaveBeenCalled()
   })
 
+  it('rejects a bad folder without applying the rename that came with it', async () => {
+    // The three operations are separate transactions, so validation has to run
+    // before the first write — otherwise a rejected PATCH still renames.
+    mockFindActiveFolder.mockResolvedValue(null)
+
+    const res = await callPatch({
+      workspaceId: 'ws-1',
+      name: 'Renamed',
+      folderId: 'folder-elsewhere',
+    })
+
+    expect(res.status).toBe(404)
+    expect(mockPerformRenameTable).not.toHaveBeenCalled()
+    expect(mockPerformUpdateTableLocks).not.toHaveBeenCalled()
+    expect(mockSignalSchemaChanged).not.toHaveBeenCalled()
+  })
+
+  it('rejects a lock change from a non-admin without applying the rename beside it', async () => {
+    mockCheckAccess.mockImplementation(async (_tableId, _userId, level) =>
+      level === 'admin' ? { ok: false, status: 403 } : { ok: true, table: TABLE }
+    )
+
+    const res = await callPatch({
+      workspaceId: 'ws-1',
+      name: 'Renamed',
+      locks: { deleteLocked: true },
+    })
+
+    expect(res.status).toBe(403)
+    expect(mockPerformRenameTable).not.toHaveBeenCalled()
+  })
+
+  it('still signals collaborators when a later operation fails after an earlier one landed', async () => {
+    // A mid-write fault can't be rolled back across three transactions, so the
+    // clients must at least be told to refetch what did apply.
+    mockPerformRenameTable.mockResolvedValue({ success: true })
+    mockPerformMoveTableToFolder.mockResolvedValue({
+      success: false,
+      errorCode: 'not_found',
+      error: 'gone',
+    })
+
+    const res = await callPatch({ workspaceId: 'ws-1', name: 'Renamed', folderId: 'folder-1' })
+
+    expect(res.status).toBe(404)
+    expect(mockPerformRenameTable).toHaveBeenCalled()
+    expect(mockSignalSchemaChanged).toHaveBeenCalledWith('table-1')
+  })
+
   it('rejects a lock change from a write-level caller', async () => {
     mockCheckAccess.mockImplementation(async (_tableId, _userId, level) =>
       level === 'admin' ? { ok: false, status: 403 } : { ok: true, table: TABLE }

apps/sim/app/api/v2/tables/[tableId]/route.ts

@@ -1,6 +1,6 @@
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
-import type { NextRequest } from 'next/server'
+import type { NextRequest, NextResponse } from 'next/server'
 import {
   v2DeleteTableContract,
   v2GetTableContract,
@@ -127,6 +127,11 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
       return v2Error('NOT_FOUND', 'Table not found')
     }
 
+    // ── Validate every field BEFORE the first write ──
+    // The three operations are separate transactions, so a rejection
+    // discovered partway through would leave the earlier ones persisted while
+    // the response reports failure. Everything a request can be rejected for
+    // is therefore checked up front: a rejected PATCH changes nothing.
     if (validated.locks !== undefined) {
       // Only a lock transitioning off→on needs the feature; comparing against
       // the stored state is what lets a caller submitting the full flag set
@@ -151,65 +156,82 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
       if (!adminResult.ok) {
         return v2Error('FORBIDDEN', 'Admin access required to change table locks')
       }
+    }
+
+    if (validated.folderId != null) {
+      // Scoped to `resourceType: 'table'` so a folder id from another resource's
+      // tree can't file the table somewhere Tables never lists.
+      if (!(await findActiveFolder(validated.folderId, table.workspaceId, 'table'))) {
+        return v2Error('NOT_FOUND', 'Folder not found in this workspace')
+      }
+    }
+
+    // ── Apply ──
+    // `applied` tracks whether anything reached the database, so a failure
+    // partway through still signals open clients. Skipping the signal there
+    // would leave every viewer rendering state that has already changed.
+    let applied = false
+    let failure: NextResponse | null = null
 
+    if (validated.locks !== undefined) {
       const outcome = await performUpdateTableLocks({
         tableId,
         partial: validated.locks,
         userId,
         requestId,
         request,
       })
-      if (!outcome.success) {
-        return v2ErrorForOrchestration(
+      if (outcome.success) {
+        applied = true
+      } else {
+        failure = v2ErrorForOrchestration(
           outcome.errorCode,
           outcome.error ?? 'Failed to update table locks'
         )
       }
     }
 
-    if (validated.name !== undefined) {
+    if (!failure && validated.name !== undefined) {
       const outcome = await performRenameTable({
         table,
         newName: validated.name,
         userId,
         requestId,
         request,
       })
-      if (!outcome.success) {
-        return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to rename table')
+      if (outcome.success) {
+        applied = true
+      } else {
+        failure = v2ErrorForOrchestration(
+          outcome.errorCode,
+          outcome.error ?? 'Failed to rename table'
+        )
       }
     }
 
-    if (validated.folderId !== undefined) {
-      // Scoped to `resourceType: 'table'` so a folder id from another resource's
-      // tree can't file the table somewhere Tables never lists.
-      if (
-        validated.folderId !== null &&
-        !(await findActiveFolder(validated.folderId, table.workspaceId, 'table'))
-      ) {
-        return v2Error('NOT_FOUND', 'Folder not found in this workspace')
-      }
+    if (!failure && validated.folderId !== undefined) {
       const outcome = await performMoveTableToFolder({
         table,
         folderId: validated.folderId,
         userId,
         requestId,
         request,
       })
-      if (!outcome.success) {
-        // The move re-asserts workspace and active state, so a miss means the
-        // table was archived between `checkAccess` and the write.
-        return v2ErrorForOrchestration(
+      if (outcome.success) applied = true
+      // The move re-asserts workspace and active state, so a miss means the
+      // table was archived between `checkAccess` and the write.
+      else
+        failure = v2ErrorForOrchestration(
           outcome.errorCode,
           outcome.errorCode === 'not_found'
             ? 'Table not found'
             : (outcome.error ?? 'Failed to move table')
         )
-      }
     }
 
     // Live-collab: tell open viewers the definition changed so they refetch.
-    signalTableSchemaChanged(tableId)
+    if (applied) signalTableSchemaChanged(tableId)
+    if (failure) return failure
 
     // Re-read so the response reflects every applied change at once.
     const updated = await getTableById(tableId)

apps/sim/app/api/v2/tables/utils.ts

@@ -166,9 +166,15 @@ export function v2TableAccessError(result: { ok: false; status: 404 | 403 }): Ne
  * Maps a delete/write rejected by a table lock to the v2 `LOCKED` envelope,
  * mirroring v1's {@link tableLockErrorResponse}. Returns `null` for anything
  * else so the caller falls through to its own classification.
+ *
+ * `details.lock` names the flag that rejected the write. A table carries four
+ * independent locks, so "locked" on its own does not tell a caller which one to
+ * clear — every 423 on the surface reports it.
  */
 export function v2TableLockError(error: unknown): NextResponse | null {
-  if (error instanceof TableLockedError) return v2Error('LOCKED', error.message)
+  if (error instanceof TableLockedError) {
+    return v2Error('LOCKED', error.message, { details: { lock: error.lock } })
+  }
   return null
 }