diff --git a/api/server/services/Files/process.js b/api/server/services/Files/process.js index 8272e0ecfe..09c0b0dc57 100644 --- a/api/server/services/Files/process.js +++ b/api/server/services/Files/process.js @@ -349,6 +349,7 @@ async function sweepExpiredFiles(options = {}) { return sweepExpiredFilesWithDeps(options, { getExpiredFiles: db.getExpiredFiles, processDeleteRequest, + incrementFileDeletionAttempts: db.incrementFileDeletionAttempts, deferExpiredFile: db.deferExpiredFile, logger, }); diff --git a/api/server/services/Files/process.spec.js b/api/server/services/Files/process.spec.js index 4cf45e3b57..20562fa627 100644 --- a/api/server/services/Files/process.spec.js +++ b/api/server/services/Files/process.spec.js @@ -172,6 +172,7 @@ jest.mock('~/models', () => ({ findFileById: jest.fn(), getConvo: jest.fn(), getExpiredFiles: jest.fn(), + incrementFileDeletionAttempts: jest.fn(), deferExpiredFile: jest.fn(), addAgentResourceFile: jest.fn().mockResolvedValue({}), removeAgentResourceFiles: jest.fn(), @@ -1841,6 +1842,7 @@ describe('sweepExpiredFiles', () => { expect.objectContaining({ getExpiredFiles: db.getExpiredFiles, processDeleteRequest: expect.any(Function), + incrementFileDeletionAttempts: db.incrementFileDeletionAttempts, deferExpiredFile: db.deferExpiredFile, logger: expect.objectContaining({ error: expect.any(Function), diff --git a/packages/api/src/files/sweep.spec.ts b/packages/api/src/files/sweep.spec.ts index 0718c633f0..96b37fb1ce 100644 --- a/packages/api/src/files/sweep.spec.ts +++ b/packages/api/src/files/sweep.spec.ts @@ -16,9 +16,13 @@ describe('expired file sweep helpers', () => { }; let deferExpiredFile: jest.Mock; + let incrementFileDeletionAttempts: jest.Mock; + let attemptCount: number; beforeEach(() => { jest.clearAllMocks(); + attemptCount = 0; + incrementFileDeletionAttempts = jest.fn().mockImplementation(() => ++attemptCount); deferExpiredFile = jest.fn().mockResolvedValue(undefined); delete process.env.FILE_RETENTION_SWEEP_INTERVAL_MS; delete process.env.FILE_RETENTION_SWEEP_MAX_ATTEMPTS; @@ -51,7 +55,13 @@ describe('expired file sweep helpers', () => { const result = await sweepExpiredFiles( { appConfig: {} as AppConfig, loadAppConfig, limit: 1 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); expect(getExpiredFiles).toHaveBeenCalledWith(1, { maxAttempts: 10 }); @@ -75,7 +85,13 @@ describe('expired file sweep helpers', () => { const result = await sweepExpiredFiles( { appConfig: {} as AppConfig, limit: 1 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); expect(processDeleteRequest).not.toHaveBeenCalled(); @@ -92,18 +108,26 @@ describe('expired file sweep helpers', () => { ])('defers the file after %s', async (_case, outcome) => { const getExpiredFiles = jest .fn() - .mockResolvedValue([{ file_id: 'stuck-file', user: 'user-1', deletionAttempts: 2 }]); + .mockResolvedValue([{ file_id: 'stuck-file', user: 'user-1' }]); const processDeleteRequest = jest.fn().mockResolvedValue(outcome); const result = await sweepExpiredFiles( { appConfig: {} as AppConfig, limit: 1 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); const [fileId, retryAt] = deferExpiredFile.mock.calls[0]; expect(fileId).toBe('stuck-file'); - /* Third consecutive failure, so the backoff has doubled twice. */ - expect(retryAt.getTime() - Date.now()).toBeCloseTo(4 * 60 * 60 * 1000, -3); + /* Backoff comes off the attempt the increment returned, not the count + * the queried snapshot carried. */ + expect(incrementFileDeletionAttempts).toHaveBeenCalledWith('stuck-file'); + expect(retryAt.getTime() - Date.now()).toBeCloseTo(60 * 60 * 1000, -3); expect(result).toEqual({ scanned: 1, deleted: 0, failed: 1 }); }); @@ -115,7 +139,13 @@ describe('expired file sweep helpers', () => { const result = await sweepExpiredFiles( { appConfig: {} as AppConfig, limit: 1 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); expect(deferExpiredFile).toHaveBeenCalledWith('stuck-file', expect.any(Date)); @@ -124,16 +154,23 @@ describe('expired file sweep helpers', () => { it('reports giving up once the file reaches the attempt cap', async () => { process.env.FILE_RETENTION_SWEEP_MAX_ATTEMPTS = '3'; + attemptCount = 2; const getExpiredFiles = jest .fn() - .mockResolvedValue([{ file_id: 'stranded-file', user: 'user-1', deletionAttempts: 2 }]); + .mockResolvedValue([{ file_id: 'stranded-file', user: 'user-1' }]); const processDeleteRequest = jest .fn() .mockResolvedValue({ deletedFileIds: [], failedFileIds: ['stranded-file'] }); await sweepExpiredFiles( { appConfig: {} as AppConfig, limit: 1 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); expect(getExpiredFiles).toHaveBeenCalledWith(1, { maxAttempts: 3 }); @@ -151,11 +188,17 @@ describe('expired file sweep helpers', () => { .fn() .mockResolvedValueOnce({ deletedFileIds: [], failedFileIds: ['stuck-file'] }) .mockResolvedValueOnce({ deletedFileIds: ['deletable-file'], failedFileIds: [] }); - deferExpiredFile.mockRejectedValue(new Error('mongo unreachable')); + incrementFileDeletionAttempts.mockRejectedValue(new Error('mongo unreachable')); const result = await sweepExpiredFiles( { appConfig: {} as AppConfig, limit: 2 }, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }, ); expect(logger.error).toHaveBeenCalledWith( @@ -170,6 +213,16 @@ describe('expired file sweep helpers', () => { expect(getExpiredFileRetryDelay(4)).toBe(8 * 60 * 60 * 1000); expect(getExpiredFileRetryDelay(64)).toBe(24 * 60 * 60 * 1000); + /* The base tracks the configured cadence rather than a fixed hour. */ + process.env.FILE_RETENTION_SWEEP_INTERVAL_MS = String(5 * 60 * 1000); + expect(getExpiredFileRetryDelay(1)).toBe(5 * 60 * 1000); + expect(getExpiredFileRetryDelay(3)).toBe(20 * 60 * 1000); + + /* ...but never drops below the floor that protects the give-up budget. */ + process.env.FILE_RETENTION_SWEEP_INTERVAL_MS = '5'; + expect(getExpiredFileRetryDelay(1)).toBe(60 * 1000); + delete process.env.FILE_RETENTION_SWEEP_INTERVAL_MS; + expect(getFileRetentionMaxAttempts('0')).toBe(10); expect(getFileRetentionMaxAttempts('2.5')).toBe(10); expect(getFileRetentionMaxAttempts(' ')).toBe(10); diff --git a/packages/api/src/files/sweep.ts b/packages/api/src/files/sweep.ts index d3f7485de7..75dbd6e543 100644 --- a/packages/api/src/files/sweep.ts +++ b/packages/api/src/files/sweep.ts @@ -8,7 +8,7 @@ import type { AppConfig } from '@librechat/data-schemas'; const DEFAULT_FILE_RETENTION_SWEEP_INTERVAL_MS = 60 * 60 * 1000; const DEFAULT_FILE_RETENTION_MAX_ATTEMPTS = 10; -const FILE_RETENTION_RETRY_BASE_MS = 60 * 60 * 1000; +const MIN_FILE_RETENTION_RETRY_BASE_MS = 60 * 1000; const FILE_RETENTION_RETRY_MAX_MS = 24 * 60 * 60 * 1000; type ExpiredFile = { @@ -16,7 +16,6 @@ type ExpiredFile = { source?: string; user?: string | { toString?: () => string }; tenantId?: string; - deletionAttempts?: number; }; type SweepRequest = { @@ -58,6 +57,7 @@ type SweepDependencies = { req: SweepRequest; files: ExpiredFile[]; }) => Promise<{ deletedFileIds: string[]; failedFileIds: string[] }>; + incrementFileDeletionAttempts: (file_id: string) => Promise; deferExpiredFile: (file_id: string, deletionRetryAt: Date) => Promise; logger: SweepLogger; }; @@ -119,10 +119,21 @@ export function getFileRetentionMaxAttempts( * Backoff before the next attempt, doubling from one sweep interval up to a * day. `attempts` is the file's failure count including the one just * recorded, so the first retry lands on the following sweep. + * + * The base tracks the configured interval so the schedule stays meaningful + * at any cadence — a fixed hour would be inert for the first three attempts + * of a six-hour sweep, and would skip twelve passes of a five-minute one. + * The floor keeps a pathologically short interval from spending the whole + * give-up budget on a transient outage. */ export function getExpiredFileRetryDelay(attempts: number): number { + const interval = getFileRetentionSweepInterval(); + const base = Math.max( + interval > 0 ? interval : DEFAULT_FILE_RETENTION_SWEEP_INTERVAL_MS, + MIN_FILE_RETENTION_RETRY_BASE_MS, + ); const exponent = Math.max(0, attempts - 1); - return Math.min(FILE_RETENTION_RETRY_BASE_MS * 2 ** exponent, FILE_RETENTION_RETRY_MAX_MS); + return Math.min(base * 2 ** exponent, FILE_RETENTION_RETRY_MAX_MS); } export function getExpiredFileEndpoint(source?: string): string { @@ -244,7 +255,13 @@ export async function resolveExpiredFileSweepConfig({ export async function sweepExpiredFiles( { appConfig, limit = 100, loadAppConfig }: ExpiredFileSweepOptions | undefined = {}, - { getExpiredFiles, processDeleteRequest, deferExpiredFile, logger }: SweepDependencies, + { + getExpiredFiles, + processDeleteRequest, + incrementFileDeletionAttempts, + deferExpiredFile, + logger, + }: SweepDependencies, ): Promise { const maxAttempts = getFileRetentionMaxAttempts(); const files = (await getExpiredFiles(limit, { maxAttempts })) ?? []; @@ -258,10 +275,17 @@ export async function sweepExpiredFiles( * expired after it for the life of the deployment. */ const recordFailure = async (file: ExpiredFile): Promise => { failed++; - const attempts = (file.deletionAttempts ?? 0) + 1; - const retryAt = new Date(Date.now() + getExpiredFileRetryDelay(attempts)); + let attempts: number; try { - await deferExpiredFile(file.file_id, retryAt); + /* The attempt number comes from the increment rather than from the + * snapshot this batch queried, so two nodes sweeping the same file + * each get a distinct one and exactly one of them observes the cap + * being reached. */ + attempts = await incrementFileDeletionAttempts(file.file_id); + await deferExpiredFile( + file.file_id, + new Date(Date.now() + getExpiredFileRetryDelay(attempts)), + ); } catch (error) { logger.error( `[sweepExpiredFiles] Error recording failed deletion of expired file ${file.file_id}:`, diff --git a/packages/data-schemas/src/methods/file.spec.ts b/packages/data-schemas/src/methods/file.spec.ts index ef19ea43c9..ce885d671d 100644 --- a/packages/data-schemas/src/methods/file.spec.ts +++ b/packages/data-schemas/src/methods/file.spec.ts @@ -471,15 +471,11 @@ describe('File Methods', () => { }); describe('deferExpiredFile', () => { - it('increments the attempt count and stamps the next retry', async () => { - const userId = new mongoose.Types.ObjectId(); - const fileId = uuidv4(); - const retryAt = new Date('2030-02-01T00:00:00.000Z'); - + const createDeferrableFile = async (file_id: string) => { await fileMethods.createFile( { - file_id: fileId, - user: userId, + file_id, + user: new mongoose.Types.ObjectId(), filename: 'deferred.txt', filepath: '/uploads/deferred.txt', type: 'text/plain', @@ -488,14 +484,49 @@ describe('File Methods', () => { }, true, ); + }; - await fileMethods.deferExpiredFile(fileId, retryAt); + it('stamps the next retry and counts the attempt', async () => { + const fileId = uuidv4(); + const retryAt = new Date('2030-02-01T00:00:00.000Z'); + await createDeferrableFile(fileId); + + expect(await fileMethods.incrementFileDeletionAttempts(fileId)).toBe(1); await fileMethods.deferExpiredFile(fileId, retryAt); const [file] = (await fileMethods.getFiles({ file_id: fileId }))!; - expect(file.deletionAttempts).toBe(2); + expect(file.deletionAttempts).toBe(1); expect(file.deletionRetryAt).toEqual(retryAt); }); + + it('hands concurrent sweeps distinct attempt numbers', async () => { + const fileId = uuidv4(); + await createDeferrableFile(fileId); + + /* Two nodes recording a failure for the same file in the same pass. + * Deriving the attempt from the queried snapshot would give both the + * same number and neither would see the cap being reached. */ + const attempts = await Promise.all([ + fileMethods.incrementFileDeletionAttempts(fileId), + fileMethods.incrementFileDeletionAttempts(fileId), + fileMethods.incrementFileDeletionAttempts(fileId), + ]); + + expect([...attempts].sort()).toEqual([1, 2, 3]); + }); + + it('never pulls a deferral earlier than one already committed', async () => { + const fileId = uuidv4(); + const later = new Date('2030-02-01T00:00:00.000Z'); + const earlier = new Date('2030-01-01T00:00:00.000Z'); + await createDeferrableFile(fileId); + + await fileMethods.deferExpiredFile(fileId, later); + await fileMethods.deferExpiredFile(fileId, earlier); + + const [file] = (await fileMethods.getFiles({ file_id: fileId }))!; + expect(file.deletionRetryAt).toEqual(later); + }); }); describe('getToolFilesByIds', () => { diff --git a/packages/data-schemas/src/methods/file.ts b/packages/data-schemas/src/methods/file.ts index 19ffc1ca93..78135971b9 100644 --- a/packages/data-schemas/src/methods/file.ts +++ b/packages/data-schemas/src/methods/file.ts @@ -42,6 +42,7 @@ export function createFileMethods(mongoose: typeof import('mongoose')): { selectFields?: Record | string | null, ) => Promise; getExpiredFiles: (limit?: number, options?: ExpiredFileQueryOptions) => Promise; + incrementFileDeletionAttempts: (file_id: string) => Promise; deferExpiredFile: (file_id: string, deletionRetryAt: Date) => Promise; getToolFilesByIds: ( fileIds: string[], @@ -169,17 +170,37 @@ export function createFileMethods(mongoose: typeof import('mongoose')): { } /** - * Records a failed sweep deletion and holds the file back until - * `deletionRetryAt`. The counter is incremented server-side so - * concurrent sweeps on separate nodes cannot overwrite each other's - * progress toward the give-up cap. + * Records one failed sweep deletion and returns the file's resulting + * consecutive-failure count. + * + * The count comes back from the increment itself rather than being + * re-derived from the caller's snapshot. Two nodes sweeping the same file + * would otherwise each read the same value and each believe itself to be + * the same attempt, pushing the stored count past the give-up cap while + * both still think it is below — so the give-up would never be reported. + * Returning it here gives every caller a distinct attempt number. + */ + async function incrementFileDeletionAttempts(file_id: string): Promise { + const File = mongoose.models.File as Model; + const file = await File.findOneAndUpdate( + { file_id }, + { $inc: { deletionAttempts: 1 } }, + { new: true, projection: { deletionAttempts: 1 } }, + ).lean | null>(); + + return file?.deletionAttempts ?? 0; + } + + /** + * Holds a file back from the sweep until `deletionRetryAt`. + * + * Written with `$max` so a deferral can only ever move later. A node that + * computed a shorter backoff from a lower attempt count cannot pull the + * file forward past a longer one another node already committed. */ async function deferExpiredFile(file_id: string, deletionRetryAt: Date): Promise { const File = mongoose.models.File as Model; - await File.updateOne( - { file_id }, - { $inc: { deletionAttempts: 1 }, $set: { deletionRetryAt } }, - ).exec(); + await File.updateOne({ file_id }, { $max: { deletionRetryAt } }).exec(); } /** @@ -723,6 +744,7 @@ export function createFileMethods(mongoose: typeof import('mongoose')): { findFileById, getFiles, getExpiredFiles, + incrementFileDeletionAttempts, deferExpiredFile, getToolFilesByIds, getCodeGeneratedFiles,