Skip to content

Commit 1959644

Browse files
committed
fix(api): classify transient database reads and client-caused failures instead of answering 500
1 parent 1e0a887 commit 1959644

18 files changed

Lines changed: 319 additions & 41 deletions

File tree

‎apps/sim/app/api/files/serve/[...path]/route.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type { StorageContext } from '@/lib/uploads/config'
1919
import { readOrganizationChatAttachment } from '@/lib/uploads/contexts/organization-assistant/application'
2020
import type { WorkspaceFileRecord } from '@/lib/uploads/contexts/workspace'
2121
import { parseWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/file-manager'
22+
import { hasObjectNotFoundCause } from '@/lib/uploads/core/errors'
2223
import { downloadFile, downloadFileStream, headObject } from '@/lib/uploads/core/storage-service'
2324
import { resolveServableDocBytes } from '@/lib/uploads/documents/compile'
2425
import { DocCompileUserError } from '@/lib/uploads/documents/compile-error'
@@ -34,6 +35,7 @@ import {
3435
inferContextFromKey,
3536
isRenderableDocumentName,
3637
resolveEffectiveMimeType,
38+
tryInferContextFromKey,
3739
} from '@/lib/uploads/utils/file'
3840
import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api'
3941
import { fetchAuthorizedServableWorkspaceFileBuffer } from '@/lib/workspace-files/application/fetch-servable-workspace-file-buffer'
@@ -71,7 +73,11 @@ const logger = createLogger('FilesServeAPI')
7173
* its way out of, not something on call needs to look at.
7274
*/
7375
function logServeFailure(message: string, error: unknown): void {
74-
if (error instanceof FileNotFoundError || isPayloadSizeLimitError(error)) {
76+
if (
77+
error instanceof FileNotFoundError ||
78+
hasObjectNotFoundCause(error) ||
79+
isPayloadSizeLimitError(error)
80+
) {
7581
logger.info(message, { reason: getErrorMessage(error) })
7682
return
7783
}
@@ -389,6 +395,9 @@ export const GET = withRouteHandler(
389395
})
390396
}
391397

398+
/** No storage context owns a key without a known prefix, so no file can live there. */
399+
if (!tryInferContextFromKey(cloudKey)) throw new FileNotFoundError('File not found')
400+
392401
const isPublicByKeyPrefix =
393402
cloudKey.startsWith('profile-pictures/') ||
394403
cloudKey.startsWith('og-images/') ||
@@ -536,6 +545,10 @@ export const GET = withRouteHandler(
536545
return createFileErrorResponse(error)
537546
}
538547

548+
if (hasObjectNotFoundCause(error)) {
549+
return createFileErrorResponse(new FileNotFoundError('File not found'))
550+
}
551+
539552
return createFileErrorResponse(
540553
error instanceof Error ? error : new Error('Failed to serve file')
541554
)

‎apps/sim/app/api/logs/export/route.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { createLogger } from '@sim/logger'
44
import { getErrorMessage } from '@sim/utils/errors'
55
import { and, desc, eq, lt, or, type SQL, sql } from 'drizzle-orm'
66
import { type NextRequest, NextResponse } from 'next/server'
7+
import { validationErrorResponse } from '@/lib/api/server/validation'
78
import { getSession } from '@/lib/auth'
89
import { MATERIALIZE_CONCURRENCY, mapWithConcurrency } from '@/lib/core/utils/concurrency'
910
import { formatCsvValue, toCsvLinePieces, toCsvRow } from '@/lib/core/utils/csv'
@@ -50,7 +51,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
5051

5152
const userId = session.user.id
5253
const { searchParams } = new URL(request.url)
53-
const params = LogFilterParamsSchema.parse(Object.fromEntries(searchParams.entries()))
54+
const parsedParams = LogFilterParamsSchema.safeParse(Object.fromEntries(searchParams.entries()))
55+
if (!parsedParams.success) return validationErrorResponse(parsedParams.error)
56+
const params = parsedParams.data
5457

5558
const selectColumns = {
5659
id: workflowExecutionLogs.id,

‎apps/sim/app/api/mothership/integrations/catalog/route.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { readIntegrationCatalogContract } from '@/lib/api/contracts/mothership-integrations'
22
import {
33
defineInternalJsonRoute,
4+
internalErrorResponse,
45
internalOrchestrationErrorPolicy,
56
internalRateLimits,
67
} from '@/lib/api/server/routes'
8+
import { categorizeMcpServerError } from '@/lib/mcp/utils'
79
import { internalCopilotAuth } from '@/lib/mothership/auth/internal'
810
import {
911
INTEGRATION_CATALOG_AUDIENCE,
@@ -18,7 +20,15 @@ export const POST = defineInternalJsonRoute({
1820
rateLimit: internalRateLimits.none({
1921
reason: 'Private bounded catalog discovery rechecks current authorization on every request.',
2022
}),
21-
errorPolicy: internalOrchestrationErrorPolicy,
23+
errorPolicy: {
24+
/** A targeted MCP server that cannot be reached is the server's failure, not Sim's. */
25+
project(error) {
26+
const mcpFailure = categorizeMcpServerError(error)
27+
if (mcpFailure) return internalErrorResponse(mcpFailure.status, { error: mcpFailure.message })
28+
return internalOrchestrationErrorPolicy.project(error)
29+
},
30+
unhandled: internalOrchestrationErrorPolicy.unhandled,
31+
},
2232
mapInput: ({ body }) => body,
2333
useCase: readIntegrationCatalog,
2434
})

‎apps/sim/app/api/workspaces/[id]/background-work/route.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { db } from '@sim/db'
22
import { type NextRequest, NextResponse } from 'next/server'
33
import { getWorkspaceBackgroundWorkContract } from '@/lib/api/contracts/workspace-fork'
44
import { parseRequest } from '@/lib/api/server'
5+
import { orchestrationFailureResponse } from '@/lib/api/server/orchestration-response'
56
import { getSession } from '@/lib/auth'
67
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
78
import { listSurfacedBackgroundWork } from '@/ee/workspace-forking/lib/background-work/store'
@@ -21,7 +22,13 @@ export const GET = withRouteHandler(
2122

2223
// The fork Activity feed is a fork feature: gate it behind the same forking-enabled +
2324
// workspace-admin check the other fork routes use, instead of a bare access check.
24-
await assertWorkspaceAdminAccess(id, session.user.id)
25+
try {
26+
await assertWorkspaceAdminAccess(id, session.user.id)
27+
} catch (error) {
28+
const refused = orchestrationFailureResponse(error)
29+
if (refused) return refused
30+
throw error
31+
}
2532

2633
const { rows, nextCursor } = await listSurfacedBackgroundWork(db, id, { cursor, limit })
2734
return NextResponse.json({

‎apps/sim/app/api/workspaces/members/[id]/route.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { removeWorkspaceMemberContract } from '@/lib/api/contracts/invitations'
22
import {
33
defineInternalJsonRoute,
44
internalErrorResponse,
5+
internalOrchestrationErrorPolicy,
56
internalRateLimits,
67
internalSessionAuth,
78
} from '@/lib/api/server/routes'
@@ -32,7 +33,7 @@ export const DELETE = defineInternalJsonRoute({
3233
error instanceof NoWorkspaceAccessError
3334
)
3435
return internalErrorResponse(403, { error: 'Insufficient permissions' })
35-
return null
36+
return internalOrchestrationErrorPolicy.project(error)
3637
},
3738
unhandled: () => internalErrorResponse(500, { error: 'Failed to remove workspace member' }),
3839
},

‎apps/sim/lib/core/utils/stream-limits.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,32 @@ describe('stream limits', () => {
293293
}
294294
)
295295

296+
it.each([
297+
['a JSON body', 'application/json', '{"name":"example"}', true],
298+
[
299+
'a boundary the body never uses',
300+
'multipart/form-data; boundary=declared',
301+
'--other\r\n',
302+
true,
303+
],
304+
['a streamed body the parser cannot decode', 'text/plain', 'name=example', false],
305+
])('rejects %s as invalid multipart input', async (_label, contentType, body, declareLength) => {
306+
const bytes = new TextEncoder().encode(body)
307+
const request = new Request('http://localhost/upload', {
308+
method: 'POST',
309+
headers: {
310+
'content-type': contentType,
311+
...(declareLength ? { 'content-length': String(bytes.byteLength) } : {}),
312+
},
313+
body: declareLength ? bytes : streamFromChunks([bytes]),
314+
duplex: 'half',
315+
} as RequestInit)
316+
317+
await expect(
318+
readFormDataWithLimit(request, { maxBytes: 1024 * 1024, label: 'multipart body' })
319+
).rejects.toBeInstanceOf(MultipartFieldValidationError)
320+
})
321+
296322
it('rejects multipart streams without content-length once bytes exceed the limit', async () => {
297323
const request = new Request('http://localhost/upload', {
298324
method: 'POST',

‎apps/sim/lib/core/utils/stream-limits.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ export async function readFormDataWithLimit(
138138
assertContentLengthWithinLimit(request.headers, options.maxBytes, options.label)
139139

140140
if (request.headers?.get('content-length') || !request.body) {
141-
const formData = await request.formData()
141+
const formData = await parseFormData(request)
142142
assertMultipartFieldsAreStorable(formData)
143143
return formData
144144
}
@@ -149,11 +149,28 @@ export async function readFormDataWithLimit(
149149
headers: request.headers,
150150
body: new Uint8Array(body),
151151
})
152-
const formData = await boundedRequest.formData()
152+
const formData = await parseFormData(boundedRequest)
153153
assertMultipartFieldsAreStorable(formData)
154154
return formData
155155
}
156156

157+
/**
158+
* The fetch body parser rejects a body that does not decode as the content type it
159+
* declares (a JSON body, a boundary the body never uses) with a bare `TypeError`. That
160+
* is the caller's malformed input, so it surfaces as the same validation failure every
161+
* multipart route already answers with a 400.
162+
*/
163+
async function parseFormData(request: Pick<Request, 'formData'>): Promise<FormData> {
164+
try {
165+
return await request.formData()
166+
} catch (error) {
167+
if (error instanceof TypeError) {
168+
throw new MultipartFieldValidationError('Request body must be valid multipart form data')
169+
}
170+
throw error
171+
}
172+
}
173+
157174
export interface ReadStreamWithLimitOptions {
158175
maxBytes: number
159176
label: string

‎apps/sim/lib/core/utils/with-route-handler.test.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { loggerMock } from '@sim/testing'
2+
import { DrizzleQueryError } from 'drizzle-orm/errors'
23
import { NextRequest, NextResponse } from 'next/server'
34
import { describe, expect, it, vi } from 'vitest'
45
import { HttpError } from '@/lib/core/utils/http-error'
@@ -142,4 +143,89 @@ describe('withRouteHandler', () => {
142143
await expect(response.json()).resolves.toEqual({ family: 'generic' })
143144
}
144145
)
146+
147+
describe('a database that is temporarily unavailable', () => {
148+
const SQL = 'select "secret_column" from "workflow_execution_logs" where "id" = $1'
149+
150+
function failedQuery(code: string, message: string): DrizzleQueryError {
151+
return new DrizzleQueryError(SQL, ['id-1'], Object.assign(new Error(message), { code }))
152+
}
153+
154+
function familyRoute(error: unknown) {
155+
return withRouteHandler(
156+
async () => {
157+
throw error
158+
},
159+
{
160+
typedErrorResponse: ({ error: typed, status }) =>
161+
NextResponse.json({ family: 'typed', error: typed.message }, { status }),
162+
unhandledErrorResponse: () => NextResponse.json({ family: 'generic' }, { status: 500 }),
163+
}
164+
)
165+
}
166+
167+
it.each([
168+
[
169+
'a replica recovery conflict',
170+
failedQuery('40001', 'canceling statement due to conflict with recovery'),
171+
],
172+
['a statement timeout', failedQuery('57014', 'canceling statement due to statement timeout')],
173+
[
174+
'a connection lost under the query',
175+
failedQuery('CONNECTION_CLOSED', 'write CONNECTION_CLOSED'),
176+
],
177+
[
178+
'a server restarting',
179+
failedQuery('57P01', 'terminating connection due to administrator command'),
180+
],
181+
])('answers a read failing on %s with a retryable 503', async (_, error) => {
182+
const response = await familyRoute(error)(
183+
new NextRequest('http://localhost/api/test'),
184+
undefined
185+
)
186+
187+
expect(response.status).toBe(503)
188+
expect(Number(response.headers.get('Retry-After'))).toBeGreaterThan(0)
189+
const body = await response.json()
190+
expect(body.family).toBe('typed')
191+
expect(JSON.stringify(body)).not.toContain('secret_column')
192+
})
193+
194+
it('answers a HEAD the same way as its GET', async () => {
195+
const response = await familyRoute(
196+
failedQuery('40001', 'canceling statement due to conflict with recovery')
197+
)(new NextRequest('http://localhost/api/test', { method: 'HEAD' }), undefined)
198+
199+
expect(response.status).toBe(503)
200+
})
201+
202+
it.each(['POST', 'PUT', 'PATCH', 'DELETE'])(
203+
'leaves a %s unhandled, since an earlier statement may already have committed',
204+
async (method) => {
205+
const response = await familyRoute(
206+
failedQuery('40001', 'canceling statement due to conflict with recovery')
207+
)(new NextRequest('http://localhost/api/test', { method }), undefined)
208+
209+
expect(response.status).toBe(500)
210+
expect(response.headers.get('Retry-After')).toBeNull()
211+
}
212+
)
213+
214+
it.each([
215+
['a user-requested cancel', failedQuery('57014', 'canceling statement due to user request')],
216+
['a permanent query error', failedQuery('42601', 'syntax error at or near ")"')],
217+
[
218+
'a socket reset outside any query',
219+
Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }),
220+
],
221+
])('leaves %s on a read unhandled', async (_, error) => {
222+
const response = await familyRoute(error)(
223+
new NextRequest('http://localhost/api/test'),
224+
undefined
225+
)
226+
227+
expect(response.status).toBe(500)
228+
await expect(response.json()).resolves.toEqual({ family: 'generic' })
229+
})
230+
})
145231
})

‎apps/sim/lib/core/utils/with-route-handler.ts‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
11
import { createLogger, runWithRequestContext } from '@sim/logger'
22
import { resolveClientInfo } from '@sim/utils/client-info'
3-
import { describeError, findCause, getErrorMessage, redactBoundParameters } from '@sim/utils/errors'
3+
import {
4+
describeError,
5+
findCause,
6+
getErrorMessage,
7+
getTransientDatabaseFailure,
8+
redactBoundParameters,
9+
} from '@sim/utils/errors'
410
import type { NextRequest } from 'next/server'
511
import { NextResponse } from 'next/server'
612
import { hasExternalApiCredentials } from '@/lib/api/server/credential-headers'
713
import { getRateLimitHeaders } from '@/lib/api/server/rate-limit-context'
14+
import { ADMISSION_RETRY_AFTER_SECONDS } from '@/lib/core/admission/transient-failure'
815
import { FILE_DELIVERY_CSP_PATH_PATTERN, getMainCSPPolicy } from '@/lib/core/security/csp'
916
import { HttpError } from '@/lib/core/utils/http-error'
1017
import { generateRequestId } from '@/lib/core/utils/request'
@@ -61,6 +68,34 @@ function readTypedError(error: unknown): RouteHandlerTypedErrorContext['error']
6168
return error
6269
}
6370

71+
class DatabaseTemporarilyUnavailableError extends HttpError {
72+
readonly statusCode = 503
73+
74+
constructor() {
75+
super('Service temporarily unavailable. Please try again.')
76+
this.name = 'DatabaseTemporarilyUnavailableError'
77+
}
78+
}
79+
80+
/** Methods with no side effects (RFC 9110 §9.2.1), so a failed attempt left nothing behind. */
81+
const SAFE_METHODS = new Set(['GET', 'HEAD'])
82+
83+
/**
84+
* A safe request whose query failed for a reason that clears on its own — a replica
85+
* recovery conflict, a statement or lock timeout, a lost connection, a server restart —
86+
* answers a retryable 503 rather than an opaque 500. Writes stay unhandled: an earlier
87+
* statement in the same request may already have committed, so "retry later" would be
88+
* unsafe advice. The classifier only counts socket errors that a database query carried,
89+
* so an upstream HTTP reset is never reported as a database outage.
90+
*/
91+
function readDatabaseUnavailableError(
92+
method: string,
93+
error: unknown
94+
): DatabaseTemporarilyUnavailableError | undefined {
95+
if (!SAFE_METHODS.has(method) || !getTransientDatabaseFailure(error)) return undefined
96+
return new DatabaseTemporarilyUnavailableError()
97+
}
98+
6499
/**
65100
* Stamps the request id, plus the rate-limit trio when the route consulted a
66101
* bucket for this request. Applied on both the success and the unhandled-error
@@ -198,7 +233,8 @@ export function withRouteHandler<T>(
198233
return response
199234
}
200235

201-
const typedError = readTypedError(error)
236+
const databaseUnavailable = readDatabaseUnavailableError(method, error)
237+
const typedError = readTypedError(error) ?? databaseUnavailable
202238
if (typedError) {
203239
const typedStatus = typedError.statusCode
204240
if (typedStatus >= 500) {
@@ -213,7 +249,10 @@ export function withRouteHandler<T>(
213249
}
214250
response = options.typedErrorResponse
215251
? options.typedErrorResponse({ error: typedError, requestId, status: typedStatus })
216-
: NextResponse.json({ error: message, requestId }, { status: typedStatus })
252+
: NextResponse.json({ error: typedError.message, requestId }, { status: typedStatus })
253+
if (databaseUnavailable && !response.headers.has('Retry-After')) {
254+
response.headers.set('Retry-After', ADMISSION_RETRY_AFTER_SECONDS.toString())
255+
}
217256
applyResponseHeaders(response, request, requestId)
218257
return response
219258
}

0 commit comments

Comments
 (0)