mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-01-14 14:38:51 +01:00
🌅 fix: Agent Avatar S3 URL Refresh Pagination and Persistence (#11323)
* Refresh all S3 avatars for this user's accessible agent set, not the first page * Cleaner debug messages * Log errors as errors * refactor: avatar refresh logic to process agents in batches and improve error handling. Introduced new utility functions for refreshing S3 avatars and updating agent records. Updated tests to cover various scenarios including cache hits, user ownership checks, and error handling. Added constants for maximum refresh limits. * refactor: update avatar refresh logic to allow users with VIEW access to refresh avatars for all accessible agents. Removed checks for agent ownership and author presence, and updated related tests to reflect new behavior. * chore: Remove YouTube toolkit due to #11331 --------- Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
David Newman
GitHub
parent
10f591ab1c
commit
a95fea19bb
6 changed files with 743 additions and 51 deletions
|
|
@ -5,7 +5,9 @@ const { logger } = require('@librechat/data-schemas');
|
|||
const {
|
||||
agentCreateSchema,
|
||||
agentUpdateSchema,
|
||||
refreshListAvatars,
|
||||
mergeAgentOcrConversion,
|
||||
MAX_AVATAR_REFRESH_AGENTS,
|
||||
convertOcrToContextInPlace,
|
||||
} = require('@librechat/api');
|
||||
const {
|
||||
|
|
@ -56,46 +58,6 @@ const systemTools = {
|
|||
const MAX_SEARCH_LEN = 100;
|
||||
const escapeRegex = (str = '') => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
|
||||
/**
|
||||
* Opportunistically refreshes S3-backed avatars for agent list responses.
|
||||
* Only list responses are refreshed because they're the highest-traffic surface and
|
||||
* the avatar URLs have a short-lived TTL. The refresh is cached per-user for 30 minutes
|
||||
* via {@link CacheKeys.S3_EXPIRY_INTERVAL} so we refresh once per interval at most.
|
||||
* @param {Array} agents - Agents being enriched with S3-backed avatars
|
||||
* @param {string} userId - User identifier used for the cache refresh key
|
||||
*/
|
||||
const refreshListAvatars = async (agents, userId) => {
|
||||
if (!agents?.length) {
|
||||
return;
|
||||
}
|
||||
|
||||
const cache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL);
|
||||
const refreshKey = `${userId}:agents_list`;
|
||||
const alreadyChecked = await cache.get(refreshKey);
|
||||
if (alreadyChecked) {
|
||||
return;
|
||||
}
|
||||
|
||||
await Promise.all(
|
||||
agents.map(async (agent) => {
|
||||
if (agent?.avatar?.source !== FileSources.s3 || !agent?.avatar?.filepath) {
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
const newPath = await refreshS3Url(agent.avatar);
|
||||
if (newPath && newPath !== agent.avatar.filepath) {
|
||||
agent.avatar = { ...agent.avatar, filepath: newPath };
|
||||
}
|
||||
} catch (err) {
|
||||
logger.debug('[/Agents] Avatar refresh error for list item', err);
|
||||
}
|
||||
}),
|
||||
);
|
||||
|
||||
await cache.set(refreshKey, true, Time.THIRTY_MINUTES);
|
||||
};
|
||||
|
||||
/**
|
||||
* Creates an Agent.
|
||||
* @route POST /Agents
|
||||
|
|
@ -544,6 +506,35 @@ const getListAgentsHandler = async (req, res) => {
|
|||
requiredPermissions: PermissionBits.VIEW,
|
||||
});
|
||||
|
||||
/**
|
||||
* Refresh all S3 avatars for this user's accessible agent set (not only the current page)
|
||||
* This addresses page-size limits preventing refresh of agents beyond the first page
|
||||
*/
|
||||
const cache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL);
|
||||
const refreshKey = `${userId}:agents_avatar_refresh`;
|
||||
const alreadyChecked = await cache.get(refreshKey);
|
||||
if (alreadyChecked) {
|
||||
logger.debug('[/Agents] S3 avatar refresh already checked, skipping');
|
||||
} else {
|
||||
try {
|
||||
const fullList = await getListAgentsByAccess({
|
||||
accessibleIds,
|
||||
otherParams: {},
|
||||
limit: MAX_AVATAR_REFRESH_AGENTS,
|
||||
after: null,
|
||||
});
|
||||
await refreshListAvatars({
|
||||
agents: fullList?.data ?? [],
|
||||
userId,
|
||||
refreshS3Url,
|
||||
updateAgent,
|
||||
});
|
||||
await cache.set(refreshKey, true, Time.THIRTY_MINUTES);
|
||||
} catch (err) {
|
||||
logger.error('[/Agents] Error refreshing avatars for full list: %o', err);
|
||||
}
|
||||
}
|
||||
|
||||
// Use the new ACL-aware function
|
||||
const data = await getListAgentsByAccess({
|
||||
accessibleIds,
|
||||
|
|
@ -571,15 +562,9 @@ const getListAgentsHandler = async (req, res) => {
|
|||
return agent;
|
||||
});
|
||||
|
||||
// Opportunistically refresh S3 avatar URLs for list results with caching
|
||||
try {
|
||||
await refreshListAvatars(data.data, req.user.id);
|
||||
} catch (err) {
|
||||
logger.debug('[/Agents] Skipping avatar refresh for list', err);
|
||||
}
|
||||
return res.json(data);
|
||||
} catch (error) {
|
||||
logger.error('[/Agents] Error listing Agents', error);
|
||||
logger.error('[/Agents] Error listing Agents: %o', error);
|
||||
res.status(500).json({ error: error.message });
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,8 +1,9 @@
|
|||
const mongoose = require('mongoose');
|
||||
const { v4: uuidv4 } = require('uuid');
|
||||
const { nanoid } = require('nanoid');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
const { v4: uuidv4 } = require('uuid');
|
||||
const { agentSchema } = require('@librechat/data-schemas');
|
||||
const { FileSources } = require('librechat-data-provider');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
|
||||
// Only mock the dependencies that are not database-related
|
||||
jest.mock('~/server/services/Config', () => ({
|
||||
|
|
@ -54,6 +55,15 @@ jest.mock('~/models', () => ({
|
|||
getCategoriesWithCounts: jest.fn(),
|
||||
}));
|
||||
|
||||
// Mock cache for S3 avatar refresh tests
|
||||
const mockCache = {
|
||||
get: jest.fn(),
|
||||
set: jest.fn(),
|
||||
};
|
||||
jest.mock('~/cache', () => ({
|
||||
getLogStores: jest.fn(() => mockCache),
|
||||
}));
|
||||
|
||||
const {
|
||||
createAgent: createAgentHandler,
|
||||
updateAgent: updateAgentHandler,
|
||||
|
|
@ -65,6 +75,8 @@ const {
|
|||
findPubliclyAccessibleResources,
|
||||
} = require('~/server/services/PermissionService');
|
||||
|
||||
const { refreshS3Url } = require('~/server/services/Files/S3/crud');
|
||||
|
||||
/**
|
||||
* @type {import('mongoose').Model<import('@librechat/data-schemas').IAgent>}
|
||||
*/
|
||||
|
|
@ -1207,4 +1219,349 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
|
|||
expect(response.data[0].is_promoted).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('S3 Avatar Refresh', () => {
|
||||
let userA, userB;
|
||||
let agentWithS3Avatar, agentWithLocalAvatar, agentOwnedByOther;
|
||||
|
||||
beforeEach(async () => {
|
||||
await Agent.deleteMany({});
|
||||
jest.clearAllMocks();
|
||||
|
||||
// Reset cache mock
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
mockCache.set.mockResolvedValue(undefined);
|
||||
|
||||
userA = new mongoose.Types.ObjectId();
|
||||
userB = new mongoose.Types.ObjectId();
|
||||
|
||||
// Create agent with S3 avatar owned by userA
|
||||
agentWithS3Avatar = await Agent.create({
|
||||
id: `agent_${nanoid(12)}`,
|
||||
name: 'Agent with S3 Avatar',
|
||||
description: 'Has S3 avatar',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: userA,
|
||||
avatar: {
|
||||
source: FileSources.s3,
|
||||
filepath: 'old-s3-path.jpg',
|
||||
},
|
||||
versions: [
|
||||
{
|
||||
name: 'Agent with S3 Avatar',
|
||||
description: 'Has S3 avatar',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
createdAt: new Date(),
|
||||
updatedAt: new Date(),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
// Create agent with local avatar owned by userA
|
||||
agentWithLocalAvatar = await Agent.create({
|
||||
id: `agent_${nanoid(12)}`,
|
||||
name: 'Agent with Local Avatar',
|
||||
description: 'Has local avatar',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: userA,
|
||||
avatar: {
|
||||
source: 'local',
|
||||
filepath: 'local-path.jpg',
|
||||
},
|
||||
versions: [
|
||||
{
|
||||
name: 'Agent with Local Avatar',
|
||||
description: 'Has local avatar',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
createdAt: new Date(),
|
||||
updatedAt: new Date(),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
// Create agent with S3 avatar owned by userB
|
||||
agentOwnedByOther = await Agent.create({
|
||||
id: `agent_${nanoid(12)}`,
|
||||
name: 'Agent Owned By Other',
|
||||
description: 'Owned by userB',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: userB,
|
||||
avatar: {
|
||||
source: FileSources.s3,
|
||||
filepath: 'other-s3-path.jpg',
|
||||
},
|
||||
versions: [
|
||||
{
|
||||
name: 'Agent Owned By Other',
|
||||
description: 'Owned by userB',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
createdAt: new Date(),
|
||||
updatedAt: new Date(),
|
||||
},
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
test('should skip avatar refresh if cache hit', async () => {
|
||||
mockCache.get.mockResolvedValue(true);
|
||||
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Should not call refreshS3Url when cache hit
|
||||
expect(refreshS3Url).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('should refresh and persist S3 avatars on cache miss', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockResolvedValue('new-s3-path.jpg');
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Verify S3 URL was refreshed
|
||||
expect(refreshS3Url).toHaveBeenCalled();
|
||||
|
||||
// Verify cache was set
|
||||
expect(mockCache.set).toHaveBeenCalled();
|
||||
|
||||
// Verify response was returned
|
||||
expect(mockRes.json).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('should refresh avatars for all accessible agents (VIEW permission)', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
// User A has access to both their own agent and userB's agent
|
||||
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id, agentOwnedByOther._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockResolvedValue('new-path.jpg');
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Should be called for both agents - any user with VIEW access can refresh
|
||||
expect(refreshS3Url).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
test('should skip non-S3 avatars', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
findAccessibleResources.mockResolvedValue([agentWithLocalAvatar._id, agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockResolvedValue('new-path.jpg');
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Should only be called for S3 avatar agent
|
||||
expect(refreshS3Url).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
test('should not update if S3 URL unchanged', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
// Return the same path - no update needed
|
||||
refreshS3Url.mockResolvedValue('old-s3-path.jpg');
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Verify refreshS3Url was called
|
||||
expect(refreshS3Url).toHaveBeenCalled();
|
||||
|
||||
// Response should still be returned
|
||||
expect(mockRes.json).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('should handle S3 refresh errors gracefully', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockRejectedValue(new Error('S3 error'));
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
// Should not throw - handles error gracefully
|
||||
await expect(getListAgentsHandler(mockReq, mockRes)).resolves.not.toThrow();
|
||||
|
||||
// Response should still be returned
|
||||
expect(mockRes.json).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('should process agents in batches', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
|
||||
// Create 25 agents (should be processed in batches of 20)
|
||||
const manyAgents = [];
|
||||
for (let i = 0; i < 25; i++) {
|
||||
const agent = await Agent.create({
|
||||
id: `agent_${nanoid(12)}`,
|
||||
name: `Agent ${i}`,
|
||||
description: `Agent ${i} description`,
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: userA,
|
||||
avatar: {
|
||||
source: FileSources.s3,
|
||||
filepath: `path${i}.jpg`,
|
||||
},
|
||||
versions: [
|
||||
{
|
||||
name: `Agent ${i}`,
|
||||
description: `Agent ${i} description`,
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
createdAt: new Date(),
|
||||
updatedAt: new Date(),
|
||||
},
|
||||
],
|
||||
});
|
||||
manyAgents.push(agent);
|
||||
}
|
||||
|
||||
const allAgentIds = manyAgents.map((a) => a._id);
|
||||
findAccessibleResources.mockResolvedValue(allAgentIds);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockImplementation((avatar) =>
|
||||
Promise.resolve(avatar.filepath.replace('.jpg', '-new.jpg')),
|
||||
);
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// All 25 should be processed
|
||||
expect(refreshS3Url).toHaveBeenCalledTimes(25);
|
||||
});
|
||||
|
||||
test('should skip agents without id or author', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
|
||||
// Create agent without proper id field (edge case)
|
||||
const agentWithoutId = await Agent.create({
|
||||
id: `agent_${nanoid(12)}`,
|
||||
name: 'Agent without ID field',
|
||||
description: 'Testing',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
author: userA,
|
||||
avatar: {
|
||||
source: FileSources.s3,
|
||||
filepath: 'test-path.jpg',
|
||||
},
|
||||
versions: [
|
||||
{
|
||||
name: 'Agent without ID field',
|
||||
description: 'Testing',
|
||||
provider: 'openai',
|
||||
model: 'gpt-4',
|
||||
createdAt: new Date(),
|
||||
updatedAt: new Date(),
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
findAccessibleResources.mockResolvedValue([agentWithoutId._id, agentWithS3Avatar._id]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
refreshS3Url.mockResolvedValue('new-path.jpg');
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Should still complete without errors
|
||||
expect(mockRes.json).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('should use MAX_AVATAR_REFRESH_AGENTS limit for full list query', async () => {
|
||||
mockCache.get.mockResolvedValue(false);
|
||||
findAccessibleResources.mockResolvedValue([]);
|
||||
findPubliclyAccessibleResources.mockResolvedValue([]);
|
||||
|
||||
const mockReq = {
|
||||
user: { id: userA.toString(), role: 'USER' },
|
||||
query: {},
|
||||
};
|
||||
const mockRes = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
};
|
||||
|
||||
await getListAgentsHandler(mockReq, mockRes);
|
||||
|
||||
// Verify that the handler completed successfully
|
||||
expect(mockRes.json).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ const { Calculator } = require('@librechat/agents');
|
|||
const { logger } = require('@librechat/data-schemas');
|
||||
const { zodToJsonSchema } = require('zod-to-json-schema');
|
||||
const { Tools, ImageVisionTool } = require('librechat-data-provider');
|
||||
const { getToolkitKey, oaiToolkit, ytToolkit, geminiToolkit } = require('@librechat/api');
|
||||
const { getToolkitKey, oaiToolkit, geminiToolkit } = require('@librechat/api');
|
||||
const { toolkits } = require('~/app/clients/tools/manifest');
|
||||
|
||||
/**
|
||||
|
|
@ -83,7 +83,6 @@ function loadAndFormatTools({ directory, adminFilter = [], adminIncluded = [] })
|
|||
const basicToolInstances = [
|
||||
new Calculator(),
|
||||
...Object.values(oaiToolkit),
|
||||
...Object.values(ytToolkit),
|
||||
...Object.values(geminiToolkit),
|
||||
];
|
||||
for (const toolInstance of basicToolInstances) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue