Skip to content

Commit 1faac4e

Browse files
TheodoreSpeaksTheodoreSpeaks
authored
fix(tools): sanitize database execution errors (#6645)
* fix(tools): sanitize database execution errors * fix(tools): retry transient permission failures * fix(tools): preserve preflight cancellation
1 parent 9f8d4d1 commit 1faac4e

2 files changed

Lines changed: 210 additions & 7 deletions

File tree

‎apps/sim/tools/index.test.ts‎

Lines changed: 137 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
setEnvFlags,
2626
} from '@sim/testing'
2727
import { sleep } from '@sim/utils/helpers'
28+
import { DrizzleQueryError } from 'drizzle-orm/errors'
2829
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
2930
import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution'
3031
import { projectToolResultForCopilot } from '@/lib/copilot/request/tools/resolved-secret-result'
@@ -54,6 +55,7 @@ const {
5455
mockGenerateInternalDelegationToken,
5556
mockGenerateInternalToken,
5657
mockResolveWorkspaceFileReference,
58+
mockAssertPermissionsAllowed,
5759
} = vi.hoisted(() => ({
5860
mockGetBYOKKey: vi.fn(),
5961
mockGetToolAsync: vi.fn(),
@@ -71,6 +73,7 @@ const {
7173
mockGenerateInternalDelegationToken: vi.fn(),
7274
mockGenerateInternalToken: vi.fn(),
7375
mockResolveWorkspaceFileReference: vi.fn(),
76+
mockAssertPermissionsAllowed: vi.fn(),
7477
}))
7578

7679
const mockSecureFetchWithPinnedIP = inputValidationMockFns.mockSecureFetchWithPinnedIP
@@ -94,7 +97,7 @@ vi.mock('@/lib/core/security/encryption', () => ({
9497
}))
9598

9699
vi.mock('@/ee/access-control/utils/permission-check', () => ({
97-
assertPermissionsAllowed: vi.fn().mockResolvedValue(undefined),
100+
assertPermissionsAllowed: mockAssertPermissionsAllowed,
98101
validateBlockType: vi.fn().mockResolvedValue(undefined),
99102
validateMcpToolsAllowed: vi.fn().mockResolvedValue(undefined),
100103
validateCustomToolsAllowed: vi.fn().mockResolvedValue(undefined),
@@ -460,6 +463,7 @@ vi.spyOn(getQueryClientModule, 'getQueryClient').mockImplementation(createMockQu
460463

461464
beforeEach(() => {
462465
vi.spyOn(getQueryClientModule, 'getQueryClient').mockImplementation(createMockQueryClient)
466+
mockAssertPermissionsAllowed.mockResolvedValue(undefined)
463467
mockGenerateInternalDelegationToken.mockResolvedValue('executor-token')
464468
mockRunWorkflowTool.mockResolvedValue({ success: true, output: {} })
465469
// Suites below call vi.resetAllMocks(), which wipes the shared env/urls mock
@@ -692,6 +696,138 @@ describe('executeTool Function', () => {
692696
tools.function_execute = originalFunctionTool
693697
})
694698

699+
it('retries transient database failures during permission preflight', async () => {
700+
const driverError = Object.assign(new Error('read ECONNRESET'), {
701+
code: 'ECONNRESET',
702+
errno: 'ECONNRESET',
703+
syscall: 'read',
704+
})
705+
const databaseError = new DrizzleQueryError(
706+
'select "id" from "workspace" where "workspace"."id" = $1 limit $2',
707+
['workspace-secret-id', 1],
708+
driverError
709+
)
710+
mockAssertPermissionsAllowed.mockRejectedValueOnce(databaseError)
711+
mockToolsLogger.warn.mockClear()
712+
713+
const result = await executeTool(
714+
'function_execute',
715+
{ code: 'return 1' },
716+
{ executionContext: createToolExecutionContext({ userId: 'user-123' }) }
717+
)
718+
719+
expect(result.success).toBe(true)
720+
expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(2)
721+
expect(global.fetch).toHaveBeenCalledTimes(1)
722+
expect(mockToolsLogger.warn).toHaveBeenCalledWith(
723+
expect.stringContaining('Retrying tool permission preflight after database error'),
724+
expect.objectContaining({
725+
attempt: 1,
726+
maxAttempts: 3,
727+
cause: expect.objectContaining({ code: 'ECONNRESET' }),
728+
})
729+
)
730+
})
731+
732+
it('logs exhausted database retries without exposing query details to the caller', async () => {
733+
const driverError = Object.assign(new Error('read ECONNRESET'), {
734+
code: 'ECONNRESET',
735+
errno: 'ECONNRESET',
736+
syscall: 'read',
737+
})
738+
const databaseError = new DrizzleQueryError(
739+
'select "id" from "workspace" where "workspace"."id" = $1 limit $2',
740+
['workspace-secret-id', 1],
741+
driverError
742+
)
743+
mockAssertPermissionsAllowed.mockRejectedValue(databaseError)
744+
mockToolsLogger.error.mockClear()
745+
746+
const result = await executeTool(
747+
'http_request',
748+
{ url: 'https://example.com' },
749+
{ executionContext: createToolExecutionContext({ userId: 'user-123' }) }
750+
)
751+
752+
expect(result.success).toBe(false)
753+
expect(result.error).toBe(
754+
'An internal error occurred while executing the tool. Please try again.'
755+
)
756+
expect(JSON.stringify(result)).not.toContain('Failed query')
757+
expect(JSON.stringify(result)).not.toContain('workspace-secret-id')
758+
expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(3)
759+
expect(global.fetch).not.toHaveBeenCalled()
760+
761+
const loggedError = mockToolsLogger.error.mock.calls.at(-1)?.[1]
762+
expect(loggedError).toEqual(
763+
expect.objectContaining({
764+
cause: expect.objectContaining({
765+
name: 'Error',
766+
message: 'read ECONNRESET',
767+
code: 'ECONNRESET',
768+
errno: 'ECONNRESET',
769+
syscall: 'read',
770+
causeChain: expect.arrayContaining([
771+
expect.stringContaining('params: [redacted]'),
772+
'Error: read ECONNRESET',
773+
]),
774+
}),
775+
})
776+
)
777+
expect(loggedError).not.toHaveProperty('stack')
778+
expect(JSON.stringify(loggedError)).not.toContain('workspace-secret-id')
779+
})
780+
781+
it('does not retry non-transient database failures during permission preflight', async () => {
782+
const databaseError = new DrizzleQueryError(
783+
'select "missing_column" from "workspace"',
784+
[],
785+
Object.assign(new Error('column does not exist'), { code: '42703' })
786+
)
787+
mockAssertPermissionsAllowed.mockRejectedValue(databaseError)
788+
789+
const result = await executeTool(
790+
'function_execute',
791+
{ code: 'return 1' },
792+
{ executionContext: createToolExecutionContext({ userId: 'user-123' }) }
793+
)
794+
795+
expect(result.success).toBe(false)
796+
expect(result.error).toBe(
797+
'An internal error occurred while executing the tool. Please try again.'
798+
)
799+
expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(1)
800+
expect(global.fetch).not.toHaveBeenCalled()
801+
})
802+
803+
it('surfaces cancellation instead of a concurrent permission database failure', async () => {
804+
const controller = new AbortController()
805+
const abortReason = new Error('Execution cancelled')
806+
const databaseError = new DrizzleQueryError(
807+
'select "id" from "workspace" where "workspace"."id" = $1',
808+
['workspace-secret-id'],
809+
Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' })
810+
)
811+
mockAssertPermissionsAllowed.mockImplementationOnce(async () => {
812+
controller.abort(abortReason)
813+
throw databaseError
814+
})
815+
816+
const result = await executeTool(
817+
'function_execute',
818+
{ code: 'return 1' },
819+
{
820+
executionContext: createToolExecutionContext({ userId: 'user-123' }),
821+
signal: controller.signal,
822+
}
823+
)
824+
825+
expect(result.success).toBe(false)
826+
expect(result.error).toBe('Execution cancelled')
827+
expect(mockAssertPermissionsAllowed).toHaveBeenCalledTimes(1)
828+
expect(global.fetch).not.toHaveBeenCalled()
829+
})
830+
695831
it('should call internal routes directly', async () => {
696832
const originalFunctionTool = { ...tools.function_execute }
697833
tools.function_execute = {

‎apps/sim/tools/index.ts‎

Lines changed: 73 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { createLogger } from '@sim/logger'
2-
import { getErrorMessage, toError } from '@sim/utils/errors'
2+
import { describeError, findCause, getErrorMessage, toError } from '@sim/utils/errors'
33
import { sleep } from '@sim/utils/helpers'
44
import { isPlainRecord } from '@sim/utils/object'
55
import { backoffWithJitter, parseRetryAfter } from '@sim/utils/retry'
6+
import { DrizzleQueryError } from 'drizzle-orm/errors'
67
import { getBYOKKey } from '@/lib/api-key/byok'
78
import {
89
type GenerateInternalDelegationTokenInput,
@@ -16,6 +17,7 @@ import {
1617
serializeBillingAttributionHeader,
1718
} from '@/lib/billing/core/billing-attribution'
1819
import { isHosted } from '@/lib/core/config/env-flags'
20+
import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure'
1921
import { DEFAULT_EXECUTION_TIMEOUT_MS, getMaxExecutionTimeout } from '@/lib/core/execution-limits'
2022
import { getHostedKeyRateLimiter } from '@/lib/core/rate-limiter'
2123
import {
@@ -92,6 +94,10 @@ const PRIVATE_MODEL_INPUT_DIRECT_EXECUTION_ERROR_MESSAGE =
9294
'Private model input provenance is not supported by direct execution'
9395
const PRIVATE_SECRET_PROVENANCE_DIRECT_EXECUTION_ERROR_MESSAGE =
9496
'Private secret provenance is not supported by direct execution'
97+
const INTERNAL_DATABASE_ERROR_MESSAGE =
98+
'An internal error occurred while executing the tool. Please try again.'
99+
const PERMISSION_PREFLIGHT_MAX_ATTEMPTS = 3
100+
const PERMISSION_PREFLIGHT_RETRY_BACKOFF = { baseMs: 25, maxMs: 100 } as const
95101

96102
function projectToolLogMetadata(
97103
metadata: Record<string, unknown>,
@@ -108,6 +114,53 @@ function projectToolLogMetadata(
108114
: { ...structuralFallback, redacted: true }
109115
}
110116

117+
interface ToolPermissionPreflight {
118+
userId: string
119+
workspaceId: string
120+
toolId: string
121+
toolKind?: 'skill' | 'custom' | 'mcp'
122+
ctx?: ExecutionContext
123+
requestId: string
124+
signal?: AbortSignal
125+
}
126+
127+
async function assertToolPermissionsWithRetry({
128+
requestId,
129+
signal,
130+
...permission
131+
}: ToolPermissionPreflight): Promise<void> {
132+
for (let attempt = 1; ; attempt += 1) {
133+
signal?.throwIfAborted()
134+
try {
135+
await assertPermissionsAllowed(permission)
136+
return
137+
} catch (error) {
138+
signal?.throwIfAborted()
139+
const isDatabaseQueryError = Boolean(
140+
findCause(error, (cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError)
141+
)
142+
if (
143+
attempt >= PERMISSION_PREFLIGHT_MAX_ATTEMPTS ||
144+
!isDatabaseQueryError ||
145+
!isRetryableInfrastructureError(error)
146+
) {
147+
throw error
148+
}
149+
150+
const delayMs = backoffWithJitter(attempt, null, PERMISSION_PREFLIGHT_RETRY_BACKOFF)
151+
logger.warn(`[${requestId}] Retrying tool permission preflight after database error`, {
152+
toolId: permission.toolId,
153+
attempt,
154+
maxAttempts: PERMISSION_PREFLIGHT_MAX_ATTEMPTS,
155+
delayMs,
156+
cause: describeError(error),
157+
})
158+
await sleep(delayMs)
159+
signal?.throwIfAborted()
160+
}
161+
}
162+
}
163+
111164
interface ToolExecutionScope {
112165
workspaceId?: string
113166
workflowId?: string
@@ -1534,12 +1587,14 @@ async function executeToolImplementation(
15341587
// Runs for ALL tools (not just kinded ones) so the per-tool `deniedTools`
15351588
// denylist is enforced alongside the existing mcp/custom/skill gates.
15361589
if (scope.userId && scope.workspaceId) {
1537-
await assertPermissionsAllowed({
1590+
await assertToolPermissionsWithRetry({
15381591
userId: scope.userId,
15391592
workspaceId: scope.workspaceId,
15401593
toolId: normalizedToolId,
15411594
toolKind,
15421595
ctx: executionContext,
1596+
requestId,
1597+
signal: effectiveSignal,
15431598
})
15441599
}
15451600

@@ -2043,17 +2098,27 @@ async function executeToolImplementation(
20432098
}
20442099
} catch (error: any) {
20452100
const normalizedError = toError(error)
2101+
const databaseQueryError = findCause(
2102+
error,
2103+
(cause): cause is DrizzleQueryError => cause instanceof DrizzleQueryError
2104+
)
2105+
const databaseErrorCause = databaseQueryError ? describeError(error) : undefined
20462106
logger.error(
20472107
`[${requestId}] Error executing tool ${toolId}:`,
20482108
projectToolLogMetadata(
20492109
{
2050-
error: normalizedError.message,
2051-
stack: error instanceof Error ? error.stack : undefined,
2110+
...(databaseErrorCause
2111+
? { cause: databaseErrorCause }
2112+
: {
2113+
error: normalizedError.message,
2114+
stack: error instanceof Error ? error.stack : undefined,
2115+
}),
20522116
},
20532117
resolvedSecretTraceRegistry,
20542118
{
20552119
errorName: normalizedError.name,
2056-
hasStack: Boolean(error instanceof Error && error.stack),
2120+
hasStack: !databaseErrorCause && Boolean(error instanceof Error && error.stack),
2121+
...(databaseErrorCause ? { cause: databaseErrorCause } : {}),
20572122
},
20582123
structuralOnlyToolLogs
20592124
)
@@ -2071,7 +2136,9 @@ async function executeToolImplementation(
20712136
let errorDetails = {}
20722137

20732138
if (error instanceof Error) {
2074-
errorMessage = error.message || `Error executing tool ${toolId}`
2139+
errorMessage = databaseQueryError
2140+
? INTERNAL_DATABASE_ERROR_MESSAGE
2141+
: error.message || `Error executing tool ${toolId}`
20752142
// HTTP errors are thrown as Error instances carrying `status`/`statusText`/
20762143
// `data` (see createTransformedErrorFromErrorInfo). Surface them on the
20772144
// output so callers can branch on the status (e.g. treat 404 as a clean

0 commit comments

Comments
 (0)