mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 12:13:30 +00:00
⚡ perf: Append Saved Message Ids Instead of Rebuilding the Conversation Array (#15141)
* ⚡ perf: Append Saved Message Ids Instead of Rebuilding the Conversation Array Every saveConvo read every message id in the conversation (sorted) and wrote the array back onto the document — twice per chat turn, O(n) in conversation length, from a write path. The turn's savers know exactly which message they just wrote, so they now pass it as metadata.appendMessageIds and saveConvo $addToSet-s it, skipping the read and the full-array rewrite. Every save without the option — titles, archive, fork, import, threads — still rebuilds from the database, which remains the heal point for the drift that message deletion has always left behind (deletes never ran saveConvo). The array's consumers read presence or length, or use it as an optimistic cache placeholder, so incremental maintenance is behaviorally identical; on traced turns the array stays exactly equal to the messages collection. Per-turn queries: 15 -> 13 (two Message.find gone), and the growing array payload no longer crosses the wire twice per turn. * 🎯 fix: Brand the Lineage-Only Resolved Conversation Instead of Guessing by Shape The resolved-conversation files fast path treated an absent files property as unresolved so the lineage-only partial from a bound agent-event continuation could not silently hide a conversation's uploads. But MongoDB never stores an empty files array, so nearly every real conversation also lacks the property and the fast path never fired — a follow-up turn on an upload-free conversation still paid the getConvoFiles round trip. The synthesized partial is the one object that cannot speak for the database, so it now carries an explicit symbol brand (PARTIAL_RESOLVED_CONVERSATION, non-serializing and invisible to key iteration), and a stored document without files means what it means: no files. Traced follow-up turns drop from 14 queries to 13. * 🧪 test: Expect the Appended Message Id in the Route's saveConvo Metadata messages-get.spec.js pins the exact metadata POST /api/messages passes to saveConvo; the route now forwards the saved message's _id as appendMessageIds, which is the behavior the append path depends on.
This commit is contained in:
parent
9da51cb507
commit
d864597731
9 changed files with 159 additions and 8 deletions
|
|
@ -1315,6 +1315,7 @@ class BaseClient {
|
|||
unsetFields,
|
||||
noUpsert: req?._agentEventBindingParentConversationId != null,
|
||||
createdAtOnInsert: shouldSetCreatedAtOnInsert ? validCreatedAtOnInsert : undefined,
|
||||
...(savedMessage?._id != null ? { appendMessageIds: [savedMessage._id] } : {}),
|
||||
});
|
||||
|
||||
return { message: savedMessage, conversation };
|
||||
|
|
|
|||
|
|
@ -1871,6 +1871,36 @@ describe('BaseClient', () => {
|
|||
);
|
||||
});
|
||||
|
||||
test('saveMessageToDatabase appends the saved message id instead of rebuilding the array', async () => {
|
||||
const savedId = new (require('mongoose').Types.ObjectId)();
|
||||
saveMessage.mockResolvedValueOnce({ _id: savedId, messageId: 'saved-1' });
|
||||
saveConvo.mockResolvedValueOnce({ conversationId });
|
||||
|
||||
await TestClient.saveMessageToDatabase(
|
||||
{ messageId: 'saved-1', conversationId, text: 'hi' },
|
||||
TestClient.getSaveOptions(),
|
||||
);
|
||||
|
||||
expect(saveConvo).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.anything(),
|
||||
expect.objectContaining({ appendMessageIds: [savedId] }),
|
||||
);
|
||||
});
|
||||
|
||||
test('saveMessageToDatabase rebuilds the array when the saved message has no _id', async () => {
|
||||
saveMessage.mockResolvedValueOnce({ messageId: 'saved-2' });
|
||||
saveConvo.mockResolvedValueOnce({ conversationId });
|
||||
|
||||
await TestClient.saveMessageToDatabase(
|
||||
{ messageId: 'saved-2', conversationId, text: 'hi' },
|
||||
TestClient.getSaveOptions(),
|
||||
);
|
||||
|
||||
const metadata = saveConvo.mock.calls[saveConvo.mock.calls.length - 1][2];
|
||||
expect(metadata).not.toHaveProperty('appendMessageIds');
|
||||
});
|
||||
|
||||
test('saveMessageToDatabase returns early when this.options is null (client disposed)', async () => {
|
||||
const savedOptions = TestClient.options;
|
||||
TestClient.options = null;
|
||||
|
|
|
|||
|
|
@ -443,7 +443,10 @@ describe('message route conversation ownership filters', () => {
|
|||
model: savedMessage.model,
|
||||
iconURL: savedMessage.iconURL,
|
||||
},
|
||||
{ context: 'POST /api/messages/:conversationId' },
|
||||
{
|
||||
context: 'POST /api/messages/:conversationId',
|
||||
appendMessageIds: [savedMessage._id],
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -504,6 +504,7 @@ router.post('/:conversationId', storedMessageMutationMiddleware, async (req, res
|
|||
};
|
||||
await db.saveConvo(reqCtx, conversationUpdate, {
|
||||
context: 'POST /api/messages/:conversationId',
|
||||
...(savedMessage._id != null ? { appendMessageIds: [savedMessage._id] } : {}),
|
||||
});
|
||||
res.status(201).json(savedMessage);
|
||||
} catch (error) {
|
||||
|
|
|
|||
|
|
@ -51,6 +51,15 @@ interface ResolvedConversationRequest extends Request {
|
|||
_agentEventBindingTenantId?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Brands the lineage-only conversation `isBoundEventContinuation` synthesizes: it stands in
|
||||
* for binding checks but carries none of the stored document's optional fields, so readers
|
||||
* of `req.resolvedConversation` must not treat its absent fields as authoritative.
|
||||
*/
|
||||
export const PARTIAL_RESOLVED_CONVERSATION: unique symbol = Symbol.for(
|
||||
'librechat.resolvedConversation.partial',
|
||||
);
|
||||
|
||||
function applyEventBindingContext(
|
||||
request: ResolvedConversationRequest,
|
||||
conversation: IConversation,
|
||||
|
|
@ -102,6 +111,7 @@ async function isBoundEventContinuation(
|
|||
return null;
|
||||
}
|
||||
return {
|
||||
[PARTIAL_RESOLVED_CONVERSATION]: true,
|
||||
conversationId: binding.conversationId,
|
||||
agent_id: binding.agentId,
|
||||
...(binding.tenantId == null ? {} : { tenantId: binding.tenantId }),
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
import type { IConversation } from '@librechat/data-schemas';
|
||||
import { readResolvedConversationFiles } from './initialize';
|
||||
import { PARTIAL_RESOLVED_CONVERSATION } from './guard';
|
||||
|
||||
describe('readResolvedConversationFiles', () => {
|
||||
const conversationId = 'conversation-1';
|
||||
|
|
@ -28,12 +30,23 @@ describe('readResolvedConversationFiles', () => {
|
|||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('falls back to the database when the resolved document omits files or is another conversation', () => {
|
||||
it('treats a stored document without files as having none', () => {
|
||||
expect(
|
||||
readResolvedConversationFiles(
|
||||
{ resolvedConversation: { conversationId, agent_id: 'child-agent' } },
|
||||
{ resolvedConversation: { conversationId, title: 'no uploads yet' } },
|
||||
conversationId,
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('falls back to the database for a branded lineage-only partial or another conversation', () => {
|
||||
const lineageOnly = {
|
||||
[PARTIAL_RESOLVED_CONVERSATION]: true,
|
||||
conversationId,
|
||||
agent_id: 'child-agent',
|
||||
} as unknown as IConversation;
|
||||
expect(
|
||||
readResolvedConversationFiles({ resolvedConversation: lineageOnly }, conversationId),
|
||||
).toBeUndefined();
|
||||
expect(
|
||||
readResolvedConversationFiles(
|
||||
|
|
|
|||
|
|
@ -94,6 +94,7 @@ import { assertModelBoundContent } from '../middleware/modelBoundContent';
|
|||
import { registerMemoryTools, memoryToolUsageGuard } from './memory';
|
||||
import { applyIntentLabels, sanitizeIntentLabels } from './intent';
|
||||
import { ContentFilterError } from '../middleware/contentFilter';
|
||||
import { PARTIAL_RESOLVED_CONVERSATION } from './guard';
|
||||
import { applyBackgroundToolCalls } from './background';
|
||||
import { filterFilesByEndpointConfig } from '~/files';
|
||||
import { generateArtifactsPrompt } from '~/prompts';
|
||||
|
|
@ -190,8 +191,8 @@ function appendAdditionalInstructions(agent: Agent, text?: string | null): void
|
|||
|
||||
/**
|
||||
* 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.
|
||||
* A stored document without `files` genuinely has none; only the branded lineage-only
|
||||
* partial from a bound agent-event continuation cannot speak for the database.
|
||||
*/
|
||||
export function readResolvedConversationFiles(
|
||||
req: Pick<ServerRequest, 'resolvedConversation'>,
|
||||
|
|
@ -207,7 +208,7 @@ export function readResolvedConversationFiles(
|
|||
if (
|
||||
resolved == null ||
|
||||
resolved.conversationId !== conversationId ||
|
||||
!Object.prototype.hasOwnProperty.call(resolved, 'files')
|
||||
(resolved as Record<symbol, unknown>)[PARTIAL_RESOLVED_CONVERSATION] === true
|
||||
) {
|
||||
return undefined;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -961,6 +961,85 @@ describe('Conversation Operations', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('saveConvo appendMessageIds', () => {
|
||||
const ctx = { userId: 'append-user' };
|
||||
const conversationId = 'append-conversation';
|
||||
|
||||
beforeEach(async () => {
|
||||
await Conversation.deleteMany({ user: ctx.userId });
|
||||
getMessages.mockClear();
|
||||
});
|
||||
|
||||
it('appends the provided ids without reading the messages collection', async () => {
|
||||
const seeded = [new mongoose.Types.ObjectId(), new mongoose.Types.ObjectId()];
|
||||
getMessages.mockResolvedValueOnce(seeded.map((_id) => ({ _id })));
|
||||
await saveConvo(ctx, { conversationId, title: 'seeded' });
|
||||
expect(getMessages).toHaveBeenCalledTimes(1);
|
||||
|
||||
const appended = new mongoose.Types.ObjectId();
|
||||
const result = await saveConvo(
|
||||
ctx,
|
||||
{ conversationId, title: 'appended' },
|
||||
{ appendMessageIds: [appended] },
|
||||
);
|
||||
|
||||
expect(getMessages).toHaveBeenCalledTimes(1);
|
||||
expect(result?.title).toBe('appended');
|
||||
const stored = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(stored?.messages?.map(String)).toEqual([...seeded, appended].map(String));
|
||||
});
|
||||
|
||||
it('does not duplicate an id that is already recorded', async () => {
|
||||
const id = new mongoose.Types.ObjectId();
|
||||
await saveConvo(ctx, { conversationId }, { appendMessageIds: [id] });
|
||||
await saveConvo(ctx, { conversationId }, { appendMessageIds: [id] });
|
||||
|
||||
const stored = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(stored?.messages?.map(String)).toEqual([String(id)]);
|
||||
expect(getMessages).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('creates the conversation with the appended id when it does not exist yet', async () => {
|
||||
const id = new mongoose.Types.ObjectId();
|
||||
const result = await saveConvo(
|
||||
ctx,
|
||||
{ conversationId, title: 'first turn' },
|
||||
{ appendMessageIds: [id] },
|
||||
);
|
||||
|
||||
expect(result?.conversationId).toBe(conversationId);
|
||||
const stored = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(stored?.messages?.map(String)).toEqual([String(id)]);
|
||||
expect(getMessages).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('ignores a caller-supplied messages field so $set cannot conflict with the append', async () => {
|
||||
const kept = new mongoose.Types.ObjectId();
|
||||
await saveConvo(ctx, { conversationId }, { appendMessageIds: [kept] });
|
||||
|
||||
const appended = new mongoose.Types.ObjectId();
|
||||
await saveConvo(
|
||||
ctx,
|
||||
{ conversationId, messages: [new mongoose.Types.ObjectId()] },
|
||||
{ appendMessageIds: [appended] },
|
||||
);
|
||||
|
||||
const stored = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(stored?.messages?.map(String)).toEqual([kept, appended].map(String));
|
||||
});
|
||||
|
||||
it('still rebuilds the array from the database when the option is absent', async () => {
|
||||
const rebuilt = [new mongoose.Types.ObjectId()];
|
||||
getMessages.mockResolvedValueOnce(rebuilt.map((_id) => ({ _id })));
|
||||
|
||||
await saveConvo(ctx, { conversationId, title: 'rebuild' });
|
||||
|
||||
expect(getMessages).toHaveBeenCalledWith({ conversationId, user: ctx.userId }, '_id');
|
||||
const stored = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(stored?.messages?.map(String)).toEqual(rebuilt.map(String));
|
||||
});
|
||||
});
|
||||
|
||||
describe('isTemporary conversation handling', () => {
|
||||
it('should save a conversation with expiredAt when isTemporary is true', async () => {
|
||||
mockCtx.interfaceConfig = { temporaryChatRetention: 24 };
|
||||
|
|
|
|||
|
|
@ -137,6 +137,10 @@ export interface ConversationMethods {
|
|||
noUpsert?: boolean;
|
||||
createdAtOnInsert?: Date;
|
||||
preserveUpdatedAt?: boolean;
|
||||
/** `_id`s of messages this save just wrote. When present, they are appended with
|
||||
* `$addToSet` and the O(n) read-and-rewrite of the `messages` array is skipped;
|
||||
* every save without this option still rebuilds the array from the database. */
|
||||
appendMessageIds?: Types.ObjectId[];
|
||||
},
|
||||
): Promise<IConversation | { message: string } | null>;
|
||||
setConvoPinned(
|
||||
|
|
@ -649,6 +653,7 @@ export function createConversationMethods(
|
|||
noUpsert?: boolean;
|
||||
createdAtOnInsert?: Date;
|
||||
preserveUpdatedAt?: boolean;
|
||||
appendMessageIds?: Types.ObjectId[];
|
||||
},
|
||||
) {
|
||||
try {
|
||||
|
|
@ -659,8 +664,13 @@ export function createConversationMethods(
|
|||
logger.debug(`[saveConvo] ${metadata.context}`);
|
||||
}
|
||||
|
||||
const messages = await getMessages({ conversationId, user: userId }, '_id');
|
||||
const update: Record<string, unknown> = { ...convo, messages, user: userId };
|
||||
const appendMessageIds = metadata?.appendMessageIds;
|
||||
const update: Record<string, unknown> = { ...convo, user: userId };
|
||||
if (appendMessageIds == null) {
|
||||
update.messages = await getMessages({ conversationId, user: userId }, '_id');
|
||||
} else {
|
||||
delete update.messages;
|
||||
}
|
||||
const unsetFields: Record<string, number> = { ...(metadata?.unsetFields ?? {}) };
|
||||
|
||||
if (Object.prototype.hasOwnProperty.call(update, 'chatProjectId') && update.chatProjectId) {
|
||||
|
|
@ -750,6 +760,9 @@ export function createConversationMethods(
|
|||
|
||||
const buildOperation = (setFields: Record<string, unknown>) => {
|
||||
const operation: Record<string, unknown> = { $set: setFields };
|
||||
if (appendMessageIds != null && appendMessageIds.length > 0) {
|
||||
operation.$addToSet = { messages: { $each: appendMessageIds } };
|
||||
}
|
||||
if (Object.keys(unsetFields).length > 0) {
|
||||
operation.$unset = unsetFields;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue