mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 01:53:33 +00:00
🐛 fix: route phase-2 attachment update to current-run messageId
Codex P1 review on PR #12957. `processCodeOutput` intentionally preserves the original DB `messageId` across cross-turn filename reuse so `getCodeGeneratedFiles` can still trace a file back to the assistant message that originally produced it. The phase-1 SSE emit already routes by the current run's messageId — `processCodeOutput` runtime-overlays it via `Object.assign(file, { messageId, toolCallId })` and the callback writes `result.file` directly. Phase-2 was passing the raw `updateFile` return through `attachmentFromFileMetadata`, which read `messageId` straight off the DB record. On a turn-N run that re-emitted a filename from turn-1 (e.g. agent writes `output.csv` again), the phase-2 SSE update routed to `turn-1-msg` instead of `turn-N-msg`. Frontend's `useAttachmentHandler` upserts under the wrong messageAttachmentsMap slot — turn-N's pending chip stays stuck at "preparing preview…" while turn-1's already-resolved attachment gets re-merged. Fix: thread `runtimeMessageId` through `attachmentFromFileMetadata` and pass `metadata.run_id` from the phase-2 emit site. Mirrors how phase-1 sources its messageId. Tests cover the cross-turn reuse case plus the writableEnded / null-finalize / no-finalize paths to lock in the broader phase-2 emit contract.
This commit is contained in:
Danny Avila
parent
80c478c7e4
commit
e80c4ba21c
2 changed files with 241 additions and 3 deletions
|
|
@ -326,4 +326,221 @@ describe('createToolEndCallback', () => {
|
|||
expect(res.write).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('code execution two-phase preview emit', () => {
|
||||
/* The two-phase code-execution flow emits the attachment twice over
|
||||
* SSE: phase-1 with `status: 'pending'` and the current run's
|
||||
* messageId, phase-2 with the resolved record. The phase-2 emit
|
||||
* must use the CURRENT run's messageId (not the persisted DB one)
|
||||
* because `processCodeOutput` intentionally preserves the original
|
||||
* `messageId` on cross-turn filename reuse — `getCodeGeneratedFiles`
|
||||
* needs that for prior-turn priming.
|
||||
*
|
||||
* Codex P1 review on PR #12957: shipping `updated.messageId`
|
||||
* straight from the DB record routed phase-2 patches to the wrong
|
||||
* message slot, leaving the current turn's pending chip stuck. */
|
||||
|
||||
const { processCodeOutput } = require('~/server/services/Files/Code/process');
|
||||
|
||||
function makeCodeExecutionEvent({ runId, threadId, toolCallId, fileId, name }) {
|
||||
return {
|
||||
output: {
|
||||
name: 'execute_code',
|
||||
tool_call_id: toolCallId,
|
||||
artifact: {
|
||||
session_id: 'sess-1',
|
||||
files: [{ id: fileId, name, session_id: 'sess-1' }],
|
||||
},
|
||||
},
|
||||
metadata: { run_id: runId, thread_id: threadId },
|
||||
};
|
||||
}
|
||||
|
||||
/** Parse the SSE frame `res.write` produces back to a payload object. */
|
||||
function parseSseAttachment(call) {
|
||||
const frame = call[0];
|
||||
const dataLine = frame.split('\n').find((l) => l.startsWith('data: '));
|
||||
return JSON.parse(dataLine.slice('data: '.length));
|
||||
}
|
||||
|
||||
it('phase-2 emit uses the current run messageId, not the persisted DB messageId (cross-turn filename reuse)', async () => {
|
||||
/* Simulate turn-2 reusing `output.csv` from turn-1. The DB record
|
||||
* surfaced by `updateFile` carries the original `turn-1-msg`
|
||||
* messageId; the runtime emit must rewrite to `turn-2-msg`. */
|
||||
res.headersSent = true;
|
||||
const finalize = jest.fn().mockResolvedValue({
|
||||
file_id: 'fid-shared',
|
||||
filename: 'output.csv',
|
||||
filepath: '/uploads/output.csv',
|
||||
type: 'text/csv',
|
||||
conversationId: 'thread789',
|
||||
messageId: 'turn-1-original-msg', // persisted DB id (older turn)
|
||||
status: 'ready',
|
||||
text: '<table></table>',
|
||||
textFormat: 'html',
|
||||
});
|
||||
processCodeOutput.mockResolvedValue({
|
||||
file: {
|
||||
file_id: 'fid-shared',
|
||||
filename: 'output.csv',
|
||||
filepath: '/uploads/output.csv',
|
||||
type: 'text/csv',
|
||||
conversationId: 'thread789',
|
||||
messageId: 'turn-2-current-run', // runtime overlay (current turn)
|
||||
toolCallId: 'tool-2',
|
||||
status: 'pending',
|
||||
text: null,
|
||||
textFormat: null,
|
||||
},
|
||||
finalize,
|
||||
});
|
||||
|
||||
const toolEndCallback = createToolEndCallback({ req, res, artifactPromises });
|
||||
const event = makeCodeExecutionEvent({
|
||||
runId: 'turn-2-current-run',
|
||||
threadId: 'thread789',
|
||||
toolCallId: 'tool-2',
|
||||
fileId: 'fid-shared',
|
||||
name: 'output.csv',
|
||||
});
|
||||
await toolEndCallback({ output: event.output }, event.metadata);
|
||||
await Promise.all(artifactPromises);
|
||||
// Wait one more tick so the fire-and-forget finalize() chain settles.
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
|
||||
// Two SSE writes: phase-1 (pending) and phase-2 (ready).
|
||||
expect(res.write).toHaveBeenCalledTimes(2);
|
||||
const phase1 = parseSseAttachment(res.write.mock.calls[0]);
|
||||
const phase2 = parseSseAttachment(res.write.mock.calls[1]);
|
||||
|
||||
// Phase-1 already used the runtime messageId (sourced from result.file).
|
||||
expect(phase1.messageId).toBe('turn-2-current-run');
|
||||
expect(phase1.status).toBe('pending');
|
||||
|
||||
/* Phase-2 MUST also route to the current run's messageId so the
|
||||
* frontend's `useAttachmentHandler` upserts under the same
|
||||
* messageAttachmentsMap slot as phase-1. Routing to
|
||||
* `turn-1-original-msg` would land the patch on a stale message
|
||||
* and leave turn-2's pending chip stuck. */
|
||||
expect(phase2.messageId).toBe('turn-2-current-run');
|
||||
expect(phase2.file_id).toBe('fid-shared');
|
||||
expect(phase2.status).toBe('ready');
|
||||
expect(phase2.text).toBe('<table></table>');
|
||||
expect(phase2.toolCallId).toBe('tool-2');
|
||||
});
|
||||
|
||||
it('phase-2 emit is skipped when finalize resolves to null (no DB update happened)', async () => {
|
||||
res.headersSent = true;
|
||||
processCodeOutput.mockResolvedValue({
|
||||
file: {
|
||||
file_id: 'fid-1',
|
||||
filename: 'data.xlsx',
|
||||
filepath: '/uploads/data.xlsx',
|
||||
type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
|
||||
messageId: 'run-1',
|
||||
toolCallId: 'tool-1',
|
||||
status: 'pending',
|
||||
},
|
||||
finalize: jest.fn().mockResolvedValue(null),
|
||||
});
|
||||
|
||||
const toolEndCallback = createToolEndCallback({ req, res, artifactPromises });
|
||||
const event = makeCodeExecutionEvent({
|
||||
runId: 'run-1',
|
||||
threadId: 'thread-1',
|
||||
toolCallId: 'tool-1',
|
||||
fileId: 'fid-1',
|
||||
name: 'data.xlsx',
|
||||
});
|
||||
await toolEndCallback({ output: event.output }, event.metadata);
|
||||
await Promise.all(artifactPromises);
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
|
||||
// Only phase-1 fired; phase-2 noop'd because finalize returned null.
|
||||
expect(res.write).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('phase-2 emit is skipped when the response stream has already closed', async () => {
|
||||
res.headersSent = true;
|
||||
/* Hand-rolled deferred so we can hold finalize() open until
|
||||
* AFTER setting `res.writableEnded = true`. Otherwise the mock
|
||||
* resolves synchronously, the .then() runs in the same microtask
|
||||
* queue as the artifactPromises await, and writableEnded is set
|
||||
* too late. */
|
||||
let resolveFinalize;
|
||||
const finalizeDeferred = new Promise((resolve) => {
|
||||
resolveFinalize = resolve;
|
||||
});
|
||||
processCodeOutput.mockResolvedValue({
|
||||
file: {
|
||||
file_id: 'fid-1',
|
||||
filename: 'data.xlsx',
|
||||
filepath: '/uploads/data.xlsx',
|
||||
type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
|
||||
messageId: 'run-1',
|
||||
toolCallId: 'tool-1',
|
||||
status: 'pending',
|
||||
},
|
||||
finalize: jest.fn().mockReturnValue(finalizeDeferred),
|
||||
});
|
||||
|
||||
const toolEndCallback = createToolEndCallback({ req, res, artifactPromises });
|
||||
const event = makeCodeExecutionEvent({
|
||||
runId: 'run-1',
|
||||
threadId: 'thread-1',
|
||||
toolCallId: 'tool-1',
|
||||
fileId: 'fid-1',
|
||||
name: 'data.xlsx',
|
||||
});
|
||||
await toolEndCallback({ output: event.output }, event.metadata);
|
||||
await Promise.all(artifactPromises);
|
||||
// Simulate the response closing AFTER phase-1 fires but BEFORE
|
||||
// phase-2 lands. The frontend's polling path will catch the
|
||||
// resolved record on its next tick.
|
||||
res.writableEnded = true;
|
||||
// Now resolve finalize and let the .then() chain run.
|
||||
resolveFinalize({
|
||||
file_id: 'fid-1',
|
||||
filename: 'data.xlsx',
|
||||
messageId: 'run-1',
|
||||
status: 'ready',
|
||||
text: '<x/>',
|
||||
textFormat: 'html',
|
||||
});
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
|
||||
// Phase-1 wrote; phase-2 noop'd because writableEnded.
|
||||
expect(res.write).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('does not call finalize for a non-office file (no preview expected)', async () => {
|
||||
res.headersSent = true;
|
||||
processCodeOutput.mockResolvedValue({
|
||||
file: {
|
||||
file_id: 'fid-txt',
|
||||
filename: 'note.txt',
|
||||
filepath: '/uploads/note.txt',
|
||||
type: 'text/plain',
|
||||
messageId: 'run-1',
|
||||
toolCallId: 'tool-1',
|
||||
// No status — non-office files skip phase-2 entirely.
|
||||
},
|
||||
// No finalize key — caller should not call anything.
|
||||
});
|
||||
|
||||
const toolEndCallback = createToolEndCallback({ req, res, artifactPromises });
|
||||
const event = makeCodeExecutionEvent({
|
||||
runId: 'run-1',
|
||||
threadId: 'thread-1',
|
||||
toolCallId: 'tool-1',
|
||||
fileId: 'fid-txt',
|
||||
name: 'note.txt',
|
||||
});
|
||||
await toolEndCallback({ output: event.output }, event.metadata);
|
||||
await Promise.all(artifactPromises);
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
|
||||
expect(res.write).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -427,8 +427,20 @@ function writeAttachmentUpdate(res, streamId, attachment) {
|
|||
* file metadata record. Centralizes the field projection so phase-1
|
||||
* (pending), phase-2 (ready/failed), and pre-existing legacy callers
|
||||
* all emit the same shape.
|
||||
*
|
||||
* `runtimeMessageId` overrides whatever messageId the record carries
|
||||
* — required for phase-2 emits because `processCodeOutput` intentionally
|
||||
* preserves the original DB messageId across cross-turn filename reuse
|
||||
* (so `getCodeGeneratedFiles` can still trace the file back to the
|
||||
* assistant message that originally produced it). Routing the SSE
|
||||
* update by the persisted id would land the patch on a stale message
|
||||
* slot — turn-N's pending placeholder would stay stuck pending while
|
||||
* turn-1's already-resolved attachment got re-merged. The phase-1
|
||||
* emit path mirrors this by reading `result.file.messageId`, which
|
||||
* `processCodeOutput` runtime-overlays via `Object.assign`. (Codex P1
|
||||
* review on PR #12957.)
|
||||
*/
|
||||
function attachmentFromFileMetadata(fileMetadata, toolCallId) {
|
||||
function attachmentFromFileMetadata(fileMetadata, toolCallId, runtimeMessageId) {
|
||||
return {
|
||||
file_id: fileMetadata.file_id,
|
||||
filename: fileMetadata.filename,
|
||||
|
|
@ -437,7 +449,7 @@ function attachmentFromFileMetadata(fileMetadata, toolCallId) {
|
|||
width: fileMetadata.width,
|
||||
height: fileMetadata.height,
|
||||
conversationId: fileMetadata.conversationId,
|
||||
messageId: fileMetadata.messageId,
|
||||
messageId: runtimeMessageId ?? fileMetadata.messageId,
|
||||
toolCallId: toolCallId ?? fileMetadata.toolCallId,
|
||||
/* Inline text / sanitized HTML preview from the extractor — drives
|
||||
* the file artifact panel's rich preview for DOCX/XLSX/CSV/PPTX.
|
||||
|
|
@ -669,7 +681,16 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null })
|
|||
writeAttachmentUpdate(
|
||||
res,
|
||||
streamId,
|
||||
attachmentFromFileMetadata(updated, toolCallId),
|
||||
/* Pass `metadata.run_id` as the runtime messageId
|
||||
* override — see attachmentFromFileMetadata's JSDoc.
|
||||
* `updated` carries the DB-persisted messageId which,
|
||||
* on cross-turn filename reuse (e.g. agent emits
|
||||
* `output.csv` again on turn N), is the original
|
||||
* turn's id rather than the current one. Without this
|
||||
* override the SSE update routes to the wrong
|
||||
* message slot and the current turn's pending chip
|
||||
* stays stuck. */
|
||||
attachmentFromFileMetadata(updated, toolCallId, metadata.run_id),
|
||||
);
|
||||
})
|
||||
.catch((error) => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue