fix(import): store documents outside the public image tree and bound imports per node

Two review findings on the import job path.

The strategies default basePath to images, which on the local strategy is client/public/images: served statically, with authentication off unless secureImageLinks is set, so an imported PDF or audio file was retrievable by URL without a session. Non-image assets now take the uploads base every other document upload uses, and the base path travels with the backend so the image and document strategies each get the one they mean.

The per-user limit also said nothing about aggregate work: enough accounts each within their own limit still parse an export apiece in one heap, and inspection runs unbounded in the upload request besides. Both stages now sit under a node-wide ceiling, CONVERSATION_IMPORT_MAX_CONCURRENT, defaulting to three; requests over it get a 429 they can retry rather than a slot.
This commit is contained in:
Marco Beretta 2026-08-03 01:37:03 +02:00
parent c7556a76c9
commit df579d8ebf
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
8 changed files with 370 additions and 6 deletions

View file

@ -975,6 +975,12 @@ HELP_AND_FAQ_URL=https://librechat.ai
# export ships. Values above 512 MiB are ignored: V8 cannot build a string that long.
# CONVERSATION_IMPORT_MAX_SHARD_SIZE_BYTES=268435456
# How many import stages one instance runs at once, counting both archive inspection and the
# background runs, across all users. Per-user limits do not bound aggregate memory: enough
# accounts each within their own limit will still exhaust the process. Default: 3. Uploads and
# starts over the ceiling are answered with 429 and can be retried.
# CONVERSATION_IMPORT_MAX_CONCURRENT=3
# Max size (bytes) of a code-execution artifact (docx/xlsx/csv/pptx/text/pdf) rendered as an
# inline preview. Larger files fall back to download-only. Default: 2 MB (2097152). Note the
# rendered HTML is independently capped at 512 KB, so very rich files may still skip preview.

View file

@ -4,6 +4,7 @@ module.exports = {
api: (overrides = {}) => ({
isEnabled: jest.fn(),
resolveImportMaxFileSize: jest.fn(() => 262144000),
resolveImportMaxConcurrency: jest.fn(() => 3),
createAxiosInstance: jest.fn(() => ({
get: jest.fn(),
post: jest.fn(),

View file

@ -0,0 +1,147 @@
const fs = require('fs');
const os = require('os');
const path = require('path');
const express = require('express');
const request = require('supertest');
const mongoose = require('mongoose');
const { MongoMemoryServer } = require('mongodb-memory-server');
const { buildChatGptExportZip, cleanupChatGptExportZips } = require('~/test/chatgptExport');
const { createModels, createMethods } = require('@librechat/data-schemas');
const { FileSources } = require('librechat-data-provider');
/** One stage at a time, so a single running import puts the node at capacity
* and the next request has to be refused rather than admitted. */
process.env.CONVERSATION_IMPORT_MAX_CONCURRENT = '1';
jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next());
jest.mock('~/server/middleware', () => ({
createImportLimiters: () => ({
importIpLimiter: (req, res, next) => next(),
importUserLimiter: (req, res, next) => next(),
}),
createForkLimiters: () => ({
forkIpLimiter: (req, res, next) => next(),
forkUserLimiter: (req, res, next) => next(),
}),
configMiddleware: (req, res, next) => next(),
validateConvoAccess: (req, res, next) => next(),
}));
jest.mock('~/server/utils/import/defaults', () => ({
resolveImportDefaultModel: jest.fn().mockResolvedValue('gpt-4o-mini'),
}));
jest.mock('~/server/services/Files/strategies', () => ({
getStrategyFunctions: jest.fn(() => ({ saveBuffer: jest.fn() })),
}));
/** Holds the run open. Everything else in the package stays real: the point
* is to observe the ceiling while a genuine run occupies its slot, and a run
* over these fixtures finishes far too quickly to observe otherwise. */
const mockRun = { release: null };
jest.mock('@librechat/api', () => {
const actual = jest.requireActual('@librechat/api');
return {
...actual,
runImport: jest.fn(
() =>
new Promise((resolve) => {
mockRun.release = () => resolve({ imported: 0, skipped: 0, failed: 0, errors: [] });
}),
),
};
});
/** Waits for the mocked run to be entered, i.e. for the job to hold the slot. */
async function waitForRunStart() {
for (let i = 0; i < 40; i++) {
if (mockRun.release) {
return;
}
await new Promise((resolve) => setTimeout(resolve, 25));
}
throw new Error('The import run never started');
}
describe('import concurrency ceiling', () => {
let app;
let mongoServer;
let userId;
const uploadDirs = [];
beforeAll(async () => {
mongoServer = await MongoMemoryServer.create();
await mongoose.connect(mongoServer.getUri());
const models = createModels(mongoose);
Object.assign(mongoose.models, models);
await createMethods(mongoose).seedDefaultRoles();
const convosRouter = require('../convos');
app = express();
app.use((req, res, next) => {
req.user = { id: userId, role: 'USER' };
const uploadsDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lc-import-cap-uploads-'));
uploadDirs.push(uploadsDir);
req.config = {
paths: { uploads: uploadsDir },
fileStrategy: FileSources.local,
interfaceConfig: {},
};
next();
});
app.use('/api/convos', convosRouter);
});
afterAll(async () => {
const collections = mongoose.connection.collections;
for (const key in collections) {
await collections[key].deleteMany({});
}
await mongoose.disconnect();
await mongoServer.stop();
for (const dir of uploadDirs) {
fs.rmSync(dir, { recursive: true, force: true });
}
cleanupChatGptExportZips();
delete process.env.CONVERSATION_IMPORT_MAX_CONCURRENT;
});
beforeEach(() => {
userId = new mongoose.Types.ObjectId().toString();
mockRun.release = null;
});
/** The per-user limit lets a second account through; the node-wide ceiling
* is what keeps two exports from being parsed in one heap at once. */
it("refuses a second user's upload while the node is at capacity, and admits it once the run ends", async () => {
const filepath = await buildChatGptExportZip();
const uploaded = await request(app)
.post('/api/convos/import')
.attach('file', filepath)
.expect(202);
await request(app).post(`/api/convos/import/jobs/${uploaded.body.jobId}/start`).expect(202);
await waitForRunStart();
const owner = userId;
userId = new mongoose.Types.ObjectId().toString();
const refused = await request(app)
.post('/api/convos/import')
.attach('file', filepath)
.expect(429);
expect(refused.body.message).toBe('Too many imports are running, try again shortly');
userId = owner;
mockRun.release();
for (let i = 0; i < 40; i++) {
const status = await request(app).get(`/api/convos/import/jobs/${uploaded.body.jobId}`);
if (status.body.phase === 'completed') {
break;
}
await new Promise((resolve) => setTimeout(resolve, 25));
}
await request(app).post('/api/convos/import').attach('file', filepath).expect(202);
});
});

View file

@ -6,7 +6,11 @@ const request = require('supertest');
const mongoose = require('mongoose');
const multer = require('multer');
const { MongoMemoryServer } = require('mongodb-memory-server');
const { buildChatGptExportZip, cleanupChatGptExportZips } = require('~/test/chatgptExport');
const {
buildChatGptExportZip,
buildMixedAssetExportZip,
cleanupChatGptExportZips,
} = require('~/test/chatgptExport');
const {
bareClaudeExport,
buildClaudeExportZip,
@ -32,6 +36,9 @@ jest.mock('~/server/middleware', () => ({
jest.mock('~/server/utils/import/defaults', () => ({
resolveImportDefaultModel: jest.fn().mockResolvedValue('gpt-4o-mini'),
}));
/** Every `saveBuffer` the job pipeline issued, so a test can assert which
* storage base path each asset type was written under. */
const mockSavedAssets = [];
/**
* The real local storage strategy writes under the repo's `client/public`
* tree. This route test only cares that the job pipeline calls `saveBuffer`
@ -40,7 +47,8 @@ jest.mock('~/server/utils/import/defaults', () => ({
*/
jest.mock('~/server/services/Files/strategies', () => ({
getStrategyFunctions: jest.fn(() => ({
saveBuffer: async ({ buffer, fileName }) => {
saveBuffer: async ({ buffer, fileName, basePath }) => {
mockSavedAssets.push({ fileName, basePath });
const nodeFs = require('fs');
const nodePath = require('path');
const nodeOs = require('os');
@ -306,6 +314,30 @@ describe('conversation import job API (real router, real Mongo)', () => {
expect(savedConvos).toBe(2);
});
/** `client/public/images` is served statically, and authentication over it
* is off unless `secureImageLinks` is enabled so an imported PDF or audio
* file written to the strategy's default base would be anonymously
* retrievable by URL. Only images, which already render from that tree,
* belong there. */
it('writes non-image assets under uploads and images under images', async () => {
mockSavedAssets.length = 0;
const filepath = await buildMixedAssetExportZip();
const uploaded = await request(app)
.post('/api/convos/import')
.attach('file', filepath)
.expect(202);
await request(app).post(`/api/convos/import/jobs/${uploaded.body.jobId}/start`).expect(202);
const completed = await waitForTerminal(app, uploaded.body.jobId);
expect(completed.body.phase).toBe('completed');
const basePathFor = (suffix) =>
mockSavedAssets.find((call) => call.fileName.endsWith(suffix))?.basePath;
expect(basePathFor('photo.jpg')).toBe('images');
expect(basePathFor('voice.wav')).toBe('uploads');
});
it('rejects a file that is neither json nor zip', async () => {
const rejected = await request(app)
.post('/api/convos/import')

View file

@ -14,6 +14,7 @@ const {
deleteAgentCheckpoints,
sanitizeImportError,
resolveImportMaxFileSize,
resolveImportMaxConcurrency,
restoreTenantContextFromReq,
deleteAllSharedLinksWithCleanup,
deleteConvoSharedLinksWithCleanup,
@ -320,6 +321,16 @@ const importJobs = new ImportJobStore(getLogStores(CacheKeys.IMPORT_JOBS));
*/
const activeImports = new Map();
const MAX_CONCURRENT_IMPORTS_PER_USER = 1;
/**
* Heavy import stages in flight across every user on this node. The per-user
* map bounds what one account can start; it says nothing about aggregate work,
* so twenty accounts each inside their own limit are still twenty concurrent
* shard parses on one process. Inspection is counted with the runs because it
* is the same cost in the same heap it decompresses and JSON-parses the
* whole export in the upload request to build the summary.
*/
const MAX_CONCURRENT_IMPORT_STAGES = resolveImportMaxConcurrency();
let activeImportStages = 0;
/** Minimum gap between job-store progress writes. The client polls every two
* seconds, so anything below that is written for nobody to read. */
const PROGRESS_WRITE_INTERVAL_MS = 500;
@ -330,9 +341,11 @@ function activeImportCount(userId) {
function trackImportStart(userId) {
activeImports.set(userId, activeImportCount(userId) + 1);
activeImportStages += 1;
}
function trackImportEnd(userId) {
activeImportStages = Math.max(activeImportStages - 1, 0);
const remaining = activeImportCount(userId) - 1;
if (remaining > 0) {
activeImports.set(userId, remaining);
@ -341,6 +354,22 @@ function trackImportEnd(userId) {
activeImports.delete(userId);
}
function atImportCapacity() {
return activeImportStages >= MAX_CONCURRENT_IMPORT_STAGES;
}
/** Runs the archive inspection under the process-wide ceiling, so a burst of
* uploads queues behind the ceiling as 429s instead of parsing every archive
* at once. */
async function inspectUnderCapacity(filepath) {
activeImportStages += 1;
try {
return await inspectExport(filepath);
} finally {
activeImportStages = Math.max(activeImportStages - 1, 0);
}
}
function handleUpload(req, res, next) {
uploadSingle(req, res, (err) => {
if (err && err.code === 'LIMIT_FILE_SIZE') {
@ -453,8 +482,24 @@ async function runImportJob(context, job) {
*/
const imageSource = getFileStrategy(appConfig, { isImage: true });
const documentSource = getFileStrategy(appConfig, { isImage: false });
const imageBackend = { source: imageSource, fns: getStrategyFunctions(imageSource) };
const documentBackend = { source: documentSource, fns: getStrategyFunctions(documentSource) };
/**
* `basePath` travels with the backend because the strategies default it to
* `images`, and on the local strategy that base is `client/public/images`,
* which is served statically with no auth unless `secureImageLinks` is on.
* A PDF or audio attachment written there is anonymously retrievable by
* URL, so non-images take the same `uploads` base every other document
* upload in the app uses, reachable only through the file download route.
*/
const imageBackend = {
source: imageSource,
fns: getStrategyFunctions(imageSource),
basePath: 'images',
};
const documentBackend = {
source: documentSource,
fns: getStrategyFunctions(documentSource),
basePath: 'uploads',
};
const backendFor = (type) =>
(type ?? '').startsWith('image/') ? imageBackend : documentBackend;
@ -464,6 +509,7 @@ async function runImportJob(context, job) {
userId: owner,
buffer,
fileName,
basePath: backend.basePath,
tenantId: tenant,
});
return { filepath, source: backend.source };
@ -618,11 +664,20 @@ router.post(
return;
}
/** Rejected before the archive is opened rather than queued: the upload is
* already on disk and the client polls anyway, so asking it to retry costs
* a request, while admitting it costs another whole export in heap. */
if (atImportCapacity()) {
await fs.promises.unlink(req.file.path).catch(() => undefined);
res.status(429).json({ message: 'Too many imports are running, try again shortly' });
return;
}
const isZip = path.extname(req.file.originalname).toLowerCase() === '.zip';
let inspected;
try {
inspected = await inspectExport(req.file.path);
inspected = await inspectUnderCapacity(req.file.path);
} catch (error) {
if (!isZip && error instanceof Error && error.message === 'Unsupported import type') {
await importLegacyConversation(req, res);
@ -698,6 +753,15 @@ router.post(
return;
}
/** Same handling for the node-wide ceiling: this user is within their own
* limit, but the process is not, so the job waits where it was rather than
* failing. */
if (atImportCapacity()) {
await importJobs.patch(req.user.id, result.job.jobId, { phase: 'awaiting_confirmation' });
res.status(429).json({ message: 'Too many imports are running, try again shortly' });
return;
}
/** Everything the background run needs, read while the request is still
* the thing being handled. Holding `req` instead would pin the socket and
* its buffers for the length of a multi-minute import, per concurrent job. */

View file

@ -100,6 +100,56 @@ async function buildChatGptExportZip() {
return filepath;
}
/**
* Export archive whose single conversation references one image and one audio
* attachment, for tests that care about how each asset type is stored rather
* than about sharding or manifests.
* @returns {Promise<string>} path to the written zip
*/
async function buildMixedAssetExportZip() {
const zip = new JSZip();
zip.file(
'conversations.json',
JSON.stringify([
conversation('ext-mixed', 'Trip notes', 1700002000, {
a1: {
id: 'a1',
parent: 'u1',
children: [],
message: {
id: 'a1',
author: { role: 'assistant', name: null },
create_time: 1700002002,
content: {
content_type: 'multimodal_text',
parts: [
'Here you go.',
{ content_type: 'image_asset_pointer', asset_pointer: 'file-service://asset-img' },
{ content_type: 'audio_asset_pointer', asset_pointer: 'file-service://asset-aud' },
],
},
metadata: { model_slug: 'gpt-4o' },
},
},
}),
]),
);
zip.file(
'conversation_asset_file_names.json',
JSON.stringify({ 'asset-img.dat': 'photo.jpg', 'asset-aud.dat': 'voice.wav' }),
);
zip.file('asset-img.dat', Buffer.from([1, 2, 3, 4]));
zip.file('asset-aud.dat', Buffer.from([5, 6, 7, 8]));
const buffer = await zip.generateAsync({ type: 'nodebuffer' });
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lc-import-route-fixture-'));
createdDirs.push(dir);
const filepath = path.join(dir, 'chatgpt-mixed-assets.zip');
fs.writeFileSync(filepath, buffer);
return filepath;
}
function cleanupChatGptExportZips() {
while (createdDirs.length > 0) {
const dir = createdDirs.pop();
@ -107,4 +157,8 @@ function cleanupChatGptExportZips() {
}
}
module.exports = { buildChatGptExportZip, cleanupChatGptExportZips };
module.exports = {
buildChatGptExportZip,
buildMixedAssetExportZip,
cleanupChatGptExportZips,
};

View file

@ -5,8 +5,10 @@ jest.mock('@librechat/data-schemas', () => ({
import {
DEFAULT_IMPORT_MAX_FILE_SIZE,
DEFAULT_IMPORT_MAX_SHARD_SIZE,
DEFAULT_IMPORT_MAX_CONCURRENCY,
resolveImportMaxFileSize,
resolveImportMaxShardSize,
resolveImportMaxConcurrency,
} from '../import';
import { logger } from '@librechat/data-schemas';
@ -107,3 +109,34 @@ describe('resolveImportMaxShardSize', () => {
expect(resolveImportMaxShardSize()).toBe(DEFAULT_IMPORT_MAX_SHARD_SIZE);
});
});
describe('resolveImportMaxConcurrency', () => {
const original = process.env.CONVERSATION_IMPORT_MAX_CONCURRENT;
afterEach(() => {
if (original === undefined) {
delete process.env.CONVERSATION_IMPORT_MAX_CONCURRENT;
return;
}
process.env.CONVERSATION_IMPORT_MAX_CONCURRENT = original;
});
it('defaults to three stages', () => {
delete process.env.CONVERSATION_IMPORT_MAX_CONCURRENT;
expect(resolveImportMaxConcurrency()).toBe(DEFAULT_IMPORT_MAX_CONCURRENCY);
expect(DEFAULT_IMPORT_MAX_CONCURRENCY).toBe(3);
});
it('honours a configured override', () => {
process.env.CONVERSATION_IMPORT_MAX_CONCURRENT = '8';
expect(resolveImportMaxConcurrency()).toBe(8);
});
/** A fractional ceiling would admit a stage the operator never allowed for:
* `2.5` reads as "two and a half imports", and the comparison rounds it up
* to three. */
it.each(['0', '-1', '2.5', 'not-a-number'])('falls back to the default for %s', (raw) => {
process.env.CONVERSATION_IMPORT_MAX_CONCURRENT = raw;
expect(resolveImportMaxConcurrency()).toBe(DEFAULT_IMPORT_MAX_CONCURRENCY);
});
});

View file

@ -19,6 +19,33 @@ export function resolveImportMaxFileSize(): number {
return parsed;
}
/** Default number of import stages one node will run at once. */
export const DEFAULT_IMPORT_MAX_CONCURRENCY = 3;
/**
* Resolves the node-wide ceiling on concurrent import stages.
*
* Per-user limits bound what one account can start and say nothing about the
* aggregate: each stage buffers, decodes and `JSON.parse`s a shard at roughly
* 3.2x its size in heap, so enough accounts each inside their own limit will
* still exhaust the process. This is the ceiling on all of them together, and
* the env var is how a deployment with more headroom raises it.
*/
export function resolveImportMaxConcurrency(): number {
const raw = process.env.CONVERSATION_IMPORT_MAX_CONCURRENT;
if (!raw) {
return DEFAULT_IMPORT_MAX_CONCURRENCY;
}
const parsed = Number(raw);
if (!Number.isInteger(parsed) || parsed <= 0) {
logger.warn(
`[imports] Invalid CONVERSATION_IMPORT_MAX_CONCURRENT="${raw}"; using default ${DEFAULT_IMPORT_MAX_CONCURRENCY}`,
);
return DEFAULT_IMPORT_MAX_CONCURRENCY;
}
return parsed;
}
/** 256 MiB — default cap on a single decompressed archive entry. */
export const DEFAULT_IMPORT_MAX_SHARD_SIZE = 268435456;