fix(api): keep reporting applied operations when the PATCH re-read fails

The composite table PATCH promises that `error.details.applied` names the
operations that are live despite an error, but `applied` was scoped
inside the try. A rename or move that committed and was then followed by
a throw in the final re-read — or a re-read finding the table archived —
returned a bare 500/404 with no details, telling the caller nothing had
landed. It would then retry into a duplicate-name conflict or repeat the
move.

`applied` is now function-scoped so every post-write exit carries it: the
404 on a missing re-read, a thrown lock error, a classified orchestration
error, and the generic 500. `v2TableLockError` gains the same
`extraDetails` parameter `v2TableOrchestrationError` already had.

Author: TheodoreSpeaks

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

@@ -341,6 +341,40 @@ describe('PATCH /api/v2/tables/[tableId]', () => {
     expect(mockPerformRenameTable).not.toHaveBeenCalled()
   })
 
+  /**
+   * The re-read runs after the writes have committed, so a failure there must
+   * still name what landed. Reporting a bare 500 tells the caller nothing took
+   * effect and it retries into a duplicate-name conflict.
+   */
+  it('reports the applied operations when the final re-read throws', async () => {
+    mockPerformRenameTable.mockResolvedValue({ success: true })
+    mockGetTableById.mockRejectedValue(new Error('connection reset'))
+
+    const res = await callPatch({ workspaceId: 'ws-1', name: 'Renamed' })
+
+    expect(res.status).toBe(500)
+    expect((await res.json()).error.details).toEqual({ applied: ['name'] })
+  })
+
+  it('reports the applied operations when the re-read finds the table archived', async () => {
+    mockPerformRenameTable.mockResolvedValue({ success: true })
+    mockGetTableById.mockResolvedValue(null)
+
+    const res = await callPatch({ workspaceId: 'ws-1', name: 'Renamed' })
+
+    expect(res.status).toBe(404)
+    expect((await res.json()).error.details).toEqual({ applied: ['name'] })
+  })
+
+  it('omits applied details when the failure happened before any write', async () => {
+    mockGetTableById.mockRejectedValue(new Error('connection reset'))
+
+    const res = await callPatch({ workspaceId: 'ws-1', folderId: 'nope' })
+
+    // Absence is meaningful: nothing is live, so a retry is safe.
+    expect((await res.json()).error.details).toBeUndefined()
+  })
+
   it('still reports the stored lock flags on the table it returns', async () => {
     // The response is a re-read, so the locked state has to come from there.
     mockGetTableById.mockResolvedValue({

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

@@ -7,6 +7,7 @@ import {
   v2UpdateTableContract,
 } from '@/lib/api/contracts/v2/tables'
 import { parseRequest } from '@/lib/api/server'
+import { asOrchestrationError } from '@/lib/core/orchestration/types'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { findActiveFolder } from '@/lib/folders/queries'
@@ -21,7 +22,6 @@ import { checkAccess } from '@/app/api/table/utils'
 import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware'
 import { v2ApiGateError } from '@/app/api/v2/lib/gate'
 import {
-  v2CaughtOrchestrationError,
   v2Data,
   v2Error,
   v2RateLimitError,
@@ -38,6 +38,17 @@ import {
 
 const logger = createLogger('V2TableDetailAPI')
 
+/**
+ * `details` payload naming the operations of a composite write that committed,
+ * or `undefined` when none did — so `details.applied` being present always
+ * means "these changes are live despite the error".
+ */
+function appliedDetails(
+  applied: readonly ('name' | 'folderId')[]
+): { applied: readonly string[] } | undefined {
+  return applied.length > 0 ? { applied } : undefined
+}
+
 export const dynamic = 'force-dynamic'
 export const revalidate = 0
 
@@ -101,6 +112,15 @@ export const GET = withRouteHandler(async (request: NextRequest, context: TableR
 export const PATCH = withRouteHandler(async (request: NextRequest, context: TableRouteParams) => {
   const requestId = generateRequestId()
 
+  /**
+   * Hoisted above the `try` so every exit path can report it. Once a write has
+   * committed, the response must say so even when the failure came *after* the
+   * writes — a throw in the final re-read, or the re-read finding the table
+   * archived. Reporting a bare 500 there tells the caller nothing landed, and
+   * it retries into a duplicate-name conflict or a repeated move.
+   */
+  const applied: ('name' | 'folderId')[] = []
+
   try {
     const rateLimit = await checkRateLimit(request, 'table-detail')
     if (!rateLimit.allowed) return v2RateLimitError(rateLimit)
@@ -151,7 +171,6 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
     // per-operation audits — so instead of pretending atomicity the response
     // states exactly which operations landed. A caller that gets an error can
     // then reconcile rather than having to re-read and diff.
-    const applied: ('name' | 'folderId')[] = []
     let failure: { outcome: OrchestrationOutcome; fallback: string } | null = null
 
     if (validated.name !== undefined) {
@@ -190,31 +209,38 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
     // Live-collab: tell open viewers the definition changed so they refetch.
     if (applied.length > 0) signalTableSchemaChanged(tableId)
     if (failure) {
-      return v2TableOrchestrationError(
-        failure.outcome,
-        failure.fallback,
-        // Omitted when nothing landed, so `details.applied` present always
-        // means "these changes are live despite the error".
-        applied.length > 0 ? { applied } : undefined
-      )
+      return v2TableOrchestrationError(failure.outcome, failure.fallback, appliedDetails(applied))
     }
 
-    // Re-read so the response reflects every applied change at once.
+    // Re-read so the response reflects every applied change at once. A miss
+    // means the table was archived after the writes committed, so the caller
+    // still has to be told what landed.
     const updated = await getTableById(tableId)
-    if (!updated) return v2Error('NOT_FOUND', 'Table not found')
+    if (!updated) {
+      return v2Error('NOT_FOUND', 'Table not found', { details: appliedDetails(applied) })
+    }
 
     return v2Data({ table: toApiTable(updated) }, { rateLimit })
   } catch (error) {
-    const lockError = v2TableLockError(error)
+    const details = appliedDetails(applied)
+
+    const lockError = v2TableLockError(error, details)
     if (lockError) return lockError
 
-    const classified = v2CaughtOrchestrationError(error)
-    if (classified) return classified
+    const classified = asOrchestrationError(error)
+    if (classified) {
+      return v2TableOrchestrationError(
+        { errorCode: classified.code, error: classified.message },
+        'Failed to update table',
+        details
+      )
+    }
 
     logger.error(`[${requestId}] Error updating table`, {
       error: getErrorMessage(error, 'Unknown error'),
+      applied,
     })
-    return v2Error('INTERNAL_ERROR', 'Internal server error')
+    return v2Error('INTERNAL_ERROR', 'Internal server error', { details })
   }
 })
 

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

@@ -184,9 +184,13 @@ export function v2TableAccessError(result: { ok: false; status: 404 | 403 }): Ne
  * 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 {
+export function v2TableLockError(
+  error: unknown,
+  /** Merged into `details` — e.g. which operations of a composite write landed. */
+  extraDetails?: Record<string, unknown>
+): NextResponse | null {
   if (error instanceof TableLockedError) {
-    return v2Error('LOCKED', error.message, { details: { lock: error.lock } })
+    return v2Error('LOCKED', error.message, { details: { lock: error.lock, ...extraDetails } })
   }
   return null
 }