From 18d8e712a5f3ffde0ac4d0877a27c178acedc1e2 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 27 Jul 2026 01:22:41 -0400 Subject: [PATCH] fix: quiesce schedules on admin deletion, recover unarmed schedules, clear on error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin user-delete handler called deleteUserById directly. Its cascade is deliberately thin (conversations, messages and files are all deferred to a future shared service), but scheduled runs are not dormant data: an in-flight fire keeps persisting messages and billing after the user document is gone, and the engine keeps claiming occurrences. It now quiesces first and refuses with 503 on an unconfirmed drain, mirroring the self-service controller. NOTE the route is currently commented out, so this was not reachable — it is a landmine for whoever enables it, not a live defect. Insert-then-arm had no recovery: a crash or a failed arm left an enabled schedule with no nextRunAt, which claimDueSchedule can never select, while it still occupied a user slot. My own comment claimed the owner's next edit would recover it, which was wrong — updateSchedule only recomputed nextRunAt when the cadence, timezone or enabled flag changed, so a name or prompt edit left it inert. Any edit now re-arms a schedule that is enabled and unarmed. The generation-error path had the same preserved-job leak just fixed on the success path: completeJob returns early for an already-terminal job (what a schedule delete leaves behind), and the now-terminal run is no longer reconciled, so nothing would ever reap it. --- api/server/controllers/agents/request.js | 8 ++++ api/server/routes/admin/users.js | 2 + packages/api/src/admin/users.spec.ts | 50 +++++++++++++++++++++++- packages/api/src/admin/users.ts | 33 +++++++++++++++- packages/api/src/schedules/handlers.ts | 7 +++- 5 files changed, 96 insertions(+), 4 deletions(-) diff --git a/api/server/controllers/agents/request.js b/api/server/controllers/agents/request.js index 9ecef679ba..ae38397678 100644 --- a/api/server/controllers/agents/request.js +++ b/api/server/controllers/agents/request.js @@ -1278,6 +1278,14 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit completeErr, ); }); + // Same early return as the success path: completeJob no-ops on a job that is + // already terminal (what a schedule delete leaves behind), and the now-terminal + // run is no longer reconciled, so nothing else would reap it. + if (scheduleId && errorScheduleOutcomeRecorded) { + await clearScheduledJob(streamId, { scheduleId, scheduledFor }).catch((err) => + logger.warn('[ResumableAgentController] Failed to clear reconciled job', err), + ); + } } try { diff --git a/api/server/routes/admin/users.js b/api/server/routes/admin/users.js index 20d4eb1797..411d3d4d22 100644 --- a/api/server/routes/admin/users.js +++ b/api/server/routes/admin/users.js @@ -2,6 +2,7 @@ const express = require('express'); const { createAdminUsersHandlers } = require('@librechat/api'); const { SystemCapabilities } = require('@librechat/data-schemas'); const { requireCapability } = require('~/server/middleware/roles/capabilities'); +const { quiesceUserSchedules } = require('~/server/services/Schedules'); const { requireJwtAuth } = require('~/server/middleware'); const db = require('~/models'); @@ -17,6 +18,7 @@ const handlers = createAdminUsersHandlers({ deleteUserById: db.deleteUserById, deleteConfig: db.deleteConfig, deleteAclEntries: db.deleteAclEntries, + quiesceUserSchedules, }); router.use(requireJwtAuth, requireAdminAccess); diff --git a/packages/api/src/admin/users.spec.ts b/packages/api/src/admin/users.spec.ts index 1d0e5fbac8..4b598bb34d 100644 --- a/packages/api/src/admin/users.spec.ts +++ b/packages/api/src/admin/users.spec.ts @@ -44,9 +44,10 @@ function createReqRes( const json = jest.fn(); const status = jest.fn().mockReturnValue({ json }); - const res = { status, json } as unknown as Response; + const set = jest.fn(); + const res = { status, json, set } as unknown as Response; - return { req, res, status, json }; + return { req, res, status, json, set }; } function createDeps(overrides: Partial = {}): AdminUsersDeps { @@ -58,6 +59,7 @@ function createDeps(overrides: Partial = {}): AdminUsersDeps { .mockResolvedValue({ deletedCount: 1, message: 'User was deleted successfully.' }), deleteConfig: jest.fn().mockResolvedValue(null), deleteAclEntries: jest.fn().mockResolvedValue(undefined), + quiesceUserSchedules: jest.fn().mockResolvedValue(true), ...overrides, }; } @@ -489,6 +491,50 @@ describe('createAdminUsersHandlers', () => { expect(json).toHaveBeenCalledWith({ error: 'User not found' }); }); + /** + * The rest of this endpoint's cascade is deliberately deferred, but scheduled runs + * are not dormant data: an in-flight fire keeps persisting messages and billing + * after the user document is gone. + */ + it('quiesces the user schedules before removing the user', async () => { + const deps = createDeps(); + const handlers = createAdminUsersHandlers(deps); + const { req, res } = createReqRes({ params: { id: validUserId } }); + + await handlers.deleteUser(req, res); + + expect(deps.quiesceUserSchedules).toHaveBeenCalledWith(validUserId); + const quiesceOrder = (deps.quiesceUserSchedules as jest.Mock).mock.invocationCallOrder[0]; + const deleteOrder = (deps.deleteUserById as jest.Mock).mock.invocationCallOrder[0]; + expect(quiesceOrder).toBeLessThan(deleteOrder); + }); + + it('refuses the delete when the schedule drain is not confirmed', async () => { + const deps = createDeps({ quiesceUserSchedules: jest.fn().mockResolvedValue(false) }); + const handlers = createAdminUsersHandlers(deps); + const { req, res, status } = createReqRes({ params: { id: validUserId } }); + + await handlers.deleteUser(req, res); + + // Deleting on an unconfirmed drain would let a live generation persist data for a + // user that no longer exists. + expect(status).toHaveBeenCalledWith(503); + expect(deps.deleteUserById).not.toHaveBeenCalled(); + }); + + it('refuses the delete when quiescing throws', async () => { + const deps = createDeps({ + quiesceUserSchedules: jest.fn().mockRejectedValue(new Error('mongo down')), + }); + const handlers = createAdminUsersHandlers(deps); + const { req, res, status } = createReqRes({ params: { id: validUserId } }); + + await handlers.deleteUser(req, res); + + expect(status).toHaveBeenCalledWith(503); + expect(deps.deleteUserById).not.toHaveBeenCalled(); + }); + it('returns 500 on error', async () => { const deps = createDeps({ deleteUserById: jest.fn().mockRejectedValue(new Error('db crash')), diff --git a/packages/api/src/admin/users.ts b/packages/api/src/admin/users.ts index d24eb1b759..0dbd885447 100644 --- a/packages/api/src/admin/users.ts +++ b/packages/api/src/admin/users.ts @@ -40,6 +40,14 @@ export interface AdminUsersDeps { principalType: PrincipalType; principalId: string | Types.ObjectId; }) => Promise; + /** + * Stops the user's scheduled work and confirms the drain. Unlike the rest of the + * cascade this endpoint defers, scheduled runs are ACTIVE: a fire already generating + * keeps persisting messages (and billing) after the user document is gone, and the + * engine keeps claiming occurrences. Returns false when the drain could not be + * confirmed, in which case deletion must be refused rather than proceed. + */ + quiesceUserSchedules: (userId: string) => Promise; } export function createAdminUsersHandlers(deps: AdminUsersDeps): { @@ -47,7 +55,14 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): { searchUsers: (req: ServerRequest, res: Response) => Promise; deleteUser: (req: ServerRequest, res: Response) => Promise; } { - const { findUsers, countUsers, deleteUserById, deleteConfig, deleteAclEntries } = deps; + const { + findUsers, + countUsers, + deleteUserById, + deleteConfig, + deleteAclEntries, + quiesceUserSchedules, + } = deps; async function listUsersHandler(req: ServerRequest, res: Response) { try { @@ -146,6 +161,22 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): { } } + // Stop scheduled work BEFORE removing the user. The rest of this endpoint's + // cascade is deliberately deferred (see deleteUserById), but scheduled runs are + // not dormant data: an in-flight fire keeps persisting messages and billing after + // the user document is gone. Refuse rather than delete on an unconfirmed drain, + // mirroring the self-service controller. + const quiesced = await quiesceUserSchedules(id).catch((error) => { + logger.error('[adminUsers] Failed to quiesce scheduled chats', error); + return false; + }); + if (!quiesced) { + res.set('Retry-After', '30'); + return res.status(503).json({ + error: 'Scheduled work for this user is still settling. Please retry shortly.', + }); + } + const result = await deleteUserById(id); if (result.deletedCount === 0) { diff --git a/packages/api/src/schedules/handlers.ts b/packages/api/src/schedules/handlers.ts index 94bb268c54..27ced87654 100644 --- a/packages/api/src/schedules/handlers.ts +++ b/packages/api/src/schedules/handlers.ts @@ -358,8 +358,13 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH const cadenceChanged = parsed.data.cadence != null || parsed.data.timezone != null || parsed.data.enabled != null; const reEnabled = parsed.data.enabled === true && existing.enabled === false; + // RECOVERY: an enabled schedule with no nextRunAt is inert — claimDueSchedule sorts + // on nextRunAt and can never select it. Creation arms in a second write, so a crash + // or a failed arm leaves exactly this state; re-arm on ANY edit rather than only a + // cadence one, or a name/prompt edit would silently leave it dead. + const needsArming = existing.nextRunAt == null; const update: Partial = { ...parsed.data } as Partial; - if (enabled && cadenceChanged) { + if (enabled && (cadenceChanged || needsArming)) { const nextRunAt = computeNextRunAt({ cadence, timezone, scheduleId: existing.id }); if (nextRunAt == null) { res.status(400).json({ error: 'Schedule has no computable next run' });