diff --git a/apps/sim/tools/index.test.ts b/apps/sim/tools/index.test.ts index fbeda47c273..ef15e5194b9 100644 --- a/apps/sim/tools/index.test.ts +++ b/apps/sim/tools/index.test.ts @@ -25,6 +25,7 @@ import { setEnvFlags, } from '@sim/testing' import { sleep } from '@sim/utils/helpers' +import { DrizzleQueryError } from 'drizzle-orm/errors' import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution' import { projectToolResultForCopilot } from '@/lib/copilot/request/tools/resolved-secret-result' @@ -54,6 +55,7 @@ const { mockGenerateInternalDelegationToken, mockGenerateInternalToken, mockResolveWorkspaceFileReference, + mockAssertPermissionsAllowed, } = vi.hoisted(() => ({ mockGetBYOKKey: vi.fn(), mockGetToolAsync: vi.fn(), @@ -71,6 +73,7 @@ const { mockGenerateInternalDelegationToken: vi.fn(), mockGenerateInternalToken: vi.fn(), mockResolveWorkspaceFileReference: vi.fn(), + mockAssertPermissionsAllowed: vi.fn(), })) const mockSecureFetchWithPinnedIP = inputValidationMockFns.mockSecureFetchWithPinnedIP @@ -94,7 +97,7 @@ vi.mock('@/lib/core/security/encryption', () => ({ })) vi.mock('@/ee/access-control/utils/permission-check', () => ({ - assertPermissionsAllowed: vi.fn().mockResolvedValue(undefined), + assertPermissionsAllowed: mockAssertPermissionsAllowed, validateBlockType: vi.fn().mockResolvedValue(undefined), validateMcpToolsAllowed: vi.fn().mockResolvedValue(undefined), validateCustomToolsAllowed: vi.fn().mockResolvedValue(undefined), @@ -460,6 +463,7 @@ vi.spyOn(getQueryClientModule, 'getQueryClient').mockImplementation(createMockQu beforeEach(() => { vi.spyOn(getQueryClientModule, 'getQueryClient').mockImplementation(createMockQueryClient) + mockAssertPermissionsAllowed.mockResolvedValue(undefined) mockGenerateInternalDelegationToken.mockResolvedValue('executor-token') mockRunWorkflowTool.mockResolvedValue({ success: true, output: {} }) // Suites below call vi.resetAllMocks(), which wipes the shared env/urls mock @@ -692,6 +696,138 @@ describe('executeTool Function', () => { tools.function_execute = originalFunctionTool }) + 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(global.fetch).toHaveBeenCalledTimes(1) + 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 () => { + 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.mockRejectedValue(databaseError) + mockToolsLogger.error.mockClear() + + const result = await executeTool( + 'http_request', + { url: 'https://example.com' }, + { 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(JSON.stringify(result)).not.toContain('Failed query') + expect(JSON.stringify(result)).not.toContain('workspace-secret-id') + expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(3) + expect(global.fetch).not.toHaveBeenCalled() + + const loggedError = mockToolsLogger.error.mock.calls.at(-1)?.[1] + expect(loggedError).toEqual( + expect.objectContaining({ + cause: expect.objectContaining({ + name: 'Error', + message: 'read ECONNRESET', + code: 'ECONNRESET', + errno: 'ECONNRESET', + syscall: 'read', + causeChain: expect.arrayContaining([ + expect.stringContaining('params: [redacted]'), + 'Error: read ECONNRESET', + ]), + }), + }) + ) + expect(loggedError).not.toHaveProperty('stack') + 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') + const databaseError = new DrizzleQueryError( + 'select "id" from "workspace" where "workspace"."id" = $1', + ['workspace-secret-id'], + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }) + ) + mockAssertPermissionsAllowed.mockImplementationOnce(async () => { + controller.abort(abortReason) + throw databaseError + }) + + const result = await executeTool( + 'function_execute', + { code: 'return 1' }, + { + executionContext: createToolExecutionContext({ userId: 'user-123' }), + signal: controller.signal, + } + ) + + expect(result.success).toBe(false) + expect(result.error).toBe('Execution cancelled') + expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(1) + expect(global.fetch).not.toHaveBeenCalled() + }) + it('should call internal routes directly', async () => { const originalFunctionTool = { ...tools.function_execute } tools.function_execute = { diff --git a/apps/sim/tools/index.ts b/apps/sim/tools/index.ts index 04e2b11525c..8eae5583f39 100644 --- a/apps/sim/tools/index.ts +++ b/apps/sim/tools/index.ts @@ -1,8 +1,9 @@ import { createLogger } from '@sim/logger' -import { getErrorMessage, toError } from '@sim/utils/errors' +import { describeError, findCause, getErrorMessage, toError } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { isPlainRecord } from '@sim/utils/object' import { backoffWithJitter, parseRetryAfter } from '@sim/utils/retry' +import { DrizzleQueryError } from 'drizzle-orm/errors' import { getBYOKKey } from '@/lib/api-key/byok' import { type GenerateInternalDelegationTokenInput, @@ -16,6 +17,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 { DEFAULT_EXECUTION_TIMEOUT_MS, getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { getHostedKeyRateLimiter } from '@/lib/core/rate-limiter' import { @@ -92,6 +94,10 @@ const PRIVATE_MODEL_INPUT_DIRECT_EXECUTION_ERROR_MESSAGE = 'Private model input provenance is not supported by direct execution' const PRIVATE_SECRET_PROVENANCE_DIRECT_EXECUTION_ERROR_MESSAGE = 'Private secret provenance is not supported by direct execution' +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, @@ -108,6 +114,53 @@ 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() + } + } +} + interface ToolExecutionScope { workspaceId?: string workflowId?: string @@ -1534,12 +1587,14 @@ 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 assertPermissionsAllowed({ + await assertToolPermissionsWithRetry({ userId: scope.userId, workspaceId: scope.workspaceId, toolId: normalizedToolId, toolKind, ctx: executionContext, + requestId, + signal: effectiveSignal, }) } @@ -2034,17 +2089,27 @@ async function executeToolImplementation( } } catch (error: any) { const normalizedError = toError(error) + const databaseQueryError = findCause( + error, + (cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError + ) + const databaseErrorCause = databaseQueryError ? describeError(error) : undefined logger.error( `[${requestId}] Error executing tool ${toolId}:`, projectToolLogMetadata( { - error: normalizedError.message, - stack: error instanceof Error ? error.stack : undefined, + ...(databaseErrorCause + ? { cause: databaseErrorCause } + : { + error: normalizedError.message, + stack: error instanceof Error ? error.stack : undefined, + }), }, resolvedSecretTraceRegistry, { errorName: normalizedError.name, - hasStack: Boolean(error instanceof Error && error.stack), + hasStack: !databaseErrorCause && Boolean(error instanceof Error && error.stack), + ...(databaseErrorCause ? { cause: databaseErrorCause } : {}), }, structuralOnlyToolLogs ) @@ -2062,7 +2127,9 @@ async function executeToolImplementation( let errorDetails = {} if (error instanceof Error) { - errorMessage = error.message || `Error executing tool ${toolId}` + errorMessage = databaseQueryError + ? INTERNAL_DATABASE_ERROR_MESSAGE + : error.message || `Error executing tool ${toolId}` // HTTP errors are thrown as Error instances carrying `status`/`statusText`/ // `data` (see createTransformedErrorFromErrorInfo). Surface them on the // output so callers can branch on the status (e.g. treat 404 as a clean