mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-21 09:32:24 +00:00
📋 fix: Cap Default Limit on Agent List Queries (#13382)
* 🛡️ 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.
This commit is contained in:
parent
62dff69300
commit
94c73123ee
7 changed files with 152 additions and 10 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<t.AgentListResponse> {
|
||||
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<t.TPlugin[]>
|
|||
};
|
||||
|
||||
/**
|
||||
* 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 = <TData = t.AgentListResponse>(
|
||||
params: t.AgentListParams = defaultAgentParams,
|
||||
|
|
@ -44,12 +71,8 @@ export const useListAgentsQuery = <TData = t.AgentListResponse>(
|
|||
const enabled = !!endpointsConfig?.[EModelEndpoint.agents];
|
||||
return useQuery<t.AgentListResponse, unknown, TData>(
|
||||
[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,
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> };
|
||||
};
|
||||
|
||||
const planString = JSON.stringify(explain?.queryPlanner?.winningPlan ?? {});
|
||||
expect(planString).toContain('IXSCAN');
|
||||
expect(planString).not.toContain('COLLSCAN');
|
||||
});
|
||||
});
|
||||
|
||||
describe('aggregateAclEntries', () => {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<IAgent>;
|
||||
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<string, unknown> = {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue