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' });