minor comments

Author: icecrasher321

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/promote-workspace-modal/promote-workspace-modal.tsx

@@ -1,7 +1,7 @@
 import { workflow, workflowBlocks, workflowFolder } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { generateId } from '@sim/utils/id'
-import { and, eq, inArray, isNull, ne } from 'drizzle-orm'
+import { and, eq, inArray, isNull } from 'drizzle-orm'
 import type { DbOrTx } from '@/lib/db/types'
 import { remapConditionEdgeHandle } from '@/lib/workflows/condition-ids'
 import {
@@ -222,39 +222,23 @@ export async function loadTargetDraftSubBlocks(
   return byWorkflow
 }
 
-async function resolveTargetWorkflowName(
-  tx: DbOrTx,
-  workspaceId: string,
+/**
+ * Pick a non-colliding name for a copied workflow against the preloaded registry, which
+ * mirrors the workspace's (folder, name, not-archived, exclude-self) predicate from one
+ * query instead of one per candidate. Mirrors {@link deduplicateWorkflowName}'s ` (n)`
+ * numbering, but reads from memory so the copy loop issues no per-workflow name queries.
+ */
+function resolveTargetWorkflowName(
+  registry: WorkflowNameRegistry,
   folderId: string | null,
   name: string,
-  excludeWorkflowId: string | null,
-  registry?: WorkflowNameRegistry
-): Promise<string> {
-  const folderCondition = folderId ? eq(workflow.folderId, folderId) : isNull(workflow.folderId)
-
-  const nameTaken = async (candidate: string): Promise<boolean> => {
-    // The registry mirrors the same (workspace, folder, name, not-archived, exclude-self)
-    // predicate as the DB query below, from one preload instead of a query per call.
-    if (registry) return registry.isTaken(folderId, candidate, excludeWorkflowId)
-    const conditions = [
-      eq(workflow.workspaceId, workspaceId),
-      folderCondition,
-      eq(workflow.name, candidate),
-      isNull(workflow.archivedAt),
-    ]
-    if (excludeWorkflowId) conditions.push(ne(workflow.id, excludeWorkflowId))
-    const [row] = await tx
-      .select({ id: workflow.id })
-      .from(workflow)
-      .where(and(...conditions))
-      .limit(1)
-    return Boolean(row)
-  }
-
-  if (!(await nameTaken(name))) return name
+  excludeWorkflowId: string | null
+): string {
+  const taken = (candidate: string) => registry.isTaken(folderId, candidate, excludeWorkflowId)
+  if (!taken(name)) return name
   for (let i = 2; i < 100; i++) {
     const candidate = `${name} (${i})`
-    if (!(await nameTaken(candidate))) return candidate
+    if (!taken(candidate)) return candidate
   }
   return `${name} (${generateId().slice(0, 6)})`
 }
@@ -314,7 +298,7 @@ export interface CopyWorkflowStateParams {
    * Preloaded name registry so name-collision resolution reads from memory instead of one
    * query per workflow inside the tx. Build once per copy loop via {@link loadWorkflowNameRegistry}.
    */
-  nameRegistry?: WorkflowNameRegistry
+  nameRegistry: WorkflowNameRegistry
   requestId?: string
 }
 
@@ -481,17 +465,15 @@ export async function copyWorkflowStateIntoTarget(
     }
   }
 
-  const resolvedName = await resolveTargetWorkflowName(
-    tx,
-    targetWorkspaceId,
+  const resolvedName = resolveTargetWorkflowName(
+    nameRegistry,
     targetFolderId,
     sourceMeta.name,
-    mode === 'replace' ? targetWorkflowId : null,
-    nameRegistry
+    mode === 'replace' ? targetWorkflowId : null
   )
   // Claim the resolved name so the next workflow in this copy loop sees it taken. The DB
   // write below uses the same (folderId, name), so the registry stays consistent with it.
-  nameRegistry?.claim(targetFolderId, resolvedName, targetWorkflowId)
+  nameRegistry.claim(targetFolderId, resolvedName, targetWorkflowId)
 
   if (mode === 'create') {
     await tx.insert(workflow).values({

apps/sim/lib/workspaces/fork/copy/copy-workflows.ts

@@ -138,10 +138,19 @@ export async function loadSourceDeployedStates(sourceWorkspaceId: string): Promi
       400
     )
   }
+  // Read states in bounded-concurrency batches instead of one serial await per workflow:
+  // serial cost is O(workflows) round trips (this also runs on the diff preview, refetched
+  // while the sync modal is open). The cap keeps concurrent global-pool checkouts well
+  // under the pool max even at the workflow ceiling, and this runs BEFORE any transaction.
   const sourceStates = new Map<string, WorkflowState>()
-  for (const wf of deployedWorkflows) {
-    const state = await readDeployedState(wf.id, sourceWorkspaceId)
-    if (state) sourceStates.set(wf.id, state)
+  const READ_CONCURRENCY = 5
+  for (let i = 0; i < deployedWorkflows.length; i += READ_CONCURRENCY) {
+    const batch = deployedWorkflows.slice(i, i + READ_CONCURRENCY)
+    const states = await Promise.all(batch.map((wf) => readDeployedState(wf.id, sourceWorkspaceId)))
+    batch.forEach((wf, index) => {
+      const state = states[index]
+      if (state) sourceStates.set(wf.id, state)
+    })
   }
   return { deployedWorkflows, sourceStates }
 }

apps/sim/lib/workspaces/fork/copy/deploy-bridge.ts

@@ -312,36 +312,49 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
   // content to copy in the background the row stays `processing` until the runner
   // finishes it (merging in copied/failed); otherwise the fork is already complete.
   const forkedName = result.workspace.name
-  const statusId = await startBackgroundWork(db, {
-    workspaceId: source.id,
-    kind: 'fork_content_copy',
-    // Append-only: each fork is a distinct entry in the source workspace's fork history.
-    supersede: false,
-    message: hasContent ? `Copying resources to "${forkedName}"` : `Forked into "${forkedName}"`,
-    metadata: {
+  // The fork already committed; failing to record the tracking row must not turn it into
+  // a 500. Log and continue without a status row - the background content copy below still
+  // runs (its runner no-ops the status update when statusId is absent).
+  let statusId: string | undefined
+  try {
+    statusId = await startBackgroundWork(db, {
+      workspaceId: source.id,
+      kind: 'fork_content_copy',
+      // Append-only: each fork is a distinct entry in the source workspace's fork history.
+      supersede: false,
+      message: hasContent ? `Copying resources to "${forkedName}"` : `Forked into "${forkedName}"`,
+      metadata: {
+        childWorkspaceId: result.workspace.id,
+        childWorkspaceName: forkedName,
+        actorName: params.actorName,
+        workflowsCopied: result.workflowsCopied,
+        tables: contentPlan.tables.length,
+        knowledgeBases: contentPlan.knowledgeBases.length,
+        files: blobTasks.length,
+        workflowNames: forkedWorkflowNames,
+        tableNames: forkedResourceNames.tables,
+        knowledgeBaseNames: forkedResourceNames.knowledgeBases,
+        fileNames: blobTasks.map((task) => task.fileName),
+        customToolNames: forkedResourceNames.customTools,
+        skillNames: forkedResourceNames.skills,
+        mcpServerNames: forkedResourceNames.mcpServers,
+      },
+    })
+  } catch (error) {
+    logger.error(`[${requestId}] Failed to record fork background-work status`, {
       childWorkspaceId: result.workspace.id,
-      childWorkspaceName: forkedName,
-      actorName: params.actorName,
-      workflowsCopied: result.workflowsCopied,
-      tables: contentPlan.tables.length,
-      knowledgeBases: contentPlan.knowledgeBases.length,
-      files: blobTasks.length,
-      workflowNames: forkedWorkflowNames,
-      tableNames: forkedResourceNames.tables,
-      knowledgeBaseNames: forkedResourceNames.knowledgeBases,
-      fileNames: blobTasks.map((task) => task.fileName),
-      customToolNames: forkedResourceNames.customTools,
-      skillNames: forkedResourceNames.skills,
-      mcpServerNames: forkedResourceNames.mcpServers,
-    },
-  })
+      error: getErrorMessage(error),
+    })
+  }
 
   if (!hasContent) {
-    await finishBackgroundWork(db, statusId, {
-      status: 'completed',
-      message: `Forked into "${forkedName}"`,
-      metadata: { copied: 0, failed: 0 },
-    }).catch(() => {})
+    if (statusId) {
+      await finishBackgroundWork(db, statusId, {
+        status: 'completed',
+        message: `Forked into "${forkedName}"`,
+        metadata: { copied: 0, failed: 0 },
+      }).catch(() => {})
+    }
     return result
   }
 
@@ -366,10 +379,12 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
       childWorkspaceId: result.workspace.id,
       error: getErrorMessage(error),
     })
-    await finishBackgroundWork(db, statusId, {
-      status: 'failed',
-      error: getErrorMessage(error, 'Could not start the background copy'),
-    }).catch(() => {})
+    if (statusId) {
+      await finishBackgroundWork(db, statusId, {
+        status: 'failed',
+        error: getErrorMessage(error, 'Could not start the background copy'),
+      }).catch(() => {})
+    }
   }
 
   return result

