diff --git a/apps/sim/ee/access-control/utils/permission-check.ts b/apps/sim/ee/access-control/utils/permission-check.ts index 6a3c189c95c..4dc5b900f8b 100644 --- a/apps/sim/ee/access-control/utils/permission-check.ts +++ b/apps/sim/ee/access-control/utils/permission-check.ts @@ -1,10 +1,15 @@ import { createLogger } from '@sim/logger' +import { describeError } from '@sim/utils/errors' +import { sleep } from '@sim/utils/helpers' +import { backoffWithJitter } from '@sim/utils/retry' import type { ShareAuthType } from '@/lib/api/contracts/public-shares' import { getAllowedIntegrationsFromEnv, isInvitationsDisabled, isPublicApiDisabled, } from '@/lib/core/config/env-flags' +import { findDatabaseQueryError } from '@/lib/core/errors/database-query-error' +import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure' import { isBlockTypeAccessControlExempt } from '@/lib/permission-groups/block-access' import { CAPABILITY_RULES, @@ -199,43 +204,78 @@ function governedSubjectUserId( return declared ?? undefined } +const PERMISSION_CONFIG_LOAD_MAX_ATTEMPTS = 3 +const PERMISSION_CONFIG_LOAD_RETRY_BACKOFF = { baseMs: 25, maxMs: 100 } as const + /** - * Cache-aware wrapper around `getUserPermissionConfig`. When an - * `ExecutionContext` is provided, the resolved config is memoized on the - * context so repeated checks during a single workflow run share one DB hit. - * - * The subject is resolved HERE rather than by each caller, because the memo is - * keyed by nothing but the context. `validateModelProvider` and - * `validateBlockType` take the actor's id positionally, so a run declaring a - * different gate subject had the first model check fill the cache with the - * BILLING actor's group — and every later `assertPermissionsAllowed`, having - * correctly resolved the governed subject, was handed that stale entry. Doing - * the derivation at the one place the config is loaded makes the memo correct - * by construction: within a run `capabilityGovernedUserId` is fixed, so every - * path resolves and caches the same person. + * Loads a permission config, retrying a transient database read failure a bounded number of times. + * The last failure is rethrown: resolving `null` would turn every gate off. + */ +async function loadPermissionConfig( + userId: string, + workspaceId: string, + signal: AbortSignal | undefined +): Promise { + for (let attempt = 1; ; attempt += 1) { + signal?.throwIfAborted() + try { + return await getUserPermissionConfig(userId, workspaceId) + } catch (error) { + signal?.throwIfAborted() + if ( + attempt >= PERMISSION_CONFIG_LOAD_MAX_ATTEMPTS || + !findDatabaseQueryError(error) || + !isRetryableInfrastructureError(error) + ) { + throw error + } + + const delayMs = backoffWithJitter(attempt, null, PERMISSION_CONFIG_LOAD_RETRY_BACKOFF) + logger.warn('Retrying permission config load after database error', { + workspaceId, + attempt, + maxAttempts: PERMISSION_CONFIG_LOAD_MAX_ATTEMPTS, + delayMs, + cause: describeError(error), + }) + await sleep(delayMs) + } + } +} + +/** + * Loads the governed subject's permission config. The subject is resolved here, not by callers, + * so every gate reads the same person's group. On a run context the in-flight load is memoized per + * subject and workspace in the run's `permissionConfigCache`, and a failed load is evicted. A shared + * load observes only the run's abort signal, so one caller's cancellation cannot fail it for others; + * an unshared load observes the caller's `signal`. */ async function getPermissionConfig( actorUserId: string | undefined, workspaceId: string | undefined, - ctx?: ExecutionContext + ctx?: ExecutionContext, + signal?: AbortSignal ): Promise { const userId = governedSubjectUserId(actorUserId, ctx) if (!userId || !workspaceId) { return mergeEnvAllowlist(null) } - if (ctx) { - if (ctx.permissionConfigLoaded) { - return ctx.permissionConfig ?? null - } - - const config = await getUserPermissionConfig(userId, workspaceId) - ctx.permissionConfig = config - ctx.permissionConfigLoaded = true - return config + const cache = ctx?.permissionConfigCache + if (!cache) { + return loadPermissionConfig(userId, workspaceId, signal ?? ctx?.abortSignal) } - return getUserPermissionConfig(userId, workspaceId) + const key = `${userId}:${workspaceId}` + const cached = cache.get(key) + if (cached) return cached + + const pending = loadPermissionConfig(userId, workspaceId, ctx?.abortSignal) + cache.set(key, pending) + pending.catch(() => { + if (cache.get(key) === pending) cache.delete(key) + }) + return pending } /** @@ -499,6 +539,8 @@ interface PermissionAssertion { toolId?: string toolKind?: ToolKind ctx?: ExecutionContext + /** Caller cancellation, observed while loading a config that is not shared through a run cache. */ + signal?: AbortSignal } /** @@ -516,7 +558,7 @@ interface PermissionAssertion { /** permission-group-enforced: custom_tools.use — gates tool invocation during a run, not an operation */ /** permission-group-enforced: skills.use — gates skill loading during a run, not an operation */ export async function assertPermissionsAllowed(req: PermissionAssertion): Promise { - const { workspaceId, model, blockType, toolId, toolKind, ctx } = req + const { workspaceId, model, blockType, toolId, toolKind, ctx, signal } = req const userId = governedSubjectUserId(req.userId, ctx) const blockTypeExempt = blockType ? isBlockTypeAccessControlExempt(blockType) : false @@ -527,7 +569,7 @@ export async function assertPermissionsAllowed(req: PermissionAssertion): Promis const config = userId && workspaceId - ? await getPermissionConfig(userId, workspaceId, ctx) + ? await getPermissionConfig(userId, workspaceId, ctx, signal) : mergeEnvAllowlist(null) const subject = { userId, workspaceId } diff --git a/apps/sim/ee/access-control/utils/permission-gate-subject.test.ts b/apps/sim/ee/access-control/utils/permission-gate-subject.test.ts index 5c1cfdfb3bb..9516d8a36e7 100644 --- a/apps/sim/ee/access-control/utils/permission-gate-subject.test.ts +++ b/apps/sim/ee/access-control/utils/permission-gate-subject.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { DrizzleQueryError } from 'drizzle-orm/errors' import { beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ @@ -18,6 +19,7 @@ vi.mock('@/lib/billing/core/subscription', () => ({ isOrganizationOnEnterprisePlan: vi.fn(), })) vi.mock('@/lib/workspaces/permissions/utils', () => ({ getWorkspaceWithOwner: vi.fn() })) +vi.mock('@sim/utils/helpers', () => ({ sleep: vi.fn().mockResolvedValue(undefined) })) vi.mock('@/providers/utils', () => ({ isFunctionToolCall: () => false, getProviderFromModel: () => 'openai', @@ -36,7 +38,10 @@ import { * field and keeps gating on the caller. */ function runDeclaring(capabilityGovernedUserId?: string | null): ExecutionContext { - return { metadata: { capabilityGovernedUserId } } as unknown as ExecutionContext + return { + metadata: { capabilityGovernedUserId }, + permissionConfigCache: new Map(), + } as unknown as ExecutionContext } describe('the subject a run’s permission gate is decided about', () => { @@ -164,3 +169,183 @@ describe('the group a run’s later gates read from its cache', () => { expect(mocks.getUserPermissionConfig).not.toHaveBeenCalled() }) }) + +function databaseError(code = 'ECONNRESET'): DrizzleQueryError { + return new DrizzleQueryError( + 'select "billing_blocked" from "user_stats" where "user_stats"."user_id" = $1', + ['owner-secret-id'], + Object.assign(new Error(`driver failure ${code}`), { code }) + ) +} + +/** Every block runs on a shallow copy of the run's context, so the memo lives in a Map they share. */ +describe('the run-scoped permission config cache', () => { + function runContext(overrides: Partial = {}): ExecutionContext { + return { + metadata: {}, + permissionConfigCache: new Map(), + ...overrides, + } as unknown as ExecutionContext + } + + function gate(ctx: ExecutionContext, workspaceId = 'workspace-1') { + return assertPermissionsAllowed({ + userId: 'user-1', + workspaceId, + toolId: 'http_request', + ctx, + }) + } + + beforeEach(() => { + vi.clearAllMocks() + mocks.getUserPermissionConfig.mockResolvedValue({ deniedTools: [] }) + }) + + it('loads once across the per-block copies of one run', async () => { + const run = runContext() + + await gate({ ...run }) + await gate({ ...run }) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledExactlyOnceWith('user-1', 'workspace-1') + }) + + it('shares one in-flight load between concurrent parallel branches', async () => { + const run = runContext() + let release!: (config: unknown) => void + mocks.getUserPermissionConfig.mockReturnValueOnce( + new Promise((resolve) => { + release = resolve + }) + ) + + const branches = Promise.all(Array.from({ length: 5 }, () => gate({ ...run }))) + release({ deniedTools: [] }) + await branches + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(1) + }) + + it('keeps a separate entry per workspace', async () => { + const run = runContext() + mocks.getUserPermissionConfig.mockImplementation(async (_userId, workspaceId) => + workspaceId === 'workspace-2' ? { deniedTools: ['http_request'] } : { deniedTools: [] } + ) + + await gate({ ...run }, 'workspace-1') + await expect(gate({ ...run }, 'workspace-2')).rejects.toBeInstanceOf(ToolNotAllowedError) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + }) + + it('evicts a failed load so a later gate loads again', async () => { + const run = runContext() + mocks.getUserPermissionConfig.mockRejectedValueOnce(new Error('config unavailable')) + + await expect(gate({ ...run })).rejects.toThrow('config unavailable') + await gate({ ...run }) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + }) + + it('retries a transient database failure and then caches the result', async () => { + const run = runContext() + mocks.getUserPermissionConfig.mockRejectedValueOnce(databaseError()) + + await gate({ ...run }) + await gate({ ...run }) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + }) + + it('does not retry a database failure that is not transient', async () => { + const sqlError = databaseError('42703') + mocks.getUserPermissionConfig.mockRejectedValue(sqlError) + + await expect(gate(runContext())).rejects.toBe(sqlError) + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(1) + }) + + it('fails closed with the last error once retries are exhausted', async () => { + const error = databaseError() + mocks.getUserPermissionConfig.mockRejectedValue(error) + + await expect(gate(runContext())).rejects.toBe(error) + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(3) + }) + + it('stops retrying when the run is cancelled', async () => { + const controller = new AbortController() + const reason = new Error('Execution cancelled') + mocks.getUserPermissionConfig.mockImplementationOnce(async () => { + controller.abort(reason) + throw databaseError() + }) + + await expect(gate(runContext({ abortSignal: controller.signal }))).rejects.toBe(reason) + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(1) + }) + + it('does not memoize on a context that carries no run cache', async () => { + const ctx = { metadata: {} } as unknown as ExecutionContext + + await gate(ctx) + await gate(ctx) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + expect(ctx.permissionConfigCache).toBeUndefined() + }) + + it('stops retrying when the caller of a check outside a run cancels', async () => { + const controller = new AbortController() + const reason = new Error('Tool cancelled') + mocks.getUserPermissionConfig.mockImplementationOnce(async () => { + controller.abort(reason) + throw databaseError() + }) + + await expect( + assertPermissionsAllowed({ + userId: 'user-1', + workspaceId: 'workspace-1', + toolId: 'http_request', + signal: controller.signal, + }) + ).rejects.toBe(reason) + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(1) + }) + + it('does not let one caller cancel a load shared through the run cache', async () => { + const run = runContext() + const controller = new AbortController() + mocks.getUserPermissionConfig.mockImplementationOnce(async () => { + controller.abort(new Error('Tool cancelled')) + throw databaseError() + }) + + const cancelled = assertPermissionsAllowed({ + userId: 'user-1', + workspaceId: 'workspace-1', + toolId: 'http_request', + ctx: { ...run }, + signal: controller.signal, + }) + const other = gate({ ...run }) + + await expect(Promise.all([cancelled, other])).resolves.toBeDefined() + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + }) + + it('retries a transient failure for a check made outside a run', async () => { + mocks.getUserPermissionConfig.mockRejectedValueOnce(databaseError()) + + await assertPermissionsAllowed({ + userId: 'user-1', + workspaceId: 'workspace-1', + toolId: 'http_request', + }) + + expect(mocks.getUserPermissionConfig).toHaveBeenCalledTimes(2) + }) +}) diff --git a/apps/sim/executor/execution/block-executor.test.ts b/apps/sim/executor/execution/block-executor.test.ts index 1cae20df636..2753bca8b3e 100644 --- a/apps/sim/executor/execution/block-executor.test.ts +++ b/apps/sim/executor/execution/block-executor.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { loggerMock } from '@sim/testing' +import { DrizzleQueryError } from 'drizzle-orm/errors' import { beforeEach, describe, expect, it, vi } from 'vitest' import { clearLargeValueCacheForTests } from '@/lib/execution/payloads/cache' import { createLargeArrayManifest } from '@/lib/execution/payloads/large-array-manifest' @@ -9,6 +10,7 @@ import { isLargeArrayManifest } from '@/lib/execution/payloads/large-array-manif import { isLargeValueRef } from '@/lib/execution/payloads/large-value-ref' import { projectTraceSpansForSecrets } from '@/lib/logs/execution/trace-secret-projection' import { buildTraceSpans } from '@/lib/logs/execution/trace-spans/trace-spans' +import { validateBlockType } from '@/ee/access-control/utils/permission-check' import { BlockType, EDGE } from '@/executor/constants' import type { DAGNode } from '@/executor/dag/builder' import { BlockExecutor } from '@/executor/execution/block-executor' @@ -752,6 +754,49 @@ describe('BlockExecutor', () => { expect(JSON.stringify(ctx.blockLogs)).not.toContain('"x"') }) + it('never surfaces the SQL or bound parameters of a database failure the block raises', async () => { + const block = createBlock() + const workflow: SerializedWorkflow = { + version: '1', + blocks: [block], + connections: [], + loops: {}, + parallels: {}, + } + const state = new ExecutionState() + const resolver = new VariableResolver(workflow, {}, state) + const handler: BlockHandler = { canHandle: () => true, execute: vi.fn() } + const executor = new BlockExecutor([handler], resolver, {}, state) + const ctx = createContext(state) + const driverError = Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }) + const databaseError = new DrizzleQueryError( + 'select "billing_blocked" from "user_stats" where "user_stats"."user_id" = $1 limit $2', + ['owner-secret-id', 1], + driverError + ) + vi.mocked(validateBlockType).mockRejectedValueOnce(databaseError) + const message = 'An internal error occurred while executing the block. Please try again.' + + const thrown = await executor.execute(ctx, createNode(block), block).catch((error) => error) + + expect(thrown).toBeInstanceOf(Error) + expect(thrown.message).toBe(`Function: ${message}`) + expect(thrown.cause.cause).toBe(databaseError) + expect(handler.execute).not.toHaveBeenCalled() + expect(state.getBlockOutput(block.id)).toEqual({ error: message }) + expect(ctx.blockLogs[0]?.error).toBe(message) + const surfaced = JSON.stringify([state.getBlockOutput(block.id), ctx.blockLogs]) + expect(surfaced).not.toContain('Failed query') + expect(surfaced).not.toContain('owner-secret-id') + + const executionLogger = blockExecutorBaseLogger.withMetadata.mock.results.at(-1)?.value + const logged = executionLogger.error.mock.calls.at(-1)?.[1] + expect(logged).toEqual( + expect.objectContaining({ cause: expect.objectContaining({ code: 'ECONNRESET' }) }) + ) + expect(JSON.stringify(logged)).not.toContain('owner-secret-id') + }) + it('fires block completion callbacks for pausing blocks so clients receive pause output', async () => { const block = { ...createBlock(), diff --git a/apps/sim/executor/execution/block-executor.ts b/apps/sim/executor/execution/block-executor.ts index 5da5299e51d..e19b7a464ac 100644 --- a/apps/sim/executor/execution/block-executor.ts +++ b/apps/sim/executor/execution/block-executor.ts @@ -1,6 +1,8 @@ import { createLogger, type Logger } from '@sim/logger' +import { describeError } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { isRecordLike } from '@sim/utils/object' +import { DrizzleQueryError } from 'drizzle-orm/errors' import { isTimeoutAbortReason } from '@/lib/core/execution-limits/types' import { redactApiKeys } from '@/lib/core/security/redaction' import { normalizeStringArray } from '@/lib/core/utils/arrays' @@ -97,6 +99,10 @@ function addTrustedExecutionCosts( } } +/** Replaces a database query failure's message, which carries SQL text and bound parameters. */ +const INTERNAL_DATABASE_ERROR_MESSAGE = + 'An internal error occurred while executing the block. Please try again.' + export class BlockExecutor { private execLogger: Logger @@ -625,7 +631,8 @@ export class BlockExecutor { ): Promise { const endedAt = new Date().toISOString() const duration = performance.now() - startTime - const errorMessage = normalizeError(error) + const isDatabaseError = error instanceof DrizzleQueryError + const errorMessage = isDatabaseError ? INTERNAL_DATABASE_ERROR_MESSAGE : normalizeError(error) const hasLogInputs = inputsForLog && typeof inputsForLog === 'object' && Object.keys(inputsForLog).length > 0 const input = hasLogInputs @@ -764,10 +771,12 @@ export class BlockExecutor { ) { diagnosticRegistry.mergeToolCallRegistry(ctx.resolvedSecretTraceRegistry) } - const errorDiagnostic = projectResolvedSecretDiagnosticError( - error, - diagnosticRegistry ?? ctx.resolvedSecretTraceRegistry - ) + const errorDiagnostic = isDatabaseError + ? { cause: describeError(error) } + : projectResolvedSecretDiagnosticError( + error, + diagnosticRegistry ?? ctx.resolvedSecretTraceRegistry + ) this.execLogger.error( phase === 'input_resolution' ? 'Failed to resolve block inputs' : 'Block execution failed', @@ -818,7 +827,11 @@ export class BlockExecutor { return errorOutput } - const errorToThrow = error instanceof Error ? error : new Error(errorMessage) + const errorToThrow = isDatabaseError + ? new Error(errorMessage, { cause: error }) + : error instanceof Error + ? error + : new Error(errorMessage) throw buildBlockExecutionError({ block, diff --git a/apps/sim/executor/execution/executor.test.ts b/apps/sim/executor/execution/executor.test.ts index ae4280ce553..e03a2b160b3 100644 --- a/apps/sim/executor/execution/executor.test.ts +++ b/apps/sim/executor/execution/executor.test.ts @@ -452,3 +452,34 @@ describe('DAGExecutor executor delegation origin', () => { expect(context.executorDelegationOrigin).toBe(executorDelegationOrigin) }) }) + +describe('DAGExecutor run-scoped permission config cache', () => { + function createContext(executor: DAGExecutor): ExecutionContext { + return ( + executor as unknown as { + createExecutionContext: (workflowId: string) => { context: ExecutionContext } + } + ).createExecutionContext('wf-1').context + } + + it('seeds one cache per run that survives per-block context copies', () => { + const executor = new DAGExecutor({ + workflow: { version: '1', blocks: [], connections: [] }, + contextExtensions: { workspaceId: 'ws-1' }, + }) + + const context = createContext(executor) + const blockContext = { ...context } + + expect(context.permissionConfigCache).toBeInstanceOf(Map) + expect(blockContext.permissionConfigCache).toBe(context.permissionConfigCache) + }) + + it('never shares the cache between runs', () => { + const workflow = { version: '1', blocks: [], connections: [] } + const parent = createContext(new DAGExecutor({ workflow, contextExtensions: {} })) + const child = createContext(new DAGExecutor({ workflow, contextExtensions: {} })) + + expect(child.permissionConfigCache).not.toBe(parent.permissionConfigCache) + }) +}) diff --git a/apps/sim/executor/execution/executor.ts b/apps/sim/executor/execution/executor.ts index c567df277c8..95e7c4eb0e6 100644 --- a/apps/sim/executor/execution/executor.ts +++ b/apps/sim/executor/execution/executor.ts @@ -493,6 +493,7 @@ export class DAGExecutor { : new Set(), // Deliberately not restored from a snapshot: it is a cache, so a resumed run re-resolves. toolBindingLabelCache: new Map(), + permissionConfigCache: new Map(), loopExecutions: snapshotState?.loopExecutions ? new Map( Object.entries(snapshotState.loopExecutions).map(([loopId, scope]) => [ diff --git a/apps/sim/executor/types.ts b/apps/sim/executor/types.ts index 5fa4b81d0ce..a2e4ae658af 100644 --- a/apps/sim/executor/types.ts +++ b/apps/sim/executor/types.ts @@ -421,8 +421,11 @@ export interface ExecutionContext { /** In-flight block-output PII redaction policy (resolved `blockOutputs` stage). */ piiBlockOutputRedaction?: PiiBlockOutputRedaction - permissionConfig?: PermissionGroupConfig | null - permissionConfigLoaded?: boolean + /** + * Per-run memo of permission config loads, keyed by subject and workspace. A Map so the per-block + * shallow copies of this context share it; never inherited by a child workflow's context. + */ + permissionConfigCache?: Map> /** * Resolved display names for the resources an agent tool is bound to, keyed `${kind}:${id}`, diff --git a/apps/sim/lib/core/errors/database-query-error.ts b/apps/sim/lib/core/errors/database-query-error.ts new file mode 100644 index 00000000000..2d7cfde1474 --- /dev/null +++ b/apps/sim/lib/core/errors/database-query-error.ts @@ -0,0 +1,10 @@ +import { findCause } from '@sim/utils/errors' +import { DrizzleQueryError } from 'drizzle-orm/errors' + +/** + * The Drizzle query failure anywhere in `error`'s cause chain. Its message carries the SQL text and + * bound parameters, so it must never reach a user; its `cause` holds the driver error and code. + */ +export function findDatabaseQueryError(error: unknown): DrizzleQueryError | undefined { + return findCause(error, (cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError) +} diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index 09fbb27c2d7..bd5c24234db 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -7,7 +7,7 @@ import { resolvePrincipalSubject } from '@sim/auth/principal' import { db } from '@sim/db' import { organization, workspace } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' +import { getErrorMessage, redactBoundParameters } from '@sim/utils/errors' import { filterUndefined, isPlainRecord, isRecordLike } from '@sim/utils/object' import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks' import type { Edge } from '@xyflow/react' @@ -100,7 +100,7 @@ function describeErrorCause(error: unknown): Record | undefined if (!driver) return undefined return filterUndefined({ name: driver.name, - message: driver.message, + message: redactBoundParameters(driver.message), code: driver.code, severity: driver.severity, detail: driver.detail, diff --git a/apps/sim/tools/index.test.ts b/apps/sim/tools/index.test.ts index e2c0fb8508e..360ddd0d67f 100644 --- a/apps/sim/tools/index.test.ts +++ b/apps/sim/tools/index.test.ts @@ -1348,41 +1348,7 @@ describe('executeTool Function', () => { ) }) - it('retries transient database failures during permission preflight', async () => { - const driverError = Object.assign(new Error('read ECONNRESET'), { - code: 'ECONNRESET', - errno: 'ECONNRESET', - syscall: 'read', - }) - const databaseError = new DrizzleQueryError( - 'select "id" from "workspace" where "workspace"."id" = $1 limit $2', - ['workspace-secret-id', 1], - driverError - ) - mockAssertPermissionsAllowed.mockRejectedValueOnce(databaseError) - mockToolsLogger.warn.mockClear() - - const result = await executeTool( - 'function_execute', - { code: 'return 1' }, - { executionContext: createToolExecutionContext({ userId: 'user-123' }) } - ) - - expect(result.success).toBe(true) - expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(2) - expect(mockExecuteFunction).toHaveBeenCalledTimes(1) - expect(global.fetch).not.toHaveBeenCalled() - expect(mockToolsLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Retrying tool permission preflight after database error'), - expect.objectContaining({ - attempt: 1, - maxAttempts: 3, - cause: expect.objectContaining({ code: 'ECONNRESET' }), - }) - ) - }) - - it('logs exhausted database retries without exposing query details to the caller', async () => { + it('logs a permission database failure without exposing query details to the caller', async () => { const driverError = Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET', errno: 'ECONNRESET', @@ -1408,7 +1374,7 @@ describe('executeTool Function', () => { ) expect(JSON.stringify(result)).not.toContain('Failed query') expect(JSON.stringify(result)).not.toContain('workspace-secret-id') - expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(3) + expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(1) expect(global.fetch).not.toHaveBeenCalled() const loggedError = mockToolsLogger.error.mock.calls.at(-1)?.[1] @@ -1431,28 +1397,6 @@ describe('executeTool Function', () => { expect(JSON.stringify(loggedError)).not.toContain('workspace-secret-id') }) - it('does not retry non-transient database failures during permission preflight', async () => { - const databaseError = new DrizzleQueryError( - 'select "missing_column" from "workspace"', - [], - Object.assign(new Error('column does not exist'), { code: '42703' }) - ) - mockAssertPermissionsAllowed.mockRejectedValue(databaseError) - - const result = await executeTool( - 'function_execute', - { code: 'return 1' }, - { executionContext: createToolExecutionContext({ userId: 'user-123' }) } - ) - - expect(result.success).toBe(false) - expect(result.error).toBe( - 'An internal error occurred while executing the tool. Please try again.' - ) - expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(1) - expect(global.fetch).not.toHaveBeenCalled() - }) - it('surfaces cancellation instead of a concurrent permission database failure', async () => { const controller = new AbortController() const abortReason = new Error('Execution cancelled') diff --git a/apps/sim/tools/index.ts b/apps/sim/tools/index.ts index 748c4d8a7c7..63282079e88 100644 --- a/apps/sim/tools/index.ts +++ b/apps/sim/tools/index.ts @@ -1,10 +1,9 @@ import { createLogger } from '@sim/logger' import { isLoopbackIp, unwrapIpv6Brackets } from '@sim/security/ssrf' -import { describeError, findCause, getErrorMessage, toError } from '@sim/utils/errors' +import { describeError, getErrorMessage, toError } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { isPlainRecord, isRecordLike } from '@sim/utils/object' import { backoffWithJitter, parseRetryAfter } from '@sim/utils/retry' -import { DrizzleQueryError } from 'drizzle-orm/errors' import { ApiClientError } from '@/lib/api/client/errors' import { requestJson } from '@/lib/api/client/request' import type { FunctionExecuteBody } from '@/lib/api/contracts' @@ -17,7 +16,7 @@ import { serializeBillingAttributionHeader, } from '@/lib/billing/core/billing-attribution' import { isHosted } from '@/lib/core/config/env-flags' -import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure' +import { findDatabaseQueryError } from '@/lib/core/errors/database-query-error' import { createTimeoutAbortController, DEFAULT_EXECUTION_TIMEOUT_MS, @@ -116,8 +115,6 @@ const logger = createLogger('Tools') const PRIVATE_TOOL_METADATA_ERROR_MESSAGE = 'Internal tool response metadata could not be verified' const INTERNAL_DATABASE_ERROR_MESSAGE = 'An internal error occurred while executing the tool. Please try again.' -const PERMISSION_PREFLIGHT_MAX_ATTEMPTS = 3 -const PERMISSION_PREFLIGHT_RETRY_BACKOFF = { baseMs: 25, maxMs: 100 } as const function projectToolLogMetadata( metadata: Record, @@ -134,53 +131,6 @@ function projectToolLogMetadata( : { ...structuralFallback, redacted: true } } -interface ToolPermissionPreflight { - userId: string - workspaceId: string - toolId: string - toolKind?: 'skill' | 'custom' | 'mcp' - ctx?: ExecutionContext - requestId: string - signal?: AbortSignal -} - -async function assertToolPermissionsWithRetry({ - requestId, - signal, - ...permission -}: ToolPermissionPreflight): Promise { - for (let attempt = 1; ; attempt += 1) { - signal?.throwIfAborted() - try { - await assertPermissionsAllowed(permission) - return - } catch (error) { - signal?.throwIfAborted() - const isDatabaseQueryError = Boolean( - findCause(error, (cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError) - ) - if ( - attempt >= PERMISSION_PREFLIGHT_MAX_ATTEMPTS || - !isDatabaseQueryError || - !isRetryableInfrastructureError(error) - ) { - throw error - } - - const delayMs = backoffWithJitter(attempt, null, PERMISSION_PREFLIGHT_RETRY_BACKOFF) - logger.warn(`[${requestId}] Retrying tool permission preflight after database error`, { - toolId: permission.toolId, - attempt, - maxAttempts: PERMISSION_PREFLIGHT_MAX_ATTEMPTS, - delayMs, - cause: describeError(error), - }) - await sleep(delayMs) - signal?.throwIfAborted() - } - } -} - /** * Which environment-variable reference forms a caller's `user-only` params may use. * @@ -1796,15 +1746,20 @@ async function executeToolImplementation( // Runs for ALL tools (not just kinded ones) so the per-tool `deniedTools` // denylist is enforced alongside the existing mcp/custom/skill gates. if (scope.userId && scope.workspaceId) { - await assertToolPermissionsWithRetry({ - userId: scope.userId, - workspaceId: scope.workspaceId, - toolId: normalizedToolId, - toolKind, - ctx: executionContext, - requestId, - signal: effectiveSignal, - }) + effectiveSignal?.throwIfAborted() + try { + await assertPermissionsAllowed({ + userId: scope.userId, + workspaceId: scope.workspaceId, + toolId: normalizedToolId, + toolKind, + ctx: executionContext, + signal: effectiveSignal, + }) + } catch (error) { + effectiveSignal?.throwIfAborted() + throw error + } } if (normalizedToolId === 'load_skill') { @@ -2327,10 +2282,7 @@ async function executeToolImplementation( } } catch (error: any) { const normalizedError = toError(error) - const databaseQueryError = findCause( - error, - (cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError - ) + const databaseQueryError = findDatabaseQueryError(error) const databaseErrorCause = databaseQueryError ? describeError(error) : undefined logger.error( `[${requestId}] Error executing tool ${toolId}:`,