fix(tools): sanitize database execution errors (#6645)

* fix(tools): sanitize database execution errors * fix(tools): retry transient permission failures * fix(tools): preserve preflight cancellation
2026-09-24 15:45:35 +08:00 · 2026-08-12 18:35:30 -04:00
parent 9f8d4d1310
commit 1faac4e8ef
2 changed files with 210 additions and 7 deletions
@@ -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 = {
@@ -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<string, unknown>,
@@ -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<void> {
+  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,
      })
    }

@@ -2043,17 +2098,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
      )
@@ -2071,7 +2136,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