From 498cdb6af01adaef9552511aae56ec51dd4c39fa Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Fri, 28 Aug 2026 00:57:02 -0700 Subject: [PATCH 1/6] improvement(api): retire route contracts that no route serves (#7206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(api): retire route contracts that no route serves The staging integ alarm fired because the session knowledge-document inline-create check got a 405. #7179 moved tool operations in process and deleted the routes that only existed to serve them, but `createKnowledgeDocumentsContract` kept declaring `POST /api/knowledge/[id]/documents` — a path whose surviving GET/PATCH make Next.js answer POST with 405 rather than an honest 404. Nothing in the repo called it: the KB UI creates documents through the presigned upload flow, and the capability itself is unaffected because `knowledge_create_document` reaches the same use case in process. Audited all 1125 contracts for the same drift. It was the only one whose path resolves to a live route missing the declared method; 259 others declare paths of routes that were deleted outright, which 404 honestly and are left alone. - Drop the create-documents route contract for plain `params`/`body` schemas plus a named response schema, so nothing declares an endpoint we do not serve. The schemas stay in the contracts tree next to the siblings they share (`documentDataSchema` is used by the v2 contracts, and `createKnowledgeDocumentsBodySchema` already backed the in-process operation). - Delete four contracts with no consumer at all — both TTS contracts, docusign, and mistral. Their handlers own better schemas: TTS dispatches by `toolId` with eight per-provider schemas instead of one passthrough superset, and mistral bounds `pages` by the OCR request policy. crowdstrike and windchill look similar but are load-bearing (schema and derived types are imported by live code), so they stay. - Add `check:api-contract-routes`, picked up automatically by `run-audits`. `check:route-verbs` scans routes to contracts, so a contract whose route method was deleted is invisible to it — verified it passes clean against the exact regression this catches. Co-Authored-By: Claude Opus 5 (1M context) * fix(api): read contracts by import, and retire two more stale declarations Greptile flagged the audit's brace counter as blind to braces inside strings, template literals, regexes and comments. It was, but the bigger problem was that a text scan can only see contracts whose `method`/`path` are inline literals — the 70-plus built through `definePostSelector(path, …)` and friends were never checked at all. Comparing raw `defineRouteContract(` occurrences against parsed ones showed the scanner silently skipping declarations. Read the contracts by importing each contract module and inspecting its exported objects instead, the way `check-route-verbs.ts` already resolves the contract behind a route. Contract modules are pure Zod so importing them is safe; route files stay a static scan because importing one drags in `@sim/db`, auth and `next/server`. Barrels re-export the same object, so entries are keyed by identity. Coverage goes from 1125 contracts to 1283. That immediately surfaced two more instances of exactly what this PR retires. `/api/tools/confluence/page` kept its `PUT` and `DELETE` contracts after #7179 reduced the route to the selector `POST`, so both declared verbs the live route answers with 405. Neither is fetched — `lib/internal/confluence/execute-tool.ts` is the only consumer — so they become plain schemas like the knowledge one, and `executeOperation` now delegates to a schema form rather than growing a second pattern beside it. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../lib/api/contracts/knowledge/documents.ts | 23 +- .../lib/api/contracts/selectors/confluence.ts | 20 +- apps/sim/lib/api/contracts/tools/docusign.ts | 20 -- apps/sim/lib/api/contracts/tools/index.ts | 1 - .../contracts/tools/media/document-parse.ts | 22 +- .../lib/api/contracts/tools/media/index.ts | 1 - apps/sim/lib/api/contracts/tools/media/tts.ts | 92 -------- apps/sim/lib/api/contracts/types.ts | 27 +++ .../lib/internal/confluence/execute-tool.ts | 56 +++-- .../lib/internal/knowledge/execute-tool.ts | 24 +- .../tool-operations/parse-contract-input.ts | 58 ++++- package.json | 1 + scripts/check-api-contract-routes.ts | 206 ++++++++++++++++++ 13 files changed, 363 insertions(+), 188 deletions(-) delete mode 100644 apps/sim/lib/api/contracts/tools/docusign.ts delete mode 100644 apps/sim/lib/api/contracts/tools/media/tts.ts create mode 100644 scripts/check-api-contract-routes.ts diff --git a/apps/sim/lib/api/contracts/knowledge/documents.ts b/apps/sim/lib/api/contracts/knowledge/documents.ts index bc8ad27d841..151cae080e5 100644 --- a/apps/sim/lib/api/contracts/knowledge/documents.ts +++ b/apps/sim/lib/api/contracts/knowledge/documents.ts @@ -319,16 +319,23 @@ export const listKnowledgeDocumentsContract = defineRouteContract({ }, }) -export const createKnowledgeDocumentsContract = defineRouteContract({ - method: 'POST', - path: '/api/knowledge/[id]/documents', +/** + * Document creation from inline content has no HTTP route: `POST + * /api/knowledge/[id]/documents` was retired when tool operations moved + * in-process, and the surviving `GET`/`PATCH` on that path would answer a `POST` + * with 405. So these stay plain schemas rather than a `defineRouteContract` — + * `lib/internal/knowledge/execute-tool.ts` validates `knowledge_create_document` + * against them directly. Callers wanting an HTTP upload use v1 or v2, both of + * which take multipart file bodies rather than inline content. + */ +export const createKnowledgeDocumentsSchemas = { params: knowledgeBaseParamsSchema, body: createKnowledgeDocumentsBodySchema, - response: { - mode: 'json', - schema: successResponseSchema(z.union([bulkCreateDocumentsResponseSchema, documentDataSchema])), - }, -}) +} as const + +export const createKnowledgeDocumentsResponseSchema = successResponseSchema( + z.union([bulkCreateDocumentsResponseSchema, documentDataSchema]) +) export const updateKnowledgeDocumentContract = defineRouteContract({ method: 'PUT', diff --git a/apps/sim/lib/api/contracts/selectors/confluence.ts b/apps/sim/lib/api/contracts/selectors/confluence.ts index b680a1d8c93..360e61911ee 100644 --- a/apps/sim/lib/api/contracts/selectors/confluence.ts +++ b/apps/sim/lib/api/contracts/selectors/confluence.ts @@ -399,14 +399,16 @@ export const confluencePageSelectorContract = definePostSelector( z.object({ id: z.string(), title: z.string() }).passthrough() ) -export const confluenceUpdatePageContract = defineConfluencePutContract( - '/api/tools/confluence/page', - confluenceUpdatePageBodySchema -) -export const confluenceDeletePageContract = defineConfluenceDeleteContract( - '/api/tools/confluence/page', - confluenceDeletePageBodySchema -) +/** + * Page update and delete have no contract because they have no route: the + * `PUT`/`DELETE` handlers on `/api/tools/confluence/page` were retired when the + * tool moved in process, and the surviving selector `POST` on that path would + * answer either verb with 405. `lib/internal/confluence/execute-tool.ts` + * validates both against `confluenceUpdatePageBodySchema` / + * `confluenceDeletePageBodySchema` directly. + */ +export type ConfluenceUpdatePageBody = z.output +export type ConfluenceDeletePageBody = z.output export const confluenceDeleteAttachmentContract = defineConfluenceDeleteContract( '/api/tools/confluence/attachment', confluenceDeleteAttachmentBodySchema @@ -562,8 +564,6 @@ export const confluenceUserContract = defineConfluencePostContract( export type ConfluencePagesBody = ContractBody export type ConfluencePageBody = ContractBody -export type ConfluenceUpdatePageBody = ContractBody -export type ConfluenceDeletePageBody = ContractBody export type ConfluenceDeleteAttachmentBody = ContractBody export type ConfluenceListAttachmentsQuery = ContractQuery export type ConfluenceListBlogPostsQuery = ContractQuery diff --git a/apps/sim/lib/api/contracts/tools/docusign.ts b/apps/sim/lib/api/contracts/tools/docusign.ts deleted file mode 100644 index 5ebdfae4a87..00000000000 --- a/apps/sim/lib/api/contracts/tools/docusign.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { z } from 'zod' -import { defineRouteContract } from '@/lib/api/contracts/types' - -export const docusignToolBodySchema = z - .object({ - accessToken: z.string().min(1, 'Access token is required'), - operation: z.string().min(1, 'Operation is required'), - }) - .passthrough() - -export const docusignToolContract = defineRouteContract({ - method: 'POST', - path: '/api/tools/docusign', - body: docusignToolBodySchema, - response: { - mode: 'json', - // untyped-response: forwards DocuSign API response unchanged; shape varies by operation (envelope, listing, base64 download, etc.) - schema: z.unknown(), - }, -}) diff --git a/apps/sim/lib/api/contracts/tools/index.ts b/apps/sim/lib/api/contracts/tools/index.ts index 69859fe778e..dfc47d74ab8 100644 --- a/apps/sim/lib/api/contracts/tools/index.ts +++ b/apps/sim/lib/api/contracts/tools/index.ts @@ -4,7 +4,6 @@ export * from './communication' export * from './crowdstrike' export * from './custom' export * from './databases' -export * from './docusign' export * from './file' export * from './google' export * from './imap' diff --git a/apps/sim/lib/api/contracts/tools/media/document-parse.ts b/apps/sim/lib/api/contracts/tools/media/document-parse.ts index 39b7e1ed15c..3344a497b5e 100644 --- a/apps/sim/lib/api/contracts/tools/media/document-parse.ts +++ b/apps/sim/lib/api/contracts/tools/media/document-parse.ts @@ -3,7 +3,7 @@ import { resolvedSecretTraceProvenanceSchema } from '@/lib/api/contracts/primiti import { AWS_REGION_PATTERN, toolJsonResponseSchema } from '@/lib/api/contracts/tools/media/shared' import { defineRouteContract } from '@/lib/api/contracts/types' import { RESOLVED_SECRET_PROVENANCE_FIELD } from '@/lib/execution/private-tool-metadata' -import { FileInputSchema, RawFileInputSchema } from '@/lib/uploads/utils/file-schemas' +import { RawFileInputSchema } from '@/lib/uploads/utils/file-schemas' const textractQuerySchema = z.object({ Text: z.string().min(1), @@ -110,19 +110,6 @@ export const textractAnalyzeIdBodySchema = z } }) -export const mistralParseBodySchema = z.object({ - apiKey: z.string().min(1, 'API key is required'), - filePath: z.string().min(1, 'File path is required').optional(), - fileData: FileInputSchema.optional(), - file: FileInputSchema.optional(), - resultType: z.string().optional(), - pages: z.array(z.number()).optional(), - includeImageBase64: z.boolean().optional(), - imageLimit: z.number().optional(), - imageMinSize: z.number().optional(), - [RESOLVED_SECRET_PROVENANCE_FIELD]: resolvedSecretTraceProvenanceSchema.optional(), -}) - export const textractParseContract = defineRouteContract({ method: 'POST', path: '/api/tools/textract/parse', @@ -143,10 +130,3 @@ export const textractAnalyzeIdContract = defineRouteContract({ body: textractAnalyzeIdBodySchema, response: { mode: 'json', schema: toolJsonResponseSchema }, }) - -export const mistralParseContract = defineRouteContract({ - method: 'POST', - path: '/api/tools/mistral/parse', - body: mistralParseBodySchema, - response: { mode: 'json', schema: toolJsonResponseSchema }, -}) diff --git a/apps/sim/lib/api/contracts/tools/media/index.ts b/apps/sim/lib/api/contracts/tools/media/index.ts index d8d3f5817bb..17dcec1dbe8 100644 --- a/apps/sim/lib/api/contracts/tools/media/index.ts +++ b/apps/sim/lib/api/contracts/tools/media/index.ts @@ -1,5 +1,4 @@ export * from '@/lib/api/contracts/tools/media/document-parse' export * from '@/lib/api/contracts/tools/media/image' export * from '@/lib/api/contracts/tools/media/shared' -export * from '@/lib/api/contracts/tools/media/tts' export * from '@/lib/api/contracts/tools/media/video' diff --git a/apps/sim/lib/api/contracts/tools/media/tts.ts b/apps/sim/lib/api/contracts/tools/media/tts.ts deleted file mode 100644 index fdff389ccde..00000000000 --- a/apps/sim/lib/api/contracts/tools/media/tts.ts +++ /dev/null @@ -1,92 +0,0 @@ -import { z } from 'zod' -import { toolJsonResponseSchema } from '@/lib/api/contracts/tools/media/shared' -import { defineRouteContract } from '@/lib/api/contracts/types' - -export const ttsToolBodySchema = z.object({ - text: z.string({ error: 'Missing required parameters' }).min(1, 'Missing required parameters'), - voiceId: z.string({ error: 'Missing required parameters' }).min(1, 'Missing required parameters'), - apiKey: z.string({ error: 'Missing required parameters' }).min(1, 'Missing required parameters'), - modelId: z.string().optional().default('eleven_monolingual_v1'), - stability: z.coerce.number().min(0).max(1).optional(), - similarityBoost: z.coerce.number().min(0).max(1).optional(), - workspaceId: z.string().optional(), - workflowId: z.string().optional(), - executionId: z.string().optional(), -}) - -export const ttsOutputFormatSchema = z.union([z.record(z.string(), z.unknown()), z.string()]) -export const playHtOutputFormatSchema = z.enum(['mp3', 'wav', 'ogg', 'flac', 'mulaw']) - -export const ttsUnifiedToolBodySchema = z - .object({ - provider: z.enum( - ['openai', 'deepgram', 'elevenlabs', 'cartesia', 'google', 'azure', 'playht'], - { - error: 'Missing required fields: provider, text, and apiKey', - } - ), - text: z - .string({ error: 'Missing required fields: provider, text, and apiKey' }) - .min(1, 'Missing required fields: provider, text, and apiKey'), - apiKey: z - .string({ error: 'Missing required fields: provider, text, and apiKey' }) - .min(1, 'Missing required fields: provider, text, and apiKey'), - model: z.enum(['tts-1', 'tts-1-hd', 'gpt-4o-mini-tts']).optional(), - voice: z.string().optional(), - responseFormat: z.enum(['mp3', 'opus', 'aac', 'flac', 'wav', 'pcm']).optional(), - speed: z.coerce.number().optional(), - encoding: z.enum(['linear16', 'mp3', 'opus', 'aac', 'flac', 'mulaw', 'alaw']).optional(), - sampleRate: z.coerce.number().optional(), - bitRate: z.coerce.number().optional(), - container: z.enum(['none', 'wav', 'ogg']).optional(), - voiceId: z.string().optional(), - modelId: z.string().optional(), - stability: z.coerce.number().optional(), - similarityBoost: z.coerce.number().optional(), - style: z.union([z.coerce.number(), z.string()]).optional(), - useSpeakerBoost: z.boolean().optional(), - language: z.string().optional(), - outputFormat: ttsOutputFormatSchema.optional().nullable(), - emotion: z.array(z.string()).optional(), - languageCode: z.string().optional(), - gender: z.enum(['MALE', 'FEMALE', 'NEUTRAL']).optional(), - audioEncoding: z.enum(['LINEAR16', 'MP3', 'OGG_OPUS', 'MULAW', 'ALAW']).optional(), - speakingRate: z.coerce.number().optional(), - pitch: z.union([z.number(), z.string()]).optional(), - volumeGainDb: z.coerce.number().optional(), - sampleRateHertz: z.coerce.number().optional(), - effectsProfileId: z.array(z.string()).optional(), - region: z - .string() - .regex( - /^[a-z][a-z0-9-]{1,30}[a-z0-9]$/, - 'region must be a valid Azure region identifier (e.g. eastus, westeurope)' - ) - .optional(), - rate: z.string().optional(), - styleDegree: z.coerce.number().optional(), - role: z.string().optional(), - userId: z.string().optional(), - quality: z.enum(['draft', 'standard', 'premium']).optional(), - temperature: z.coerce.number().optional(), - voiceGuidance: z.coerce.number().optional(), - textGuidance: z.coerce.number().optional(), - workspaceId: z.string().optional(), - workflowId: z.string().optional(), - executionId: z.string().optional(), - }) - .passthrough() - -export const ttsToolContract = defineRouteContract({ - method: 'POST', - path: '/api/tools/tts', - body: ttsToolBodySchema, - response: { mode: 'json', schema: toolJsonResponseSchema }, -}) - -export const ttsUnifiedToolContract = defineRouteContract({ - method: 'POST', - path: '/api/tools/tts/unified', - body: ttsUnifiedToolBodySchema, - response: { mode: 'json', schema: toolJsonResponseSchema }, -}) diff --git a/apps/sim/lib/api/contracts/types.ts b/apps/sim/lib/api/contracts/types.ts index 713801f75f6..eaaf2d8911b 100644 --- a/apps/sim/lib/api/contracts/types.ts +++ b/apps/sim/lib/api/contracts/types.ts @@ -52,6 +52,33 @@ export type ResponseMode = | StreamResponseMode | RedirectResponseMode +/** + * A contract is consumed in one of two modes, and `method`/`path` only describe + * the first. + * + * **Boundary mode** — the common one. The contract bridges the client/server + * gap: a route builder under `app/api/**` serves `method` at `path`, and + * `requestJson(contract, …)` on the client parses the request out and validates + * the response back. Both sides read the same declaration, so `method` and + * `path` are load-bearing. + * + * **In-process mode.** Tool operations that once self-hopped over HTTP now + * execute in the same process (`lib/internal//execute-tool.ts`), and + * they kept their contract as the input/response schema bundle — + * `parseInternalContractInput` reads only `params`, `query`, and `body`, and + * never looks at `method` or `path`. For these there is no route and no client + * fetch; `method` and `path` are vestigial, describing the HTTP endpoint the + * operation *used* to expose. Do not read them as evidence that an endpoint + * exists, and do not point a client at one. + * + * The distinction is not expressed in the type, so which mode a contract is in + * is derived, never annotated per file — `bun run check:api-contract-routes + * --list-in-process` enumerates the in-process set from the tree rather than + * from a hand-maintained list that would drift. That same audit enforces the + * part which actually matters: an in-process contract may not claim a `path` + * whose live route serves other methods, because a caller trusting the + * declaration gets a 405 rather than an honest 404. + */ export interface ApiRouteContract< TParams extends ApiSchema | undefined = undefined, TQuery extends ApiSchema | undefined = undefined, diff --git a/apps/sim/lib/internal/confluence/execute-tool.ts b/apps/sim/lib/internal/confluence/execute-tool.ts index 671902f4a8a..acc85ad3225 100644 --- a/apps/sim/lib/internal/confluence/execute-tool.ts +++ b/apps/sim/lib/internal/confluence/execute-tool.ts @@ -1,5 +1,10 @@ import { getErrorMessage } from '@sim/utils/errors' -import type { AnyApiRouteContract, ContractBody, ContractQuery } from '@/lib/api/contracts' +import type { + AnyApiRouteContract, + ApiSchema, + ContractBody, + ContractQuery, +} from '@/lib/api/contracts' import { confluenceBlogPostOperationContract, confluenceCreateCommentContract, @@ -10,7 +15,7 @@ import { confluenceDeleteBlogPostContract, confluenceDeleteCommentContract, confluenceDeleteLabelContract, - confluenceDeletePageContract, + confluenceDeletePageBodySchema, confluenceDeletePagePropertyContract, confluenceDeleteSpaceContract, confluenceGetSpaceContract, @@ -37,7 +42,7 @@ import { confluenceTasksContract, confluenceUpdateBlogPostContract, confluenceUpdateCommentContract, - confluenceUpdatePageContract, + confluenceUpdatePageBodySchema, confluenceUpdateSpaceContract, confluenceUploadAttachmentContract, confluenceUserContract, @@ -94,12 +99,10 @@ import type { type ContractInput = NonNullable | ContractQuery> -function parsePreparedRequest( - contract: C, +function parsePreparedInput( + schema: ApiSchema, request: InternalToolOperationCall -): { success: true; data: ContractInput } | { success: false; response: Response } { - const schema = contract.query ?? contract.body - if (!schema) throw new Error(`Confluence contract ${contract.path} has no request input`) +): { success: true; data: T } | { success: false; response: Response } { const parsed = schema.safeParse(request.input) if (!parsed.success) { return { @@ -110,16 +113,21 @@ function parsePreparedRequest( ), } } - return { success: true, data: parsed.data as ContractInput } + return { success: true, data: parsed.data as T } } -async function executeOperation( - contract: C, +/** + * Operations whose HTTP route was retired hold a bare request schema rather than + * a contract, so they cannot declare a `method` and `path` nothing serves. The + * contract form below feeds this the schema it would have parsed anyway. + */ +async function executeSchemaOperation( + schema: ApiSchema, request: InternalToolOperationCall, - execute: (input: ContractInput, context: ConfluenceOperationContext) => Promise + execute: (input: T, context: ConfluenceOperationContext) => Promise ): Promise { request.signal?.throwIfAborted() - const parsed = parsePreparedRequest(contract, request) + const parsed = parsePreparedInput(schema, request) if (!parsed.success) return parsed.response try { const result = await execute(parsed.data, { @@ -141,6 +149,16 @@ async function executeOperation( } } +function executeOperation( + contract: C, + request: InternalToolOperationCall, + execute: (input: ContractInput, context: ConfluenceOperationContext) => Promise +): Promise { + const schema = contract.query ?? contract.body + if (!schema) throw new Error(`Confluence contract ${contract.path} has no request input`) + return executeSchemaOperation>(schema, request, execute) +} + export const executeConfluenceTool: InternalToolOperationHandler = async (request) => { switch (request.toolId) { case 'confluence_add_label': @@ -194,7 +212,11 @@ export const executeConfluenceTool: InternalToolOperationHandler = async (reques case 'confluence_delete_label': return executeOperation(confluenceDeleteLabelContract, request, executeConfluenceDeleteLabel) case 'confluence_delete_page': - return executeOperation(confluenceDeletePageContract, request, executeConfluenceDeletePage) + return executeSchemaOperation( + confluenceDeletePageBodySchema, + request, + executeConfluenceDeletePage + ) case 'confluence_delete_page_property': return executeOperation( confluenceDeletePagePropertyContract, @@ -327,7 +349,11 @@ export const executeConfluenceTool: InternalToolOperationHandler = async (reques executeConfluenceSearchInSpace ) case 'confluence_update': - return executeOperation(confluenceUpdatePageContract, request, executeConfluenceUpdatePage) + return executeSchemaOperation( + confluenceUpdatePageBodySchema, + request, + executeConfluenceUpdatePage + ) case 'confluence_update_blogpost': return executeOperation( confluenceUpdateBlogPostContract, diff --git a/apps/sim/lib/internal/knowledge/execute-tool.ts b/apps/sim/lib/internal/knowledge/execute-tool.ts index 15f59baeb92..a9fc25818ae 100644 --- a/apps/sim/lib/internal/knowledge/execute-tool.ts +++ b/apps/sim/lib/internal/knowledge/execute-tool.ts @@ -1,8 +1,9 @@ import { createLogger } from '@sim/logger' -import type { AnyApiRouteContract } from '@/lib/api/contracts' +import type { AnyApiRouteContract, ApiSchema } from '@/lib/api/contracts' import { createKnowledgeChunkContract, - createKnowledgeDocumentsContract, + createKnowledgeDocumentsResponseSchema, + createKnowledgeDocumentsSchemas, deleteKnowledgeChunkContract, deleteKnowledgeDocumentContract, getKnowledgeConnectorContract, @@ -36,7 +37,10 @@ import { upsertDocumentOperation, } from '@/lib/internal/knowledge/operations' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' -import { parseInternalContractInput } from '@/lib/internal/tool-operations/parse-contract-input' +import { + parseInternalContractInput, + parseInternalOperationInput, +} from '@/lib/internal/tool-operations/parse-contract-input' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { internalKnowledgeErrorPolicies } from '@/lib/knowledge/api/route-policies' import { KNOWLEDGE_DELEGATION_AUDIENCE } from '@/lib/knowledge/application/authorization' @@ -92,6 +96,11 @@ function projectError( ) } +function schemaSuccessResponse(schema: ApiSchema, result: KnowledgeOperationResponse): Response { + const validated = schema.parse(result.body) as Record + return Response.json({ ...validated, ...result.bodyFields }, { headers: result.headers }) +} + function successResponse( contract: C, result: KnowledgeOperationResponse @@ -99,8 +108,7 @@ function successResponse( if (contract.response.mode !== 'json') { throw new Error('Knowledge tool contract must return JSON') } - const validated = contract.response.schema.parse(result.body) as Record - return Response.json({ ...validated, ...result.bodyFields }, { headers: result.headers }) + return schemaSuccessResponse(contract.response.schema, result) } /** Executes every Knowledge tool through the same authorized application use cases as HTTP. */ @@ -132,10 +140,10 @@ export const executeKnowledgeTool: InternalToolOperationHandler = async (request switch (toolId) { case 'knowledge_create_document': { policy = internalKnowledgeErrorPolicies.uploads - const parsed = parseInternalContractInput(createKnowledgeDocumentsContract, input) + const parsed = parseInternalOperationInput(createKnowledgeDocumentsSchemas, input) if (!parsed.success) return parsed.response - return successResponse( - createKnowledgeDocumentsContract, + return schemaSuccessResponse( + createKnowledgeDocumentsResponseSchema, await createDocumentsOperation(parsed.data.params.id, parsed.data.body, context) ) } diff --git a/apps/sim/lib/internal/tool-operations/parse-contract-input.ts b/apps/sim/lib/internal/tool-operations/parse-contract-input.ts index b7fffc5201f..a7432849f14 100644 --- a/apps/sim/lib/internal/tool-operations/parse-contract-input.ts +++ b/apps/sim/lib/internal/tool-operations/parse-contract-input.ts @@ -1,9 +1,11 @@ import type { z } from 'zod' import type { AnyApiRouteContract, + ApiSchema, ContractBody, ContractParams, ContractQuery, + EmptySchemaOutput, } from '@/lib/api/contracts' import { serializeZodIssues } from '@/lib/api/server/validation' @@ -13,6 +15,21 @@ export interface ParsedInternalContractInput { body: B } +/** + * The request slices an in-process operation validates, for an operation whose + * HTTP route has been retired: it passes its schemas directly rather than + * keeping a contract that declares a `method` and `path` nothing serves. + */ +export interface InternalOperationSchemas { + params?: ApiSchema + query?: ApiSchema + body?: ApiSchema +} + +type ParseResult = + | { success: true; data: ParsedInternalContractInput } + | { success: false; response: Response } + function validationError(error: z.ZodError): Response { return Response.json( { error: 'Validation error', details: serializeZodIssues(error) }, @@ -20,16 +37,33 @@ function validationError(error: z.ZodError): Response { ) } +/** + * Contract callers keep their own entry point because `ContractParams` and + * friends `infer` each slice out of the contract's generics. Reading the same + * slices off an optional-property shape widens every one of them with + * `undefined`, which breaks narrowing at every call site. + */ export function parseInternalContractInput( contract: C, input: unknown, options: { maxInputBytes?: number } = {} -): - | { - success: true - data: ParsedInternalContractInput, ContractQuery, ContractBody> - } - | { success: false; response: Response } { +): ParseResult, ContractQuery, ContractBody> { + return parseInternalOperationInput(contract, input, options) as ParseResult< + ContractParams, + ContractQuery, + ContractBody + > +} + +export function parseInternalOperationInput( + schemas: S, + input: unknown, + options: { maxInputBytes?: number } = {} +): ParseResult< + EmptySchemaOutput, + EmptySchemaOutput, + EmptySchemaOutput +> { if (options.maxInputBytes !== undefined) { let serialized: string try { @@ -53,21 +87,21 @@ export function parseInternalContractInput( } } - const params = contract.params?.safeParse(input) + const params = schemas.params?.safeParse(input) if (params && !params.success) return { success: false, response: validationError(params.error) } - const query = contract.query?.safeParse(input) + const query = schemas.query?.safeParse(input) if (query && !query.success) return { success: false, response: validationError(query.error) } - const body = contract.body?.safeParse(input) + const body = schemas.body?.safeParse(input) if (body && !body.success) return { success: false, response: validationError(body.error) } return { success: true, data: { - params: (params?.data ?? undefined) as ContractParams, - query: (query?.data ?? undefined) as ContractQuery, - body: (body?.data ?? undefined) as ContractBody, + params: (params?.data ?? undefined) as EmptySchemaOutput, + query: (query?.data ?? undefined) as EmptySchemaOutput, + body: (body?.data ?? undefined) as EmptySchemaOutput, }, } } diff --git a/package.json b/package.json index 45d05f955ee..35e5619bd11 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ "check": "turbo run format:check", "check:boundaries": "bun run scripts/check-monorepo-boundaries.ts", "check:api-validation": "bun run scripts/check-api-validation-contracts.ts --check", + "check:api-contract-routes": "bun run scripts/check-api-contract-routes.ts", "check:fork-dependent-coverage": "bun run scripts/check-fork-dependent-coverage.ts", "generate:openapi": "bun run scripts/generate-openapi.ts", "check:openapi": "bun run scripts/check-openapi.ts", diff --git a/scripts/check-api-contract-routes.ts b/scripts/check-api-contract-routes.ts new file mode 100644 index 00000000000..7dd5e8b78dd --- /dev/null +++ b/scripts/check-api-contract-routes.ts @@ -0,0 +1,206 @@ +#!/usr/bin/env bun +/** + * Fails when a route contract declares a `method` on a `path` whose route file + * exists but does not export that method. + * + * Contracts are consumed in two modes (see `ApiRouteContract`). A boundary + * contract is served by a route under `app/api/**` and fetched by a client. An + * in-process contract is only an input/response schema bundle for a tool + * operation in `lib/internal//execute-tool.ts`, where `method` and + * `path` are vestigial. + * + * A vestigial path whose route segment no longer exists is harmless: a caller + * gets an honest 404. A vestigial path that still resolves to a live route + * serving *other* methods is not — Next.js answers 405, which reads as "wrong + * verb, endpoint is fine" and sends the caller looking in the wrong place. That + * is the only case this script rejects, so it stays silent on the in-process + * contracts whose routes were deleted outright. + * + * Contracts are read by importing each contract module and inspecting its + * exported objects, the same way `check-route-verbs.ts` resolves the contract + * behind a route. Scanning the source text instead would have to re-implement a + * TypeScript lexer to know which braces are code and which sit inside a string, + * template literal, regex or comment, and it could only ever see contracts whose + * `method`/`path` are inline literals — the 70-plus built through helpers like + * `definePostSelector(path, …)` would be invisible. Route files stay a static + * scan on purpose: importing one drags in `@sim/db`, auth and `next/server`, + * whereas contract modules are pure Zod. + */ +import { existsSync } from 'node:fs' +import { readdir, readFile, stat } from 'node:fs/promises' +import path from 'node:path' + +const ROOT = path.resolve(import.meta.dir, '..') +const CONTRACTS_DIR = path.join(ROOT, 'apps/sim/lib/api/contracts') +const APP_API_DIR = path.join(ROOT, 'apps/sim/app/api') +const SKIP_DIRS = new Set(['node_modules', 'dist', '.next', '.turbo', 'coverage', '__tests__']) +const HTTP_METHODS = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'HEAD', 'OPTIONS'] as const + +type HttpMethod = (typeof HTTP_METHODS)[number] + +interface DeclaredContract { + name: string + method: HttpMethod + routePath: string + module: string +} + +async function listContractModules(dir: string, results: string[] = []): Promise { + for (const entry of await readdir(dir, { withFileTypes: true })) { + if (SKIP_DIRS.has(entry.name)) continue + const full = path.join(dir, entry.name) + if (entry.isDirectory()) await listContractModules(full, results) + else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) results.push(full) + } + return results +} + +function isRouteContract(value: unknown): value is { method: HttpMethod; path: string } { + if (typeof value !== 'object' || value === null) return false + const candidate = value as Record + return ( + typeof candidate.method === 'string' && + (HTTP_METHODS as readonly string[]).includes(candidate.method) && + typeof candidate.path === 'string' && + typeof candidate.response === 'object' && + candidate.response !== null + ) +} + +async function readIfFile(candidate: string): Promise { + try { + if (!(await stat(candidate)).isFile()) return null + return await readFile(candidate, 'utf8') + } catch { + return null + } +} + +/** + * Resolves a contract path the way Next.js does: an exact segment match wins, + * and only when none exists does the nearest catch-all ancestor + * (`[...all]`, `[[...segments]]`) take the request. Without the fallback every + * path served by a catch-all — all of `/api/auth/**`, `/api/v2/**` without its + * own file — would look routeless and be silently exempted from the check. + */ +async function readRouteFile(routePath: string): Promise { + if (!routePath.startsWith('/api/')) return null + const segments = routePath.slice('/api/'.length).split('/').filter(Boolean) + + const exact = await readIfFile(path.join(APP_API_DIR, ...segments, 'route.ts')) + if (exact !== null) return exact + + for (let depth = segments.length; depth > 0; depth--) { + const ancestor = path.join(APP_API_DIR, ...segments.slice(0, depth - 1)) + if (!existsSync(ancestor)) continue + for (const entry of await readdir(ancestor, { withFileTypes: true })) { + if (!entry.isDirectory()) continue + if (!entry.name.startsWith('[...') && !entry.name.startsWith('[[...')) continue + const source = await readIfFile(path.join(ancestor, entry.name, 'route.ts')) + if (source !== null) return source + } + } + return null +} + +function exportedMethods(source: string): Set { + const methods = new Set() + const group = HTTP_METHODS.join('|') + for (const m of source.matchAll( + new RegExp(`export\\s+(?:const|async\\s+function|function)\\s+(${group})\\b`, 'g') + )) { + methods.add(m[1]) + } + for (const block of source.matchAll(/export\s*(?:const\s*)?\{([^}]*)\}/g)) { + for (const clause of block[1].split(',')) { + const local = clause + .split(/\s+as\s+|:/) + .pop() + ?.trim() + if (local && (HTTP_METHODS as readonly string[]).includes(local)) methods.add(local) + } + } + return methods +} + +async function collectContracts(): Promise { + const modules = await listContractModules(CONTRACTS_DIR) + // Barrels re-export the same object, so keying by identity keeps one entry per + // contract. Defining modules sort before `index.ts` so the report names them. + modules.sort((a, b) => { + const aBarrel = path.basename(a) === 'index.ts' + const bBarrel = path.basename(b) === 'index.ts' + return aBarrel === bBarrel ? a.localeCompare(b) : aBarrel ? 1 : -1 + }) + + const seen = new Map() + for (const file of modules) { + let loaded: Record + try { + loaded = (await import(file)) as Record + } catch (error) { + console.error(`✗ Could not import ${path.relative(ROOT, file)} to read its contracts:`) + console.error(` ${error instanceof Error ? error.message : String(error)}`) + process.exit(1) + } + for (const [name, value] of Object.entries(loaded)) { + if (!isRouteContract(value)) continue + if (seen.has(value)) continue + seen.set(value, { + name, + method: value.method, + routePath: value.path, + module: path.relative(ROOT, file), + }) + } + } + return [...seen.values()] +} + +async function main() { + const contracts = await collectContracts() + + const violations: Array = [] + const inProcess: DeclaredContract[] = [] + for (const contract of contracts) { + const routeSource = await readRouteFile(contract.routePath) + if (routeSource === null) { + inProcess.push(contract) + continue + } + const served = exportedMethods(routeSource) + if (!served.has(contract.method)) violations.push({ ...contract, served: [...served].sort() }) + } + + if (process.argv.includes('--list-in-process')) { + for (const c of [...inProcess].sort((a, b) => a.routePath.localeCompare(b.routePath))) { + console.log(` ${c.method.padEnd(6)} ${c.routePath} ${c.name} (${c.module})`) + } + } + + if (violations.length > 0) { + console.error( + `✗ ${violations.length} contract(s) declare a method their live route does not serve:\n` + ) + for (const v of violations) { + console.error(` ${v.method} ${v.routePath}`) + console.error(` contract: ${v.name} (${v.module})`) + console.error(` route serves: ${v.served.join(', ') || '(no methods)'}`) + console.error( + ` fix: export ${v.method} from the route, or drop the declaration if the endpoint is retired\n` + ) + } + process.exit(1) + } + + console.log( + `✓ ${contracts.length} route contracts agree with the methods their routes serve ` + + `(${contracts.length - inProcess.length} boundary, ${inProcess.length} in-process; ` + + `--list-in-process to enumerate)` + ) +} + +main().catch((error) => { + console.error(error) + process.exit(1) +}) From f775fe01077231630e3e09dad86f93fff4a28597 Mon Sep 17 00:00:00 2001 From: Waleed Date: Fri, 28 Aug 2026 01:52:58 -0700 Subject: [PATCH 2/6] improvement(ci): bound docker layer caches and right-size ten runners (#7210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Blacksmith sticky disks backing our docker layer caches had no eviction policy. setup-docker-builder skips pruning entirely unless max-cache-size-mb is set, and BuildKit's own GC is time-based only (8 days unused), so on a repo that builds this often nothing ever aged out: app.Dockerfile/linux-amd64 hit 351 GB within a day of being created, and realtime — an image under 300 MB — sat at 249 GB. Layer caches alone were 920 GB across ten disks. Cap them per image via a cache_mb matrix field, alongside the bs_runner field that already encodes per-image sizing. The app image keeps 100 GB (several generations over its working set of ~34 layers plus monorepo apt/bun cache mounts); everything else takes the 25 GB default, which is still 4x the tightest working set in the matrix (pii, whose spaCy models are ~2.2 GB). The fallback lives in the composite action rather than an input default, because an unset matrix key arrives as the empty string and would bypass a default — silently restoring unbounded growth on any row that forgot the field. Runner sizes follow measured CPU and memory percentiles over 30 days: - CodeQL splits per language. javascript-typescript peaks at 19.5 GB so it stays on 8 vCPU; actions peaks at 1.3 GB and averages 22% CPU over a 39s median run, and drops to 4 vCPU. - The pii and realtime image builds drop to 2 vCPU. Both already ran on 8 vCPU earlier in the window, so the 8->4 step is measured, not modelled: realtime went 52s -> 51s and pii 24s -> 27s. - Five desktop release jobs drop to 2 vCPU. They peak under 0.4 GB and finish in 4-13s. None of these sit on their group's critical path — each has 190-275s of slack behind an app build that dominates it — so wall-clock is unchanged. Those five desktop jobs also hardcoded a Blacksmith label with no CI_PROVIDER fallback, against the invariant stated at the top of ci.yml. In GitHub break-glass mode they would have sat in `queued` forever; they now fall back like every other job. Left alone deliberately: the 16 vCPU app builds (memory-bound, and 16 vCPU measured 2.1x faster and 6% cheaper than 8 vCPU), Lint and Test (CPU-bound, 62% of the run above 80%), and the push-path Build App (build-amd64 has no needs:, so it is what stops a migration applying for a build that cannot ship). --- .github/actions/docker-build/action.yml | 20 ++++++++++++++++ .github/workflows/ci.yml | 31 ++++++++++++++++++------- .github/workflows/codeql.yml | 13 +++++++++-- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/.github/actions/docker-build/action.yml b/.github/actions/docker-build/action.yml index a607a720afd..b8727501f52 100644 --- a/.github/actions/docker-build/action.yml +++ b/.github/actions/docker-build/action.yml @@ -19,6 +19,13 @@ inputs: tags: description: Comma-separated list of tags to push. required: true + max-cache-size-mb: + description: >- + Layer cache to retain after the post-job prune, in MB. Must stay above one + build's working set (base + dependency layers + RUN --mount=type=cache + dirs) or every build evicts what the next one needs. Falls back to the + small-image default below when empty. + required: false # Registry logins must precede this action. provenance/sbom stay off: attestation # manifests break `imagetools create` retagging in promote-images. @@ -42,11 +49,24 @@ runs: PLATFORMS: ${{ inputs.platforms }} run: echo "value=${GITHUB_REPOSITORY##*/}/${FILE#./}/${PLATFORMS//\//-}" >> "$GITHUB_OUTPUT" + # max-cache-size-mb is what bounds the disk: BuildKit's default GC is + # time-based only (layers unused for 8 days), and setup-docker-builder skips + # pruning altogether when the value is empty. On a repo that builds this + # often nothing ever ages out, so the disks grew without limit — + # app.Dockerfile/linux-amd64 reached 351 GB inside a day, and realtime, whose + # image is under 300 MB, sat at 249 GB. Sticky disks bill at ~$0.51/GB-month, + # so that was real money for layers no build would ever read again. + # + # The fallback is here rather than an input `default:` because callers pass + # this from a matrix field, and an unset matrix key arrives as the empty + # string — which counts as "provided", so a `default:` would never apply and + # a row that forgot the field would silently go back to unbounded growth. - name: Set up Blacksmith builder if: inputs.provider == '' || inputs.provider == 'blacksmith' uses: useblacksmith/setup-docker-builder@a5256a73e30f09e37e3eceb8ca36043d17621d24 # v2 with: cache-key: ${{ steps.cache-key.outputs.value }} + max-cache-size-mb: ${{ inputs.max-cache-size-mb || '25600' }} - name: Build and push (Blacksmith) if: inputs.provider == '' || inputs.provider == 'blacksmith' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 746c341cae8..aa1be99ea97 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,7 +76,7 @@ jobs: # (/api/desktop/update) starts offering automatically. detect-desktop-changes: name: Detect Desktop Changes - runs-on: blacksmith-4vcpu-ubuntu-2404 + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 5 if: github.event_name == 'push' && (github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/staging') outputs: @@ -165,7 +165,15 @@ jobs: # build` ~260s). The same `next build` runs on 16 vCPU in the separate # Build App verification job, which does not gate anything; this one # was doing comparable work on half the cores. + # + # cache_mb is the layer cache the post-job prune retains, and it is the + # only reason the sticky disks stay bounded — see docker-build's + # action.yml. Rows that omit it take the small-image default there. The + # app image overrides because it carries ~34 layers plus apt and bun + # cache mounts for the whole monorepo; 100 GB is several builds' worth + # of headroom over that working set. - dockerfile: ./docker/app.Dockerfile + cache_mb: '102400' ecr_repo_secret: ECR_APP gh_runner: linux-x64-8-core bs_runner: blacksmith-16vcpu-ubuntu-2404 @@ -176,11 +184,11 @@ jobs: - dockerfile: ./docker/realtime.Dockerfile ecr_repo_secret: ECR_REALTIME gh_runner: ubuntu-latest - bs_runner: blacksmith-4vcpu-ubuntu-2404 + bs_runner: blacksmith-2vcpu-ubuntu-2404 - dockerfile: ./docker/pii.Dockerfile ecr_repo_secret: ECR_PII gh_runner: ubuntu-latest - bs_runner: blacksmith-4vcpu-ubuntu-2404 + bs_runner: blacksmith-2vcpu-ubuntu-2404 steps: - name: Checkout code uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 @@ -214,6 +222,7 @@ jobs: file: ${{ matrix.dockerfile }} platforms: linux/amd64 tags: ${{ steps.login-ecr.outputs.registry }}/${{ steps.ecr-repo.outputs.name }}:dev + max-cache-size-mb: ${{ matrix.cache_mb }} # Dev: deploy Trigger.dev background tasks to the preview "dev-sim" branch. # Gated after migrate-dev for the same reason as build-dev — the new task @@ -280,6 +289,7 @@ jobs: matrix: include: - dockerfile: ./docker/app.Dockerfile + cache_mb: '102400' ghcr_image: ghcr.io/simstudioai/simstudio ecr_repo_secret: ECR_APP gh_runner: linux-x64-8-core @@ -293,12 +303,12 @@ jobs: ghcr_image: ghcr.io/simstudioai/realtime ecr_repo_secret: ECR_REALTIME gh_runner: ubuntu-latest - bs_runner: blacksmith-4vcpu-ubuntu-2404 + bs_runner: blacksmith-2vcpu-ubuntu-2404 - dockerfile: ./docker/pii.Dockerfile ghcr_image: ghcr.io/simstudioai/pii ecr_repo_secret: ECR_PII gh_runner: ubuntu-latest - bs_runner: blacksmith-4vcpu-ubuntu-2404 + bs_runner: blacksmith-2vcpu-ubuntu-2404 # No ECR repo is provisioned for cron, so it publishes to GHCR only. # The tag step below omits the ECR tag when the repo name is empty. - dockerfile: ./docker/cron.Dockerfile @@ -382,6 +392,7 @@ jobs: file: ${{ matrix.dockerfile }} platforms: linux/amd64 tags: ${{ steps.meta.outputs.tags }} + max-cache-size-mb: ${{ matrix.cache_mb }} # Promote the sha-tagged ECR images to the deploy tags once tests and # migrations pass. Pushing the ECR latest/staging tag is what triggers @@ -484,6 +495,7 @@ jobs: # hang a release in `queued` rather than fail a PR. include: - dockerfile: ./docker/app.Dockerfile + cache_mb: '102400' image: ghcr.io/simstudioai/simstudio gh_runner: linux-arm64-8-core bs_runner: blacksmith-8vcpu-ubuntu-2404-arm @@ -522,6 +534,7 @@ jobs: file: ${{ matrix.dockerfile }} platforms: linux/arm64 tags: ${{ matrix.image }}:${{ github.sha }}-arm64 + max-cache-size-mb: ${{ matrix.cache_mb }} # Publish all mutable GHCR tags (latest, latest-amd64/arm64, version tags) # and the multi-arch manifests from the immutable sha tags — only on main, @@ -675,7 +688,7 @@ jobs: # Job-level `if:` cannot read the secrets context, hence the probe job. check-desktop-signing: name: Check Desktop Signing Secrets - runs-on: blacksmith-4vcpu-ubuntu-2404 + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 2 needs: [detect-version, detect-desktop-changes] # !cancelled(): detect-desktop-changes is skipped on main (and @@ -724,7 +737,7 @@ jobs: # remains testable end to end with a manual download. create-desktop-prerelease: name: Create Desktop Prerelease - runs-on: blacksmith-4vcpu-ubuntu-2404 + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 5 needs: [detect-desktop-changes, check-desktop-signing] # Requires the signing probe to have actually succeeded (not just "not @@ -813,7 +826,7 @@ jobs: # point of view. publish-desktop-prerelease: name: Publish Desktop Prerelease - runs-on: blacksmith-4vcpu-ubuntu-2404 + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 5 needs: [create-desktop-prerelease, desktop-prerelease] permissions: @@ -837,7 +850,7 @@ jobs: # are always garbage by this point — the current run's release is published. prune-desktop-prereleases: name: Prune Desktop Prereleases - runs-on: blacksmith-4vcpu-ubuntu-2404 + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 5 needs: [publish-desktop-prerelease] permissions: diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index f44ca430c7c..1e7e99ce165 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -54,7 +54,12 @@ permissions: jobs: analyze: name: Analyze ${{ matrix.language }} - runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-8vcpu-ubuntu-2404' || 'ubuntu-latest' }} + # Sized per language, not per workflow. The two analyses are nothing alike: + # javascript-typescript peaks at 19.5 GB (p95 over 3090 runs), so it needs + # the 8 vCPU tier's 30.4 GB and would OOM on the 4 vCPU tier's 15.2 GB; the + # actions analysis peaks at 1.3 GB and averages 22% CPU over a 39s median + # run, so 8 vCPU was 4x more machine than it ever used. + runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && matrix.bs_runner || 'ubuntu-latest' }} timeout-minutes: 60 if: github.event.pull_request.draft != true permissions: @@ -71,7 +76,11 @@ jobs: # entries default setup listed were one analysis, not three. # `javascript-typescript` is the documented spelling. Python dropped: # 7 files in the tree. - language: [javascript-typescript, actions] + include: + - language: javascript-typescript + bs_runner: blacksmith-8vcpu-ubuntu-2404 + - language: actions + bs_runner: blacksmith-4vcpu-ubuntu-2404 steps: - name: Checkout repository From 9e79dd28ea4e5b7e72d6b45779d36f5a21bde407 Mon Sep 17 00:00:00 2001 From: Waleed Date: Fri, 28 Aug 2026 01:59:40 -0700 Subject: [PATCH 3/6] improvement(background): right-size the two knowledge task machines (#7212) Both knowledge tasks reserved machine presets well above their measured ceilings. Sized each from production telemetry on both memory and CPU: - knowledge-connector-sync: large-2x -> large-1x. Peak sampled RSS 2.6 GB and peak 1.4 vCPU, so 8 GB/4 vCPU keeps ~3x memory and ~2.8x CPU headroom against a preset that reserved 16 GB. - knowledge-process-document: large-1x -> medium-2x. Peak sampled RSS 902 MB and peak 1.2 vCPU, with no document exceeding 2 GB, so 4 GB/2 vCPU keeps ~4x memory and ~1.7x CPU headroom. CPU figures are core-normalized (OTel process.cpu.utilization divides by cores available), so neither task loses headroom it was actually using and neither can be throttled by the smaller preset. No retry or concurrency semantics change. --- apps/sim/background/knowledge-connector-sync.ts | 9 ++++++++- apps/sim/background/knowledge-processing.ts | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/apps/sim/background/knowledge-connector-sync.ts b/apps/sim/background/knowledge-connector-sync.ts index f7ba8e80e4d..ca34b89c62c 100644 --- a/apps/sim/background/knowledge-connector-sync.ts +++ b/apps/sim/background/knowledge-connector-sync.ts @@ -98,7 +98,14 @@ export async function executeConnectorSyncJob(payload: unknown) { export const knowledgeConnectorSync = task({ id: 'knowledge-connector-sync', maxDuration: CONNECTOR_SYNC_MAX_DURATION_SECONDS, - machine: 'large-2x', + /** + * Sized from production telemetry: peak sampled RSS 2.6 GB and peak 1.4 vCPU, + * so `large-1x` holds ~3x memory and ~2.8x CPU headroom. No `outOfMemory` + * escalation: an OOM is a SIGKILL, so the run never reaches the terminal + * write that clears `syncLockToken`, and the escalated attempt would find the + * row still `syncing` and skip. The stale-lock reaper owns that recovery. + */ + machine: 'large-1x', retry: { maxAttempts: 3, factor: 2, diff --git a/apps/sim/background/knowledge-processing.ts b/apps/sim/background/knowledge-processing.ts index 43256bed6d6..8ff7e1aedf2 100644 --- a/apps/sim/background/knowledge-processing.ts +++ b/apps/sim/background/knowledge-processing.ts @@ -135,7 +135,13 @@ export async function runDocumentProcessing( export const processDocument = task({ id: 'knowledge-process-document', maxDuration: envNumber(env.KB_CONFIG_MAX_DURATION, 600), - machine: 'large-1x', // 4 vCPU, 8GB RAM - needed for large PDF processing + /** + * Sized from production telemetry: peak sampled RSS 902 MB and peak 1.2 vCPU + * across a corpus where no document exceeded 2 GB, so `medium-2x` holds ~4x + * memory and ~1.7x CPU headroom over the observed worst case. The prior + * `large-1x` reserved 8 GB against a worst case using an eighth of it. + */ + machine: 'medium-2x', retry: { maxAttempts: envNumber(env.KB_CONFIG_MAX_ATTEMPTS, 3), factor: envNumber(env.KB_CONFIG_RETRY_FACTOR, 2), From dca1fd60bed11ca9d776e5b7b14e6873c9c4269e Mon Sep 17 00:00:00 2001 From: Waleed Date: Fri, 28 Aug 2026 02:06:48 -0700 Subject: [PATCH 4/6] improvement(ci): scan CodeQL PRs at the promotion boundary, refresh main daily (#7213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feature PRs land on staging and are ~90% of PR scan volume (90 of the last 100 PRs target staging, 4 target main). Every one of them is scanned again — against the exact tree being promoted — when the staging->main PR opens, so restricting PR scans to main defers the signal to the promotion boundary rather than dropping it. No ruleset or branch protection requires a CodeQL check, and the alert view is fed by the push-to-main and scheduled analyses, not by PR runs. Deliberately a branch cut rather than an activity-type cut. Dropping `synchronize` would have cut a similar share of runs, but it scans a PR's first commit and never its final state — backwards, since review fixups land in later pushes. The scheduled scan moves from weekly to daily. Pushes to main are rare, so with PR scans limited to main the default-branch alert view leans on the cron more than it used to, and a week is too long to leave it stale. It also reseeds the overlay-base database that PR runs restore from: that cache key embeds the CodeQL bundle version, so a bundle bump invalidates it, and an unused Actions cache is evicted after 7 days. Also records, in codeql-config.yml, why the obvious speed-up is a trap: adding `queries:`/`packs:`/`query-filters:` trips OverlayDisabledReason.NonDefaultQueries and permanently disables overlay analysis, trading a documented up-to-10x win on the extraction phase (~53% of a run) for a few percent off the query phase. --- .github/codeql/codeql-config.yml | 8 ++++++++ .github/workflows/codeql.yml | 20 ++++++++++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml index b8e56f708d9..f50b20410aa 100644 --- a/.github/codeql/codeql-config.yml +++ b/.github/codeql/codeql-config.yml @@ -31,3 +31,11 @@ paths-ignore: - '**/dist/**' - '**/.next/**' - 'apps/docs/content/**' + +# Do NOT add `queries:`, `packs:`, `query-filters:`, or `disable-default-queries` +# here to try to speed the scan up. Under the code-scanning feature flag the +# action's checkOverlayAnalysisFeatureEnabled treats any of those as +# OverlayDisabledReason.NonDefaultQueries and permanently turns off overlay +# (incremental) analysis. Extraction is ~53% of a run and is exactly what overlay +# skips, so scoping the queries trades a documented up-to-10x win for a few +# percent off the 27% query phase. diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 1e7e99ce165..d6e5122f2c4 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -20,8 +20,16 @@ on: # created, prevents developers from introducing new vulnerabilities." push: branches: [main] + # main only, not staging. Feature PRs land on staging and are ~90% of PR scan + # volume, and every one of them is scanned again — against the exact tree being + # promoted — when the staging->main PR opens. Scanning at the promotion + # boundary defers the signal rather than dropping it. + # + # Deliberately a branch cut and not an activity-type cut: dropping + # `synchronize` would have scanned each PR's first commit and never its final + # state, which is backwards, since review fixups land in later pushes. pull_request: - branches: [main, staging] + branches: [main] # `ready_for_review` is not a default activity type, so it has to be listed # alongside the defaults it replaces. Without it, a PR opened as a draft and # then marked ready is skipped by the job-level draft guard and never @@ -41,7 +49,15 @@ on: # Safety net behind the push trigger, and the thing that keeps the # default-branch alert view fresh when main is quiet. Only fires once this # file is on the default branch — schedule events ignore other branches. - - cron: '17 8 * * 1' + # + # Daily rather than weekly. Pushes to main are rare, and with PR scans now + # limited to main the alert view leans on this more than it used to; a week + # is too long to leave it stale. It also reseeds the overlay-base database + # that PR runs restore from — that cache key embeds the CodeQL bundle + # version, so a bundle bump invalidates it, and an unused Actions cache is + # evicted after 7 days. One 8 vCPU default-branch scan a day is a few + # dollars a month against a PR scan that halves when the base is warm. + - cron: '17 8 * * *' workflow_dispatch: concurrency: From db25446b35860179e4875ef96b9b5b84286815bc Mon Sep 17 00:00:00 2001 From: Waleed Date: Fri, 28 Aug 2026 02:08:08 -0700 Subject: [PATCH 5/6] improvement(ci): bound the local Turborepo cache (#7214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turborepo's local cache eviction is opt-in until 3.0, so without cacheMaxAge and cacheMaxSize the filesystem cache grows forever. In CI each cache dir lives on a Blacksmith sticky disk that is mounted many times a day, so it never idles long enough for Blacksmith's own 7-day inactivity purge to fire, and one cache-missing app build writes a ~400 MB artifact. The build cache disk reached 206 GB over 43 days — roughly 4.8 GB/day of sediment — at ~$0.51/GB-month. Size is the real bound; age is hygiene. A cache hit only happens when a task's input hash is unchanged, which recurs within hours, not weeks, so nothing written days ago can ever be read again — the sibling PR-keyed disk does the same job in 30 GB. The cache still earns its keep: the app build hits ~17% of the time and a hit saves ~7 minutes, so this is a ceiling, not a removal. That distinguishes it from the Turbopack persistent cache, which was removed because it measured 3.2x SLOWER; this one is measurably faster, just unbounded. Set in turbo.json rather than per-step env vars so there is one source of truth and turbo validates it. Verified against the installed 2.9.14: both keys are in its schema, turbo parses them, and the task hash is byte-identical with and without them, so enabling eviction does not invalidate the existing cache. --- turbo.json | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/turbo.json b/turbo.json index bdb8730df0f..619997da6ee 100644 --- a/turbo.json +++ b/turbo.json @@ -1,6 +1,23 @@ { "$schema": "https://v2-9-12.turborepo.dev/schema.json", "envMode": "loose", + // Local cache eviction is opt-in until Turborepo 3.0, so without these the + // filesystem cache grows forever. In CI each cache dir is a Blacksmith sticky + // disk that is mounted many times a day, so it never goes idle long enough for + // Blacksmith's own 7-day inactivity purge to fire, and one cache-missing app + // build writes a ~400 MB artifact: the build cache reached 206 GB in 43 days. + // + // Size is the real bound and age is hygiene. A hit only happens when a task's + // input hash is unchanged — a re-run, or a commit touching only unrelated + // workspaces — which recurs within hours, so nothing older than a day or two + // can ever be read again. Measured hit rate on the app build is ~17%, worth + // ~7 minutes each, so the cache earns its keep; it just needs a ceiling. + // 40 GB is ~100 app-sized artifacts against ~4.8 GB/day of real accumulation. + // + // Neither key participates in the task hash, so changing them does not + // invalidate the cache. + "cacheMaxAge": "7d", + "cacheMaxSize": "40GB", "tasks": { "transit": { "dependsOn": ["^transit"], From a96ea5c7a02be7d8ee6aafccbc81e47de9526d88 Mon Sep 17 00:00:00 2001 From: Waleed Date: Fri, 28 Aug 2026 02:40:45 -0700 Subject: [PATCH 6/6] fix(connectors): honor provider retry deadlines (#7211) * fix(connectors): honor provider retry deadlines * fix(connectors): validate retry response lifecycles --- apps/sim/lib/atlassian/discovery.test.ts | 26 +++++- apps/sim/lib/atlassian/discovery.ts | 30 +++++-- .../knowledge/connectors/sync-engine.test.ts | 24 +++++ .../lib/knowledge/connectors/sync-engine.ts | 32 +++++-- .../documents/secure-fetch.server.ts | 17 +--- .../sim/lib/knowledge/documents/utils.test.ts | 43 ++++++++- apps/sim/lib/knowledge/documents/utils.ts | 87 +++++++++++++++---- 7 files changed, 213 insertions(+), 46 deletions(-) diff --git a/apps/sim/lib/atlassian/discovery.test.ts b/apps/sim/lib/atlassian/discovery.test.ts index d3c3f4b34bc..d839c95f662 100644 --- a/apps/sim/lib/atlassian/discovery.test.ts +++ b/apps/sim/lib/atlassian/discovery.test.ts @@ -205,6 +205,30 @@ describe('resolveAtlassianCloudId', () => { it('rejects when the token can see no sites', async () => { fetchMock.mockResolvedValue(sites([])) - await expect(resolveAtlassianCloudId(options())).rejects.toThrow('No Jira resources found') + await expect(resolveAtlassianCloudId(options())).rejects.toThrow( + 'No Jira sites are accessible to this credential. Reconnect the credential and grant access to the configured Atlassian site.' + ) + }) + + it('distinguishes a malformed discovery payload from an empty site grant', async () => { + fetchMock.mockResolvedValue(createMockResponse({ json: { id: CLOUD_ID, url: SITE } })) + + await expect(resolveAtlassianCloudId(options())).rejects.toThrow( + 'Invalid Jira accessible-resources response' + ) + }) + + it.each([ + [{ url: SITE }], + [{ id: CLOUD_ID }], + [{ id: '', url: SITE }], + [{ id: CLOUD_ID, url: '' }], + [null], + ])('rejects malformed resource entries in an otherwise valid array', async (resources) => { + fetchMock.mockResolvedValue(createMockResponse({ json: resources })) + + await expect(resolveAtlassianCloudId(options())).rejects.toThrow( + 'Invalid Jira accessible-resources response' + ) }) }) diff --git a/apps/sim/lib/atlassian/discovery.ts b/apps/sim/lib/atlassian/discovery.ts index 5a27b134dc2..c1a55718807 100644 --- a/apps/sim/lib/atlassian/discovery.ts +++ b/apps/sim/lib/atlassian/discovery.ts @@ -102,6 +102,17 @@ interface AccessibleResource { url: string } +function isAccessibleResource(value: unknown): value is AccessibleResource { + if (typeof value !== 'object' || value === null) return false + const resource = value as Record + return ( + typeof resource.id === 'string' && + resource.id.trim().length > 0 && + typeof resource.url === 'string' && + resource.url.trim().length > 0 + ) +} + interface ResolveAtlassianCloudIdOptions { domain: string accessToken: string @@ -203,21 +214,26 @@ export function selectAtlassianCloudId( domain: string, product: string ): string { - if (!Array.isArray(resources) || resources.length === 0) { - throw new Error(`No ${product} resources found`) + if (!Array.isArray(resources) || !resources.every(isAccessibleResource)) { + throw new Error(`Invalid ${product} accessible-resources response`) + } + + if (resources.length === 0) { + throw new Error( + `No ${product} sites are accessible to this credential. ` + + 'Reconnect the credential and grant access to the configured Atlassian site.' + ) } const siteUrl = normalizeAtlassianSiteUrl(domain) - const match = (resources as AccessibleResource[]).find( - (r) => normalizeAtlassianSiteUrl(r.url) === siteUrl - ) + const match = resources.find((r) => normalizeAtlassianSiteUrl(r.url) === siteUrl) if (match) return match.id - if (resources.length === 1) return (resources as AccessibleResource[])[0].id + if (resources.length === 1) return resources[0].id throw new Error( `Could not match ${product} domain "${domain}" to any accessible resource. ` + - `Available sites: ${(resources as AccessibleResource[]).map((r) => r.url).join(', ')}` + `Available sites: ${resources.map((r) => r.url).join(', ')}` ) } diff --git a/apps/sim/lib/knowledge/connectors/sync-engine.test.ts b/apps/sim/lib/knowledge/connectors/sync-engine.test.ts index dbb58359347..7ce9da29933 100644 --- a/apps/sim/lib/knowledge/connectors/sync-engine.test.ts +++ b/apps/sim/lib/knowledge/connectors/sync-engine.test.ts @@ -2215,6 +2215,30 @@ describe('buildSyncFailureUpdate', () => { expect(buildSyncFailureUpdate(now, undefined, 'boom').nextSyncAt).toEqual(minutesAfter(30)) }) + it('does not schedule before a longer provider retry deadline', async () => { + const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine') + + expect(buildSyncFailureUpdate(now, 0, 'rate limited', 45 * 60 * 1000).nextSyncAt).toEqual( + minutesAfter(45) + ) + }) + + it('does not let a shorter provider delay weaken the failure backoff', async () => { + const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine') + + expect(buildSyncFailureUpdate(now, 0, 'rate limited', 5 * 60 * 1000).nextSyncAt).toEqual( + minutesAfter(30) + ) + }) + + it('caps an unreasonable provider delay at the existing one-day retry ceiling', async () => { + const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine') + + expect( + buildSyncFailureUpdate(now, 0, 'rate limited', 30 * 24 * 60 * 60 * 1000).nextSyncAt + ).toEqual(minutesAfter(24 * 60)) + }) + it('disables exactly at the threshold, not before it', async () => { const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine') const { MAX_CONSECUTIVE_FAILURES } = await import('@/lib/knowledge/connectors/sync-limits') diff --git a/apps/sim/lib/knowledge/connectors/sync-engine.ts b/apps/sim/lib/knowledge/connectors/sync-engine.ts index bdca8b716f0..c57713c31e8 100644 --- a/apps/sim/lib/knowledge/connectors/sync-engine.ts +++ b/apps/sim/lib/knowledge/connectors/sync-engine.ts @@ -35,6 +35,7 @@ import { getInternalApiBaseUrl } from '@/lib/core/utils/urls' import { resolveCredentialTokenIdentity } from '@/lib/credentials/access' import { CONNECTOR_AUTO_DISABLED_ERROR, + CONNECTOR_FAILURE_BACKOFF_CAP_MINUTES, connectorFailureBackoffMinutes, MAX_CONSECUTIVE_FAILURES, SYNC_LOCK_HEARTBEAT_INTERVAL_MS, @@ -52,6 +53,7 @@ import { MAX_PROCESSING_ATTEMPTS, QUEUED_DISPATCH_GRACE_MS, } from '@/lib/knowledge/documents/types' +import { getRetryAfterMs } from '@/lib/knowledge/documents/utils' import { refreshAccessTokenIfNeeded } from '@/lib/oauth/credential-service' import { StorageService } from '@/lib/uploads' import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key' @@ -1321,22 +1323,32 @@ export function buildReconciliationHoldNotice( * it applies need to be assertable without standing up the whole sync. The * in-process ladder here and the reaper's SQL ladder must agree — they are two * writers of one policy, both sourced from - * {@link connectorFailureBackoffMinutes}. + * {@link connectorFailureBackoffMinutes}. A validated provider retry delay is + * an additional lower bound, capped at the same one-day ceiling: a short hint + * cannot weaken the failure ladder, while an untrusted extreme value cannot + * pin the connector indefinitely. */ export function buildSyncFailureUpdate( now: Date, previousFailures: number | null | undefined, - errorMessage: string + errorMessage: string, + retryAfterMs?: number ) { const failures = (previousFailures ?? 0) + 1 const disabled = failures >= MAX_CONSECUTIVE_FAILURES + const failureBackoffMs = connectorFailureBackoffMinutes(failures) * 60 * 1000 + const maximumBackoffMs = CONNECTOR_FAILURE_BACKOFF_CAP_MINUTES * 60 * 1000 + const providerBackoffMs = + typeof retryAfterMs === 'number' && Number.isFinite(retryAfterMs) && retryAfterMs > 0 + ? Math.min(retryAfterMs, maximumBackoffMs) + : 0 return { status: (disabled ? 'disabled' : 'error') as 'disabled' | 'error', lastSyncError: disabled ? CONNECTOR_AUTO_DISABLED_ERROR : errorMessage, nextSyncAt: disabled ? null - : new Date(now.getTime() + connectorFailureBackoffMinutes(failures) * 60 * 1000), + : new Date(now.getTime() + Math.max(failureBackoffMs, providerBackoffMs)), consecutiveFailures: failures, // Releases the lock so a stale token can never match a later run, and closes // its lease so the reaper is not left waiting out a TTL on a finished run. @@ -3160,7 +3172,12 @@ export async function executeSync( } const errorMessage = toError(error).message - logger.error('Sync failed', { connectorId, error: errorMessage }) + const retryAfterMs = getRetryAfterMs(error) + logger.error('Sync failed', { + connectorId, + error: errorMessage, + ...(retryAfterMs === undefined ? {} : { retryAfterMs }), + }) try { await completeSyncLog(syncLogId, 'failed', result, { errorMessage }) @@ -3168,7 +3185,12 @@ export async function executeSync( const failureUpdate = error instanceof ConnectorSyncCapacityError ? buildSyncCapacityUpdate(new Date(), connector.consecutiveFailures, errorMessage) - : buildSyncFailureUpdate(new Date(), connector.consecutiveFailures, errorMessage) + : buildSyncFailureUpdate( + new Date(), + connector.consecutiveFailures, + errorMessage, + retryAfterMs + ) if (failureUpdate.status === 'disabled') { logger.warn('Connector disabled after repeated failures', { diff --git a/apps/sim/lib/knowledge/documents/secure-fetch.server.ts b/apps/sim/lib/knowledge/documents/secure-fetch.server.ts index 8c40b863542..818174760af 100644 --- a/apps/sim/lib/knowledge/documents/secure-fetch.server.ts +++ b/apps/sim/lib/knowledge/documents/secure-fetch.server.ts @@ -4,12 +4,9 @@ import { secureFetchWithValidation, } from '@/lib/core/security/input-validation.server' import { - attachRetryHeaders, - type HTTPError, + createRetryableHttpError, isRetryableError, type RetryOptions, - readBoundedHttpErrorBody, - resolveRetryDelayMs, retryWithExponentialBackoff, } from '@/lib/knowledge/documents/utils' @@ -56,17 +53,7 @@ export async function secureFetchWithRetry( * limit) use instead. */ if (!response.ok && isRetryableError({ status: response.status, headers: response.headers })) { - const errorText = await readBoundedHttpErrorBody(response) - const error: HTTPError = new Error(`HTTP ${response.status} - ${errorText}`) - error.status = response.status - attachRetryHeaders(error, response.headers) - - const waitMs = resolveRetryDelayMs(response.headers) - if (waitMs !== undefined) { - error.retryAfterMs = waitMs - } - - throw error + throw await createRetryableHttpError(response) } return response diff --git a/apps/sim/lib/knowledge/documents/utils.test.ts b/apps/sim/lib/knowledge/documents/utils.test.ts index 260743cf5d9..4f8c307a4e1 100644 --- a/apps/sim/lib/knowledge/documents/utils.test.ts +++ b/apps/sim/lib/knowledge/documents/utils.test.ts @@ -14,6 +14,7 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({ import { secureFetchWithRetry } from './secure-fetch.server' import { fetchWithRetry, + getRetryAfterMs, type HTTPError, hasRateLimitEvidence, isRetryableError, @@ -535,13 +536,37 @@ describe('fetchWithRetry rate-limit handling', () => { .mockResolvedValueOnce(response(200)) globalThis.fetch = fetchMock - await expect(fetchWithRetry('https://api.github.com/repos', {}, FAST_RETRY)).rejects.toThrow( - 'HTTP 403' + const error = await fetchWithRetry('https://api.github.com/repos', {}, FAST_RETRY).then( + () => undefined, + (caught) => caught as Error ) + expect(error?.message).toBe('HTTP 403 - upstream rate limit exceeded') + expect(getRetryAfterMs(error)).toBeGreaterThan(899_000) + expect(getRetryAfterMs(error)).toBeLessThanOrEqual(900_000) expect(fetchMock).toHaveBeenCalledTimes(1) }) + it('cancels an omitted rate-limit response body before throwing', async () => { + let cancelled = false + const body = new ReadableStream({ + cancel() { + cancelled = true + }, + }) + globalThis.fetch = vi.fn().mockResolvedValue( + new Response(body, { + status: 429, + headers: { 'retry-after': '900' }, + }) + ) + + await expect( + fetchWithRetry('https://api.github.com/repos', {}, { ...FAST_RETRY, maxRetries: 0 }) + ).rejects.toThrow('HTTP 429 - upstream rate limit exceeded') + expect(cancelled).toBe(true) + }) + it('waits until an admitted x-rate-limit-reset instant before retrying', async () => { vi.useFakeTimers() const now = 1_700_000_000_000 @@ -601,6 +626,20 @@ describe('fetchWithRetry rate-limit handling', () => { }) }) +describe('getRetryAfterMs', () => { + it('finds a validated retry delay through an error cause chain', () => { + const providerError = Object.assign(new Error('rate limited'), { retryAfterMs: 45_000 }) + expect(getRetryAfterMs(new Error('connector failed', { cause: providerError }))).toBe(45_000) + }) + + it.each([undefined, null, 0, -1, Number.NaN, Number.POSITIVE_INFINITY, '30000'])( + 'ignores an invalid retry delay: %s', + (retryAfterMs) => { + expect(getRetryAfterMs(Object.assign(new Error('invalid'), { retryAfterMs }))).toBeUndefined() + } + ) +}) + describe('retryWithExponentialBackoff retry budget', () => { afterEach(() => { vi.useRealTimers() diff --git a/apps/sim/lib/knowledge/documents/utils.ts b/apps/sim/lib/knowledge/documents/utils.ts index 5a54d0d7204..dc61d725553 100644 --- a/apps/sim/lib/knowledge/documents/utils.ts +++ b/apps/sim/lib/knowledge/documents/utils.ts @@ -183,6 +183,30 @@ export function attachRetryHeaders(error: HTTPError, headers: HeaderReader): voi }) } +/** + * Reads a validated provider retry delay from an error or one of its causes. + * + * The HTTP retry layer attaches this value when a provider supplies + * `Retry-After` or an exhausted-quota reset header. Keeping the accessor here + * lets longer-lived schedulers honor the same evidence without depending on a + * concrete error class or parsing a diagnostic message. + */ +export function getRetryAfterMs(error: unknown): number | undefined { + const seen = new Set() + let current = error + + while (current instanceof Error && !seen.has(current) && seen.size < 10) { + seen.add(current) + const retryAfterMs = (current as HTTPError).retryAfterMs + if (typeof retryAfterMs === 'number' && Number.isFinite(retryAfterMs) && retryAfterMs > 0) { + return retryAfterMs + } + current = current.cause + } + + return undefined +} + /** * True when response headers positively identify a rate-limit rejection rather * than an authorization denial. @@ -254,6 +278,52 @@ export function resolveRetryDelayMs( return undefined } +interface RetryableHttpResponse { + status: number + headers: { get(name: string): string | null } + body?: ReadableStream | null + arrayBuffer?: () => Promise + text?: () => Promise +} + +/** Releases a response stream when its provider-controlled body is intentionally omitted. */ +async function cancelHttpResponseBody(response: RetryableHttpResponse): Promise { + if (!response.body) return + try { + await response.body.cancel() + } catch { + return + } +} + +/** + * Builds the bounded error shared by direct and SSRF-safe connector fetches. + * Rate-limit responses are named from trusted status/header evidence while all + * provider-controlled bodies remain omitted. + */ +export async function createRetryableHttpError( + response: RetryableHttpResponse +): Promise { + const rateLimited = + response.status === 429 || (response.status === 403 && hasRateLimitEvidence(response.headers)) + if (rateLimited) { + await cancelHttpResponseBody(response) + } + const diagnostic = rateLimited + ? 'upstream rate limit exceeded' + : await readBoundedHttpErrorBody(response) + const error: HTTPError = new Error(`HTTP ${response.status} - ${diagnostic}`) + error.status = response.status + attachRetryHeaders(error, response.headers) + + const waitMs = resolveRetryDelayMs(response.headers) + if (waitMs !== undefined) { + error.retryAfterMs = waitMs + } + + return error +} + /** * Default retry condition for rate limiting errors */ @@ -471,22 +541,7 @@ export async function fetchWithRetry( const response = await fetch(url, options) if (!response.ok && isRetryableError({ status: response.status, headers: response.headers })) { - const errorText = await readBoundedHttpErrorBody(response) - const error: HTTPError = new Error(`HTTP ${response.status} - ${errorText}`) - error.status = response.status - // The retry loop re-runs the retry condition against this error, so the - // headers must travel with it or a rate-limit 403 would throw immediately. - attachRetryHeaders(error, response.headers) - - // Pass the server-stated wait to the retry loop so it replaces exponential - // backoff. Falls back to the epoch-seconds reset header when the provider - // sends no Retry-After (X never does). - const waitMs = resolveRetryDelayMs(response.headers) - if (waitMs !== undefined) { - error.retryAfterMs = waitMs - } - - throw error + throw await createRetryableHttpError(response) } return response