diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 34078b2250..a2c0d55186 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -530,10 +530,10 @@ const getListAgentsHandler = async (req, res) => { */ 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 { + let cachedRefresh = await cache.get(refreshKey); + const isValidCachedRefresh = + cachedRefresh != null && typeof cachedRefresh === 'object' && cachedRefresh.urlCache != null; + if (!isValidCachedRefresh) { try { const fullList = await getListAgentsByAccess({ accessibleIds, @@ -541,16 +541,19 @@ const getListAgentsHandler = async (req, res) => { limit: MAX_AVATAR_REFRESH_AGENTS, after: null, }); - await refreshListAvatars({ + const { urlCache } = await refreshListAvatars({ agents: fullList?.data ?? [], userId, refreshS3Url, updateAgent, }); - await cache.set(refreshKey, true, Time.THIRTY_MINUTES); + cachedRefresh = { urlCache }; + await cache.set(refreshKey, cachedRefresh, Time.THIRTY_MINUTES); } catch (err) { logger.error('[/Agents] Error refreshing avatars for full list: %o', err); } + } else { + logger.debug('[/Agents] S3 avatar refresh already checked, skipping'); } // Use the new ACL-aware function @@ -568,11 +571,20 @@ const getListAgentsHandler = async (req, res) => { const publicSet = new Set(publiclyAccessibleIds.map((oid) => oid.toString())); + const urlCache = cachedRefresh?.urlCache; data.data = agents.map((agent) => { try { if (agent?._id && publicSet.has(agent._id.toString())) { agent.isPublic = true; } + if ( + urlCache && + agent?.id && + agent?.avatar?.source === FileSources.s3 && + urlCache[agent.id] + ) { + agent.avatar = { ...agent.avatar, filepath: urlCache[agent.id] }; + } } catch (e) { // Silently ignore mapping errors void e; @@ -658,6 +670,14 @@ const uploadAgentAvatarHandler = async (req, res) => { const updatedAgent = await updateAgent({ id: agent_id }, data, { updatingUserId: req.user.id, }); + + try { + const avatarCache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL); + await avatarCache.delete(`${req.user.id}:agents_avatar_refresh`); + } catch (cacheErr) { + logger.error('[/:agent_id/avatar] Error invalidating avatar refresh cache', cacheErr); + } + res.status(201).json(updatedAgent); } catch (error) { const message = 'An error occurred while updating the Agent Avatar'; diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index 8b2a57d903..ce68cc241f 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -59,6 +59,7 @@ jest.mock('~/models', () => ({ const mockCache = { get: jest.fn(), set: jest.fn(), + delete: jest.fn(), }; jest.mock('~/cache', () => ({ getLogStores: jest.fn(() => mockCache), @@ -1309,7 +1310,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => { }); test('should skip avatar refresh if cache hit', async () => { - mockCache.get.mockResolvedValue(true); + mockCache.get.mockResolvedValue({ urlCache: {} }); findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]); findPubliclyAccessibleResources.mockResolvedValue([]); @@ -1348,8 +1349,12 @@ describe('Agent Controllers - Mass Assignment Protection', () => { // Verify S3 URL was refreshed expect(refreshS3Url).toHaveBeenCalled(); - // Verify cache was set - expect(mockCache.set).toHaveBeenCalled(); + // Verify cache was set with urlCache map, not a plain boolean + expect(mockCache.set).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ urlCache: expect.any(Object) }), + expect.any(Number), + ); // Verify response was returned expect(mockRes.json).toHaveBeenCalled(); @@ -1563,5 +1568,83 @@ describe('Agent Controllers - Mass Assignment Protection', () => { // Verify that the handler completed successfully expect(mockRes.json).toHaveBeenCalled(); }); + + test('should treat legacy boolean cache entry as a miss and run refresh', async () => { + // Simulate a cache entry written by the pre-fix code + mockCache.get.mockResolvedValue(true); + 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); + + // Boolean true fails the shape guard, so refresh must run + expect(refreshS3Url).toHaveBeenCalled(); + // Cache is overwritten with the proper format + expect(mockCache.set).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ urlCache: expect.any(Object) }), + expect.any(Number), + ); + }); + + test('should apply cached urlCache filepath to paginated response on cache hit', async () => { + const agentId = agentWithS3Avatar.id; + const cachedUrl = 'cached-presigned-url.jpg'; + + mockCache.get.mockResolvedValue({ urlCache: { [agentId]: cachedUrl } }); + 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); + + expect(refreshS3Url).not.toHaveBeenCalled(); + + const responseData = mockRes.json.mock.calls[0][0]; + const agent = responseData.data.find((a) => a.id === agentId); + // Cached URL is served, not the stale DB value 'old-s3-path.jpg' + expect(agent.avatar.filepath).toBe(cachedUrl); + }); + + test('should preserve DB filepath for agents absent from urlCache on cache hit', async () => { + mockCache.get.mockResolvedValue({ urlCache: {} }); + 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); + + expect(refreshS3Url).not.toHaveBeenCalled(); + + const responseData = mockRes.json.mock.calls[0][0]; + const agent = responseData.data.find((a) => a.id === agentWithS3Avatar.id); + expect(agent.avatar.filepath).toBe('old-s3-path.jpg'); + }); }); }); diff --git a/packages/api/src/agents/avatars.spec.ts b/packages/api/src/agents/avatars.spec.ts index ac97964837..db82b311ee 100644 --- a/packages/api/src/agents/avatars.spec.ts +++ b/packages/api/src/agents/avatars.spec.ts @@ -7,6 +7,16 @@ import { refreshListAvatars, } from './avatars'; +jest.mock('@librechat/data-schemas', () => ({ + logger: { + debug: jest.fn(), + info: jest.fn(), + error: jest.fn(), + }, +})); + +import { logger } from '@librechat/data-schemas'; + describe('refreshListAvatars', () => { let mockRefreshS3Url: jest.MockedFunction; let mockUpdateAgent: jest.MockedFunction; @@ -15,6 +25,7 @@ describe('refreshListAvatars', () => { beforeEach(() => { mockRefreshS3Url = jest.fn(); mockUpdateAgent = jest.fn(); + jest.clearAllMocks(); }); const createAgent = (overrides: Partial = {}): Agent => ({ @@ -44,6 +55,7 @@ describe('refreshListAvatars', () => { }); expect(stats.updated).toBe(0); + expect(stats.urlCache).toEqual({}); expect(mockRefreshS3Url).not.toHaveBeenCalled(); expect(mockUpdateAgent).not.toHaveBeenCalled(); }); @@ -62,6 +74,7 @@ describe('refreshListAvatars', () => { expect(stats.not_s3).toBe(1); expect(stats.updated).toBe(0); + expect(stats.urlCache).toEqual({}); expect(mockRefreshS3Url).not.toHaveBeenCalled(); }); @@ -109,6 +122,7 @@ describe('refreshListAvatars', () => { }); expect(stats.updated).toBe(1); + expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg' }); expect(mockRefreshS3Url).toHaveBeenCalledWith(agent.avatar); expect(mockUpdateAgent).toHaveBeenCalledWith( { id: 'agent1' }, @@ -130,6 +144,7 @@ describe('refreshListAvatars', () => { expect(stats.no_change).toBe(1); expect(stats.updated).toBe(0); + expect(stats.urlCache).toEqual({}); expect(mockUpdateAgent).not.toHaveBeenCalled(); }); @@ -146,6 +161,7 @@ describe('refreshListAvatars', () => { expect(stats.s3_error).toBe(1); expect(stats.updated).toBe(0); + expect(stats.urlCache).toEqual({}); }); it('should handle database persist errors gracefully', async () => { @@ -162,6 +178,7 @@ describe('refreshListAvatars', () => { expect(stats.persist_error).toBe(1); expect(stats.updated).toBe(0); + expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg' }); }); it('should process agents in batches', async () => { @@ -186,10 +203,49 @@ describe('refreshListAvatars', () => { }); expect(stats.updated).toBe(25); + expect(Object.keys(stats.urlCache)).toHaveLength(25); expect(mockRefreshS3Url).toHaveBeenCalledTimes(25); expect(mockUpdateAgent).toHaveBeenCalledTimes(25); }); + it('should not populate urlCache when refreshS3Url resolves with falsy', async () => { + const agent = createAgent(); + mockRefreshS3Url.mockResolvedValue(undefined); + + const stats = await refreshListAvatars({ + agents: [agent], + userId, + refreshS3Url: mockRefreshS3Url, + updateAgent: mockUpdateAgent, + }); + + expect(stats.no_change).toBe(1); + expect(stats.urlCache).toEqual({}); + expect(mockUpdateAgent).not.toHaveBeenCalled(); + }); + + it('should redact urlCache from log output', async () => { + const agent = createAgent(); + mockRefreshS3Url.mockResolvedValue('new-path.jpg'); + mockUpdateAgent.mockResolvedValue({}); + + await refreshListAvatars({ + agents: [agent], + userId, + refreshS3Url: mockRefreshS3Url, + updateAgent: mockUpdateAgent, + }); + + const loggerInfo = logger.info as jest.Mock; + const summaryCall = loggerInfo.mock.calls.find(([msg]) => + msg.includes('Avatar refresh summary'), + ); + expect(summaryCall).toBeDefined(); + const loggedPayload = summaryCall[1]; + expect(loggedPayload).toHaveProperty('urlCacheSize', 1); + expect(loggedPayload).not.toHaveProperty('urlCache'); + }); + it('should track mixed statistics correctly', async () => { const agents = [ createAgent({ id: 'agent1' }), @@ -214,6 +270,7 @@ describe('refreshListAvatars', () => { expect(stats.updated).toBe(2); // agent1 and agent2 (other user's agent now refreshed) expect(stats.not_s3).toBe(1); // agent3 expect(stats.no_id).toBe(1); // agent with empty id + expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg', agent2: 'new-path.jpg' }); }); }); diff --git a/packages/api/src/agents/avatars.ts b/packages/api/src/agents/avatars.ts index 7c92f352b2..25adfdc717 100644 --- a/packages/api/src/agents/avatars.ts +++ b/packages/api/src/agents/avatars.ts @@ -29,6 +29,8 @@ export type RefreshStats = { no_change: number; s3_error: number; persist_error: number; + /** Maps agentId to the latest valid presigned filepath for re-application on cache hits */ + urlCache: Record; }; /** @@ -55,6 +57,7 @@ export const refreshListAvatars = async ({ no_change: 0, s3_error: 0, persist_error: 0, + urlCache: {}, }; if (!agents?.length) { @@ -86,28 +89,23 @@ export const refreshListAvatars = async ({ logger.debug('[refreshListAvatars] Refreshing S3 avatar for agent: %s', agent._id); const newPath = await refreshS3Url(agent.avatar); - if (newPath && newPath !== agent.avatar.filepath) { - try { - await updateAgent( - { id: agent.id }, - { - avatar: { - filepath: newPath, - source: agent.avatar.source, - }, - }, - { - updatingUserId: userId, - skipVersioning: true, - }, - ); - stats.updated++; - } catch (persistErr) { - logger.error('[refreshListAvatars] Avatar refresh persist error: %o', persistErr); - stats.persist_error++; - } - } else { + if (!newPath || newPath === agent.avatar.filepath) { stats.no_change++; + return; + } + + stats.urlCache[agent.id] = newPath; + + try { + await updateAgent( + { id: agent.id }, + { avatar: { filepath: newPath, source: agent.avatar.source } }, + { updatingUserId: userId, skipVersioning: true }, + ); + stats.updated++; + } catch (persistErr) { + logger.error('[refreshListAvatars] Avatar refresh persist error: %o', persistErr); + stats.persist_error++; } } catch (err) { logger.error('[refreshListAvatars] S3 avatar refresh error: %o', err); @@ -117,6 +115,10 @@ export const refreshListAvatars = async ({ ); } - logger.info('[refreshListAvatars] Avatar refresh summary: %o', stats); + const { urlCache: _urlCache, ...loggableStats } = stats; + logger.info('[refreshListAvatars] Avatar refresh summary: %o', { + ...loggableStats, + urlCacheSize: Object.keys(_urlCache).length, + }); return stats; };