mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-30 06:47:42 +00:00
🧵 fix: Close Child-Thread Read and Search-Cleanup Gaps (#15055)
* fix: close child thread read and cleanup gaps * fix: preserve child search cleanup invariants * test: complete mocked update result * perf: parallelize scoped message reads * fix: close child thread compatibility gaps * test: expect preserved cleanup failure * fix: reconcile legacy Meili cleanup markers
This commit is contained in:
parent
c7e355b219
commit
d0f9d5625e
14 changed files with 732 additions and 115 deletions
|
|
@ -89,6 +89,48 @@ describe('validateMessageReq', () => {
|
|||
expect(next).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('returns not found for a direct child-thread message read', async () => {
|
||||
const req = {
|
||||
method: 'GET',
|
||||
params: { conversationId: 'child-convo', messageId: 'child-message' },
|
||||
body: {},
|
||||
user: { id: userId },
|
||||
};
|
||||
const res = createResponse();
|
||||
const next = jest.fn();
|
||||
getConvoOwnership.mockResolvedValue({
|
||||
user: userId,
|
||||
subagentThread: { parentConversationId: 'parent-convo' },
|
||||
});
|
||||
|
||||
await validateMessageReq(req, res, next);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(404);
|
||||
expect(res.json).toHaveBeenCalledWith({ error: 'Conversation not found' });
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns not found for a HEAD request to a child thread', async () => {
|
||||
const req = {
|
||||
method: 'HEAD',
|
||||
params: { conversationId: 'child-convo' },
|
||||
body: {},
|
||||
user: { id: userId },
|
||||
};
|
||||
const res = createResponse();
|
||||
const next = jest.fn();
|
||||
getConvoOwnership.mockResolvedValue({
|
||||
user: userId,
|
||||
subagentThread: { parentConversationId: 'parent-convo' },
|
||||
});
|
||||
|
||||
await validateMessageReq(req, res, next);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(404);
|
||||
expect(res.json).toHaveBeenCalledWith({ error: 'Conversation not found' });
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should allow message reads for an owned active generation job before the conversation is saved', async () => {
|
||||
const req = {
|
||||
method: 'GET',
|
||||
|
|
|
|||
|
|
@ -4,7 +4,11 @@ const validateRegistration = require('./validateRegistration');
|
|||
const buildEndpointOption = require('./buildEndpointOption');
|
||||
const validateEmailLogin = require('./validateEmailLogin');
|
||||
const validateMessageReq = require('./validateMessageReq');
|
||||
const { prepareMessageRequestValidation, sendValidationResponse } = require('./messageValidation');
|
||||
const {
|
||||
canReadActiveJobConversation,
|
||||
prepareMessageRequestValidation,
|
||||
sendValidationResponse,
|
||||
} = require('./messageValidation');
|
||||
const checkDomainAllowed = require('./checkDomainAllowed');
|
||||
const requireLocalAuth = require('./requireLocalAuth');
|
||||
const canDeleteAccount = require('./canDeleteAccount');
|
||||
|
|
@ -49,6 +53,7 @@ module.exports = {
|
|||
configMiddleware,
|
||||
checkDomainAllowed,
|
||||
validateMessageReq,
|
||||
canReadActiveJobConversation,
|
||||
sendValidationResponse,
|
||||
prepareMessageRequestValidation,
|
||||
buildEndpointOption,
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ jest.mock('~/server/services/Endpoints/agents/subagentThreadStore', () =>
|
|||
describe('Convos Routes', () => {
|
||||
let app;
|
||||
let convosRouter;
|
||||
const { deleteToolCalls, deleteConvos, saveConvo } = require('~/models');
|
||||
const { deleteToolCalls, deleteConvos, getConvo, saveConvo } = require('~/models');
|
||||
const {
|
||||
deleteAgentCheckpoints,
|
||||
deleteAllSharedLinksWithCleanup,
|
||||
|
|
@ -52,6 +52,35 @@ describe('Convos Routes', () => {
|
|||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('GET /:conversationId', () => {
|
||||
it('returns an ordinary owned conversation', async () => {
|
||||
getConvo.mockResolvedValue({ conversationId: 'ordinary', title: 'Ordinary' });
|
||||
|
||||
const response = await request(app).get('/api/convos/ordinary');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual({ conversationId: 'ordinary', title: 'Ordinary' });
|
||||
expect(getConvo).toHaveBeenCalledWith('test-user-123', 'ordinary');
|
||||
});
|
||||
|
||||
it('returns the same not-found response for an owned child thread', async () => {
|
||||
getConvo.mockResolvedValue({
|
||||
conversationId: 'child',
|
||||
subagentThread: { parentConversationId: 'parent' },
|
||||
});
|
||||
|
||||
const childResponse = await request(app).get('/api/convos/child');
|
||||
getConvo.mockResolvedValue(null);
|
||||
const missingResponse = await request(app).get('/api/convos/missing');
|
||||
|
||||
expect(childResponse.status).toBe(404);
|
||||
expect(childResponse.text).toBe('');
|
||||
expect(childResponse.status).toBe(missingResponse.status);
|
||||
expect(childResponse.text).toBe(missingResponse.text);
|
||||
expect(getConvo).toHaveBeenNthCalledWith(1, 'test-user-123', 'child');
|
||||
});
|
||||
});
|
||||
|
||||
describe('DELETE /all', () => {
|
||||
it('prunes the deleted conversations’ agent checkpoints (bulk, ids from deleteConvos)', async () => {
|
||||
// HITL: a paused conversation's durable checkpoint must not outlive the conversation.
|
||||
|
|
|
|||
|
|
@ -169,4 +169,30 @@ describe('GET /api/messages/:conversationId with real validation middleware', ()
|
|||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual([{ messageId: 'message-1', conversationId: 'convo-1' }]);
|
||||
});
|
||||
|
||||
it('does not return messages for a directly addressed child thread', async () => {
|
||||
getConvoOwnership.mockResolvedValue({
|
||||
conversationId: 'child-convo',
|
||||
user: authenticatedUserId,
|
||||
subagentThread: { parentConversationId: 'parent-convo' },
|
||||
});
|
||||
getMessages.mockResolvedValue([{ messageId: 'child-message', conversationId: 'child-convo' }]);
|
||||
|
||||
const response = await request(app).get('/api/messages/child-convo');
|
||||
|
||||
expect(response.status).toBe(404);
|
||||
expect(response.body).toEqual({ error: 'Conversation not found' });
|
||||
});
|
||||
|
||||
it('does not expose a directly addressed child thread through HEAD', async () => {
|
||||
getConvoOwnership.mockResolvedValue({
|
||||
conversationId: 'child-convo',
|
||||
user: authenticatedUserId,
|
||||
subagentThread: { parentConversationId: 'parent-convo' },
|
||||
});
|
||||
|
||||
const response = await request(app).head('/api/messages/child-convo');
|
||||
|
||||
expect(response.status).toBe(404);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -36,6 +36,7 @@ jest.mock('~/models', () => ({
|
|||
getMessage: jest.fn(),
|
||||
saveMessage: jest.fn(),
|
||||
getMessages: jest.fn(),
|
||||
getConvoOwnership: jest.fn(),
|
||||
updateMessage: jest.fn(),
|
||||
deleteMessages: jest.fn(),
|
||||
getConvosQueried: jest.fn(),
|
||||
|
|
@ -52,6 +53,7 @@ jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next()
|
|||
|
||||
jest.mock('~/server/middleware', () => {
|
||||
const validateMessageReq = jest.fn((req, res, next) => next());
|
||||
const canReadActiveJobConversation = jest.fn().mockResolvedValue(false);
|
||||
const prepareMessageRequestValidation = jest.fn((req, res, next) => {
|
||||
req.messageRequestValidation = {
|
||||
conversationId: 'convo-1',
|
||||
|
|
@ -70,6 +72,7 @@ jest.mock('~/server/middleware', () => {
|
|||
return {
|
||||
requireJwtAuth: (req, res, next) => next(),
|
||||
validateMessageReq,
|
||||
canReadActiveJobConversation,
|
||||
sendValidationResponse,
|
||||
prepareMessageRequestValidation,
|
||||
configMiddleware: (req, res, next) => next(),
|
||||
|
|
@ -86,8 +89,17 @@ jest.mock('~/db/models', () => ({
|
|||
|
||||
describe('message route conversation ownership filters', () => {
|
||||
let app;
|
||||
const { getMessages, saveConvo, saveMessage } = require('~/models');
|
||||
const { prepareMessageRequestValidation } = require('~/server/middleware');
|
||||
const {
|
||||
getConvoOwnership,
|
||||
getMessages,
|
||||
getMessagesByCursor,
|
||||
saveConvo,
|
||||
saveMessage,
|
||||
} = require('~/models');
|
||||
const {
|
||||
canReadActiveJobConversation,
|
||||
prepareMessageRequestValidation,
|
||||
} = require('~/server/middleware');
|
||||
|
||||
const authenticatedUserId = 'user-owner-123';
|
||||
|
||||
|
|
@ -105,6 +117,7 @@ describe('message route conversation ownership filters', () => {
|
|||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
canReadActiveJobConversation.mockResolvedValue(false);
|
||||
prepareMessageRequestValidation.mockImplementation((req, res, next) => {
|
||||
req.messageRequestValidation = {
|
||||
conversationId: 'convo-1',
|
||||
|
|
@ -180,6 +193,111 @@ describe('message route conversation ownership filters', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('returns indistinguishable not-found responses for child and missing query reads', async () => {
|
||||
getConvoOwnership.mockResolvedValueOnce({
|
||||
user: authenticatedUserId,
|
||||
subagentThread: { parentConversationId: 'parent-convo' },
|
||||
});
|
||||
|
||||
const childResponse = await request(app).get(
|
||||
'/api/messages?conversationId=child-convo&messageId=child-message',
|
||||
);
|
||||
getConvoOwnership.mockResolvedValueOnce(null);
|
||||
const missingResponse = await request(app).get(
|
||||
'/api/messages?conversationId=missing-convo&messageId=missing-message',
|
||||
);
|
||||
|
||||
expect(childResponse.status).toBe(404);
|
||||
expect(childResponse.body).toEqual({ error: 'Conversation not found' });
|
||||
expect(childResponse.status).toBe(missingResponse.status);
|
||||
expect(childResponse.body).toEqual(missingResponse.body);
|
||||
expect(getMessages).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
name: 'single-message',
|
||||
path: '/api/messages?conversationId=convo-1&messageId=message-1',
|
||||
readMock: getMessages,
|
||||
readResult: [{ messageId: 'message-1', conversationId: 'convo-1' }],
|
||||
},
|
||||
{
|
||||
name: 'cursor',
|
||||
path: '/api/messages?conversationId=convo-1',
|
||||
readMock: getMessagesByCursor,
|
||||
readResult: { messages: [], nextCursor: null },
|
||||
},
|
||||
])(
|
||||
'starts the $name query read before ownership validation resolves',
|
||||
async ({ path, readMock, readResult }) => {
|
||||
const events = [];
|
||||
let resolveOwnership;
|
||||
const ownershipPromise = new Promise((resolve) => {
|
||||
resolveOwnership = resolve;
|
||||
});
|
||||
getConvoOwnership.mockImplementationOnce(() => {
|
||||
events.push('ownership-started');
|
||||
return ownershipPromise;
|
||||
});
|
||||
|
||||
let resolveReadStarted;
|
||||
const readStartedPromise = new Promise((resolve) => {
|
||||
resolveReadStarted = resolve;
|
||||
});
|
||||
readMock.mockImplementationOnce(() => {
|
||||
events.push('messages-started');
|
||||
resolveReadStarted();
|
||||
return Promise.resolve(readResult);
|
||||
});
|
||||
|
||||
const responsePromise = new Promise((resolve, reject) => {
|
||||
request(app)
|
||||
.get(path)
|
||||
.end((error, response) => (error ? reject(error) : resolve(response)));
|
||||
});
|
||||
|
||||
await Promise.race([readStartedPromise, new Promise((resolve) => setTimeout(resolve, 100))]);
|
||||
const eventsBeforeValidation = [...events];
|
||||
resolveOwnership({ user: authenticatedUserId });
|
||||
const response = await responsePromise;
|
||||
|
||||
expect(eventsBeforeValidation).toEqual(['ownership-started', 'messages-started']);
|
||||
expect(response.status).toBe(200);
|
||||
},
|
||||
);
|
||||
|
||||
it('allows an ordinary owned conversation query read', async () => {
|
||||
getConvoOwnership.mockResolvedValue({ user: authenticatedUserId });
|
||||
getMessages.mockResolvedValue([{ messageId: 'message-1', conversationId: 'convo-1' }]);
|
||||
|
||||
const response = await request(app).get(
|
||||
'/api/messages?conversationId=convo-1&messageId=message-1',
|
||||
);
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body.messages).toEqual([{ messageId: 'message-1', conversationId: 'convo-1' }]);
|
||||
});
|
||||
|
||||
it('allows an owner-scoped active generation before its conversation row exists', async () => {
|
||||
getConvoOwnership.mockResolvedValue(null);
|
||||
canReadActiveJobConversation.mockResolvedValue(true);
|
||||
getMessagesByCursor.mockResolvedValue({
|
||||
messages: [{ messageId: 'prompt-1', conversationId: 'active-convo' }],
|
||||
nextCursor: null,
|
||||
});
|
||||
|
||||
const response = await request(app).get('/api/messages?conversationId=active-convo');
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body.messages).toEqual([
|
||||
{ messageId: 'prompt-1', conversationId: 'active-convo' },
|
||||
]);
|
||||
expect(canReadActiveJobConversation).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ user: { id: authenticatedUserId } }),
|
||||
'active-convo',
|
||||
);
|
||||
});
|
||||
|
||||
it('should start conversation message reads before validation resolves', async () => {
|
||||
const events = [];
|
||||
let resolveValidation;
|
||||
|
|
|
|||
|
|
@ -83,7 +83,7 @@ router.get('/:conversationId', async (req, res) => {
|
|||
const { conversationId } = req.params;
|
||||
const convo = await db.getConvo(req.user.id, conversationId);
|
||||
|
||||
if (convo) {
|
||||
if (convo && convo.subagentThread == null) {
|
||||
res.status(200).json(convo);
|
||||
} else {
|
||||
res.status(404).end();
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@ const {
|
|||
validateMessageReq,
|
||||
configMiddleware,
|
||||
sendValidationResponse,
|
||||
canReadActiveJobConversation,
|
||||
prepareMessageRequestValidation,
|
||||
} = require('~/server/middleware');
|
||||
const db = require('~/models');
|
||||
|
|
@ -68,14 +69,45 @@ router.get('/', async (req, res) => {
|
|||
: 'createdAt';
|
||||
const sortOrder = sortDirection === 'asc' ? 1 : -1;
|
||||
|
||||
let scopedMessageRead;
|
||||
if (typeof conversationId === 'string') {
|
||||
const ownershipRead = db.getConvoOwnership(user, conversationId);
|
||||
const messageRead = messageId
|
||||
? db.getMessages({ conversationId, messageId, user })
|
||||
: db.getMessagesByCursor(
|
||||
{ conversationId, user },
|
||||
{ sortField, sortOrder, limit: pageSize, cursor },
|
||||
);
|
||||
scopedMessageRead = Promise.resolve(messageRead).then(
|
||||
(value) => ({ ok: true, value }),
|
||||
(error) => ({ ok: false, error }),
|
||||
);
|
||||
|
||||
const conversation = await ownershipRead;
|
||||
const canReadActiveJob =
|
||||
conversation == null &&
|
||||
!messageId &&
|
||||
(await canReadActiveJobConversation(req, conversationId));
|
||||
if ((!conversation && !canReadActiveJob) || conversation?.subagentThread != null) {
|
||||
return res.status(404).json({ error: 'Conversation not found' });
|
||||
}
|
||||
} else if (conversationId) {
|
||||
return res.status(404).json({ error: 'Conversation not found' });
|
||||
}
|
||||
|
||||
if (conversationId && messageId) {
|
||||
const messages = await db.getMessages({ conversationId, messageId, user });
|
||||
const messageResult = await scopedMessageRead;
|
||||
if (!messageResult.ok) {
|
||||
throw messageResult.error;
|
||||
}
|
||||
const messages = messageResult.value;
|
||||
response = { messages: messages?.length ? [messages[0]] : [], nextCursor: null };
|
||||
} else if (conversationId) {
|
||||
response = await db.getMessagesByCursor(
|
||||
{ conversationId, user },
|
||||
{ sortField, sortOrder, limit: pageSize, cursor },
|
||||
);
|
||||
const messageResult = await scopedMessageRead;
|
||||
if (!messageResult.ok) {
|
||||
throw messageResult.error;
|
||||
}
|
||||
response = messageResult.value;
|
||||
} else if (search) {
|
||||
const searchResults = await db.searchMessages(search, { filter: `user = "${user}"` }, true);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue