From 94c73123ee3f5f995340e0d1a1598e88f67ca7c0 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 28 May 2026 21:37:53 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=8B=20fix:=20Cap=20Default=20Limit=20o?= =?UTF-8?q?n=20Agent=20List=20Queries=20(#13382)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ›ก๏ธ fix: Cap Default Limit on Agent List Queries (#13363) `GET /api/agents` accepted unbounded requests: when the client omitted `limit`, the value flowed straight into `getListAgentsByAccess`, which set `isPaginated = false` and issued an uncapped MongoDB query. Combined with the unindexed `findPubliclyAccessibleResources` AclEntry scan run on every request, this produced 10-19s response times and stalled the connection pool on instances with 100+ agents. - Default `limit` to 100 in the route handler so client requests without `?limit=` paginate by default. - Default `limit` to 100 in `getListAgentsByAccess` itself as defense-in-depth. The function already caps numeric limits at 100, so there is no client-facing change. - Pass `limit: null` explicitly in the actions route, which legitimately needs the full editable-agent set, to preserve its existing behavior. - Add regression tests covering the default cap and the explicit unbounded opt-out. * ๐Ÿ›ก๏ธ fix: Avoid agent-list regression for users with 100+ agents Codex review pointed out that capping `getListAgentsByAccess` at 100 silently truncated agents past the first page for the four consumers (`useAgentsMap`, `AgentSelect`, `ModelSelectorContext`, `useMentions`) that read `res.data` without following `has_more`/`after`. - Raise the function's hard cap from 100 to 1000 to match `MAX_AVATAR_REFRESH_AGENTS`, the realistic upper bound the avatar-refresh path already assumes. (Side effect: the avatar refresh call site was silently being capped at 100 by the old normalize step.) - In `useListAgentsQuery`, merge `limit: 1000` into params so the four consumers above get the user's full accessible set in a single round-trip instead of needing cursor pagination. - Route handler default stays at 100 as defense-in-depth for any other caller that omits `limit`. - Add a regression test asserting an explicit `limit` above 100 now returns the full set instead of being clipped. * ๐Ÿชข fix: Keep agent-list cache key stable for mutations Codex P2 review noted that folding `limit: 1000` into the cache key broke `allAgentViewAndEditQueryKeys` in `Agents/mutations.ts`, which references `[QueryKeys.agents, { requiredPermission }]` directly across eight mutation handlers. After my prior change the cached entry lived under `[QueryKeys.agents, { limit: 1000, requiredPermission }]`, so create/update/delete/avatar/action mutations stopped updating the list the four consumer hooks render โ€” and with `refetchOnMount` and focus/ reconnect refetches disabled, the UI would stay stale until something else triggered a fetch. Split the merged limit out of the cache key: the request to `dataService.listAgents` still uses `requestParams` (with the default limit applied), but the React Query cache key uses the caller's `params` as-is. The mutation cache updates land again, and the request still returns the user's full accessible set in one round-trip. * ๐Ÿ›ก๏ธ fix: Index AclEntry and paginate agent list internally (#13363) Completes the perf fix for #13363 properly โ€” resolves both the unbounded ACL scans Copilot flagged and Codex's tension between "show all agents" and "don't bypass the server cap". Backend: - Add a compound index on `{ principalType, resourceType, permBits, resourceId }` to the AclEntry schema. This is the index missing for `findPublicResourceIds` and the public branch of the `$or` in `findAccessibleResources`, both of which previously fell back to a collection scan on every `GET /api/agents`. Adds an `explain`-based regression test asserting the public query no longer COLLSCANs. Client: - Rewrite `useListAgentsQuery` to follow the server's cursor pagination internally and concatenate every page into a single flat `AgentListResponse`. Consumers (`useAgentsMap`, `AgentSelect`, `ModelSelectorContext`, `useMentions`) get the user's complete accessible-agent set without any of them needing to learn about cursors, and each individual request uses the server's default page size (so the route's 100-default defense-in-depth fires for real). Cache key shape is unchanged, so the eight mutation handlers in `Agents/mutations.ts` keep matching `allAgentViewAndEditQueryKeys` and update the cached list as before. - Drop the `FULL_AGENT_LIST_LIMIT = 1000` injection added in the previous commit โ€” no longer needed once pagination handles the full set, and removing it stops bypassing the route default. * ๐Ÿงน fix: CI fallout from C-done-properly refactor - Collapse multi-line `fetchAllAgentPages` signature in queries.ts so prettier stops complaining. - In the new public-principal index test, grant one ACL entry before calling `.explain()` so the collection exists (otherwise mongo returns `nonExistentNamespace` and there is no winning plan to inspect). - Cast the `.explain('queryPlanner')` result to a typed shape โ€” the mongoose return type doesn't expose `queryPlanner` directly and was failing the TypeScript check. * ๐Ÿงช fix: Test the AclEntry public-principal index via hint, not planner choice The previous test asserted the query planner did not pick COLLSCAN for the public-principal lookup. That assertion fails on small collections (under the planner's collection-size heuristic) โ€” the index exists and is usable, but with a single document in the test the planner correctly chooses COLLSCAN as the cheaper plan. Reshape the assertion: 1. Confirm the new compound index is actually declared by inspecting `collection.indexes()` after `syncIndexes()`. 2. Force the planner to that index via `.hint()` and assert the winning plan is `IXSCAN` โ€” proves the index is real and serves this query shape, without depending on collection-size heuristics. * ๐Ÿงน chore: Slim down verbose comments The JSDoc and inline comments added across the perf fix had drifted into multi-paragraph rationale better suited to the PR description than the source. Collapse to single-line JSDoc that just describes what each piece does; drop the inline comment in `actions.js` entirely โ€” the call is self-evident. --- api/server/controllers/agents/v1.js | 2 +- api/server/routes/agents/actions.js | 1 + client/src/data-provider/Agents/queries.ts | 35 ++++++-- .../data-schemas/src/methods/aclEntry.spec.ts | 36 +++++++++ .../data-schemas/src/methods/agent.spec.ts | 79 +++++++++++++++++++ packages/data-schemas/src/methods/agent.ts | 7 +- packages/data-schemas/src/schema/aclEntry.ts | 2 + 7 files changed, 152 insertions(+), 10 deletions(-) diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 997ced213f..95bf5c6246 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -878,7 +878,7 @@ const deleteAgentHandler = async (req, res) => { const getListAgentsHandler = async (req, res) => { try { const userId = req.user.id; - const { category, search, limit, cursor, promoted } = req.query; + const { category, search, limit = 100, cursor, promoted } = req.query; let requiredPermission = req.query.requiredPermission; if (typeof requiredPermission === 'string') { requiredPermission = parseInt(requiredPermission, 10); diff --git a/api/server/routes/agents/actions.js b/api/server/routes/agents/actions.js index a775f67f3f..6133ee24e1 100644 --- a/api/server/routes/agents/actions.js +++ b/api/server/routes/agents/actions.js @@ -51,6 +51,7 @@ router.get('/', async (req, res) => { const agentsResponse = await db.getListAgentsByAccess({ accessibleIds: editableAgentObjectIds, + limit: null, }); const editableAgentIds = agentsResponse.data.map((agent) => agent.id); diff --git a/client/src/data-provider/Agents/queries.ts b/client/src/data-provider/Agents/queries.ts index 7387a3e1ff..efc27cd995 100644 --- a/client/src/data-provider/Agents/queries.ts +++ b/client/src/data-provider/Agents/queries.ts @@ -15,6 +15,31 @@ export const defaultAgentParams: t.AgentListParams = { limit: 10, requiredPermission: PermissionBits.EDIT, }; + +/** Walk the cursor pagination and return all pages flattened into one `AgentListResponse`. */ +async function fetchAllAgentPages(params: t.AgentListParams): Promise { + const pages: t.AgentListResponse[] = []; + let cursor: string | null | undefined = params.cursor; + do { + const page = await dataService.listAgents({ + ...params, + ...(cursor ? { cursor } : {}), + }); + pages.push(page); + cursor = page.after; + } while (cursor); + + const lastPage = pages[pages.length - 1]; + return { + object: 'list', + data: pages.flatMap((p) => p.data), + has_more: false, + after: null, + first_id: pages[0]?.first_id ?? null, + last_id: lastPage?.last_id ?? null, + }; +} + /** * Hook for getting all available tools for A */ @@ -32,7 +57,9 @@ export const useAvailableAgentToolsQuery = (): QueryObserverResult }; /** - * Hook for listing all Agents, with optional parameters provided for pagination and sorting + * Hook for listing all Agents the user has access to. Follows cursor + * pagination internally and resolves with every page concatenated. + * Cache key shape matches `allAgentViewAndEditQueryKeys` in `./mutations.ts`. */ export const useListAgentsQuery = ( params: t.AgentListParams = defaultAgentParams, @@ -44,12 +71,8 @@ export const useListAgentsQuery = ( const enabled = !!endpointsConfig?.[EModelEndpoint.agents]; return useQuery( [QueryKeys.agents, params], - () => dataService.listAgents(params), + () => fetchAllAgentPages(params), { - // Example selector to sort them by created_at - // select: (res) => { - // return res.data.sort((a, b) => a.created_at - b.created_at); - // }, staleTime: 1000 * 5, refetchOnWindowFocus: false, refetchOnReconnect: false, diff --git a/packages/data-schemas/src/methods/aclEntry.spec.ts b/packages/data-schemas/src/methods/aclEntry.spec.ts index 575f43f9db..62bf13ad9c 100644 --- a/packages/data-schemas/src/methods/aclEntry.spec.ts +++ b/packages/data-schemas/src/methods/aclEntry.spec.ts @@ -1194,6 +1194,42 @@ describe('AclEntry Model Tests', () => { expect(agentIds).toHaveLength(1); expect(agentIds[0].toString()).toBe(agentRes.toString()); }); + + test('should have a public-principal compound index available for the planner', async () => { + await AclEntry.syncIndexes(); + const indexes = await AclEntry.collection.indexes(); + const publicIndex = indexes.find( + (idx) => + idx.key?.principalType === 1 && + idx.key?.resourceType === 1 && + idx.key?.permBits === 1 && + idx.key?.resourceId === 1, + ); + expect(publicIndex).toBeDefined(); + + await methods.grantPermission( + PrincipalType.PUBLIC, + null, + ResourceType.AGENT, + new mongoose.Types.ObjectId(), + PermissionBits.VIEW, + grantedById, + ); + + const explain = (await AclEntry.find({ + principalType: PrincipalType.PUBLIC, + resourceType: ResourceType.AGENT, + permBits: { $in: [PermissionBits.VIEW] }, + }) + .hint(publicIndex!.name as string) + .explain('queryPlanner')) as unknown as { + queryPlanner?: { winningPlan?: Record }; + }; + + const planString = JSON.stringify(explain?.queryPlanner?.winningPlan ?? {}); + expect(planString).toContain('IXSCAN'); + expect(planString).not.toContain('COLLSCAN'); + }); }); describe('aggregateAclEntries', () => { diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 4efb76d3ac..2ae40cf89b 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -3513,6 +3513,85 @@ describe('Support Contact Field', () => { expect(result.data[0].id).toBe(agentB1.id); expect(result.data[0].author).toBe(userB.toString()); }); + + test('should cap results at the default page size when limit is omitted', async () => { + const extraAgents = []; + for (let i = 0; i < 102; i++) { + const agent = await createAgent({ + id: `agent_${uuidv4().slice(0, 12)}`, + name: `Bulk Agent ${i}`, + description: `Bulk agent ${i}`, + provider: 'openai', + model: 'gpt-4', + author: userA, + }); + extraAgents.push(agent); + } + + const accessibleIds = extraAgents.map((a) => a._id) as mongoose.Types.ObjectId[]; + + const result = await getListAgentsByAccess({ + accessibleIds, + otherParams: {}, + }); + + expect(result.data).toHaveLength(100); + expect(result.has_more).toBe(true); + expect(result.after).toBeTruthy(); + }); + + test('should return all agents when limit is explicitly null', async () => { + const extraAgents = []; + for (let i = 0; i < 102; i++) { + const agent = await createAgent({ + id: `agent_${uuidv4().slice(0, 12)}`, + name: `Bulk Agent ${i}`, + description: `Bulk agent ${i}`, + provider: 'openai', + model: 'gpt-4', + author: userA, + }); + extraAgents.push(agent); + } + + const accessibleIds = extraAgents.map((a) => a._id) as mongoose.Types.ObjectId[]; + + const result = await getListAgentsByAccess({ + accessibleIds, + otherParams: {}, + limit: null, + }); + + expect(result.data).toHaveLength(102); + expect(result.has_more).toBe(false); + expect(result.after).toBeNull(); + }); + + test('should honor explicit limits above the legacy 100 cap', async () => { + const extraAgents = []; + for (let i = 0; i < 105; i++) { + const agent = await createAgent({ + id: `agent_${uuidv4().slice(0, 12)}`, + name: `Bulk Agent ${i}`, + description: `Bulk agent ${i}`, + provider: 'openai', + model: 'gpt-4', + author: userA, + }); + extraAgents.push(agent); + } + + const accessibleIds = extraAgents.map((a) => a._id) as mongoose.Types.ObjectId[]; + + const result = await getListAgentsByAccess({ + accessibleIds, + otherParams: {}, + limit: 500, + }); + + expect(result.data).toHaveLength(105); + expect(result.has_more).toBe(false); + }); }); }); diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index 37015c21c4..6f11fa720b 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -685,12 +685,13 @@ export function createAgentMethods(mongoose: typeof import('mongoose'), deps: Ag } /** - * Get agents by accessible IDs with optional cursor-based pagination. + * Get agents by accessible IDs with cursor pagination. Defaults to a 100-page + * limit (max 1000); pass `limit: null` to opt out entirely. */ async function getListAgentsByAccess({ accessibleIds = [], otherParams = {}, - limit = null, + limit = 100, after = null, includeSkillConfig = false, }: { @@ -710,7 +711,7 @@ export function createAgentMethods(mongoose: typeof import('mongoose'), deps: Ag const Agent = mongoose.models.Agent as Model; const isPaginated = limit !== null && limit !== undefined; const normalizedLimit = isPaginated - ? Math.min(Math.max(1, parseInt(String(limit)) || 20), 100) + ? Math.min(Math.max(1, parseInt(String(limit)) || 20), 1000) : null; const baseQuery: Record = { diff --git a/packages/data-schemas/src/schema/aclEntry.ts b/packages/data-schemas/src/schema/aclEntry.ts index c612ee2886..8fe6b9f4ef 100644 --- a/packages/data-schemas/src/schema/aclEntry.ts +++ b/packages/data-schemas/src/schema/aclEntry.ts @@ -79,5 +79,7 @@ aclEntrySchema.index({ }); aclEntrySchema.index({ resourceId: 1, principalType: 1, principalId: 1, tenantId: 1 }); aclEntrySchema.index({ principalId: 1, permBits: 1, resourceType: 1, tenantId: 1 }); +/** Covers `findPublicResourceIds` and the public branch of `findAccessibleResources`. */ +aclEntrySchema.index({ principalType: 1, resourceType: 1, permBits: 1, resourceId: 1 }); export default aclEntrySchema;