From f3c6e24f8462586cf73c37584920175ff002c611 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 23 Aug 2026 15:17:35 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=20perf:=20Cut=20Serial=20Round=20Trip?= =?UTF-8?q?s=20and=20a=20100ms=20Admission=20Stall=20from=20Chat=20Turns?= =?UTF-8?q?=20(#15138)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ⚡ perf: Stop Awaiting the Conversation Access Marker Write Without Redis the CONVO_ACCESS violations namespace is backed by keyv-file, whose debounced write resolves after ~100ms. validateConvoAccess awaited that write before calling next(), so the first message to any existing conversation waited ~100ms before the request was even admitted — once per conversation per ten-minute window, on every default deployment. The marker only short-circuits the next check, so the write no longer gates the request. The same read now stashes the full document on req.resolvedConversation (null when absent) for downstream consumers. First-turn ack on an existing conversation: 109ms -> 5ms. * ⚡ perf: Read the Conversation Once per Chat Turn A chat turn read the same conversation document four times: the access check (two fields), the subagent thread guard (full document), agent initialization (the files field), and the first save. The access check now reads the full document and leaves it on req.resolvedConversation, the guard accepts that pre-resolved document instead of re-reading, and initializeAgent takes the conversation's file refs from it rather than issuing a separate findOne. Two serial round trips removed from every turn; the same document still serves the first save as before. * ⚡ perf: Remove Duplicate JWT Authentication on Agents Routes routes/agents/index.js applies requireJwtAuth and then mounts the v1 router at '/', which applied requireJwtAuth again. Every request through the agents router — chat turns included — ran the passport strategy twice: two signature checks and two user document reads. The v1 router is mounted nowhere else; its separately exported avatar router carries its own auth in files/index.js. * ⚡ perf: Skip the History Read for Root-Parent Turns and Walk the Tree in O(n) loadHistory fetched every message in the conversation and then walked the parent chain from the request's head. For a new conversation — or a new branch from the root of an existing one — the head is the root sentinel, which no message carries as its id, so the walk was empty by construction and the fetch was wasted. It now returns early. getMessagesForConversation found each ancestor with Array.find inside the walk, O(n^2) on a linear conversation (~5ms at 1000 messages). A Map by messageId makes it O(n); first-match semantics are preserved. --- api/app/clients/BaseClient.js | 17 ++- api/app/clients/specs/BaseClient.test.js | 34 +++++ api/server/middleware/validate/convoAccess.js | 14 +- .../middleware/validate/convoAccess.spec.js | 126 ++++++++++++++++++ api/server/routes/agents/v1.js | 4 +- packages/api/src/agents/guard.spec.ts | 38 ++++++ packages/api/src/agents/guard.ts | 18 ++- .../api/src/agents/initialize.files.spec.ts | 45 +++++++ packages/api/src/agents/initialize.ts | 28 +++- packages/api/src/types/http.ts | 9 +- .../data-schemas/src/methods/conversation.ts | 16 ++- 11 files changed, 325 insertions(+), 24 deletions(-) create mode 100644 api/server/middleware/validate/convoAccess.spec.js create mode 100644 packages/api/src/agents/initialize.files.spec.ts diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index 5da4b34da4..1cacd4666e 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -1149,6 +1149,11 @@ class BaseClient { async loadHistory(conversationId, parentMessageId = null) { logger.debug('[BaseClient] Loading history:', { conversationId, parentMessageId }); + /** No message has the root sentinel as its id, so the chain walk from it is empty. */ + if (parentMessageId === Constants.NO_PARENT) { + return []; + } + const messages = (await db.getMessages({ conversationId, user: this.user })) ?? []; if (messages.length === 0) { @@ -1389,15 +1394,19 @@ class BaseClient { const orderedMessages = []; let currentMessageId = parentMessageId; const visitedMessageIds = new Set(); + const messagesById = new Map(); + for (const msg of messages) { + const messageId = msg.messageId ?? msg.id; + if (!messagesById.has(messageId)) { + messagesById.set(messageId, msg); + } + } while (currentMessageId) { if (visitedMessageIds.has(currentMessageId)) { break; } - const message = messages.find((msg) => { - const messageId = msg.messageId ?? msg.id; - return messageId === currentMessageId; - }); + const message = messagesById.get(currentMessageId); visitedMessageIds.add(currentMessageId); diff --git a/api/app/clients/specs/BaseClient.test.js b/api/app/clients/specs/BaseClient.test.js index af6a09a208..bf0e025926 100644 --- a/api/app/clients/specs/BaseClient.test.js +++ b/api/app/clients/specs/BaseClient.test.js @@ -1,4 +1,5 @@ const { Constants, ContentTypes } = require('librechat-data-provider'); +const BaseClientClass = require('../BaseClient'); const { ContentFilterError } = require('@librechat/api'); const { FakeClient, initializeFakeClient } = require('./FakeClient'); @@ -226,6 +227,39 @@ describe('BaseClient', () => { expect(result.messagesToRefine).toEqual(expectedMessagesToRefine); }); + describe('loadHistory', () => { + const receiver = Object.assign(Object.create(BaseClientClass.prototype), { + user: 'user-1', + getMessageMapMethod: null, + shouldSummarize: false, + addPreviousAttachments: async (messages) => messages, + }); + const loadHistory = (parentMessageId) => receiver.loadHistory('convo-1', parentMessageId); + + beforeEach(() => { + getMessages.mockClear(); + }); + + test('skips the database when the parent is the root sentinel: no message can match it', async () => { + const result = await loadHistory(Constants.NO_PARENT); + + expect(result).toEqual([]); + expect(getMessages).not.toHaveBeenCalled(); + }); + + test('still loads and walks the chain for a real parent', async () => { + getMessages.mockResolvedValueOnce([ + { messageId: 'root', parentMessageId: Constants.NO_PARENT, text: 'a' }, + { messageId: 'reply', parentMessageId: 'root', text: 'b' }, + ]); + + const result = await loadHistory('reply'); + + expect(getMessages).toHaveBeenCalledTimes(1); + expect(result.map((m) => m.messageId)).toEqual(['root', 'reply']); + }); + }); + describe('getMessagesForConversation', () => { it('should return an empty array if the parentMessageId does not exist', () => { const result = TestClient.constructor.getMessagesForConversation({ diff --git a/api/server/middleware/validate/convoAccess.js b/api/server/middleware/validate/convoAccess.js index ef1eea8f37..929b43dc1e 100644 --- a/api/server/middleware/validate/convoAccess.js +++ b/api/server/middleware/validate/convoAccess.js @@ -1,4 +1,5 @@ const { isEnabled } = require('@librechat/api'); +const { logger } = require('@librechat/data-schemas'); const { Constants, ViolationTypes, Time } = require('librechat-data-provider'); const denyRequest = require('~/server/middleware/denyRequest'); const { logViolation, getLogStores } = require('~/cache'); @@ -51,9 +52,13 @@ const validateConvoAccess = async (req, res, next) => { } } - const conversation = await searchConversation(conversationId); + /** One read serves the subagent guard, agent initialization, and the first save via + * `req.resolvedConversation`. `messages` is the only unbounded field and no consumer + * reads it, so it stays excluded — ownership is not yet known at this point. */ + const conversation = await searchConversation(conversationId, '-messages'); if (!conversation) { + req.resolvedConversation = null; return next(); } @@ -70,8 +75,13 @@ const validateConvoAccess = async (req, res, next) => { } if (cache) { - await cache.set(key, 'authorized', Time.TEN_MINUTES); + /** The marker only short-circuits the next check; the violations store is file-backed + * without Redis and its debounced write takes ~100ms, so it must not gate this request. */ + cache.set(key, 'authorized', Time.TEN_MINUTES).catch((error) => { + logger.warn('[validateConvoAccess] Failed to cache conversation access', error); + }); } + req.resolvedConversation = conversation; next(); } catch (error) { console.error('Error validating conversation access:', error); diff --git a/api/server/middleware/validate/convoAccess.spec.js b/api/server/middleware/validate/convoAccess.spec.js new file mode 100644 index 0000000000..49a751e7eb --- /dev/null +++ b/api/server/middleware/validate/convoAccess.spec.js @@ -0,0 +1,126 @@ +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { ViolationTypes } = require('librechat-data-provider'); + +const mockCache = { + get: jest.fn(), + set: jest.fn(), +}; + +jest.mock('~/cache', () => ({ + getLogStores: jest.fn(() => mockCache), + logViolation: jest.fn(), +})); + +jest.mock('~/server/middleware/denyRequest', () => jest.fn(async () => undefined)); + +const denyRequest = require('~/server/middleware/denyRequest'); +const { Conversation } = require('~/db/models'); +const validateConvoAccess = require('./convoAccess'); + +const OWNER_ID = new mongoose.Types.ObjectId().toString(); +const OTHER_ID = new mongoose.Types.ObjectId().toString(); +const CONVERSATION_ID = 'conversation-under-test'; + +function createRequest(userId, conversationId) { + return { + user: { id: userId }, + body: { conversationId, text: 'hello' }, + }; +} + +describe('validateConvoAccess', () => { + let mongoServer; + let res; + let next; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + await Conversation.create({ + conversationId: CONVERSATION_ID, + user: OWNER_ID, + endpoint: 'agents', + title: 'Owned conversation', + files: ['file-1', 'file-2'], + messages: Array.from({ length: 50 }, () => new mongoose.Types.ObjectId()), + }); + }); + + afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); + }); + + beforeEach(() => { + mockCache.get.mockReset().mockResolvedValue(undefined); + mockCache.set.mockReset().mockResolvedValue(true); + denyRequest.mockClear(); + res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + next = jest.fn(); + }); + + it('stashes the full conversation document for downstream readers when access is granted', async () => { + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(denyRequest).not.toHaveBeenCalled(); + expect(req.resolvedConversation).toMatchObject({ + conversationId: CONVERSATION_ID, + user: OWNER_ID, + title: 'Owned conversation', + files: ['file-1', 'file-2'], + }); + expect(req.resolvedConversation).not.toHaveProperty('messages'); + }); + + it('stashes null when the conversation does not exist so later readers skip their own lookup', async () => { + const req = createRequest(OWNER_ID, 'never-created'); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(true); + expect(req.resolvedConversation).toBeNull(); + }); + + it("denies another user's conversation without exposing the document on the request", async () => { + const req = createRequest(OTHER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(denyRequest).toHaveBeenCalledTimes(1); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(false); + }); + + it('does not wait for the access marker to be written before continuing', async () => { + mockCache.set.mockImplementation(() => new Promise(() => undefined)); + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(mockCache.set).toHaveBeenCalledWith( + expect.stringContaining(`${OWNER_ID}:${CONVERSATION_ID}`), + 'authorized', + expect.any(Number), + ); + }); + + it('skips the database entirely when access is already cached', async () => { + mockCache.get.mockResolvedValue('authorized'); + const findOne = jest.spyOn(Conversation, 'findOne'); + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(findOne).not.toHaveBeenCalled(); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(false); + expect(require('~/cache').getLogStores).toHaveBeenCalledWith(ViolationTypes.CONVO_ACCESS); + findOne.mockRestore(); + }); +}); diff --git a/api/server/routes/agents/v1.js b/api/server/routes/agents/v1.js index 1977ee2875..0a6487b574 100644 --- a/api/server/routes/agents/v1.js +++ b/api/server/routes/agents/v1.js @@ -1,7 +1,7 @@ const express = require('express'); const { generateCheckAccess } = require('@librechat/api'); const { PermissionTypes, Permissions, PermissionBits } = require('librechat-data-provider'); -const { requireJwtAuth, configMiddleware, canAccessAgentResource } = require('~/server/middleware'); +const { configMiddleware, canAccessAgentResource } = require('~/server/middleware'); const v1 = require('~/server/controllers/agents/v1'); const { getRoleByName } = require('~/models'); const actions = require('./actions'); @@ -21,8 +21,6 @@ const checkAgentCreate = generateCheckAccess({ getRoleByName, }); -router.use(requireJwtAuth); - /** * Agent actions route. * @route GET|POST /agents/actions diff --git a/packages/api/src/agents/guard.spec.ts b/packages/api/src/agents/guard.spec.ts index 8ad2979ba7..07cf516ea3 100644 --- a/packages/api/src/agents/guard.spec.ts +++ b/packages/api/src/agents/guard.spec.ts @@ -52,6 +52,7 @@ function createApp( store: SubagentThreadTaskStore, getEventBinding?: AllMethods['getAgentEventBinding'], isHumanResumeAllowed?: () => Promise, + preResolved?: { conversation: IConversation | null }, ) { const app = express(); app.use(express.json()); @@ -59,6 +60,10 @@ function createApp( req.user = { id: 'user-1', tenantId: 'tenant-1' }; (req as typeof req & { _isAgentTrigger?: boolean })._isAgentTrigger = req.get('x-test-trigger') === '1'; + if (preResolved) { + (req as typeof req & { resolvedConversation?: IConversation | null }).resolvedConversation = + preResolved.conversation; + } next(); }); const guard = createSubagentThreadTurnGuard({ @@ -110,6 +115,39 @@ describe('subagent child-thread write policy', () => { expect(getConvo).toHaveBeenCalledTimes(1); }); + it('reuses a conversation an earlier middleware already read instead of re-reading it', async () => { + const getConvo = jest.fn(); + const store = makeStore(); + + const ordinary = await request( + createApp(getConvo, store, undefined, undefined, { + conversation: { + conversationId: 'ordinary-conversation', + endpoint: 'agents', + } as IConversation, + }), + ) + .post('/chat') + .send({ conversationId: 'ordinary-conversation' }); + const child = await request( + createApp(getConvo, store, undefined, undefined, { conversation: childConversation() }), + ) + .post('/chat') + .send({ conversationId: 'child-conversation', agent_id: 'child-agent' }); + const absent = await request( + createApp(getConvo, store, undefined, undefined, { conversation: null }), + ) + .post('/chat') + .send({ conversationId: 'missing-conversation' }); + + expect(ordinary.status).toBe(200); + expect(ordinary.body).toEqual({ ok: true, resolvedConversationId: 'ordinary-conversation' }); + expect(child.status).toBe(409); + expect(child.body).toEqual({ error: CHILD_THREAD_READ_ONLY_ERROR }); + expect(absent.status).toBe(200); + expect(getConvo).not.toHaveBeenCalled(); + }); + it('rejects every model-bound human turn against a durable child conversation', async () => { const store = makeStore(); const response = await request( diff --git a/packages/api/src/agents/guard.ts b/packages/api/src/agents/guard.ts index 94d69f2460..51dab509d7 100644 --- a/packages/api/src/agents/guard.ts +++ b/packages/api/src/agents/guard.ts @@ -30,6 +30,8 @@ export interface SubagentThreadWriteTarget { userId: string; conversationId: string; tenantId?: string; + /** Conversation already read earlier in the request (`null` = looked up, absent). */ + conversation?: IConversation | null; } interface SubagentThreadWriteResolution { @@ -112,18 +114,23 @@ async function isBoundEventContinuation( async function resolveSubagentThreadWrite( { getConvo, store }: SubagentThreadWriteGuardDeps, - { userId, conversationId, tenantId }: SubagentThreadWriteTarget, + target: SubagentThreadWriteTarget, ): Promise { + const { userId, conversationId, tenantId } = target; + const readConversation = (): Promise => + target.conversation !== undefined + ? Promise.resolve(target.conversation) + : getConvo(userId, conversationId); /** New child IDs are returned synchronously by the SDK before Mongo creation can * finish. Their reserved UUID namespace closes that brief window on every replica. */ if (isReservedSubagentThreadId(conversationId)) { - const conversation = await getConvo(userId, conversationId); + const conversation = await readConversation(); return { blocked: true, conversation }; } if (store.isThreadActiveForOwner(userId, conversationId, tenantId)) { return { blocked: true }; } - const conversation = await getConvo(userId, conversationId); + const conversation = await readConversation(); return { blocked: conversation?.subagentThread != null, conversation }; } @@ -159,10 +166,14 @@ export function createSubagentThreadTurnGuard(deps: SubagentThreadWriteGuardDeps typeof user?.tenantId === 'string' && user.tenantId !== '' ? user.tenantId : undefined; try { + const resolvedRequest = request as ResolvedConversationRequest; const resolved = await resolveSubagentThreadWrite(deps, { userId, conversationId: candidateConversationId, ...(tenantId == null ? {} : { tenantId }), + ...(Object.prototype.hasOwnProperty.call(request, 'resolvedConversation') + ? { conversation: resolvedRequest.resolvedConversation } + : {}), }); if (resolved.conversation !== undefined) { (request as ResolvedConversationRequest).resolvedConversation = resolved.conversation; @@ -171,7 +182,6 @@ export function createSubagentThreadTurnGuard(deps: SubagentThreadWriteGuardDeps next(); return; } - const resolvedRequest = request as ResolvedConversationRequest; const resolvedConversation = resolved.conversation; const lineage = resolvedConversation?.subagentThread; const humanResume = deps.isHumanResumeAllowed; diff --git a/packages/api/src/agents/initialize.files.spec.ts b/packages/api/src/agents/initialize.files.spec.ts new file mode 100644 index 0000000000..9cf553641b --- /dev/null +++ b/packages/api/src/agents/initialize.files.spec.ts @@ -0,0 +1,45 @@ +import { readResolvedConversationFiles } from './initialize'; + +describe('readResolvedConversationFiles', () => { + const conversationId = 'conversation-1'; + + it('leaves the database read in place when no middleware resolved the conversation', () => { + expect(readResolvedConversationFiles({}, conversationId)).toBeUndefined(); + }); + + it('reports no files when the conversation was looked up and does not exist', () => { + expect(readResolvedConversationFiles({ resolvedConversation: null }, conversationId)).toEqual( + [], + ); + }); + + it('uses the resolved document when it carries the files field', () => { + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, files: ['file-1'] } }, + conversationId, + ), + ).toEqual(['file-1']); + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, files: [] } }, + conversationId, + ), + ).toEqual([]); + }); + + it('falls back to the database when the resolved document omits files or is another conversation', () => { + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, agent_id: 'child-agent' } }, + conversationId, + ), + ).toBeUndefined(); + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId: 'other', files: ['file-1'] } }, + conversationId, + ), + ).toBeUndefined(); + }); +}); diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index fffa9d93c5..c831f34e20 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -188,6 +188,32 @@ function appendAdditionalInstructions(agent: Agent, text?: string | null): void .join('\n\n'); } +/** + * The request middleware already read this conversation once (`null` = looked up, absent). + * Only a resolved document that actually carries `files` can stand in for the database: + * bound agent-event continuations stash a partial document built from lineage alone. + */ +export function readResolvedConversationFiles( + req: Pick, + conversationId: string, +): string[] | undefined { + if (!Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')) { + return undefined; + } + const resolved = req.resolvedConversation; + if (resolved === null) { + return []; + } + if ( + resolved == null || + resolved.conversationId !== conversationId || + !Object.prototype.hasOwnProperty.call(resolved, 'files') + ) { + return undefined; + } + return resolved.files ?? []; +} + function getMaxCatalogSkills(req: ServerRequest): number | undefined { const endpoints = req.config?.endpoints as | Record @@ -987,7 +1013,7 @@ export async function initializeAgent( * every code-output ref. */ const [convoFileIds, threadMessages] = await Promise.all([ - db.getConvoFiles(conversationId), + readResolvedConversationFiles(req, conversationId) ?? db.getConvoFiles(conversationId), needsThreadWalk && getThreadMessages ? getThreadMessages({ conversationId }, 'messageId parentMessageId files attachments') : null, diff --git a/packages/api/src/types/http.ts b/packages/api/src/types/http.ts index b3a34df383..df1d6893e2 100644 --- a/packages/api/src/types/http.ts +++ b/packages/api/src/types/http.ts @@ -1,5 +1,5 @@ -import type { TConversation, TEndpointOption } from 'librechat-data-provider'; -import type { IUser, AppConfig } from '@librechat/data-schemas'; +import type { IUser, AppConfig, IConversation } from '@librechat/data-schemas'; +import type { TEndpointOption } from 'librechat-data-provider'; import type { Request } from 'express'; /** @@ -25,8 +25,9 @@ export type ServerRequest = Request & { config?: AppConfig; /** Server-captured conversation creation time used to anchor dynamic prompt variables. */ conversationCreatedAt?: string; - /** Conversation loaded while resolving the prompt timestamp anchor, reused by save logic. */ - resolvedConversation?: Partial | null; + /** Conversation read by request middleware (`null` = looked up, absent), reused by the + * subagent guard, agent initialization, and the first save instead of re-reading it. */ + resolvedConversation?: Partial | null; /** Passport strategy that populated req.user for this request. */ authStrategy?: string; }; diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index b09cbca203..f756075f80 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -115,7 +115,10 @@ async function refreshChatProjectStatsInBatches( export interface ConversationMethods { getConvoFiles(conversationId: string): Promise; - searchConversation(conversationId: string): Promise; + searchConversation( + conversationId: string, + fieldsToSelect?: string | null, + ): Promise; deleteNullOrEmptyConversations(): Promise<{ conversations: { deletedCount?: number }; messages: { deletedCount?: number }; @@ -259,13 +262,14 @@ export function createConversationMethods( /** * Searches for a conversation by conversationId and returns a lean document with only conversationId and user. */ - async function searchConversation(conversationId: string) { + /** `fieldsToSelect: null` returns the full document so one read can serve the whole request. */ + async function searchConversation( + conversationId: string, + fieldsToSelect: string | null = 'conversationId user', + ) { try { const Conversation = mongoose.models.Conversation as Model; - return await Conversation.findOne( - { conversationId }, - 'conversationId user', - ).lean(); + return await Conversation.findOne({ conversationId }, fieldsToSelect).lean(); } catch (error) { logger.error('[searchConversation] Error searching conversation', error); throw new Error('Error searching conversation');