diff --git a/config/__tests__/delete-user.spec.js b/config/__tests__/delete-user.spec.js new file mode 100644 index 0000000000..e0e74076c9 --- /dev/null +++ b/config/__tests__/delete-user.spec.js @@ -0,0 +1,124 @@ +const mockModelRegistry = {}; +const mockModelFor = (name) => { + if (!mockModelRegistry[name]) { + mockModelRegistry[name] = { + findOne: jest.fn(async () => null), + deleteOne: jest.fn(async () => ({ deletedCount: 1 })), + deleteMany: jest.fn(async () => ({ deletedCount: 0 })), + updateOne: jest.fn(async () => ({ modifiedCount: 0 })), + updateMany: jest.fn(async () => ({ modifiedCount: 0 })), + countDocuments: jest.fn(async () => 0), + }; + } + return mockModelRegistry[name]; +}; + +const mockMarkUserDeleting = jest.fn(async () => new Date()); +const mockSilentExit = jest.fn(); +const mockAskQuestion = jest.fn(); + +jest.mock('../connect', () => jest.fn(async () => {})); +jest.mock('mongoose', () => ({ disconnect: jest.fn(async () => {}) })); +jest.mock('@librechat/data-schemas', () => ({ + createModels: () => new Proxy({}, { get: (_target, prop) => mockModelFor(prop) }), +})); +jest.mock('~/models', () => ({ markUserDeleting: mockMarkUserDeleting })); +jest.mock('../helpers', () => ({ + ...jest.requireActual('../helpers'), + askQuestion: mockAskQuestion, + silentExit: mockSilentExit, +})); + +const USER_ID = 'user-being-deleted'; + +/** Runs the CLI to completion, resolving with the exit code it passed to silentExit. */ +const runCli = () => + new Promise((resolve, reject) => { + mockSilentExit.mockImplementation((code = 0) => resolve(code)); + jest.isolateModules(() => { + try { + require('../delete-user'); + } catch (err) { + reject(err); + } + }); + }); + +describe('Delete user CLI', () => { + const originalArgv = process.argv; + let logSpy; + + beforeEach(() => { + process.argv = ['node', 'delete-user.js', 'deleted@example.com']; + logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + jest.spyOn(console, 'error').mockImplementation(() => {}); + for (const model of Object.values(mockModelRegistry)) { + for (const fn of Object.values(model)) { + fn.mockClear(); + } + } + mockModelFor('User').findOne.mockResolvedValue({ + _id: { toString: () => USER_ID }, + email: 'deleted@example.com', + }); + mockModelFor('ScheduleRun').countDocuments.mockResolvedValue(0); + mockMarkUserDeleting.mockReset().mockResolvedValue(new Date()); + // Confirm deletion, decline transaction history. + mockAskQuestion.mockReset().mockResolvedValueOnce('y').mockResolvedValueOnce('n'); + }); + + afterEach(() => { + process.argv = originalArgv; + logSpy.mockRestore(); + jest.restoreAllMocks(); + }); + + it('raises the barrier through markUserDeleting, never a raw update', async () => { + const code = await runCli(); + + expect(code).toBe(0); + expect(mockMarkUserDeleting).toHaveBeenCalledWith(USER_ID); + // A raw User.updateOne would stamp deletionRequestedAt without dropping the cached + // auth user document, so a live server keeps admitting requests with a pre-barrier + // req.user while the cascade below runs. + expect(mockModelFor('User').updateOne).not.toHaveBeenCalled(); + expect(mockModelFor('User').deleteOne).toHaveBeenCalled(); + }); + + it('raises the barrier before counting active scheduled runs', async () => { + await runCli(); + + const barrierAt = mockMarkUserDeleting.mock.invocationCallOrder[0]; + const countAt = mockModelFor('ScheduleRun').countDocuments.mock.invocationCallOrder[0]; + // The count only means anything once new fires are refused; otherwise a fire can be + // claimed and accepted between a zero result and the deletes. + expect(barrierAt).toBeLessThan(countAt); + }); + + it('deletes nothing when the barrier cannot be raised', async () => { + mockMarkUserDeleting.mockRejectedValue( + new Error('Auth user-doc cache is enabled but unavailable'), + ); + + const code = await runCli(); + + expect(code).toBe(1); + expect(mockModelFor('User').deleteOne).not.toHaveBeenCalled(); + expect(mockModelFor('Schedule').deleteMany).not.toHaveBeenCalled(); + expect(mockModelFor('ScheduleRun').deleteMany).not.toHaveBeenCalled(); + expect(mockModelFor('Message').deleteMany).not.toHaveBeenCalled(); + }); + + it('refuses while a scheduled run is still active', async () => { + mockModelFor('ScheduleRun').countDocuments.mockResolvedValue(2); + + const code = await runCli(); + + expect(code).toBe(1); + expect(mockModelFor('User').deleteOne).not.toHaveBeenCalled(); + expect(mockModelFor('Schedule').deleteMany).not.toHaveBeenCalled(); + // The barrier stays up: it is one-way, and a live server must keep refusing fires + // for an account the operator has already committed to erasing. + expect(mockMarkUserDeleting).toHaveBeenCalledWith(USER_ID); + }); +}); diff --git a/config/delete-user.js b/config/delete-user.js index d8ae4e2e23..2b3f03ff08 100644 --- a/config/delete-user.js +++ b/config/delete-user.js @@ -31,6 +31,7 @@ const { ScheduleRun, } = require('@librechat/data-schemas').createModels(mongoose); require('module-alias')({ base: path.resolve(__dirname, '..', 'api') }); +const { markUserDeleting } = require('~/models'); const { askQuestion, silentExit } = require('./helpers'); const connect = require('./connect'); @@ -78,7 +79,62 @@ async function gracefulExit(code = 0) { const uid = user._id.toString(); - // 5) Build and run deletion tasks + // Raise the durable deletion barrier BEFORE counting. A bare count is a + // time-of-check/time-of-use read: a fire can be claimed and accepted between the zero + // result and the deletes below, and this script cannot abort or drain it. The barrier + // is what makes the count meaningful — a live server refuses new fires at the dispatch + // boundary (fireSchedule's isOwnerDeleting probe) from this point on, so anything the + // count then misses cannot have started after it. + // + // Through markUserDeleting, never a raw update: the barrier is only in force once no + // CACHED pre-barrier user document can still populate req.user on a live server, and + // that method is what drops the auth cache entry. It fails closed, so a cache it + // cannot reach aborts the deletion rather than proceeding behind a barrier that was + // never actually raised. + try { + await markUserDeleting(uid); + } catch (err) { + console.red('✖ Could not raise the deletion barrier. Nothing was deleted.'); + console.error(err); + return gracefulExit(1); + } + + // REFUSE rather than warn when a scheduled run is in flight. This script talks to the + // database directly, so unlike the HTTP deletion paths it cannot abort a live loopback + // generation or wait for it to drain. That generation can already have passed its + // owner lookup, and it will persist its messages after the rows deleted here are gone + // — resurrecting data for an account the operator believes is erased. + const activeRuns = await ScheduleRun.countDocuments({ + user: uid, + status: { $in: ['started', 'requires_action'] }, + }); + if (activeRuns > 0) { + console.red( + `✖ ${activeRuns} scheduled run(s) are still active for this user, and this script cannot abort them.`, + ); + // Deliberately NOT "stop the server and retry": stopping it cannot transition a + // persisted row, so every offline retry would see the same active status forever. + // Only a running server settles these — by draining them (app deletion) or by + // reconciling an abandoned run whose lease expired. + console.yellow( + 'Delete the account through the app instead, which drains active runs. If the server is', + ); + console.yellow( + 'already stopped, start it and let reconciliation settle the abandoned run, then retry.', + ); + return gracefulExit(1); + } + + // Runs BEFORE schedules so a partial failure stays retryable, mirroring + // deleteSchedulesByUser. A schedule carries the user's prompt text and has no TTL, + // so leaving it behind retains that content indefinitely. + await ScheduleRun.deleteMany({ user: uid }); + await Schedule.deleteMany({ user: uid }); + + // 5) Run the deletion tasks. Constructed HERE rather than earlier: a Model.deleteMany() + // call dispatches the moment it is written, so building this list above the guards + // would start erasing the account before the barrier could be raised or an in-flight + // scheduled run could refuse the whole operation. const tasks = [ Action.deleteMany({ user: uid }), Agent.deleteMany({ author: uid }), @@ -102,42 +158,6 @@ async function gracefulExit(code = 0) { AclEntry.deleteMany({ principalId: user._id }), ]; - // Raise the durable deletion barrier BEFORE counting. A bare count is a - // time-of-check/time-of-use read: a fire can be claimed and accepted between the zero - // result and the deletes below, and this script cannot abort or drain it. The barrier - // is what makes the count meaningful — a live server refuses new fires at the dispatch - // boundary (fireSchedule's isOwnerDeleting probe) from this point on, so anything the - // count then misses cannot have started after it. - await User.updateOne( - { _id: uid, deletionRequestedAt: { $exists: false } }, - { $set: { deletionRequestedAt: new Date() } }, - ); - - // REFUSE rather than warn when a scheduled run is in flight. This script talks to the - // database directly, so unlike the HTTP deletion paths it cannot abort a live loopback - // generation or wait for it to drain. That generation can already have passed its - // owner lookup, and it will persist its messages after the rows deleted here are gone - // — resurrecting data for an account the operator believes is erased. - const activeRuns = await ScheduleRun.countDocuments({ - user: uid, - status: { $in: ['started', 'requires_action'] }, - }); - if (activeRuns > 0) { - console.red( - `✖ ${activeRuns} scheduled run(s) are still active for this user, and this script cannot abort them.`, - ); - console.yellow( - 'Stop the server (or delete the account through the app, which drains them) and retry.', - ); - return gracefulExit(1); - } - - // Runs BEFORE schedules so a partial failure stays retryable, mirroring - // deleteSchedulesByUser. A schedule carries the user's prompt text and has no TTL, - // so leaving it behind retains that content indefinitely. - await ScheduleRun.deleteMany({ user: uid }); - await Schedule.deleteMany({ user: uid }); - if (deleteTx) { tasks.push(Transaction.deleteMany({ user: uid })); } diff --git a/packages/api/src/schedules/handlers.spec.ts b/packages/api/src/schedules/handlers.spec.ts index 8c98a32989..509f2d91d8 100644 --- a/packages/api/src/schedules/handlers.spec.ts +++ b/packages/api/src/schedules/handlers.spec.ts @@ -219,4 +219,20 @@ describe('createSchedule late-create compensation', () => { expect.objectContaining({ nextRunAt: expect.any(Date) }), ); }); + + /** + * updateScheduleById filters out rows marked `deleting`, so a null arming result means + * the deletion cascade claimed this row between the barrier re-check and the arming + * write. Reporting the pre-delete snapshot as a 201 tells the client a schedule exists + * that is already hidden and pending erasure. + */ + it('does not report success when the arming write loses a delete race', async () => { + const deps = makeCreateDeps({ isUserDeleting: jest.fn(async () => false) }); + (deps.methods.updateScheduleById as jest.Mock).mockResolvedValue(null); + const { res, captured } = makeRes(); + await createSchedulesHandlers(deps).createSchedule(makeCreateReq(), res); + + expect(captured.status).toBe(410); + expect(captured.status).not.toBe(201); + }); }); diff --git a/packages/api/src/schedules/handlers.ts b/packages/api/src/schedules/handlers.ts index 72ead7f987..b2412137d9 100644 --- a/packages/api/src/schedules/handlers.ts +++ b/packages/api/src/schedules/handlers.ts @@ -297,11 +297,22 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH res.status(410).json({ error: 'This account is being deleted' }); return; } - // ARM last. A failure here leaves the schedule visible but unclaimed; the owner's - // next edit recomputes nextRunAt. - const armed = nextRunAt - ? ((await deps.methods.updateScheduleById(id, user.id, { nextRunAt })) ?? created) - : created; + // ARM last. A transient write fault THROWS, leaving the schedule visible but + // unclaimed; the owner's next edit (or the reconciler's unarmed sweep) recomputes + // nextRunAt. A null result is different in kind: updateScheduleById filters out + // deleting rows, so null means the row stopped being ours between the barrier + // re-check and here — the deletion cascade marked or erased it. Falling back to the + // pre-delete snapshot would answer 201 for a schedule that is already hidden and + // pending erasure. + let armed = created; + if (nextRunAt) { + const updated = await deps.methods.updateScheduleById(id, user.id, { nextRunAt }); + if (updated == null) { + res.status(410).json({ error: 'Schedule no longer exists' }); + return; + } + armed = updated; + } logger.info(`[schedules] created ${id} for user ${user.id}`); res.status(201).json(toWireSchedule(armed)); }