From b349f2f876acef42ff18c675e2e1077bfc4bf177 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 22 Feb 2026 18:29:31 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A3=20fix:=20Serve=20Fresh=20Presigned?= =?UTF-8?q?=20URLs=20on=20Agent=20List=20Cache=20Hits=20(#11902)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: serve cached presigned URLs on agent list cache hits On a cache hit the list endpoint was skipping the S3 refresh and returning whatever presigned URL was stored in MongoDB, which could be expired if the S3 URL TTL is shorter than the 30-minute cache window. refreshListAvatars now collects a urlCache map (agentId -> refreshed filepath) alongside its existing stats. The controller stores this map in the cache instead of a plain boolean and re-applies it to every paginated response, guaranteeing clients always receive a URL that was valid as of the last refresh rather than a potentially stale DB value. * fix: improve avatar refresh cache handling and logging Updated the avatar refresh logic to validate cached refresh data before proceeding with S3 URL updates. Enhanced logging to exclude sensitive `urlCache` details while still providing relevant statistics. Added error handling for cache invalidation during avatar updates to ensure robustness. * fix: update avatar refresh logic to clear urlCache on no change Modified the avatar refresh function to clear the urlCache when no new path is generated, ensuring that stale URLs are not retained. This change improves cache handling and aligns with the updated logic for avatar updates. * fix: enhance avatar refresh logic to handle legacy cache entries Updated the avatar refresh logic to accommodate legacy boolean cache entries, ensuring they are treated as cache misses and triggering a refresh. The cache now stores a structured `urlCache` map instead of a boolean, improving cache handling. Added tests to verify correct behavior for cache hits and misses, ensuring clients receive valid URLs based on the latest refresh. --- api/server/controllers/agents/v1.js | 32 +++++++-- api/server/controllers/agents/v1.spec.js | 89 +++++++++++++++++++++++- packages/api/src/agents/avatars.spec.ts | 57 +++++++++++++++ packages/api/src/agents/avatars.ts | 46 ++++++------ 4 files changed, 193 insertions(+), 31 deletions(-) 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; };