From ee1bac5e16827f783736160b13c933b3efe3732d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 12:50:18 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Settle=20sweep=20retry=20?= =?UTF-8?q?state=20from=20the=20write,=20not=20the=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review of 0e52924. - `recordFailure` derived the attempt number by adding one to the count the batch had queried. Two nodes sweeping the same file read the same value, so both believed themselves to be the same attempt: each `$inc` landed, the stored count crossed `FILE_RETENTION_SWEEP_MAX_ATTEMPTS`, and neither caller ever saw the threshold. The file drops out of the query — the starvation guard still holds — but the give-up is never reported, and that log line is the operator's only notice, and carries the instruction for resuming. Return the count from the increment itself so every caller gets a distinct attempt number and exactly one observes the cap. The same staleness shortened the backoff, so `deferExpiredFile` now writes with `$max`: a deferral can only move later, and a node that computed a shorter delay cannot pull the file forward past one another node already committed. - The backoff doubled from a hard-coded hour while claiming to start from one sweep interval. At the default they coincide; away from it the schedule stops meaning anything — on a six-hour sweep the first three attempts all land on consecutive passes, and on a five-minute one the first retry skips twelve. Derive the base from `FILE_RETENTION_SWEEP_INTERVAL_MS`, floored at a minute so a pathologically short interval cannot spend the whole give-up budget on a transient outage. --- api/server/services/Files/process.js | 1 + api/server/services/Files/process.spec.js | 2 + packages/api/src/files/sweep.spec.ts | 75 ++++++++++++++++--- packages/api/src/files/sweep.ts | 38 ++++++++-- .../data-schemas/src/methods/file.spec.ts | 49 +++++++++--- packages/data-schemas/src/methods/file.ts | 38 ++++++++-- 6 files changed, 168 insertions(+), 35 deletions(-) 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,