From 2bd3c5293a04f397a1801b59bf19538c9afc69a2 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 27 Jul 2026 22:31:45 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Track=20the=20configured?= =?UTF-8?q?=20approval=20window=20in=20the=20`/files/usage`=20hold?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on 9277620. `endpoints.agents.checkpointer.ttl` is a positive int with no upper bound, and its docs invite raising it for longer review windows. It drives the pending-action expiry, so a run can legitimately stay paused past 24h. The fixed 24h lifetime would then let Mongo reap an attachment while its approval was still live, and the later queue drain would send a file that no longer exists. Replace the fixed constant with `resolveFilesUsageHoldMs`, which adds the configured approval window to a 24h baseline covering upload, enqueue, and the run reaching its pause. The route reads the window from the same `getApprovalTtlMs(checkpointerCfg)` the pending action uses, so the two stay in lockstep. The replay bound is unaffected: the window is a per-deployment constant and the deadline is still `createdAt + holdMs`, so a replayed touch re-asserts the same instant and `$max` skips the write. Only an operator config change moves it, never a client. --- api/server/routes/files/files.js | 8 +++-- api/server/routes/files/files.test.js | 7 +++-- packages/api/src/files/usage.spec.ts | 42 ++++++++++++++++++++++++--- packages/api/src/files/usage.ts | 41 ++++++++++++++++++++------ 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index d4b579d13d..458d9ec1d0 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -3,6 +3,7 @@ const express = require('express'); const { logger, SystemCapabilities } = require('@librechat/data-schemas'); const { logAxiosError, + getApprovalTtlMs, refreshS3FileUrls, handleFilesUsageRequest, shouldUseUploadSse, @@ -147,13 +148,16 @@ router.get('/config', async (req, res) => { * Owner-scoped TTL hold for uploads sitting in a client-side queue (mid-run * queued messages), so the upload-window TTL cannot reap them before drain. * Extends the deadline rather than clearing it; the real release happens at - * send. Thin wrapper: validation, cap, hold window, and best-effort semantics - * live in `@librechat/api` (`handleFilesUsageRequest`). + * send. The approval window is passed through so a queue waiting on a paused + * run outlives that pause. Thin wrapper: validation, cap, hold window, and + * best-effort semantics live in `@librechat/api` (`handleFilesUsageRequest`). */ router.post('/usage', async (req, res) => { try { + const checkpointerCfg = req.config?.endpoints?.[EModelEndpoint.agents]?.checkpointer; const { status, body } = await handleFilesUsageRequest(req.user ?? {}, req.body ?? {}, { extendFilesTTL: db.extendFilesTTL, + approvalTtlMs: getApprovalTtlMs(checkpointerCfg), }); return res.status(status).json(body); } catch (error) { diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index d9bc56ec66..d9c81efa09 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -966,8 +966,11 @@ describe('File Routes - Delete with Agent Access', () => { /* The hold must remain a hold: still reapable, just later. */ expect(held.expiresAt).toBeDefined(); expect(held.expiresAt.getTime()).toBeGreaterThan(soon.getTime()); - /* Anchored to upload time, so the deadline is a fixed point per file. */ - expect(held.expiresAt.getTime()).toBe(held.createdAt.getTime() + 24 * 60 * 60 * 1000); + /* Anchored to upload time, so the deadline is a fixed point per file: + * the 24h baseline plus the default 24h approval window, so a queue + * waiting on a paused run outlives that pause. */ + const HOUR = 60 * 60 * 1000; + expect(held.expiresAt.getTime()).toBe(held.createdAt.getTime() + 24 * HOUR + 24 * HOUR); /* A queue touch is not a send, so it must not inflate usage. */ expect(held.usage).toBe(0); }); diff --git a/packages/api/src/files/usage.spec.ts b/packages/api/src/files/usage.spec.ts index e1ca1c24b2..133d775488 100644 --- a/packages/api/src/files/usage.spec.ts +++ b/packages/api/src/files/usage.spec.ts @@ -1,4 +1,9 @@ -import { handleFilesUsageRequest, FILES_USAGE_MAX_IDS, FILES_USAGE_HOLD_MS } from './usage'; +import { + handleFilesUsageRequest, + resolveFilesUsageHoldMs, + FILES_USAGE_BASE_HOLD_MS, + FILES_USAGE_MAX_IDS, +} from './usage'; describe('handleFilesUsageRequest', () => { const user = { id: 'user-1', tenantId: 'tenant-1' }; @@ -37,11 +42,11 @@ describe('handleFilesUsageRequest', () => { expect(deps.extendFilesTTL).not.toHaveBeenCalled(); }); - it('requests an owner-scoped hold of the fixed lifetime', async () => { + it('requests an owner-scoped hold of the resolved lifetime', async () => { const deps = createDeps(1); const result = await handleFilesUsageRequest(user, { file_ids: ['f1', 'f2'] }, deps); expect(deps.extendFilesTTL).toHaveBeenCalledTimes(1); - expect(deps.extendFilesTTL).toHaveBeenCalledWith(['f1', 'f2'], FILES_USAGE_HOLD_MS, { + expect(deps.extendFilesTTL).toHaveBeenCalledWith(['f1', 'f2'], FILES_USAGE_BASE_HOLD_MS, { user: 'user-1', tenantId: 'tenant-1', }); @@ -58,11 +63,40 @@ describe('handleFilesUsageRequest', () => { const [, firstHold] = deps.extendFilesTTL.mock.calls[0]; const [, secondHold] = deps.extendFilesTTL.mock.calls[1]; - expect(firstHold).toBe(FILES_USAGE_HOLD_MS); + expect(firstHold).toBe(FILES_USAGE_BASE_HOLD_MS); expect(secondHold).toBe(firstHold); expect(typeof firstHold).toBe('number'); }); + /** A paused run's approval window has no configured upper bound, so a fixed + * lifetime shorter than it would reap an attachment whose approval is + * still live and leave the drain sending a missing file. */ + it('stretches the hold to outlast a long configured approval window', async () => { + const sevenDaysMs = 7 * 24 * 60 * 60 * 1000; + const deps = { ...createDeps(1), approvalTtlMs: sevenDaysMs }; + await handleFilesUsageRequest(user, { file_ids: ['f1'] }, deps); + + const [, holdMs] = deps.extendFilesTTL.mock.calls[0]; + expect(holdMs).toBe(FILES_USAGE_BASE_HOLD_MS + sevenDaysMs); + expect(holdMs).toBeGreaterThan(sevenDaysMs); + }); + + describe('resolveFilesUsageHoldMs', () => { + it('covers the approval window on top of the baseline', () => { + expect(resolveFilesUsageHoldMs(1000)).toBe(FILES_USAGE_BASE_HOLD_MS + 1000); + }); + + it.each([ + ['undefined', undefined], + ['zero', 0], + ['negative', -1], + ['NaN', Number.NaN], + ['Infinity', Number.POSITIVE_INFINITY], + ])('falls back to the baseline for %s', (_label, value) => { + expect(resolveFilesUsageHoldMs(value)).toBe(FILES_USAGE_BASE_HOLD_MS); + }); + }); + it('returns 200 with zero held when no id resolves to an owned file', async () => { const deps = createDeps(0); const result = await handleFilesUsageRequest(user, { file_ids: ['not-owned'] }, deps); diff --git a/packages/api/src/files/usage.ts b/packages/api/src/files/usage.ts index d73fd55c16..dc7a3e024a 100644 --- a/packages/api/src/files/usage.ts +++ b/packages/api/src/files/usage.ts @@ -2,15 +2,33 @@ export const FILES_USAGE_MAX_IDS: number = 10; /** - * Total lifetime a held upload gets, measured from its upload time. Generous - * enough to outlast any realistic queue wait (long run, approval pause), - * short enough that a queue the user abandons still gets reaped. - * - * Measured from upload rather than from the request, so the deadline is a - * fixed point per file: replaying the touch re-asserts the same instant - * instead of walking the file's lifetime forward a window at a time. + * Baseline lifetime a held upload gets, measured from its upload time. Covers + * the span a queued attachment spends before a run can even pause: upload, + * enqueue, and the run itself reaching an approval point. */ -export const FILES_USAGE_HOLD_MS: number = 24 * 60 * 60 * 1000; +export const FILES_USAGE_BASE_HOLD_MS: number = 24 * 60 * 60 * 1000; + +/** + * Total lifetime granted to a held upload, measured from its upload time. + * + * A queued message legitimately outlives the upload window when a run pauses + * for approval, and that pause is bounded by the deployment's configured + * approval window (`endpoints.agents.checkpointer.ttl`), which has no upper + * limit. A fixed lifetime shorter than that window would let Mongo reap an + * attachment while its approval is still live, so the hold tracks the + * configured window instead of assuming one. + * + * Still a constant per deployment, and still measured from upload rather than + * from the request, so the deadline stays a fixed point per file: replaying + * the touch re-asserts the same instant instead of walking the file's + * lifetime forward a window at a time. + * + * @param approvalTtlMs - Configured approval window; falsy means none configured + */ +export function resolveFilesUsageHoldMs(approvalTtlMs?: number): number { + const approval = Number.isFinite(approvalTtlMs) && approvalTtlMs! > 0 ? approvalTtlMs! : 0; + return FILES_USAGE_BASE_HOLD_MS + approval; +} export interface FilesUsageUser { id?: string; @@ -34,6 +52,8 @@ export interface FilesUsageDeps { holdMs: number, owner: { user: string; tenantId?: string | null }, ) => Promise; + /** Configured approval window, so the hold outlasts a paused run. */ + approvalTtlMs?: number; } /** @@ -49,6 +69,9 @@ export interface FilesUsageDeps { * is idempotent under replay; the real release happens at send, where * `updateFilesUsage` marks the files used against an actual message. * + * The window tracks the deployment's configured approval TTL so a queue + * waiting on a paused run cannot be reaped while that approval is live. + * * Best-effort 200: ids that do not resolve to a held file are not errors * (they may be already-sent files, or not owned). */ @@ -74,7 +97,7 @@ export async function handleFilesUsageRequest( } fileIds.push(value); } - const held = await deps.extendFilesTTL(fileIds, FILES_USAGE_HOLD_MS, { + const held = await deps.extendFilesTTL(fileIds, resolveFilesUsageHoldMs(deps.approvalTtlMs), { user: user.id, tenantId: user.tenantId, });