apps/sim/lib/workspaces/fork/create-fork.ts

@@ -20,7 +20,10 @@ vi.mock('@/lib/workspaces/fork/mapping/resources', () => ({
 }))
 
 import { ForkError } from '@/lib/workspaces/fork/lineage/authz'
-import { validateForkMappingTargets } from '@/lib/workspaces/fork/mapping/mapping-service'
+import {
+  findDuplicateTargetEntry,
+  validateForkMappingTargets,
+} from '@/lib/workspaces/fork/mapping/mapping-service'
 
 type ExistingByKind = Partial<Record<ForkRemapKind, Set<string>>>
 
@@ -130,3 +133,50 @@ describe('validateForkMappingTargets', () => {
     ).rejects.toBeInstanceOf(ForkError)
   })
 })
+
+describe('findDuplicateTargetEntry', () => {
+  it('returns null when every target is used by at most one source', () => {
+    expect(
+      findDuplicateTargetEntry([
+        { resourceType: 'oauth_credential', sourceId: 'c1', targetId: 't1' },
+        { resourceType: 'oauth_credential', sourceId: 'c2', targetId: 't2' },
+      ])
+    ).toBeNull()
+  })
+
+  it('flags two distinct sources mapped to the same target', () => {
+    expect(
+      findDuplicateTargetEntry([
+        { resourceType: 'oauth_credential', sourceId: 'c1', targetId: 'shared' },
+        { resourceType: 'oauth_credential', sourceId: 'c2', targetId: 'shared' },
+      ])
+    ).toEqual({ resourceType: 'oauth_credential', targetId: 'shared' })
+  })
+
+  it('ignores cleared (null target) entries', () => {
+    expect(
+      findDuplicateTargetEntry([
+        { resourceType: 'oauth_credential', sourceId: 'c1', targetId: null },
+        { resourceType: 'oauth_credential', sourceId: 'c2', targetId: null },
+      ])
+    ).toBeNull()
+  })
+
+  it('does not flag the same source+target repeated', () => {
+    expect(
+      findDuplicateTargetEntry([
+        { resourceType: 'table', sourceId: 'c1', targetId: 't1' },
+        { resourceType: 'table', sourceId: 'c1', targetId: 't1' },
+      ])
+    ).toBeNull()
+  })
+
+  it('does not conflate the same target id across resource types', () => {
+    expect(
+      findDuplicateTargetEntry([
+        { resourceType: 'oauth_credential', sourceId: 'c1', targetId: 'same' },
+        { resourceType: 'table', sourceId: 'c2', targetId: 'same' },
+      ])
+    ).toBeNull()
+  })
+})

