From 68ff60c8205385812852bd7ba50c50271b19946b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 5 May 2026 13:46:01 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A0=EF=B8=8F=20refactor:=20address=20c?= =?UTF-8?q?odex=20audit=20findings=20(wire-shape=20parity,=20DRY,=20defens?= =?UTF-8?q?ive=20catch)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive audit on PR #12957. Resolves all valid findings: - **MAJOR #1 — Wire-shape parity**: phase-1 ships the full `fileMetadata` record over SSE; phase-2 was using a tight `attachmentFromFileMetadata` projection. Drop the projection and have phase-2 spread `{...updated, messageId, toolCallId}` so both events match the long-standing legacy phase-1 shape clients depend on. - **MAJOR #2 — DRY**: extract `runPhase2Finalize({ finalize, fileId, onResolved })` into `process.js` (alongside `processCodeOutput` whose contract it pairs with). Both `callbacks.js` paths and `tools.js` now flow through it. Single catch path eliminates divergence surface — the fix landed in 01704d4f0 (cross-turn messageId routing) was a symptom of this duplication risk. - **MINOR #3 — JSDoc accuracy**: `finalizePreview`'s buffer is bounded by `fileSizeLimit`, not the 1MB extractor cap. Updated and added a note about peak heap from queued buffers. - **MINOR #4 — Defensive catch**: `runPhase2Finalize`'s catch attempts a best-effort `updateFile({ status: 'failed', previewError: 'unexpected' })` for the file_id, so a programming bug in `finalizePreview` doesn't leave the record stuck `'pending'` until the next boot-time orphan sweep. - **NIT #6 — Stale PR refs**: 12952 → 12957 in 3 places. - **NIT #7 — Schema bound**: `previewError` capped at `maxlength: 200` to prevent a future codepath from accidentally persisting a stack trace. Skipped per audit verdict (non-blocking): - #5 (memory pressure): documented in JSDoc; impl change was reviewer's "consider", not actionable. - #8 (double DB query per poll): low cost, indexed by_id, polling is gated narrow. - #9 (TAttachment cast): the union type is intentional; the casts are safe widening, refactoring TAttachment is invasive and out of scope. Tests: 11 new (7 `runPhase2Finalize` unit tests covering happy path, null-finalize, throws, double-fail, no-fileId, no-onResolved; +4 wire-shape parity assertions in the existing cross-turn test). 328 backend tests pass; 528 frontend tests pass; lint and typecheck clean. --- .../agents/__tests__/callbacks.spec.js | 31 +++++ api/server/controllers/agents/callbacks.js | 130 +++++------------- api/server/controllers/tools.js | 15 +- api/server/routes/files/preview.spec.js | 2 +- api/server/services/Files/Code/process.js | 73 +++++++++- .../services/Files/Code/process.spec.js | 92 +++++++++++++ packages/data-provider/src/api-endpoints.ts | 2 +- packages/data-provider/src/data-service.ts | 2 +- packages/data-schemas/src/schema/file.ts | 6 + 9 files changed, 247 insertions(+), 106 deletions(-) diff --git a/api/server/controllers/agents/__tests__/callbacks.spec.js b/api/server/controllers/agents/__tests__/callbacks.spec.js index ea942a751e..7b05094aa0 100644 --- a/api/server/controllers/agents/__tests__/callbacks.spec.js +++ b/api/server/controllers/agents/__tests__/callbacks.spec.js @@ -28,6 +28,28 @@ jest.mock('~/server/services/Files/Citations', () => ({ jest.mock('~/server/services/Files/Code/process', () => ({ processCodeOutput: jest.fn(), + /* `runPhase2Finalize` is the runtime pairing for `finalize` (defined + * alongside processCodeOutput in process.js). The callback wires + * phase-2 through it; reproduce the basic happy-path here so the + * SSE-emit assertions still work. The catch/defensive-updateFile + * branch is unit-tested directly against the real helper in + * process.spec.js — exercising it here would add test coupling + * without coverage benefit. */ + runPhase2Finalize: ({ finalize, onResolved }) => { + if (typeof finalize !== 'function') { + return; + } + finalize() + .then((updated) => { + if (!updated || !onResolved) { + return; + } + onResolved(updated); + }) + .catch(() => { + /* swallowed in the mock — see process.spec.js for catch coverage */ + }); + }, })); jest.mock('~/server/services/Tools/credentials', () => ({ @@ -427,6 +449,15 @@ describe('createToolEndCallback', () => { expect(phase2.status).toBe('ready'); expect(phase2.text).toBe('
'); expect(phase2.toolCallId).toBe('tool-2'); + /* Wire-shape parity with phase-1: phase-2 emits the full updated + * record so the client doesn't see one shape on phase-1 and a + * narrower projection on phase-2. (Codex audit on PR #12957 + * Finding 1.) */ + expect(phase2.filename).toBe('output.csv'); + expect(phase2.filepath).toBe('/uploads/output.csv'); + expect(phase2.type).toBe('text/csv'); + expect(phase2.conversationId).toBe('thread789'); + expect(phase2.textFormat).toBe('html'); }); it('phase-2 emit is skipped when finalize resolves to null (no DB update happened)', async () => { diff --git a/api/server/controllers/agents/callbacks.js b/api/server/controllers/agents/callbacks.js index 544b2dfd7a..2fd60e5d51 100644 --- a/api/server/controllers/agents/callbacks.js +++ b/api/server/controllers/agents/callbacks.js @@ -15,7 +15,7 @@ const { createToolExecuteHandler, } = require('@librechat/api'); const { processFileCitations } = require('~/server/services/Files/Citations'); -const { processCodeOutput } = require('~/server/services/Files/Code/process'); +const { processCodeOutput, runPhase2Finalize } = require('~/server/services/Files/Code/process'); const { saveBase64Image } = require('~/server/services/Files/process'); class ModelEndHandler { @@ -422,52 +422,6 @@ function writeAttachmentUpdate(res, streamId, attachment) { writeAttachment(res, streamId, attachment); } -/** - * Build the wire-format attachment payload from a phase-1 or phase-2 - * 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, runtimeMessageId) { - return { - file_id: fileMetadata.file_id, - filename: fileMetadata.filename, - filepath: fileMetadata.filepath, - type: fileMetadata.type, - width: fileMetadata.width, - height: fileMetadata.height, - conversationId: fileMetadata.conversationId, - 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. - * Pass null explicitly (rather than undefined) so the wire format - * is stable for the empty-text gate on the client. */ - text: fileMetadata.text ?? null, - /* Trust signal so the client routes office types to the HTML - * bucket only when the backend produced sanitized full-document - * HTML (Codex P1 review on PR #12934). */ - textFormat: fileMetadata.textFormat ?? null, - /* Two-phase preview lifecycle: phase-1 emits 'pending', phase-2 - * patches to 'ready' or 'failed'. Absent for files that don't - * expect a preview (images, plain text). */ - status: fileMetadata.status, - previewError: fileMetadata.previewError, - }; -} - /** * * @param {Object} params @@ -671,32 +625,29 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null }) * after the HTTP response closes. If the stream is still open * when phase-2 lands, push an `attachment` update event so * the UI patches in place; otherwise React Query polling on - * `/api/files/:file_id/preview` picks it up. */ - if (typeof finalize === 'function') { - finalize() - .then((updated) => { - if (!updated) { - return; - } - writeAttachmentUpdate( - res, - streamId, - /* 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) => { - logger.error('Error in phase-2 preview extraction:', error); + * `/api/files/:file_id/preview` picks it up. + * + * Spread the full updated record (mirroring the phase-1 emit + * shape) and overlay `messageId`/`toolCallId` from the + * current run. The DB record preserves the original + * `messageId` across cross-turn filename reuse so + * `getCodeGeneratedFiles` can trace the file back to its + * original assistant message; 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 while + * turn-1's already-resolved attachment got re-merged. + * (Codex P1 review on PR #12957.) */ + runPhase2Finalize({ + finalize, + fileId: fileMetadata.file_id, + onResolved: (updated) => { + writeAttachmentUpdate(res, streamId, { + ...updated, + messageId: metadata.run_id, + toolCallId, }); - } + }, + }); return fileMetadata; })().catch((error) => { logger.error('Error processing code output:', error); @@ -937,26 +888,21 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises }) * so the client merges the resolved record over the pending * placeholder. Fire-and-forget — survives response close; * polling covers the post-close gap. */ - if (typeof finalize === 'function') { - finalize() - .then((updated) => { - if (!updated) { - return; - } - if (!res || res.writableEnded || !res.headersSent) { - return; - } - writeResponsesAttachment( - res, - tracker, - buildResponsesAttachment(updated, toolCallId), - metadata, - ); - }) - .catch((error) => { - logger.error('Error in phase-2 preview extraction:', error); - }); - } + runPhase2Finalize({ + finalize, + fileId: fileMetadata.file_id, + onResolved: (updated) => { + if (!res || res.writableEnded || !res.headersSent) { + return; + } + writeResponsesAttachment( + res, + tracker, + buildResponsesAttachment(updated, toolCallId), + metadata, + ); + }, + }); return fileMetadata; })().catch((error) => { diff --git a/api/server/controllers/tools.js b/api/server/controllers/tools.js index 1557991393..d11a4cf4eb 100644 --- a/api/server/controllers/tools.js +++ b/api/server/controllers/tools.js @@ -10,7 +10,7 @@ const { } = require('librechat-data-provider'); const { getRoleByName, createToolCall, getToolCallsByConvo, getMessage } = require('~/models'); const { processFileURL, uploadImageBuffer } = require('~/server/services/Files/process'); -const { processCodeOutput } = require('~/server/services/Files/Code/process'); +const { processCodeOutput, runPhase2Finalize } = require('~/server/services/Files/Code/process'); const { loadAuthValues } = require('~/server/services/Tools/credentials'); const { loadTools } = require('~/app/clients/tools/util'); @@ -208,14 +208,11 @@ const callTool = async (req, res) => { } /* This endpoint is non-streaming and its contract is "give * me the artifacts" — return the phase-1 record immediately - * (with `status: 'pending'` for office buckets) and run phase-2 - * extraction in the background. The client polls - * `/api/files/:file_id/preview` for the resolved record. */ - if (typeof finalize === 'function') { - finalize().catch((error) => { - logger.error('Error in phase-2 preview extraction:', error); - }); - } + * (with `status: 'pending'` for office buckets) and run + * phase-2 extraction in the background. The client polls + * `/api/files/:file_id/preview` for the resolved record. + * No `onResolved` — there's no live stream to write to here. */ + runPhase2Finalize({ finalize, fileId: fileMetadata.file_id }); return fileMetadata; })().catch((error) => { logger.error('Error processing code output:', error); diff --git a/api/server/routes/files/preview.spec.js b/api/server/routes/files/preview.spec.js index 8aec71c4ac..9eca0f778e 100644 --- a/api/server/routes/files/preview.spec.js +++ b/api/server/routes/files/preview.spec.js @@ -179,7 +179,7 @@ describe('GET /files/:file_id/preview', () => { }); it('defaults to status:ready for legacy records with no status field (back-compat)', async () => { - /* Records pre-dating PR #12952 have no `status` field. The endpoint + /* Records pre-dating PR #12957 have no `status` field. The endpoint * MUST treat that as 'ready' so legacy code-execution attachments * don't suddenly poll forever. If text exists, surface it. */ mockGetFiles.mockResolvedValueOnce([ diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 105c7820b9..2f96e4930f 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -98,8 +98,16 @@ const PHASE_TWO_TIMEOUT_MS = 60_000; * thing that can lose progress — covered by the boot-time orphan sweep. * * @param {object} params - * @param {Buffer} params.buffer - The file contents (held in closure; - * bounded by `MAX_TEXT_EXTRACT_BYTES = 1MB` from the extractor). + * @param {Buffer} params.buffer - The full downloaded file contents, + * bounded by the server's `fileSizeLimit` config (defaults far above + * the 1MB extractor cap). The buffer is captured by the closure + * returned in `{ finalize }`, so when many office files queue behind + * the inner concurrency limiter (cap 2), all queued buffers stay + * resident until each one's slot frees. For a tool result emitting + * N office files, peak heap usage from this path is up to + * `N * fileSizeLimit`. Acceptable for typical agent runs (a handful + * of files at a few hundred KB each); pathological cases are bounded + * by the inner per-file 12s timeout and the outer 60s phase-2 cap. * @param {string} params.leafName - Basename for classification. * @param {string} params.mimeType - Detected/inferred MIME. * @param {string} params.category - Classifier output. @@ -155,6 +163,66 @@ const finalizePreview = async ({ buffer, leafName, mimeType, category, file_id } } }; +/** + * Run the background phase-2 thunk returned by `processCodeOutput` and + * route the resolved record to the caller's emit logic. Shared between + * `callbacks.js` (chat-completions + Open Responses) and `tools.js` + * (direct tool endpoint) so the fire-and-forget pattern doesn't drift + * across callsites. + * + * `onResolved` receives the post-update DB record and is the only piece + * that varies — chat-completions writes the legacy `attachment` SSE + * event, Open Responses writes the spec-shaped `librechat:attachment` + * event with a sequence number, and the direct tool endpoint has no + * stream to write to (caller passes a no-op). + * + * The catch path is the safety net for unexpected programming errors + * inside `finalizePreview`. The function is designed to never throw + * (extraction and DB failures are translated to `status: 'failed'` + * inside it), but a ref error or future regression would otherwise + * leave the DB record stuck at `'pending'` until the boot-time orphan + * sweep — potentially hours away on a stable server. We attempt a + * best-effort `updateFile` to mark the record `'failed'` with + * `previewError: 'unexpected'` so the UI stops polling and the + * next-turn LLM context surfaces the failure. + * + * @param {object} params + * @param {(() => Promise) | undefined} params.finalize - The + * thunk returned by `processCodeOutput`. No-op when undefined. + * @param {string | undefined} params.fileId - DB key for the failure + * marker; if absent the catch only logs. + * @param {(updated: object) => void} [params.onResolved] - Called once + * on success with the post-update record. + */ +const runPhase2Finalize = ({ finalize, fileId, onResolved }) => { + if (typeof finalize !== 'function') { + return; + } + finalize() + .then((updated) => { + if (!updated || !onResolved) { + return; + } + onResolved(updated); + }) + .catch((error) => { + logger.error('Error in phase-2 preview extraction:', error); + if (!fileId) { + return; + } + updateFile({ + file_id: fileId, + status: 'failed', + previewError: 'unexpected', + }).catch((updateErr) => { + logger.error( + `[runPhase2Finalize] also failed to mark ${fileId} as failed after error:`, + updateErr, + ); + }); + }); +}; + /** * Process code execution output files — downloads and saves both images * and non-image files. All files are saved to local storage with @@ -782,4 +850,5 @@ module.exports = { getSessionInfo, processCodeOutput, readSandboxFile, + runPhase2Finalize, }; diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index 8d9a9eb1a7..ed4a0eef4e 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -973,6 +973,98 @@ describe('Code Process', () => { }); }); + describe('runPhase2Finalize', () => { + /* The runtime pairing for `processCodeOutput`'s `finalize` thunk. + * `finalizePreview` is designed to never throw (translates errors + * to `status: 'failed'` internally). The helper's catch is the + * safety net for unexpected programming errors that would + * otherwise leave the DB record stuck at `status: 'pending'` + * forever — we attempt a best-effort `updateFile` to mark it + * `'failed'` with `previewError: 'unexpected'` so the UI stops + * polling and the next-turn LLM context surfaces the failure. + * (Codex audit on PR #12957 Finding 4.) */ + const { runPhase2Finalize } = require('./process'); + const { updateFile } = require('~/models'); + + beforeEach(() => { + updateFile.mockReset(); + updateFile.mockResolvedValue({}); + }); + + it('is a no-op when finalize is undefined (non-office files)', () => { + expect(() => + runPhase2Finalize({ finalize: undefined, fileId: 'fid-1', onResolved: jest.fn() }), + ).not.toThrow(); + expect(updateFile).not.toHaveBeenCalled(); + }); + + it('calls onResolved with the resolved record on success', async () => { + const onResolved = jest.fn(); + const finalize = jest + .fn() + .mockResolvedValue({ file_id: 'fid-1', status: 'ready', text: '' }); + runPhase2Finalize({ finalize, fileId: 'fid-1', onResolved }); + await new Promise((resolve) => setImmediate(resolve)); + expect(onResolved).toHaveBeenCalledWith( + expect.objectContaining({ file_id: 'fid-1', status: 'ready' }), + ); + expect(updateFile).not.toHaveBeenCalled(); + }); + + it('skips onResolved when finalize resolves to null (DB write failed inside finalizePreview)', async () => { + const onResolved = jest.fn(); + const finalize = jest.fn().mockResolvedValue(null); + runPhase2Finalize({ finalize, fileId: 'fid-1', onResolved }); + await new Promise((resolve) => setImmediate(resolve)); + expect(onResolved).not.toHaveBeenCalled(); + }); + + it('marks the record as failed (previewError: "unexpected") when finalize throws', async () => { + const onResolved = jest.fn(); + const finalize = jest.fn().mockRejectedValue(new Error('unexpected ref error')); + runPhase2Finalize({ finalize, fileId: 'fid-boom', onResolved }); + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + expect(onResolved).not.toHaveBeenCalled(); + expect(updateFile).toHaveBeenCalledWith({ + file_id: 'fid-boom', + status: 'failed', + previewError: 'unexpected', + }); + expect(logger.error).toHaveBeenCalledWith( + 'Error in phase-2 preview extraction:', + expect.any(Error), + ); + }); + + it('logs but does not throw when the defensive updateFile itself fails', async () => { + const onResolved = jest.fn(); + const finalize = jest.fn().mockRejectedValue(new Error('original error')); + updateFile.mockRejectedValueOnce(new Error('mongo down')); + runPhase2Finalize({ finalize, fileId: 'fid-doublefail', onResolved }); + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + expect(onResolved).not.toHaveBeenCalled(); + // Two logger.error calls: one for the original throw, one for the failed mark. + expect(logger.error.mock.calls.some((c) => /also failed to mark/.test(c[0]))).toBe(true); + }); + + it('does not attempt the defensive updateFile when fileId is missing', async () => { + const finalize = jest.fn().mockRejectedValue(new Error('boom')); + runPhase2Finalize({ finalize, fileId: undefined }); + await new Promise((resolve) => setImmediate(resolve)); + expect(updateFile).not.toHaveBeenCalled(); + }); + + it('skips onResolved gracefully when caller omits it (e.g., tools.js direct endpoint)', async () => { + const finalize = jest.fn().mockResolvedValue({ file_id: 'fid-1', status: 'ready' }); + // No onResolved — non-streaming caller. + expect(() => runPhase2Finalize({ finalize, fileId: 'fid-1' })).not.toThrow(); + await new Promise((resolve) => setImmediate(resolve)); + expect(updateFile).not.toHaveBeenCalled(); + }); + }); + describe('readSandboxFile', () => { /** * `readSandboxFile` shells `cat ` through the sandbox diff --git a/packages/data-provider/src/api-endpoints.ts b/packages/data-provider/src/api-endpoints.ts index 8bd38427fe..be5f147b97 100644 --- a/packages/data-provider/src/api-endpoints.ts +++ b/packages/data-provider/src/api-endpoints.ts @@ -286,7 +286,7 @@ export const fileDownload = (userId: string, fileId: string) => `${BASE_URL}/api/files/download/${userId}/${fileId}`; /* Two-phase code-execution preview lifecycle endpoint. Returns * `{ status, text?, textFormat?, previewError? }` so the frontend can - * poll while phase-2 HTML extraction is in flight. See PR #12952. */ + * poll while phase-2 HTML extraction is in flight. See PR #12957. */ export const filePreview = (fileId: string) => `${BASE_URL}/api/files/${encodeURIComponent(fileId)}/preview`; export const fileConfig = () => `${BASE_URL}/api/files/config`; diff --git a/packages/data-provider/src/data-service.ts b/packages/data-provider/src/data-service.ts index 45f6ca6fdb..3ab41d4ad5 100644 --- a/packages/data-provider/src/data-service.ts +++ b/packages/data-provider/src/data-service.ts @@ -412,7 +412,7 @@ export const getFiles = (): Promise => { * - `previewError` only when `status === 'failed'`. * * Called from `useFilePreview` with React Query's `refetchInterval` - * gated on `status === 'pending'` and `isSubmitting`. See PR #12952. + * gated on `status === 'pending'` and `isSubmitting`. See PR #12957. */ export const getFilePreview = (fileId: string): Promise => { return request.get(endpoints.filePreview(fileId)); diff --git a/packages/data-schemas/src/schema/file.ts b/packages/data-schemas/src/schema/file.ts index e7eb479db0..b02cf9e736 100644 --- a/packages/data-schemas/src/schema/file.ts +++ b/packages/data-schemas/src/schema/file.ts @@ -75,6 +75,12 @@ const file: Schema = new Schema( }, previewError: { type: String, + /* Bounded to short machine-readable reasons (`'timeout'`, + * `'parser-error'`, `'orphaned'`, `'unexpected'`). Cap prevents a + * future codepath from accidentally persisting a stack trace or + * full error message — would bloat documents and ship a wall of + * text into the UI tooltip. */ + maxlength: 200, }, context: { type: String,