Skip to content

Commit f2c305f

Browse files
committed
fix(folders): row-lock restore reads, map conflict errorCode to 409
Three more review findings this round: 1. statusForOrchestrationError never mapped errorCode: 'conflict' to a status, so it silently fell to 500. The folders PUT route (rename conflicts) and the restore route (file-folder name conflicts) both used this default, turning a legitimate 409 into a misleading 500. Added the conflict -> 409 case, matching the sibling workspaceFilesOrchestrationStatus mapper that already had it. 2. Switched the folders [id] PUT/DELETE routes and the restore route from their bespoke inline errorCode->status ternaries to the shared statusForOrchestrationError helper, now that every orchestration failure branch they consume sets an explicit errorCode (verified branch by branch) -- this was deliberately deferred in an earlier commit until that was true. Also closed two errorCode gaps this surfaced: the archived-workspace check in performRestoreFolder (workflow) had no errorCode, and performRestoreFileFolder's catch-all didn't recognize the 'Cannot restore folder into an archived workspace' message, so it was being mapped to a generic 500 by the previous commit's internal-error fallback. 3. Greptile: performRestoreFolder's (workflow) and performRestoreResourceFolder's (kb/table) restore-transaction reads of the folder's own locked/deletedAt columns were plain SELECTs, not SELECT ... FOR UPDATE -- being inside the same transaction as the write closes the round-trip race but not this one, since a plain read doesn't block a concurrent UPDATE from another transaction. Added FOR UPDATE to both, matching the existing restoreKnowledgeBase precedent. The file-folder restore path doesn't need this: it already takes a workspace-scoped pg_advisory_xact_lock that the lock-toggle path also acquires, so it was already race-free.
1 parent 9df6dff commit f2c305f

5 files changed

Lines changed: 33 additions & 13 deletions

File tree

apps/sim/app/api/folders/[id]/restore/route.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { getSession } from '@/lib/auth'
1111
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1212
import { performRestoreFolder } from '@/lib/folders/orchestration'
1313
import { captureServerEvent } from '@/lib/posthog/server'
14+
import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types'
1415
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1516

1617
const logger = createLogger('RestoreFolderAPI')
@@ -52,12 +53,10 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Route
5253
})
5354

5455
if (!result.success) {
55-
// Only 'not_found' gets its own status here -- the other restore-failure
56-
// shapes (validation, and file-folder's name-conflict case which omits
57-
// errorCode) all read naturally as 400, matching this route's prior
58-
// behavior for everything except "not found".
59-
const status = result.errorCode === 'not_found' ? 404 : 400
60-
return NextResponse.json({ error: result.error }, { status })
56+
return NextResponse.json(
57+
{ error: result.error },
58+
{ status: statusForOrchestrationError(result.errorCode) }
59+
)
6160
}
6261

6362
logger.info(`Restored folder ${folderId}`, { restoredItems: result.restoredItems })

apps/sim/app/api/folders/[id]/route.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1111
import { performDeleteFolder, performUpdateFolder } from '@/lib/folders/orchestration'
1212
import { FOLDER_RESOURCE_POLICIES } from '@/lib/folders/policy'
1313
import { captureServerEvent } from '@/lib/posthog/server'
14+
import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types'
1415
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1516

1617
const logger = createLogger('FoldersIDAPI')
@@ -98,9 +99,10 @@ export const PUT = withRouteHandler(
9899
})
99100

100101
if (!result.success || !result.folder) {
101-
const status =
102-
result.errorCode === 'not_found' ? 404 : result.errorCode === 'validation' ? 400 : 500
103-
return NextResponse.json({ error: result.error }, { status })
102+
return NextResponse.json(
103+
{ error: result.error },
104+
{ status: statusForOrchestrationError(result.errorCode) }
105+
)
104106
}
105107

106108
logger.info('Updated folder:', { id, updates: parsed.data.body })
@@ -161,9 +163,10 @@ export const DELETE = withRouteHandler(
161163
})
162164

163165
if (!result.success) {
164-
const status =
165-
result.errorCode === 'not_found' ? 404 : result.errorCode === 'validation' ? 400 : 500
166-
return NextResponse.json({ error: result.error }, { status })
166+
return NextResponse.json(
167+
{ error: result.error },
168+
{ status: statusForOrchestrationError(result.errorCode) }
169+
)
167170
}
168171

169172
captureServerEvent(

apps/sim/lib/folders/orchestration.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,9 @@ async function performRestoreFileFolder(
321321
if (message === 'Folder is not archived') {
322322
return { success: false, error: message, errorCode: 'validation' }
323323
}
324+
if (message === 'Cannot restore folder into an archived workspace') {
325+
return { success: false, error: message, errorCode: 'validation' }
326+
}
324327
logger.error('Failed to restore file folder', { error })
325328
return { success: false, error: 'Internal server error', errorCode: 'internal' }
326329
}
@@ -529,6 +532,10 @@ async function performRestoreResourceFolder<TCountKey extends 'knowledgeBases' |
529532
const { folderId, workspaceId, userId, folderName } = params
530533

531534
const outcome = await db.transaction(async (tx) => {
535+
// `FOR UPDATE` row-locks this folder for the rest of the transaction -- a plain
536+
// SELECT inside a transaction does not block a concurrent UPDATE, so without this
537+
// a lock toggled on this row after this read but before the restore write below
538+
// could still be silently bypassed.
532539
const [raw] = await tx
533540
.select()
534541
.from(folderTable)
@@ -539,6 +546,7 @@ async function performRestoreResourceFolder<TCountKey extends 'knowledgeBases' |
539546
eq(folderTable.resourceType, cascade.resourceType)
540547
)
541548
)
549+
.for('update')
542550
.limit(1)
543551
if (!raw) return { kind: 'not_found' as const }
544552
if (!raw.deletedAt) return { kind: 'not_archived' as const }

apps/sim/lib/workflows/orchestration/folder-lifecycle.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,14 +477,23 @@ export async function performRestoreFolder(
477477
const { getWorkspaceWithOwner } = await import('@/lib/workspaces/permissions/utils')
478478
const ws = await getWorkspaceWithOwner(workspaceId)
479479
if (!ws || ws.archivedAt) {
480-
return { success: false, error: 'Cannot restore folder into an archived workspace' }
480+
return {
481+
success: false,
482+
error: 'Cannot restore folder into an archived workspace',
483+
errorCode: 'validation',
484+
}
481485
}
482486

483487
const outcome = await db.transaction(async (tx) => {
488+
// `FOR UPDATE` row-locks this folder for the rest of the transaction -- a plain
489+
// SELECT inside a transaction does not block a concurrent UPDATE, so without this
490+
// a lock toggled on this row after this read but before the restore write below
491+
// could still be silently bypassed.
484492
const [existingFolder] = await tx
485493
.select()
486494
.from(folder)
487495
.where(and(eq(folder.id, folderId), eq(folder.workspaceId, workspaceId), isWorkflowFolder))
496+
.for('update')
488497

489498
if (!existingFolder) return { kind: 'not_found' as const }
490499
if (!existingFolder.deletedAt) return { kind: 'not_archived' as const }

apps/sim/lib/workflows/orchestration/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,6 @@ export type OrchestrationErrorCode = 'validation' | 'not_found' | 'conflict' | '
77
export function statusForOrchestrationError(code: OrchestrationErrorCode | undefined): number {
88
if (code === 'validation') return 400
99
if (code === 'not_found') return 404
10+
if (code === 'conflict') return 409
1011
return 500
1112
}

0 commit comments

Comments
 (0)