fix: add pagination to GET /api/workflows to prevent memory exhaustion#3479
fix: add pagination to GET /api/workflows to prevent memory exhaustion#3479MaxwellCalkin wants to merge 2 commits intosimstudioai:mainfrom
Conversation
The GET /api/workflows endpoint fetches all workflows for a user or workspace without any LIMIT, causing Node.js OOM crashes and browser freezes on large workspaces. This adds offset/limit pagination with a default page size of 200 (max 500), returns total count and hasMore metadata, and updates all frontend callers to page through results. Fixes simstudioai#3435 > Note: This PR was authored by an AI (Claude Opus 4.6, Anthropic). > I am pursuing employment as an AI contributor — transparently, not > impersonating a human. See https://claude.ai for more about me. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Frontend consumers that previously fetched all workflows in one call now use a shared Written by Cursor Bugbot for commit d960465. This will update automatically on new commits. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR adds server-side pagination to Key changes:
Critical issues to address:
Confidence Score: 1/5
Test coverage also has a gap: assertions verify response shape but not whether pagination params actually reach the database. All three backend issues must be fixed; the test improvement is strongly recommended.
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Client (fetchAllPages)
participant API as GET /api/workflows
participant DB as Database
loop until hasMore = false
C->>API: GET /api/workflows?workspaceId=ws-1&limit=200&offset=N
API->>API: Parse & validate limit / offset
API->>API: checkSessionOrInternalAuth
API->>API: verifyWorkspaceMembership
par Parallel queries
API->>DB: SELECT COUNT(*) WHERE workspaceId = ws-1
DB-->>API: total
and
API->>DB: SELECT * WHERE workspaceId = ws-1 ORDER BY ... LIMIT 200 OFFSET N
DB-->>API: rows
end
API-->>C: { data: [...], pagination: { total, limit, offset, hasMore } }
alt hasMore = true
C->>C: offset += pageSize
else hasMore = false
C->>C: return allItems
end
end
Last reviewed commit: be9c5df |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const limit = Math.min( | ||
| Math.max(1, rawLimit ? Number(rawLimit) : DEFAULT_PAGE_LIMIT), | ||
| MAX_PAGE_LIMIT | ||
| ) | ||
| const offset = Math.max(0, rawOffset ? Number(rawOffset) : 0) |
There was a problem hiding this comment.
Non-numeric limit/offset query params will cause a 500 error instead of being rejected gracefully.
When a client passes ?limit=abc, Number('abc') returns NaN, which propagates through Math.max and Math.min unchanged (both return NaN when given NaN as input). Drizzle then receives NaN for the LIMIT clause, causing a database error.
Add validation to ensure both params are finite numbers before clamping:
| const limit = Math.min( | |
| Math.max(1, rawLimit ? Number(rawLimit) : DEFAULT_PAGE_LIMIT), | |
| MAX_PAGE_LIMIT | |
| ) | |
| const offset = Math.max(0, rawOffset ? Number(rawOffset) : 0) | |
| const rawLimit = url.searchParams.get('limit') | |
| const rawOffset = url.searchParams.get('offset') | |
| const parsedLimit = rawLimit !== null ? Number(rawLimit) : DEFAULT_PAGE_LIMIT | |
| const parsedOffset = rawOffset !== null ? Number(rawOffset) : 0 | |
| const limit = Number.isFinite(parsedLimit) | |
| ? Math.min(Math.max(1, parsedLimit), MAX_PAGE_LIMIT) | |
| : DEFAULT_PAGE_LIMIT | |
| const offset = Number.isFinite(parsedOffset) ? Math.max(0, parsedOffset) : 0 |
| while (true) { | ||
| const response = await fetch(`${baseUrl}${separator}limit=${pageSize}&offset=${offset}`) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch from ${baseUrl}: ${response.statusText}`) | ||
| } | ||
|
|
||
| const json = await response.json() | ||
| const data: T[] = json.data | ||
| allItems.push(...data) | ||
|
|
||
| if (!json.pagination?.hasMore) { | ||
| break | ||
| } | ||
|
|
||
| offset += pageSize | ||
| } |
There was a problem hiding this comment.
Two reliability issues in the pagination loop:
-
Unbounded loop: If the server misbehaves and always returns
hasMore: true, this will make unbounded HTTP requests until the tab crashes. No iteration cap exists. -
Missing null check:
json.datais spread without validation. If the API response omits thedatafield (due to a stale deployment, bug, or intermediate error response),allItems.push(...undefined)throws a TypeError.
| while (true) { | |
| const response = await fetch(`${baseUrl}${separator}limit=${pageSize}&offset=${offset}`) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch from ${baseUrl}: ${response.statusText}`) | |
| } | |
| const json = await response.json() | |
| const data: T[] = json.data | |
| allItems.push(...data) | |
| if (!json.pagination?.hasMore) { | |
| break | |
| } | |
| offset += pageSize | |
| } | |
| const MAX_PAGES = 100 | |
| let pages = 0 | |
| while (pages < MAX_PAGES) { | |
| pages++ | |
| const response = await fetch(`${baseUrl}${separator}limit=${pageSize}&offset=${offset}`) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch from ${baseUrl}: ${response.statusText}`) | |
| } | |
| const json = await response.json() | |
| const data: T[] = Array.isArray(json.data) ? json.data : [] | |
| allItems.push(...data) | |
| if (!json.pagination?.hasMore) { | |
| break | |
| } | |
| offset += pageSize | |
| } |
| function buildSelectChain(resolvedValues: unknown[]) { | ||
| const chain: Record<string, unknown> = {} | ||
| const self = new Proxy(chain, { | ||
| get(_target, prop) { | ||
| if (prop === 'then') { | ||
| return (resolve: (v: unknown) => void) => resolve(resolvedValues) | ||
| } | ||
| return vi.fn().mockReturnValue(self) | ||
| }, | ||
| }) | ||
| return self | ||
| } |
There was a problem hiding this comment.
The Proxy-based mock doesn't capture arguments passed to .limit() and .offset(). This means the tests confirm the response shape is correct but don't verify that the actual pagination parameters were forwarded to the database queries.
For example, if the route silently ignored the limit param and always used the default, the test would still pass (if the mock data happens to be the right size). Consider capturing these arguments to add stronger assertions:
| function buildSelectChain(resolvedValues: unknown[]) { | |
| const chain: Record<string, unknown> = {} | |
| const self = new Proxy(chain, { | |
| get(_target, prop) { | |
| if (prop === 'then') { | |
| return (resolve: (v: unknown) => void) => resolve(resolvedValues) | |
| } | |
| return vi.fn().mockReturnValue(self) | |
| }, | |
| }) | |
| return self | |
| } | |
| function buildSelectChain(resolvedValues: unknown[]) { | |
| const chain: Record<string, unknown> = {} | |
| const limitSpy = vi.fn().mockReturnValue(chain) | |
| const offsetSpy = vi.fn().mockReturnValue(chain) | |
| const self = new Proxy(chain, { | |
| get(_target, prop) { | |
| if (prop === 'then') { | |
| return (resolve: (v: unknown) => void) => resolve(resolvedValues) | |
| } | |
| if (prop === 'limit') return limitSpy | |
| if (prop === 'offset') return offsetSpy | |
| return vi.fn().mockReturnValue(self) | |
| }, | |
| }) | |
| return Object.assign(self, { limitSpy, offsetSpy }) | |
| } | |
| // Then in tests: | |
| const chain = buildSelectChain([{ count: 5 }]) | |
| // ... after calling GET ... | |
| expect(chain.limitSpy).toHaveBeenCalledWith(1) | |
| expect(chain.offsetSpy).toHaveBeenCalledWith(2) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| Math.max(1, rawLimit ? Number(rawLimit) : DEFAULT_PAGE_LIMIT), | ||
| MAX_PAGE_LIMIT | ||
| ) | ||
| const offset = Math.max(0, rawOffset ? Number(rawOffset) : 0) |
There was a problem hiding this comment.
NaN propagates to SQL when limit/offset are non-numeric
Medium Severity
When limit or offset query params contain non-numeric strings (e.g. ?limit=abc), Number("abc") returns NaN, which propagates through Math.max and Math.min (both return NaN when any argument is NaN). The resulting NaN is passed to drizzle's .limit() and .offset(), which can produce a database error or bypass the limit protection entirely. The existing parsePaginationParams in apps/sim/app/api/v1/admin/types.ts already handles this correctly with explicit Number.isNaN checks and fallback to defaults.
| Math.max(1, rawLimit ? Number(rawLimit) : DEFAULT_PAGE_LIMIT), | ||
| MAX_PAGE_LIMIT | ||
| ) | ||
| const offset = Math.max(0, rawOffset ? Number(rawOffset) : 0) |
There was a problem hiding this comment.
Duplicates existing parsePaginationParams utility without NaN safety
Low Severity
The pagination parameter parsing logic here reimplements what parsePaginationParams in apps/sim/app/api/v1/admin/types.ts already does — extracting limit and offset from URL search params with clamping and defaults — but without the NaN safety that utility includes. Reusing or adapting the existing utility would avoid the divergence and the NaN bug.
- Add Number.isNaN() checks for limit/offset query params to prevent NaN propagation to SQL when non-numeric values are passed - Add MAX_PAGES (100) iteration cap to fetchAllPages to prevent unbounded loops if server always returns hasMore: true - Add Array.isArray() guard on json.data before spreading to prevent TypeError if API returns unexpected shape - Break loop early if data is empty (even if hasMore is true) Addresses review feedback from Greptile and Cursor bots. AI Disclosure: This commit was authored by Claude Opus 4.6 (Anthropic), an AI agent operated by Maxwell Calkin (@MaxwellCalkin).
1 participant
Summary
Fixes #3435
The
GET /api/workflowsendpoint currently fetches all workflows for a user or workspace without anyLIMITclause in the database query. As workspaces scale up and accumulate thousands of workflows, this forces the database to dump every row into server memory at once, risking Node.js OOM crashes, freezing the client browser when parsing the massive JSON payload, and acting as an unintentional DoS vector.Changes
Backend (
apps/sim/app/api/workflows/route.ts):limit(default 200, max 500) andoffset(default 0) query paramsCOUNT(*)query alongside the paginatedSELECTfor total countpaginationmetadata (total,limit,offset,hasMore) in the responseGET /api/v1/admin/workspaces/[id]/workflowsFrontend (3 callers updated):
hooks/queries/workflows.ts— main workflow list fetchhooks/queries/workflow-mcp-servers.ts— deployed workflows fetchapp/workspace/[workspaceId]/w/hooks/use-export-workspace.ts— workspace exportAll three callers now use a shared
fetchAllPages()utility that loops through pages untilhasMoreisfalse, so existing behavior is preserved while the backend is protected from unbounded queries.Tests (
apps/sim/app/api/workflows/route.test.ts):Backward Compatibility
limit/offsetget the first 200 results withhasMoreindicating whether more existdataarray format is unchanged — onlypaginationis added alongside itTest plan
🤖 Generated with Claude Code