Skip to content

Commit b349f2f

Browse files
danny-aviladanny-avila
authored
🪣 fix: Serve Fresh Presigned URLs on Agent List Cache Hits (danny-avila#11902)
* 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.
1 parent 7ce898d commit b349f2f

4 files changed

Lines changed: 193 additions & 31 deletions

File tree

‎api/server/controllers/agents/v1.js‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -530,27 +530,30 @@ const getListAgentsHandler = async (req, res) => {
530530
*/
531531
const cache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL);
532532
const refreshKey = `${userId}:agents_avatar_refresh`;
533-
const alreadyChecked = await cache.get(refreshKey);
534-
if (alreadyChecked) {
535-
logger.debug('[/Agents] S3 avatar refresh already checked, skipping');
536-
} else {
533+
let cachedRefresh = await cache.get(refreshKey);
534+
const isValidCachedRefresh =
535+
cachedRefresh != null && typeof cachedRefresh === 'object' && cachedRefresh.urlCache != null;
536+
if (!isValidCachedRefresh) {
537537
try {
538538
const fullList = await getListAgentsByAccess({
539539
accessibleIds,
540540
otherParams: {},
541541
limit: MAX_AVATAR_REFRESH_AGENTS,
542542
after: null,
543543
});
544-
await refreshListAvatars({
544+
const { urlCache } = await refreshListAvatars({
545545
agents: fullList?.data ?? [],
546546
userId,
547547
refreshS3Url,
548548
updateAgent,
549549
});
550-
await cache.set(refreshKey, true, Time.THIRTY_MINUTES);
550+
cachedRefresh = { urlCache };
551+
await cache.set(refreshKey, cachedRefresh, Time.THIRTY_MINUTES);
551552
} catch (err) {
552553
logger.error('[/Agents] Error refreshing avatars for full list: %o', err);
553554
}
555+
} else {
556+
logger.debug('[/Agents] S3 avatar refresh already checked, skipping');
554557
}
555558

556559
// Use the new ACL-aware function
@@ -568,11 +571,20 @@ const getListAgentsHandler = async (req, res) => {
568571

569572
const publicSet = new Set(publiclyAccessibleIds.map((oid) => oid.toString()));
570573

574+
const urlCache = cachedRefresh?.urlCache;
571575
data.data = agents.map((agent) => {
572576
try {
573577
if (agent?._id && publicSet.has(agent._id.toString())) {
574578
agent.isPublic = true;
575579
}
580+
if (
581+
urlCache &&
582+
agent?.id &&
583+
agent?.avatar?.source === FileSources.s3 &&
584+
urlCache[agent.id]
585+
) {
586+
agent.avatar = { ...agent.avatar, filepath: urlCache[agent.id] };
587+
}
576588
} catch (e) {
577589
// Silently ignore mapping errors
578590
void e;
@@ -658,6 +670,14 @@ const uploadAgentAvatarHandler = async (req, res) => {
658670
const updatedAgent = await updateAgent({ id: agent_id }, data, {
659671
updatingUserId: req.user.id,
660672
});
673+
674+
try {
675+
const avatarCache = getLogStores(CacheKeys.S3_EXPIRY_INTERVAL);
676+
await avatarCache.delete(`${req.user.id}:agents_avatar_refresh`);
677+
} catch (cacheErr) {
678+
logger.error('[/:agent_id/avatar] Error invalidating avatar refresh cache', cacheErr);
679+
}
680+
661681
res.status(201).json(updatedAgent);
662682
} catch (error) {
663683
const message = 'An error occurred while updating the Agent Avatar';

‎api/server/controllers/agents/v1.spec.js‎

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ jest.mock('~/models', () => ({
5959
const mockCache = {
6060
get: jest.fn(),
6161
set: jest.fn(),
62+
delete: jest.fn(),
6263
};
6364
jest.mock('~/cache', () => ({
6465
getLogStores: jest.fn(() => mockCache),
@@ -1309,7 +1310,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
13091310
});
13101311

13111312
test('should skip avatar refresh if cache hit', async () => {
1312-
mockCache.get.mockResolvedValue(true);
1313+
mockCache.get.mockResolvedValue({ urlCache: {} });
13131314
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
13141315
findPubliclyAccessibleResources.mockResolvedValue([]);
13151316

@@ -1348,8 +1349,12 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
13481349
// Verify S3 URL was refreshed
13491350
expect(refreshS3Url).toHaveBeenCalled();
13501351

1351-
// Verify cache was set
1352-
expect(mockCache.set).toHaveBeenCalled();
1352+
// Verify cache was set with urlCache map, not a plain boolean
1353+
expect(mockCache.set).toHaveBeenCalledWith(
1354+
expect.any(String),
1355+
expect.objectContaining({ urlCache: expect.any(Object) }),
1356+
expect.any(Number),
1357+
);
13531358

13541359
// Verify response was returned
13551360
expect(mockRes.json).toHaveBeenCalled();
@@ -1563,5 +1568,83 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
15631568
// Verify that the handler completed successfully
15641569
expect(mockRes.json).toHaveBeenCalled();
15651570
});
1571+
1572+
test('should treat legacy boolean cache entry as a miss and run refresh', async () => {
1573+
// Simulate a cache entry written by the pre-fix code
1574+
mockCache.get.mockResolvedValue(true);
1575+
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
1576+
findPubliclyAccessibleResources.mockResolvedValue([]);
1577+
refreshS3Url.mockResolvedValue('new-s3-path.jpg');
1578+
1579+
const mockReq = {
1580+
user: { id: userA.toString(), role: 'USER' },
1581+
query: {},
1582+
};
1583+
const mockRes = {
1584+
status: jest.fn().mockReturnThis(),
1585+
json: jest.fn().mockReturnThis(),
1586+
};
1587+
1588+
await getListAgentsHandler(mockReq, mockRes);
1589+
1590+
// Boolean true fails the shape guard, so refresh must run
1591+
expect(refreshS3Url).toHaveBeenCalled();
1592+
// Cache is overwritten with the proper format
1593+
expect(mockCache.set).toHaveBeenCalledWith(
1594+
expect.any(String),
1595+
expect.objectContaining({ urlCache: expect.any(Object) }),
1596+
expect.any(Number),
1597+
);
1598+
});
1599+
1600+
test('should apply cached urlCache filepath to paginated response on cache hit', async () => {
1601+
const agentId = agentWithS3Avatar.id;
1602+
const cachedUrl = 'cached-presigned-url.jpg';
1603+
1604+
mockCache.get.mockResolvedValue({ urlCache: { [agentId]: cachedUrl } });
1605+
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
1606+
findPubliclyAccessibleResources.mockResolvedValue([]);
1607+
1608+
const mockReq = {
1609+
user: { id: userA.toString(), role: 'USER' },
1610+
query: {},
1611+
};
1612+
const mockRes = {
1613+
status: jest.fn().mockReturnThis(),
1614+
json: jest.fn().mockReturnThis(),
1615+
};
1616+
1617+
await getListAgentsHandler(mockReq, mockRes);
1618+
1619+
expect(refreshS3Url).not.toHaveBeenCalled();
1620+
1621+
const responseData = mockRes.json.mock.calls[0][0];
1622+
const agent = responseData.data.find((a) => a.id === agentId);
1623+
// Cached URL is served, not the stale DB value 'old-s3-path.jpg'
1624+
expect(agent.avatar.filepath).toBe(cachedUrl);
1625+
});
1626+
1627+
test('should preserve DB filepath for agents absent from urlCache on cache hit', async () => {
1628+
mockCache.get.mockResolvedValue({ urlCache: {} });
1629+
findAccessibleResources.mockResolvedValue([agentWithS3Avatar._id]);
1630+
findPubliclyAccessibleResources.mockResolvedValue([]);
1631+
1632+
const mockReq = {
1633+
user: { id: userA.toString(), role: 'USER' },
1634+
query: {},
1635+
};
1636+
const mockRes = {
1637+
status: jest.fn().mockReturnThis(),
1638+
json: jest.fn().mockReturnThis(),
1639+
};
1640+
1641+
await getListAgentsHandler(mockReq, mockRes);
1642+
1643+
expect(refreshS3Url).not.toHaveBeenCalled();
1644+
1645+
const responseData = mockRes.json.mock.calls[0][0];
1646+
const agent = responseData.data.find((a) => a.id === agentWithS3Avatar.id);
1647+
expect(agent.avatar.filepath).toBe('old-s3-path.jpg');
1648+
});
15661649
});
15671650
});

‎packages/api/src/agents/avatars.spec.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,16 @@ import {
77
refreshListAvatars,
88
} from './avatars';
99

10+
jest.mock('@librechat/data-schemas', () => ({
11+
logger: {
12+
debug: jest.fn(),
13+
info: jest.fn(),
14+
error: jest.fn(),
15+
},
16+
}));
17+
18+
import { logger } from '@librechat/data-schemas';
19+
1020
describe('refreshListAvatars', () => {
1121
let mockRefreshS3Url: jest.MockedFunction<RefreshS3UrlFn>;
1222
let mockUpdateAgent: jest.MockedFunction<UpdateAgentFn>;
@@ -15,6 +25,7 @@ describe('refreshListAvatars', () => {
1525
beforeEach(() => {
1626
mockRefreshS3Url = jest.fn();
1727
mockUpdateAgent = jest.fn();
28+
jest.clearAllMocks();
1829
});
1930

2031
const createAgent = (overrides: Partial<Agent> = {}): Agent => ({
@@ -44,6 +55,7 @@ describe('refreshListAvatars', () => {
4455
});
4556

4657
expect(stats.updated).toBe(0);
58+
expect(stats.urlCache).toEqual({});
4759
expect(mockRefreshS3Url).not.toHaveBeenCalled();
4860
expect(mockUpdateAgent).not.toHaveBeenCalled();
4961
});
@@ -62,6 +74,7 @@ describe('refreshListAvatars', () => {
6274

6375
expect(stats.not_s3).toBe(1);
6476
expect(stats.updated).toBe(0);
77+
expect(stats.urlCache).toEqual({});
6578
expect(mockRefreshS3Url).not.toHaveBeenCalled();
6679
});
6780

@@ -109,6 +122,7 @@ describe('refreshListAvatars', () => {
109122
});
110123

111124
expect(stats.updated).toBe(1);
125+
expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg' });
112126
expect(mockRefreshS3Url).toHaveBeenCalledWith(agent.avatar);
113127
expect(mockUpdateAgent).toHaveBeenCalledWith(
114128
{ id: 'agent1' },
@@ -130,6 +144,7 @@ describe('refreshListAvatars', () => {
130144

131145
expect(stats.no_change).toBe(1);
132146
expect(stats.updated).toBe(0);
147+
expect(stats.urlCache).toEqual({});
133148
expect(mockUpdateAgent).not.toHaveBeenCalled();
134149
});
135150

@@ -146,6 +161,7 @@ describe('refreshListAvatars', () => {
146161

147162
expect(stats.s3_error).toBe(1);
148163
expect(stats.updated).toBe(0);
164+
expect(stats.urlCache).toEqual({});
149165
});
150166

151167
it('should handle database persist errors gracefully', async () => {
@@ -162,6 +178,7 @@ describe('refreshListAvatars', () => {
162178

163179
expect(stats.persist_error).toBe(1);
164180
expect(stats.updated).toBe(0);
181+
expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg' });
165182
});
166183

167184
it('should process agents in batches', async () => {
@@ -186,10 +203,49 @@ describe('refreshListAvatars', () => {
186203
});
187204

188205
expect(stats.updated).toBe(25);
206+
expect(Object.keys(stats.urlCache)).toHaveLength(25);
189207
expect(mockRefreshS3Url).toHaveBeenCalledTimes(25);
190208
expect(mockUpdateAgent).toHaveBeenCalledTimes(25);
191209
});
192210

211+
it('should not populate urlCache when refreshS3Url resolves with falsy', async () => {
212+
const agent = createAgent();
213+
mockRefreshS3Url.mockResolvedValue(undefined);
214+
215+
const stats = await refreshListAvatars({
216+
agents: [agent],
217+
userId,
218+
refreshS3Url: mockRefreshS3Url,
219+
updateAgent: mockUpdateAgent,
220+
});
221+
222+
expect(stats.no_change).toBe(1);
223+
expect(stats.urlCache).toEqual({});
224+
expect(mockUpdateAgent).not.toHaveBeenCalled();
225+
});
226+
227+
it('should redact urlCache from log output', async () => {
228+
const agent = createAgent();
229+
mockRefreshS3Url.mockResolvedValue('new-path.jpg');
230+
mockUpdateAgent.mockResolvedValue({});
231+
232+
await refreshListAvatars({
233+
agents: [agent],
234+
userId,
235+
refreshS3Url: mockRefreshS3Url,
236+
updateAgent: mockUpdateAgent,
237+
});
238+
239+
const loggerInfo = logger.info as jest.Mock;
240+
const summaryCall = loggerInfo.mock.calls.find(([msg]) =>
241+
msg.includes('Avatar refresh summary'),
242+
);
243+
expect(summaryCall).toBeDefined();
244+
const loggedPayload = summaryCall[1];
245+
expect(loggedPayload).toHaveProperty('urlCacheSize', 1);
246+
expect(loggedPayload).not.toHaveProperty('urlCache');
247+
});
248+
193249
it('should track mixed statistics correctly', async () => {
194250
const agents = [
195251
createAgent({ id: 'agent1' }),
@@ -214,6 +270,7 @@ describe('refreshListAvatars', () => {
214270
expect(stats.updated).toBe(2); // agent1 and agent2 (other user's agent now refreshed)
215271
expect(stats.not_s3).toBe(1); // agent3
216272
expect(stats.no_id).toBe(1); // agent with empty id
273+
expect(stats.urlCache).toEqual({ agent1: 'new-path.jpg', agent2: 'new-path.jpg' });
217274
});
218275
});
219276

‎packages/api/src/agents/avatars.ts‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ export type RefreshStats = {
2929
no_change: number;
3030
s3_error: number;
3131
persist_error: number;
32+
/** Maps agentId to the latest valid presigned filepath for re-application on cache hits */
33+
urlCache: Record<string, string>;
3234
};
3335

3436
/**
@@ -55,6 +57,7 @@ export const refreshListAvatars = async ({
5557
no_change: 0,
5658
s3_error: 0,
5759
persist_error: 0,
60+
urlCache: {},
5861
};
5962

6063
if (!agents?.length) {
@@ -86,28 +89,23 @@ export const refreshListAvatars = async ({
8689
logger.debug('[refreshListAvatars] Refreshing S3 avatar for agent: %s', agent._id);
8790
const newPath = await refreshS3Url(agent.avatar);
8891

89-
if (newPath && newPath !== agent.avatar.filepath) {
90-
try {
91-
await updateAgent(
92-
{ id: agent.id },
93-
{
94-
avatar: {
95-
filepath: newPath,
96-
source: agent.avatar.source,
97-
},
98-
},
99-
{
100-
updatingUserId: userId,
101-
skipVersioning: true,
102-
},
103-
);
104-
stats.updated++;
105-
} catch (persistErr) {
106-
logger.error('[refreshListAvatars] Avatar refresh persist error: %o', persistErr);
107-
stats.persist_error++;
108-
}
109-
} else {
92+
if (!newPath || newPath === agent.avatar.filepath) {
11093
stats.no_change++;
94+
return;
95+
}
96+
97+
stats.urlCache[agent.id] = newPath;
98+
99+
try {
100+
await updateAgent(
101+
{ id: agent.id },
102+
{ avatar: { filepath: newPath, source: agent.avatar.source } },
103+
{ updatingUserId: userId, skipVersioning: true },
104+
);
105+
stats.updated++;
106+
} catch (persistErr) {
107+
logger.error('[refreshListAvatars] Avatar refresh persist error: %o', persistErr);
108+
stats.persist_error++;
111109
}
112110
} catch (err) {
113111
logger.error('[refreshListAvatars] S3 avatar refresh error: %o', err);
@@ -117,6 +115,10 @@ export const refreshListAvatars = async ({
117115
);
118116
}
119117

120-
logger.info('[refreshListAvatars] Avatar refresh summary: %o', stats);
118+
const { urlCache: _urlCache, ...loggableStats } = stats;
119+
logger.info('[refreshListAvatars] Avatar refresh summary: %o', {
120+
...loggableStats,
121+
urlCacheSize: Object.keys(_urlCache).length,
122+
});
121123
return stats;
122124
};

0 commit comments

Comments
 (0)