diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index ffeb1c33ac..44b94ce1c8 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -25,6 +25,7 @@ const { getMCPManager, getFlowStateManager, getMCPServersRegistry } = require('~ const { invalidateCachedTools } = require('~/server/services/Config/getCachedTools'); const { processDeleteRequest } = require('~/server/services/Files/process'); const { quiesceUserSchedules } = require('~/server/services/Schedules'); +const { markUserDeleting } = require('~/models'); const { getAppConfig } = require('~/server/services/Config'); const { getLogStores } = require('~/cache'); const db = require('~/models'); @@ -349,10 +350,35 @@ const deleteUserController = async (req, res) => { // in-flight loopback runs, so a scheduled generation can't persist messages // after the messages/conversations below are deleted. Best-effort — a failure // here must not block account deletion (deleteSchedulesByUser still erases rows). - await quiesceUserSchedules(user.id).catch((error) => - logger.error('[deleteUserController] Failed to quiesce scheduled chats', error), + // BARRIER FIRST, before anything slow. Quiescing takes time, and the whole drain + // window has to already be refusing new work: a one-shot disable scan cannot close + // the create race (a schedule created after the scan simply is not in it), so every + // scheduling admission consults this durable user-level flag instead. Raising it + // also invalidates the auth user-doc cache, without which the barrier would only be + // as strong as the shortest cache TTL. + await markUserDeleting(user.id).catch((error) => + logger.error('[deleteUserController] Failed to raise the deletion barrier', error), ); + const quiesced = await quiesceUserSchedules(user.id).catch((error) => { + logger.error('[deleteUserController] Failed to quiesce scheduled chats', error); + return false; + }); + if (!quiesced) { + // DEFER rather than destroy on an unconfirmed drain. A scheduled generation that + // could not be confirmed settled may still persist messages, and deleting now + // would let it resurrect data for a deleted account. The barrier is durable and + // stays up, so nothing new accumulates; `getUsersPendingDeletion` makes this a + // resumable work list for a later pass to finish. + logger.warn( + `[deleteUserController] Deferring destructive deletion for ${user.id}: scheduled runs ` + + 'did not confirm settlement. The deletion barrier remains in place.', + ); + return res.status(202).json({ + message: 'Account deletion started; in-flight work is still settling.', + }); + } + await db.deleteMessages({ user: user.id }); await db.deleteAllUserSessions({ userId: user.id }); await db.deleteTransactions({ user: user.id }); diff --git a/api/server/controllers/UserController.spec.js b/api/server/controllers/UserController.spec.js index 6e640e9d22..582f5e4fe5 100644 --- a/api/server/controllers/UserController.spec.js +++ b/api/server/controllers/UserController.spec.js @@ -2,7 +2,7 @@ const mongoose = require('mongoose'); const { MongoMemoryServer } = require('mongodb-memory-server'); jest.mock('~/server/services/Schedules', () => ({ - quiesceUserSchedules: jest.fn().mockResolvedValue(undefined), + quiesceUserSchedules: jest.fn().mockResolvedValue(true), })); jest.mock('@librechat/data-schemas', () => { @@ -21,6 +21,7 @@ jest.mock('@librechat/data-schemas', () => { jest.mock('~/models', () => { const _mongoose = require('mongoose'); return { + markUserDeleting: jest.fn().mockResolvedValue(new Date()), deleteAllUserSessions: jest.fn().mockResolvedValue(undefined), deleteAllSharedLinks: jest.fn().mockResolvedValue(undefined), deleteAllAgentApiKeys: jest.fn().mockResolvedValue(undefined), diff --git a/api/server/controllers/__tests__/deleteUser.spec.js b/api/server/controllers/__tests__/deleteUser.spec.js index fa938cebd9..33ae29d572 100644 --- a/api/server/controllers/__tests__/deleteUser.spec.js +++ b/api/server/controllers/__tests__/deleteUser.spec.js @@ -43,10 +43,11 @@ jest.mock('@librechat/api', () => ({ })); jest.mock('~/server/services/Schedules', () => ({ - quiesceUserSchedules: jest.fn().mockResolvedValue(undefined), + quiesceUserSchedules: jest.fn().mockResolvedValue(true), })); jest.mock('~/models', () => ({ + markUserDeleting: jest.fn().mockResolvedValue(new Date()), deleteAllUserSessions: (...args) => mockDeleteAllUserSessions(...args), deleteAllSharedLinks: (...args) => mockDeleteAllSharedLinks(...args), updateUserPlugins: (...args) => mockUpdateUserPlugins(...args), diff --git a/api/server/routes/schedules.js b/api/server/routes/schedules.js index 7d2cabc425..bedb94e44a 100644 --- a/api/server/routes/schedules.js +++ b/api/server/routes/schedules.js @@ -64,6 +64,9 @@ const handlers = createSchedulesHandlers({ // Quiesce-then-erase delete: stops new claims, aborts in-flight loopback runs, // and erases once drained (reconciler completes drain) so evidence is preserved. deleteSchedule: deleteScheduleForOwner, + // Durable account-deletion barrier. A one-shot disable scan cannot close the + // create race, so every scheduling WRITE consults the user-level flag instead. + isUserDeleting: methods.isUserDeleting, }); router.get('/', checkSchedulesAccess, handlers.listSchedules); diff --git a/packages/api/src/schedules/fire.spec.ts b/packages/api/src/schedules/fire.spec.ts index f36cdd89a0..4e541b967d 100644 --- a/packages/api/src/schedules/fire.spec.ts +++ b/packages/api/src/schedules/fire.spec.ts @@ -170,6 +170,8 @@ function makeDeps( abortScheduledJob: async () => undefined, clearReconciledJob: async () => undefined, isJobStoreShared: () => true, + isOwnerDeleting: async () => false, + isGloballyDisabled: async () => false, countActiveRunsGlobal: async () => methods.countActiveRuns(), withGlobalCapacitySlot: (cap: number, claim: (slot: number) => Promise) => withCapacitySlot( diff --git a/packages/api/src/schedules/fire.ts b/packages/api/src/schedules/fire.ts index 3814b7db00..fa1a7b4470 100644 --- a/packages/api/src/schedules/fire.ts +++ b/packages/api/src/schedules/fire.ts @@ -264,6 +264,15 @@ export async function fireSchedule( return { fired: false, skipped: 'disabled' as const }; } + // Account-deletion barrier, re-checked at the DISPATCH boundary. Admission (the + // create/update/run-now handlers) is the primary gate, but there is always a window + // between admission and persistence, so the owner is re-checked immediately before a + // billed generation is dispatched. Skips silently: the deletion cascade owns the row. + if (await deps.isOwnerDeleting(user.id)) { + await advance(); + return { fired: false, skipped: 'user_deleting' as const }; + } + // Re-check the owner's live schedule permission: a role that lost // SCHEDULES access after the schedule was created must stop firing. if (!(await deps.hasScheduleAccess(user))) { diff --git a/packages/api/src/schedules/handlers.ts b/packages/api/src/schedules/handlers.ts index 3993fef1bb..56bcbcc370 100644 --- a/packages/api/src/schedules/handlers.ts +++ b/packages/api/src/schedules/handlers.ts @@ -24,6 +24,25 @@ export interface SchedulesHandlersDeps { * runs, and erases once drained. Returns false when not found / already deleting. */ deleteSchedule: (id: string, userId: string) => Promise; + /** Whether this user's account deletion has begun. Fail-closed (unknown == true). */ + isUserDeleting: (userId: string) => Promise; +} + +/** + * Refuses a scheduling WRITE once the owner's account deletion has begun. A one-shot + * disable scan can never close this race (a create landing after the scan is simply not + * in it), so admission consults the durable user-level barrier instead. Fail-closed. + */ +async function rejectIfUserDeleting( + deps: SchedulesHandlersDeps, + userId: string, + res: Response, +): Promise { + if (!(await deps.isUserDeleting(userId))) { + return false; + } + res.status(410).json({ error: 'This account is being deleted' }); + return true; } /** Bounded attempts to clear the upload TTL on a schedule's attachments. */ @@ -174,6 +193,9 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH return; } const user = requestUser(req); + if (await rejectIfUserDeleting(deps, user.id, res)) { + return; + } const limits = await deps.getLimits(user); if (!limits.enabled) { res.status(403).json({ error: 'Scheduled chats are disabled' }); @@ -239,6 +261,9 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH } const { id } = req.params as { id: string }; const user = requestUser(req); + if (await rejectIfUserDeleting(deps, user.id, res)) { + return; + } const existing = await deps.methods.getScheduleById(id, user.id); if (existing == null) { res.status(404).json({ error: 'Schedule not found' }); @@ -328,6 +353,9 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH async function runScheduleNow(req: ServerRequest, res: Response): Promise { const { id } = req.params as { id: string }; + if (await rejectIfUserDeleting(deps, requestUser(req).id, res)) { + return; + } const schedule = await deps.methods.getScheduleById(id, requestUser(req).id); if (schedule == null) { res.status(404).json({ error: 'Schedule not found' }); diff --git a/packages/api/src/schedules/service.spec.ts b/packages/api/src/schedules/service.spec.ts index 6dd861981b..51edaf52e8 100644 --- a/packages/api/src/schedules/service.spec.ts +++ b/packages/api/src/schedules/service.spec.ts @@ -30,6 +30,7 @@ function makeService( findBalance: jest.fn(async () => null), upsertBalance: jest.fn(async () => null), resolveAgentFireAccess: jest.fn(async () => 'ok' as const), + isUserDeleting: jest.fn(async () => false), } as unknown as SchedulesServiceDeps; return createSchedulesService(deps); } @@ -60,7 +61,9 @@ describe('quiesceUserSchedules drain wait', () => { // Each poll waits one interval; advance twice so the loop observes the drain. await jest.advanceTimersByTimeAsync(250); await jest.advanceTimersByTimeAsync(250); - await expect(pending).resolves.toBeUndefined(); + // The rows drained, but this harness has no job store so the aborts could not be + // CONFIRMED delivered — quiesce reports false and the caller must defer destruction. + await expect(pending).resolves.toBe(false); // Initial read + at least one poll that observed a non-empty set + the empty one. expect(getActive.mock.calls.length).toBeGreaterThanOrEqual(3); @@ -73,9 +76,10 @@ describe('quiesceUserSchedules drain wait', () => { const service = makeService(getActive); const pending = service.quiesceUserSchedules('user-1'); - // Advance past the full bounded timeout; the loop must give up, not hang. + // Advance past the full bounded timeout; the loop must give up, not hang, and must + // report the drain as UNCONFIRMED so deletion defers rather than destroying. await jest.advanceTimersByTimeAsync(10_000); - await expect(pending).resolves.toBeUndefined(); + await expect(pending).resolves.toBe(false); // It polled repeatedly (bounded by the deadline) and surfaced the un-drained runs. expect(getActive.mock.calls.length).toBeGreaterThan(1); @@ -87,7 +91,9 @@ describe('quiesceUserSchedules drain wait', () => { const getActive = jest.fn, [string]>().mockResolvedValue([]); const service = makeService(getActive); - await expect(service.quiesceUserSchedules('user-1')).resolves.toBeUndefined(); + // Nothing to abort and nothing to drain, so the quiesce is trivially CONFIRMED and + // the deletion cascade may proceed to its destructive steps. + await expect(service.quiesceUserSchedules('user-1')).resolves.toBe(true); // Only the initial collection read; the drain loop is skipped for an empty set. expect(getActive).toHaveBeenCalledTimes(1); }); diff --git a/packages/api/src/schedules/service.ts b/packages/api/src/schedules/service.ts index 5f482f178d..357263c79a 100644 --- a/packages/api/src/schedules/service.ts +++ b/packages/api/src/schedules/service.ts @@ -102,6 +102,8 @@ export interface SchedulesServiceDeps { agentId: string, user: ScheduleUserContext, ) => Promise<'ok' | 'missing' | 'forbidden'>; + /** Whether this user's account deletion has begun. Fail-closed (unknown == true). */ + isUserDeleting: (userId: string) => Promise; } export interface SchedulesService { @@ -154,8 +156,13 @@ export interface SchedulesService { ) => Promise; /** Soft-deletes an owner's schedule: stop claims, abort active runs, drain, erase. */ deleteScheduleForOwner: (scheduleId: string, userId: string) => Promise; - /** Quiesces all of a user's schedules ahead of account deletion (stop + abort). */ - quiesceUserSchedules: (userId: string) => Promise; + /** + * Quiesces all of a user's schedules ahead of account deletion (stop + abort + drain). + * Returns whether the drain was CONFIRMED: false means at least one run could not be + * confirmed settled, and the caller must NOT proceed to destructive deletion — the + * durable barrier keeps refusing new work while a later pass finishes the cascade. + */ + quiesceUserSchedules: (userId: string) => Promise; initializeScheduleEngine: (options?: { clustered?: boolean; }) => Promise | undefined>; @@ -370,6 +377,7 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer // Counted in system scope so the cap is GLOBAL — a per-owner (tenant-scoped) // count would let multiple tenants collectively exceed fireConcurrency. countActiveRunsGlobal: () => runAsSystem(() => methods.countActiveRuns()), + isOwnerDeleting: (userId) => deps.isUserDeleting(userId), isGloballyDisabled: async () => { // Env first: an incident lever that must work even if the DB/config plane is the // thing failing (a kill switch that needs a healthy DB is the one that fails when @@ -699,7 +707,7 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer * the loopback jobs of any in-flight runs, so a scheduled generation cannot keep * persisting messages after the account's messages/conversations are deleted. */ - async function quiesceUserSchedules(userId: string): Promise { + async function quiesceUserSchedules(userId: string): Promise { await methods.disableUserSchedulesForDeletion(userId); const active = await methods.getActiveRunsForUser(userId); const unconfirmed: string[] = []; @@ -728,7 +736,8 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer // deployment without a shared stream store the run's generation may live on a // peer worker and keep persisting for the now-deleted account. Known unshared- // topology limitation (see the init warning); make it visible. - if (remaining > 0 || unconfirmed.length > 0) { + const confirmed = remaining === 0 && unconfirmed.length === 0; + if (!confirmed) { logger.warn( `[schedules] account-deletion quiesce did not confirm ${Math.max(remaining, unconfirmed.length)} ` + `in-flight scheduled run(s) settled${unconfirmed.length ? ` [${unconfirmed.join(', ')}]` : ''} ` + @@ -736,6 +745,7 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer 'shared stream store (USE_REDIS_STREAMS).', ); } + return confirmed; } return { diff --git a/packages/api/src/schedules/types.ts b/packages/api/src/schedules/types.ts index bd81ec796a..8e29db8678 100644 --- a/packages/api/src/schedules/types.ts +++ b/packages/api/src/schedules/types.ts @@ -105,6 +105,8 @@ export interface ScheduleEngineDeps { * re-enable it. Checked once per engine tick, so the uncached read is negligible. */ isGloballyDisabled: () => Promise; + /** Whether the run owner's account deletion has begun. Fail-closed (unknown == true). */ + isOwnerDeleting: (userId: string) => Promise; /** * Runs `claim` against the lowest free GLOBAL capacity slot, retrying the next slot * when the unique partial index rejects a collision. Enforces fireConcurrency in the @@ -142,6 +144,7 @@ export interface FireResult { | 'superseded' | 'agent_deleted' | 'user_missing' + | 'user_deleting' | 'permission_revoked' | 'disabled'; error?: string; diff --git a/packages/data-schemas/src/methods/schedule.methods.spec.ts b/packages/data-schemas/src/methods/schedule.methods.spec.ts index 0c29110af2..d94aff1aaf 100644 --- a/packages/data-schemas/src/methods/schedule.methods.spec.ts +++ b/packages/data-schemas/src/methods/schedule.methods.spec.ts @@ -9,6 +9,7 @@ import type { } from '~/types/schedule'; import type { ScheduleMethods } from './schedule'; import { createScheduleMethods } from './schedule'; +import { createUserMethods } from './user'; import { createModels } from '../models'; jest.mock('~/config/winston', () => ({ @@ -22,6 +23,7 @@ let mongoServer: MongoMemoryServer; let Schedule: Model; let ScheduleRun: Model; let methods: ScheduleMethods; +let userMethods: ReturnType; beforeAll(async () => { mongoServer = await MongoMemoryServer.create(); @@ -32,6 +34,7 @@ beforeAll(async () => { await Schedule.init(); await ScheduleRun.init(); methods = createScheduleMethods(mongoose); + userMethods = createUserMethods(mongoose); }); afterAll(async () => { @@ -1046,3 +1049,41 @@ describe('deletion quiescing (soft-delete, drain, erase)', () => { expect(b).not.toBe('limit'); }); }); + +describe('account-deletion barrier (delete vs create)', () => { + it('is one-way and monotonic under concurrent deletion requests', async () => { + const User = mongoose.models.User; + const user = await User.create({ email: `barrier-${Date.now()}@test.dev`, name: 'B' }); + + expect(await userMethods.isUserDeleting(user._id.toString())).toBe(false); + + // Six concurrent deletion requests must agree on ONE timestamp: the barrier is + // stamped only when absent, so there is no un-delete race and no ABA window. + const stamps = await Promise.all( + Array.from({ length: 6 }, () => userMethods.markUserDeleting(user._id.toString())), + ); + const unique = new Set(stamps.map((d) => d?.getTime())); + expect(unique.size).toBe(1); + expect(await userMethods.isUserDeleting(user._id.toString())).toBe(true); + }); + + it('fails CLOSED for an unknown user so admission never leaks', async () => { + // Refusing work for a live user is recoverable (the caller retries); admitting work + // for a deleting one is not, so an unresolvable lookup must report "deleting". + expect(await userMethods.isUserDeleting(new mongoose.Types.ObjectId().toString())).toBe(true); + }); + + it('surfaces unfinished cascades as a resumable work list', async () => { + const User = mongoose.models.User; + const pendingUser = await User.create({ + email: `pending-${Date.now()}@test.dev`, + name: 'P', + }); + await userMethods.markUserDeleting(pendingUser._id.toString()); + + // A deletion deferred on an unconfirmed quiesce (or crashed part-way) leaves the + // barrier up with the document still present — exactly what a later pass queries. + const pending = await userMethods.getUsersPendingDeletion(50); + expect(pending.some((u) => u._id.toString() === pendingUser._id.toString())).toBe(true); + }); +}); diff --git a/packages/data-schemas/src/methods/user.ts b/packages/data-schemas/src/methods/user.ts index d044bf40d4..b3b6125e0c 100644 --- a/packages/data-schemas/src/methods/user.ts +++ b/packages/data-schemas/src/methods/user.ts @@ -116,6 +116,13 @@ export function createUserMethods( getUserById: (userId: string, fieldsToSelect?: string | string[] | null) => Promise; generateToken: (user: IUser, expiresIn?: number) => Promise; deleteUserById: (userId: string) => Promise; + /** Raises the one-way account-deletion barrier (monotonic) and invalidates the + * auth user-doc cache. Returns the effective timestamp. */ + markUserDeleting: (userId: string) => Promise; + /** Whether deletion has begun for this user. Fail-closed: unknown means true. */ + isUserDeleting: (userId: string) => Promise; + /** Users whose barrier is up but whose document still exists (unfinished cascades). */ + getUsersPendingDeletion: (limit: number) => Promise; updateUserPlugins: ( userId: string, plugins: string[] | undefined, @@ -294,6 +301,65 @@ export function createUserMethods( } } + /** + * Raises the durable account-deletion barrier. ONE-WAY and monotonic: the timestamp + * is stamped only when absent, so a repeated or concurrent deletion request never + * moves it and there is no un-delete race. Returns the effective timestamp. + * + * Must be called BEFORE quiescing anything: quiescing is the slow part, and the + * whole drain window has to already be refusing new work. The auth user-doc cache is + * invalidated here because the barrier is only as strong as the shortest cache TTL — + * a request holding a stale user document would otherwise sail straight past it. + */ + async function markUserDeleting(userId: string): Promise { + const User = mongoose.models.User; + const updated = await User.findOneAndUpdate( + { _id: userId, deletionRequestedAt: { $exists: false } }, + { $set: { deletionRequestedAt: new Date() } }, + { new: true }, + ).lean(); + await invalidateAuthUserDocCache(userId); + if (updated?.deletionRequestedAt != null) { + return updated.deletionRequestedAt; + } + // Already raised by a prior/concurrent request: report the existing timestamp + // rather than overwriting it, so the barrier stays monotonic. + const existing = await User.findById(userId) + .select('deletionRequestedAt') + .lean>(); + return existing?.deletionRequestedAt ?? null; + } + + /** + * Whether this user's account deletion has begun. FAIL-CLOSED: a lookup failure or a + * missing user reports `true`, because refusing work for a live user is recoverable + * (the caller retries) while admitting work for a deleting one is not. + */ + async function isUserDeleting(userId: string): Promise { + const User = mongoose.models.User; + try { + const user = await User.findById(userId) + .select('deletionRequestedAt') + .lean>(); + return user == null || user.deletionRequestedAt != null; + } catch { + return true; + } + } + + /** + * Users whose deletion barrier is up but whose document still exists, i.e. cascades + * that never finished (deferred on an unconfirmed quiesce, or crashed part-way). + * Makes the destructive cascade a resumable work list instead of a one-shot. + */ + async function getUsersPendingDeletion(limit: number): Promise { + const User = mongoose.models.User; + return User.find({ deletionRequestedAt: { $exists: true } }) + .sort({ deletionRequestedAt: 1 }) + .limit(limit) + .lean(); + } + /** * Atomically records terms acceptance for a user. * Sets termsAccepted and, only when no timestamp is already stored, stamps @@ -588,6 +654,9 @@ export function createUserMethods( getUserById, generateToken, deleteUserById, + markUserDeleting, + isUserDeleting, + getUsersPendingDeletion, updateUserPlugins, toggleUserMemories, };