apps/sim/lib/workspaces/fork/mapping/mapping-service.test.ts

@@ -221,6 +221,35 @@ export interface ApplyForkMappingEntry {
   targetId: string | null
 }
 
+/**
+ * The first target two distinct sources are mapped to (same resourceType + targetId,
+ * different sourceId), or null when every target is used by at most one source. Cleared
+ * entries (null target) are ignored. Used by the PUSH path only: a push row is unique on
+ * the parent (target) side, so such a pair collides on that unique index and one mapping
+ * would be silently dropped - the caller rejects it instead. Pull is the inverse (many
+ * parent sources may share one child target, which the resolver handles), so pull does not
+ * use this guard.
+ */
+export function findDuplicateTargetEntry(
+  entries: ApplyForkMappingEntry[]
+): { resourceType: ForkResourceType; targetId: string } | null {
+  const sourcesByTarget = new Map<string, Set<string>>()
+  for (const entry of entries) {
+    if (entry.targetId == null) continue
+    // Null-byte separator so a targetId containing ':' (e.g. credentialSet:...) can't
+    // be confused with a different (resourceType, targetId) pair.
+    const key = `${entry.resourceType}\u0000${entry.targetId}`
+    const sources = sourcesByTarget.get(key)
+    if (!sources) {
+      sourcesByTarget.set(key, new Set([entry.sourceId]))
+      continue
+    }
+    sources.add(entry.sourceId)
+    if (sources.size > 1) return { resourceType: entry.resourceType, targetId: entry.targetId }
+  }
+  return null
+}
+
 /**
  * Persist mapping edits for a direction. Pull maps a parent source to a child
  * target; push maps a child source to a parent target (clearing a push mapping
@@ -248,6 +277,20 @@ export async function applyForkMappingEntries(
     )
     return entries.length
   }
+  // Push rows are unique on the parent (target) side, so two distinct sources mapped to
+  // the same target would collide on that index and one would be silently dropped (its
+  // reference then resolves unmapped). Reject loudly - on push each parent target can back
+  // only one source. (Pull is the inverse: many parent sources may share one child target,
+  // which the resolver handles, so pull skips this guard. The modal also disables an
+  // already-taken target on push so users never reach this error normally.)
+  const collision = findDuplicateTargetEntry(entries)
+  if (collision) {
+    const kind = resourceTypeToForkKind(collision.resourceType) ?? collision.resourceType
+    throw new ForkError(
+      `Two sources are mapped to the same ${kind} target. Each target can be mapped from only one source.`,
+      400
+    )
+  }
   // Push rows are keyed by the child (source) side, but the table's unique key is on
   // the parent side - so clear any existing row for each source first (one grouped
   // delete), otherwise changing a push target leaves the old (parent, source) row

apps/sim/lib/workspaces/fork/mapping/mapping-service.ts

@@ -1,4 +1,4 @@
-import { workflow } from '@sim/db/schema'
+import { workflow, workspace } from '@sim/db/schema'
 import { generateId } from '@sim/utils/id'
 import { and, eq, isNull } from 'drizzle-orm'
 import type { DbOrTx } from '@/lib/db/types'
@@ -260,9 +260,17 @@ export async function computeForkPromotePlan(params: {
   const unmappedOptional = allUnmapped.filter((reference) => !reference.required)
 
   const previousRun = await getPromoteRunForEdge(executor, edge.childWorkspaceId, targetWorkspaceId)
-  const drift = Boolean(
-    previousRun && targetWorkflows.some((w) => w.updatedAt > previousRun.createdAt)
-  )
+  let driftBaseline = previousRun?.createdAt ?? null
+  if (driftBaseline == null) {
+    const [childWorkspace] = await executor
+      .select({ createdAt: workspace.createdAt })
+      .from(workspace)
+      .where(eq(workspace.id, edge.childWorkspaceId))
+      .limit(1)
+    driftBaseline = childWorkspace?.createdAt ?? null
+  }
+  const baseline = driftBaseline
+  const drift = baseline != null && targetWorkflows.some((w) => w.updatedAt > baseline)
 
   const willUpdate = items.filter((i) => i.mode === 'replace').length
   const willCreate = items.filter((i) => i.mode === 'create').length