diff --git a/.gitignore b/.gitignore index d28d18530e..89d6c2e99a 100644 --- a/.gitignore +++ b/.gitignore @@ -52,6 +52,7 @@ __blobstorage__/**/* # Deployed apps should consider commenting these lines out: # see https://npmjs.org/doc/faq.html#Should-I-check-my-node_modules-folder-into-git node_modules/ +.node_modules-* meili_data/ api/node_modules/ client/node_modules/ diff --git a/api/.node_modules-2xgm5Cvq b/api/.node_modules-2xgm5Cvq deleted file mode 120000 index 1fb9cb10df..0000000000 --- a/api/.node_modules-2xgm5Cvq +++ /dev/null @@ -1 +0,0 @@ -/Users/danny/Projects/LibreChat/api/node_modules \ No newline at end of file diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index 90d4eb81ee..d9bc56ec66 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -966,6 +966,8 @@ 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); /* A queue touch is not a send, so it must not inflate usage. */ expect(held.usage).toBe(0); }); @@ -973,17 +975,25 @@ describe('File Routes - Delete with Agent Access', () => { it('cannot be replayed to preserve a file indefinitely', async () => { const ownFileId = await createQueuedFile(new Date(Date.now() + 60 * 1000)); + const first = await request(app) + .post('/files/usage') + .send({ file_ids: [ownFileId] }); + expect(first.body).toEqual({ held: 1 }); + const afterFirst = (await File.findOne({ file_id: ownFileId }).lean()).expiresAt; + + /* Replay must be inert, not merely bounded: a deadline derived from the + * request clock would advance a window per call and never converge. */ for (let i = 0; i < 5; i++) { - const response = await request(app) + const repeat = await request(app) .post('/files/usage') .send({ file_ids: [ownFileId] }); - expect(response.status).toBe(200); + expect(repeat.status).toBe(200); + expect(repeat.body).toEqual({ held: 0 }); } const held = await File.findOne({ file_id: ownFileId }).lean(); expect(held.expiresAt).toBeDefined(); - /* Repeated touches converge on one bounded window, never unset the TTL. */ - expect(held.expiresAt.getTime()).toBeLessThan(Date.now() + 25 * 60 * 60 * 1000); + expect(held.expiresAt.getTime()).toBe(afterFirst.getTime()); }); it('never re-adds a TTL to a file that was already sent', async () => { diff --git a/client/.node_modules-101IklEQ b/client/.node_modules-101IklEQ deleted file mode 120000 index 49cee9a7d7..0000000000 --- a/client/.node_modules-101IklEQ +++ /dev/null @@ -1 +0,0 @@ -/Users/danny/Projects/LibreChat/client/node_modules \ No newline at end of file diff --git a/packages/api/.node_modules-uphMfdfi b/packages/api/.node_modules-uphMfdfi deleted file mode 120000 index 07571e485c..0000000000 --- a/packages/api/.node_modules-uphMfdfi +++ /dev/null @@ -1 +0,0 @@ -/Users/danny/Projects/LibreChat/packages/api/node_modules \ No newline at end of file diff --git a/packages/api/src/files/usage.spec.ts b/packages/api/src/files/usage.spec.ts index f705bc759a..e1ca1c24b2 100644 --- a/packages/api/src/files/usage.spec.ts +++ b/packages/api/src/files/usage.spec.ts @@ -2,11 +2,9 @@ import { handleFilesUsageRequest, FILES_USAGE_MAX_IDS, FILES_USAGE_HOLD_MS } fro describe('handleFilesUsageRequest', () => { const user = { id: 'user-1', tenantId: 'tenant-1' }; - const NOW = 1_700_000_000_000; const createDeps = (held = 0) => ({ extendFilesTTL: jest.fn().mockResolvedValue(held), - now: () => NOW, }); it('rejects unauthenticated requests without touching the DB', async () => { @@ -39,25 +37,30 @@ describe('handleFilesUsageRequest', () => { expect(deps.extendFilesTTL).not.toHaveBeenCalled(); }); - it('extends the hold owner-scoped by a bounded window', async () => { + it('requests an owner-scoped hold of the fixed 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'], - new Date(NOW + FILES_USAGE_HOLD_MS), - { user: 'user-1', tenantId: 'tenant-1' }, - ); + expect(deps.extendFilesTTL).toHaveBeenCalledWith(['f1', 'f2'], FILES_USAGE_HOLD_MS, { + user: 'user-1', + tenantId: 'tenant-1', + }); expect(result).toEqual({ status: 200, body: { held: 1 } }); }); - it('never requests an unbounded hold', async () => { + /** The hold must be a lifetime the data layer anchors to the upload, not a + * deadline derived here: a request-clock deadline would let a caller walk + * the file's lifetime forward one window per call. */ + it('passes a constant lifetime, never a request-derived deadline', async () => { const deps = createDeps(1); await handleFilesUsageRequest(user, { file_ids: ['f1'] }, deps); - const [, expiresAt] = deps.extendFilesTTL.mock.calls[0]; - expect(expiresAt).toBeInstanceOf(Date); - expect(Number.isFinite((expiresAt as Date).getTime())).toBe(true); - expect((expiresAt as Date).getTime()).toBeGreaterThan(NOW); + await handleFilesUsageRequest(user, { file_ids: ['f1'] }, deps); + + const [, firstHold] = deps.extendFilesTTL.mock.calls[0]; + const [, secondHold] = deps.extendFilesTTL.mock.calls[1]; + expect(firstHold).toBe(FILES_USAGE_HOLD_MS); + expect(secondHold).toBe(firstHold); + expect(typeof firstHold).toBe('number'); }); it('returns 200 with zero held when no id resolves to an owned file', async () => { diff --git a/packages/api/src/files/usage.ts b/packages/api/src/files/usage.ts index b0494b72c2..d73fd55c16 100644 --- a/packages/api/src/files/usage.ts +++ b/packages/api/src/files/usage.ts @@ -2,9 +2,13 @@ export const FILES_USAGE_MAX_IDS: number = 10; /** - * How far forward a single touch pushes the upload-window TTL. Generous + * 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. */ export const FILES_USAGE_HOLD_MS: number = 24 * 60 * 60 * 1000; @@ -27,11 +31,9 @@ export interface FilesUsageDeps { /** Owner-scoped TTL hold (`db.extendFilesTTL`-shaped). */ extendFilesTTL: ( fileIds: string[], - expiresAt: Date, + holdMs: number, owner: { user: string; tenantId?: string | null }, ) => Promise; - /** Injectable clock for deterministic tests. */ - now?: () => number; } /** @@ -43,7 +45,8 @@ export interface FilesUsageDeps { * A hold, not a release. The client queue is ephemeral browser state, so a * closed tab or a cleared queue leaves nothing referencing these files, and * clearing the TTL outright would strand them in storage permanently. The - * deadline is only pushed forward; the real release happens at send, where + * hold only widens the window to a fixed point measured from upload, so it + * is idempotent under replay; the real release happens at send, where * `updateFilesUsage` marks the files used against an actual message. * * Best-effort 200: ids that do not resolve to a held file are not errors @@ -71,8 +74,7 @@ export async function handleFilesUsageRequest( } fileIds.push(value); } - const nowMs = deps.now?.() ?? Date.now(); - const held = await deps.extendFilesTTL(fileIds, new Date(nowMs + FILES_USAGE_HOLD_MS), { + const held = await deps.extendFilesTTL(fileIds, FILES_USAGE_HOLD_MS, { user: user.id, tenantId: user.tenantId, }); diff --git a/packages/data-provider/.node_modules-7gquYG9s b/packages/data-provider/.node_modules-7gquYG9s deleted file mode 120000 index 8b9f8e521a..0000000000 --- a/packages/data-provider/.node_modules-7gquYG9s +++ /dev/null @@ -1 +0,0 @@ -/Users/danny/Projects/LibreChat/packages/data-provider/node_modules \ No newline at end of file diff --git a/packages/data-schemas/src/methods/file.spec.ts b/packages/data-schemas/src/methods/file.spec.ts index 6850ee36a3..2fec751042 100644 --- a/packages/data-schemas/src/methods/file.spec.ts +++ b/packages/data-schemas/src/methods/file.spec.ts @@ -1124,6 +1124,8 @@ describe('File Methods', () => { }); describe('extendFilesTTL', () => { + const HOLD_MS = 24 * 3_600_000; + const seedTempFile = async (userId: mongoose.Types.ObjectId, expiresAt: Date) => { const fileId = uuidv4(); await fileMethods.createFile({ @@ -1138,18 +1140,48 @@ describe('File Methods', () => { return fileId; }; - it('pushes the TTL forward without unsetting it', async () => { - const userId = new mongoose.Types.ObjectId(); - const soon = new Date(Date.now() + 60_000); - const fileId = await seedTempFile(userId, soon); - const target = new Date(Date.now() + 3_600_000); + const readCreatedAt = async (fileId: string) => { + const doc = await mongoose.models.File.findOne({ file_id: fileId }) + .lean<{ createdAt: Date }>() + .exec(); + return doc!.createdAt; + }; - const count = await fileMethods.extendFilesTTL([fileId], target, { user: String(userId) }); + it('widens the TTL to createdAt + holdMs without unsetting it', async () => { + const userId = new mongoose.Types.ObjectId(); + const fileId = await seedTempFile(userId, new Date(Date.now() + 60_000)); + const createdAt = await readCreatedAt(fileId); + + const count = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: String(userId) }); expect(count).toBe(1); const file = await fileMethods.findFileById(fileId); expect(file?.expiresAt).toBeDefined(); - expect(file?.expiresAt?.getTime()).toBe(target.getTime()); + expect(file?.expiresAt?.getTime()).toBe(createdAt.getTime() + HOLD_MS); + }); + + /** The bound that makes the endpoint safe to expose: the deadline is a + * function of the immutable createdAt, so replaying the call can never + * walk a file's lifetime forward one window at a time. */ + it('is idempotent under replay, never advancing the deadline', async () => { + const userId = new mongoose.Types.ObjectId(); + const fileId = await seedTempFile(userId, new Date(Date.now() + 60_000)); + const createdAt = await readCreatedAt(fileId); + + const first = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: String(userId) }); + const afterFirst = (await fileMethods.findFileById(fileId))?.expiresAt; + + for (let i = 0; i < 5; i++) { + const repeat = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { + user: String(userId), + }); + expect(repeat).toBe(0); + } + + expect(first).toBe(1); + const file = await fileMethods.findFileById(fileId); + expect(file?.expiresAt?.getTime()).toBe(afterFirst?.getTime()); + expect(file?.expiresAt?.getTime()).toBe(createdAt.getTime() + HOLD_MS); }); it('does not resurrect a TTL on an already-released file', async () => { @@ -1157,9 +1189,7 @@ describe('File Methods', () => { const fileId = await seedTempFile(userId, new Date(Date.now() + 60_000)); await fileMethods.updateFileUsage({ file_id: fileId, user: String(userId) }); - const count = await fileMethods.extendFilesTTL([fileId], new Date(Date.now() + 3_600_000), { - user: String(userId), - }); + const count = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: String(userId) }); expect(count).toBe(0); const file = await fileMethods.findFileById(fileId); @@ -1171,9 +1201,7 @@ describe('File Methods', () => { const farOut = new Date(Date.now() + 7 * 24 * 3_600_000); const fileId = await seedTempFile(userId, farOut); - const count = await fileMethods.extendFilesTTL([fileId], new Date(Date.now() + 60_000), { - user: String(userId), - }); + const count = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: String(userId) }); expect(count).toBe(0); const file = await fileMethods.findFileById(fileId); @@ -1186,7 +1214,7 @@ describe('File Methods', () => { const soon = new Date(Date.now() + 60_000); const fileId = await seedTempFile(ownerId, soon); - const count = await fileMethods.extendFilesTTL([fileId], new Date(Date.now() + 3_600_000), { + const count = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: String(attackerId), }); @@ -1200,14 +1228,25 @@ describe('File Methods', () => { const soon = new Date(Date.now() + 60_000); const fileId = await seedTempFile(userId, soon); - const count = await fileMethods.extendFilesTTL([fileId], new Date(Date.now() + 3_600_000), { - user: '', - } as { user: string }); + const count = await fileMethods.extendFilesTTL([fileId], HOLD_MS, { user: '' } as { + user: string; + }); expect(count).toBe(0); const file = await fileMethods.findFileById(fileId); expect(file?.expiresAt?.getTime()).toBe(soon.getTime()); }); + + it('is a no-op for a non-positive hold', async () => { + const userId = new mongoose.Types.ObjectId(); + const soon = new Date(Date.now() + 60_000); + const fileId = await seedTempFile(userId, soon); + + expect(await fileMethods.extendFilesTTL([fileId], 0, { user: String(userId) })).toBe(0); + expect(await fileMethods.extendFilesTTL([fileId], -1, { user: String(userId) })).toBe(0); + const file = await fileMethods.findFileById(fileId); + expect(file?.expiresAt?.getTime()).toBe(soon.getTime()); + }); }); describe('deleteFile', () => { diff --git a/packages/data-schemas/src/methods/file.ts b/packages/data-schemas/src/methods/file.ts index 68fe66c5ab..3d12454dc5 100644 --- a/packages/data-schemas/src/methods/file.ts +++ b/packages/data-schemas/src/methods/file.ts @@ -84,7 +84,7 @@ export function createFileMethods(mongoose: typeof import('mongoose')): { ) => Promise; extendFilesTTL: ( fileIds: string[], - expiresAt: Date, + holdMs: number, owner: { user: string; tenantId?: string | null }, ) => Promise; sweepOrphanedPreviews: (maxAgeMs?: number) => Promise; @@ -552,42 +552,59 @@ export function createFileMethods(mongoose: typeof import('mongoose')): { } /** - * Pushes the upload-window TTL of owned, still-temporary files forward. + * Widens the upload-window TTL of owned, still-temporary files to + * `createdAt + holdMs`. * * A renewable hold, not a release: unlike `updateFileUsage` this never * unsets `expiresAt`, so a file that is held but never actually sent is - * still reaped once the hold lapses. Two filter guards make the write - * safe to expose to a client: - * - `$exists: true`: a file whose TTL was already cleared (a real send) - * is permanent; re-adding `expiresAt` would schedule it for deletion. - * - `$lt: expiresAt`: the hold only ever moves the deadline later. + * still reaped once the hold lapses. Three properties hold by + * construction, which is what makes the write safe to drive from a + * client-supplied id list: + * - the new deadline is anchored to the immutable `createdAt`, never to + * the request clock, so replaying the call is idempotent and cannot + * walk a file's lifetime forward indefinitely; + * - `$max` against the current value means a hold only ever widens; + * - `expiresAt: { $exists: true }` means a file whose TTL was already + * cleared by a real send stays permanent. Re-adding `expiresAt` there + * would schedule a live file for deletion. * - * The owner scope is required, not optional: this write is driven by a - * client-supplied id list, so an unscoped call would hold every user's - * matching file. A missing owner is a no-op rather than a wide update. + * `createdAt` is required rather than defaulted: without the anchor there + * is no bound to enforce, so such a file is skipped instead of held. + * + * The owner scope is required, not optional: an unscoped call would hold + * every user's matching file. A missing owner is a no-op, not a wide + * update. * * @param fileIds - File IDs to hold - * @param expiresAt - New expiry; only applied where it is later than the current one + * @param holdMs - Lifetime granted from upload time * @param owner - Owner scope; mismatches leave the TTL unchanged - * @returns Number of files whose hold was extended + * @returns Number of files whose hold was widened */ async function extendFilesTTL( fileIds: string[], - expiresAt: Date, + holdMs: number, owner: { user: string; tenantId?: string | null }, ): Promise { - if (fileIds.length === 0 || !owner?.user) { + if (fileIds.length === 0 || !owner?.user || !(holdMs > 0)) { return 0; } const File = mongoose.models.File as Model; const filter = withOwnerScope( { file_id: { $in: [...new Set(fileIds)] }, - expiresAt: { $exists: true, $lt: expiresAt }, + expiresAt: { $exists: true }, + createdAt: { $exists: true }, }, { userId: owner.user, tenantId: owner.tenantId }, ); - const result = await File.updateMany(filter, { $set: { expiresAt } }); + const result = await File.updateMany( + filter, + [{ $set: { expiresAt: { $max: ['$expiresAt', { $add: ['$createdAt', holdMs] }] } } }], + /** `timestamps: false`: a hold is TTL bookkeeping, not a content write. + * Bumping `updatedAt` would also make every re-touch count as a + * modification, hiding whether the deadline actually moved. */ + { timestamps: false }, + ); return result.modifiedCount ?? 0; }