diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index ef3783cfdc..f74dadeb88 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -470,6 +470,27 @@ const deleteUserController = async (req, res) => { logger.info(`User deleted account. Email: ${user.email} ID: ${user.id}`); res.status(200).send({ message: 'User deleted' }); } catch (err) { + // The account survives this failed attempt, so its schedules must too: restore the + // exact rows this attempt suspended (re-enabled/re-armed from their snapshot). Fenced + // to the token, so a schedule the owner deleted meanwhile is not resurrected. A + // successful deletion never reaches here (userDeleted short-circuits it). + // + // RESTORE BEFORE RELEASING THE DELETION FENCE. That fence is what refuses new schedule + // writes/claims for this user; releasing it first opens a window where an owner PATCH + // could edit a still-suspended row and then have its enabled/next-run state overwritten + // by this older snapshot, and where a second deletion attempt could re-suspend these + // rows under a new token — making this restore a no-op and stranding the disabled + // snapshot permanently. + if (scheduleSuspensionToken != null && !userDeleted) { + try { + await restoreUserSchedulesFromDeletion(user.id, scheduleSuspensionToken); + } catch (restoreError) { + logger.error( + '[deleteUserController] Failed to restore suspended schedules after a cancelled deletion', + restoreError, + ); + } + } if (triggerDeletionFence != null && !userDeleted) { try { await cancelAgentTriggerUserPurge(user.id, triggerDeletionFence); @@ -485,20 +506,6 @@ const deleteUserController = async (req, res) => { logger.error('[deleteUserController] Failed to release trigger deletion fence', fenceError); } } - // The account survives this failed attempt, so its schedules must too: restore the - // exact rows this attempt suspended (re-enabled/re-armed from their snapshot). Fenced - // to the token, so a schedule the owner deleted meanwhile is not resurrected. A - // successful deletion never reaches here (userDeleted short-circuits it). - if (scheduleSuspensionToken != null && !userDeleted) { - try { - await restoreUserSchedulesFromDeletion(user.id, scheduleSuspensionToken); - } catch (restoreError) { - logger.error( - '[deleteUserController] Failed to restore suspended schedules after a cancelled deletion', - restoreError, - ); - } - } logger.error('[deleteUserController]', err); return res.status(500).json({ message: 'Something went wrong.' }); } diff --git a/api/server/controllers/UserController.spec.js b/api/server/controllers/UserController.spec.js index 1dec99a373..89af705a56 100644 --- a/api/server/controllers/UserController.spec.js +++ b/api/server/controllers/UserController.spec.js @@ -449,6 +449,12 @@ describe('deleteUserController', () => { userIdString, mockQuiesceUserSchedules.mock.calls[0][1], ); + // BEFORE the deletion fence is released: that fence is what refuses new schedule writes, + // so restoring after it would let an owner PATCH — or a second deletion attempt + // re-suspending under a new token — race the restore and strand the disabled snapshot. + expect(mockRestoreUserSchedules.mock.invocationCallOrder[0]).toBeLessThan( + cancelAgentTriggerUserDeletion.mock.invocationCallOrder[0], + ); }); it('fails closed before data cleanup when detached subagents do not drain', async () => { @@ -501,6 +507,12 @@ describe('deleteUserController', () => { userIdString, mockQuiesceUserSchedules.mock.calls[0][1], ); + // BEFORE the deletion fence is released: that fence is what refuses new schedule writes, + // so restoring after it would let an owner PATCH — or a second deletion attempt + // re-suspending under a new token — race the restore and strand the disabled snapshot. + expect(mockRestoreUserSchedules.mock.invocationCallOrder[0]).toBeLessThan( + cancelAgentTriggerUserDeletion.mock.invocationCallOrder[0], + ); }); it('should remove the user from all groups via $pullAll', async () => { diff --git a/config/__tests__/delete-user.spec.js b/config/__tests__/delete-user.spec.js index 73acd879c2..282fe37767 100644 --- a/config/__tests__/delete-user.spec.js +++ b/config/__tests__/delete-user.spec.js @@ -14,7 +14,8 @@ const mockMethods = { beginAgentTriggerUserDeletion: jest.fn(), recoverStaleAgentTriggerUserDeletion: jest.fn(), prepareAgentTriggerUserPurge: jest.fn(), - disableUserSchedulesForDeletion: jest.fn(), + suspendUserSchedulesForDeletion: jest.fn(), + restoreUserSchedulesFromDeletion: jest.fn(), countActiveAgentTriggerDeliveriesByUser: jest.fn(), deleteSchedulesByUser: jest.fn(), deleteUserById: jest.fn(), @@ -91,7 +92,8 @@ describe('Delete user CLI', () => { mockMethods.beginAgentTriggerUserDeletion.mockReset().mockResolvedValue('acquired'); mockMethods.recoverStaleAgentTriggerUserDeletion.mockReset().mockResolvedValue('acquired'); mockMethods.prepareAgentTriggerUserPurge.mockReset().mockResolvedValue(undefined); - mockMethods.disableUserSchedulesForDeletion.mockReset().mockResolvedValue(undefined); + mockMethods.suspendUserSchedulesForDeletion.mockReset().mockResolvedValue(undefined); + mockMethods.restoreUserSchedulesFromDeletion.mockReset().mockResolvedValue(undefined); mockMethods.countActiveAgentTriggerDeliveriesByUser.mockReset().mockResolvedValue(0); mockMethods.deleteSchedulesByUser.mockReset().mockResolvedValue(undefined); mockMethods.deleteUserById.mockReset().mockResolvedValue({ deletedCount: 1 }); @@ -122,14 +124,17 @@ describe('Delete user CLI', () => { expect.any(Date), 'tenant-1', ); - expect(mockMethods.disableUserSchedulesForDeletion).toHaveBeenCalledWith(USER_ID); + expect(mockMethods.suspendUserSchedulesForDeletion).toHaveBeenCalledWith( + USER_ID, + expect.any(String), + ); expect(mockMethods.beginAgentTriggerUserDeletion.mock.invocationCallOrder[0]).toBeLessThan( mockMethods.prepareAgentTriggerUserPurge.mock.invocationCallOrder[0], ); expect(mockMethods.prepareAgentTriggerUserPurge.mock.invocationCallOrder[0]).toBeLessThan( - mockMethods.disableUserSchedulesForDeletion.mock.invocationCallOrder[0], + mockMethods.suspendUserSchedulesForDeletion.mock.invocationCallOrder[0], ); - expect(mockMethods.disableUserSchedulesForDeletion.mock.invocationCallOrder[0]).toBeLessThan( + expect(mockMethods.suspendUserSchedulesForDeletion.mock.invocationCallOrder[0]).toBeLessThan( mockMethods.countActiveAgentTriggerDeliveriesByUser.mock.invocationCallOrder[0], ); expect( @@ -154,7 +159,7 @@ describe('Delete user CLI', () => { }); it('deletes nothing and releases both fences when schedule quiescing fails', async () => { - mockMethods.disableUserSchedulesForDeletion.mockRejectedValueOnce( + mockMethods.suspendUserSchedulesForDeletion.mockRejectedValueOnce( new Error('schedule write failed'), ); @@ -168,6 +173,33 @@ describe('Delete user CLI', () => { expect(mockMethods.cancelAgentTriggerUserDeletion).toHaveBeenCalledWith(USER_ID, deletionFence); }); + /** + * The suspension is reversible, so a CLI attempt that does not commit must restore the + * rows it suspended — and must do so while the user-deletion fence still refuses new + * schedule writes, or an owner edit (or a second attempt re-suspending under a new token) + * can race the restore and strand the disabled snapshot. + */ + it('restores suspended schedules BEFORE releasing the deletion fence on failure', async () => { + mockGetCleanupBlockingJobIdsForUser.mockResolvedValueOnce(['stream-1']); + mockAbortJob.mockRejectedValueOnce(new Error('provider drain failed')); + + expect(await runCli()).toBe(1); + + const token = mockMethods.suspendUserSchedulesForDeletion.mock.calls[0][1]; + expect(token).toEqual(expect.any(String)); + expect(mockMethods.restoreUserSchedulesFromDeletion).toHaveBeenCalledWith(USER_ID, token); + expect(mockMethods.restoreUserSchedulesFromDeletion.mock.invocationCallOrder[0]).toBeLessThan( + mockMethods.cancelAgentTriggerUserDeletion.mock.invocationCallOrder[0], + ); + }); + + it('never restores schedules after a committed deletion', async () => { + expect(await runCli()).toBe(0); + + expect(mockMethods.deleteSchedulesByUser).toHaveBeenCalledWith(USER_ID); + expect(mockMethods.restoreUserSchedulesFromDeletion).not.toHaveBeenCalled(); + }); + it('deletes nothing and releases both fences when provider drain fails', async () => { mockGetCleanupBlockingJobIdsForUser.mockResolvedValueOnce(['stream-1']); mockAbortJob.mockRejectedValueOnce(new Error('provider drain failed')); diff --git a/config/delete-user.js b/config/delete-user.js index 18dc4048d7..c6221d722a 100644 --- a/config/delete-user.js +++ b/config/delete-user.js @@ -3,6 +3,7 @@ // @ts-nocheck const path = require('path'); const mongoose = require('mongoose'); +const { randomUUID } = require('node:crypto'); const { createModels, createMethods, runAsSystem } = require('@librechat/data-schemas'); const { Key, @@ -118,6 +119,7 @@ async function gracefulExit(code = 0) { } let deletionFence; + let scheduleSuspensionToken; let userDeleted = false; try { @@ -158,7 +160,13 @@ async function gracefulExit(code = 0) { await runAsSystem(() => methods.prepareAgentTriggerUserPurge(uid, deletionFence, user.tenantId), ); - await runAsSystem(() => methods.disableUserSchedulesForDeletion(uid)); + // Reversible, token-fenced suspension (the same protocol the HTTP controller uses): + // an attempt that does not commit restores exactly these rows in the finally block, + // rather than leaving a surviving account with disabled, erasure-eligible schedules. + scheduleSuspensionToken = randomUUID(); + await runAsSystem(() => + methods.suspendUserSchedulesForDeletion(uid, scheduleSuspensionToken), + ); if (hasSharedGenerationStore) { const deadline = Date.now() + TRIGGER_DRAIN_TIMEOUT_MS; while ( @@ -228,6 +236,14 @@ async function gracefulExit(code = 0) { userDeleted = true; await runAsSystem(() => methods.deleteAgentTriggerDeliveriesByUser(uid)); } finally { + // RESTORE BEFORE RELEASING THE FENCE. While the user-deletion fence is still armed, new + // schedule writes/claims are refused, so this restore cannot race an owner PATCH nor be + // superseded by a second deletion attempt re-suspending these rows under a new token. + if (scheduleSuspensionToken != null && !userDeleted) { + await runAsSystem(() => + methods.restoreUserSchedulesFromDeletion(uid, scheduleSuspensionToken), + ).catch((error) => console.error('Failed to restore suspended schedules:', error)); + } if (deletionFence != null && !userDeleted) { await runAsSystem(() => methods.cancelAgentTriggerUserPurge(uid, deletionFence)).catch( (error) => console.error('Failed to disarm trigger purge recovery:', error), diff --git a/packages/data-schemas/src/methods/schedule.methods.spec.ts b/packages/data-schemas/src/methods/schedule.methods.spec.ts index d8105aba39..4875bb2355 100644 --- a/packages/data-schemas/src/methods/schedule.methods.spec.ts +++ b/packages/data-schemas/src/methods/schedule.methods.spec.ts @@ -1143,6 +1143,33 @@ describe('reversible account-deletion suspension', () => { expect(after.deletionSuspension?.nextRunAt?.getTime()).toBe(armedAt.getTime()); }); + /** + * A crash between suspend and restore leaves an ABANDONED suspension. A later attempt + * must adopt that attempt's snapshot rather than re-snapshotting the row's current + * (already-suspended) state — otherwise the true pre-suspension state is lost and the + * schedule stays disabled forever once the second attempt restores. + */ + it('a second attempt adopts the abandoned snapshot instead of capturing suspended state', async () => { + const armedAt = new Date('2030-01-01T00:00:00Z'); + const schedule = await methods.createSchedule(scheduleData({ nextRunAt: armedAt })); + await methods.suspendUserSchedulesForDeletion(schedule.user, 'attempt-1'); + + // attempt-1's process died without restoring; attempt-2 now suspends the same rows. + await methods.suspendUserSchedulesForDeletion(schedule.user, 'attempt-2'); + + const suspended = await getSchedule(schedule.id); + expect(suspended.deletionSuspension?.token).toBe('attempt-2'); + // The ORIGINAL pre-suspension state, not the suspended state attempt-2 observed. + expect(suspended.deletionSuspension?.enabled).toBe(true); + expect(suspended.deletionSuspension?.nextRunAt?.getTime()).toBe(armedAt.getTime()); + + await methods.restoreUserSchedulesFromDeletion(schedule.user, 'attempt-2'); + const restored = await getSchedule(schedule.id); + expect(restored.enabled).toBe(true); + expect(restored.nextRunAt?.getTime()).toBe(armedAt.getTime()); + expect(restored.deletionSuspension).toBeUndefined(); + }); + it('a successful deletion removes suspended rows and their snapshots', async () => { const userId = new mongoose.Types.ObjectId(); const schedule = await methods.createSchedule(scheduleData({ user: userId })); diff --git a/packages/data-schemas/src/methods/schedule.ts b/packages/data-schemas/src/methods/schedule.ts index 58ef26a688..5a21abb279 100644 --- a/packages/data-schemas/src/methods/schedule.ts +++ b/packages/data-schemas/src/methods/schedule.ts @@ -1672,33 +1672,44 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche deleting: { $ne: true }, 'deletionSuspension.token': { $ne: token }, }) - .select('id enabled nextRunAt') - .lean>>(); + .select('id enabled nextRunAt deletionSuspension') + .lean>>(); if (schedules.length === 0) { return; } - const ops = schedules.map((schedule) => ({ - updateOne: { - filter: { - id: schedule.id, - user: userId, - deleting: { $ne: true }, - 'deletionSuspension.token': { $ne: token }, - }, - update: { - $set: { - enabled: false, - claimToken: randomUUID(), - deletionSuspension: { - token, - enabled: schedule.enabled === true, - ...(schedule.nextRunAt != null ? { nextRunAt: schedule.nextRunAt } : {}), - }, + const ops = schedules.map((schedule) => { + // A row already suspended by an ABANDONED earlier attempt (its process died before + // restoring) still holds the only record of the true pre-suspension state. Carry that + // snapshot forward and merely take ownership with this attempt's token — re-reading + // the row's CURRENT values would snapshot the suspended state itself (disabled, no + // next run) and permanently strand the schedule when this attempt later restores. + const existing = schedule.deletionSuspension; + const snapshotEnabled = + existing != null ? existing.enabled === true : schedule.enabled === true; + const snapshotNextRunAt = existing != null ? existing.nextRunAt : schedule.nextRunAt; + return { + updateOne: { + filter: { + id: schedule.id, + user: userId, + deleting: { $ne: true }, + 'deletionSuspension.token': { $ne: token }, + }, + update: { + $set: { + enabled: false, + claimToken: randomUUID(), + deletionSuspension: { + token, + enabled: snapshotEnabled, + ...(snapshotNextRunAt != null ? { nextRunAt: snapshotNextRunAt } : {}), + }, + }, + $unset: { nextRunAt: 1 as const }, }, - $unset: { nextRunAt: 1 as const }, }, - }, - })); + }; + }); await tenantSafeBulkWrite(Schedule(), ops as AnyBulkWriteOperation[], { ordered: false }); }