mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: quiesce schedules on admin deletion, recover unarmed schedules, clear on error
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.
This commit is contained in:
parent
c05a8f59d3
commit
18d8e712a5
5 changed files with 96 additions and 4 deletions
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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> = {}): AdminUsersDeps {
|
||||
|
|
@ -58,6 +59,7 @@ function createDeps(overrides: Partial<AdminUsersDeps> = {}): 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')),
|
||||
|
|
|
|||
|
|
@ -40,6 +40,14 @@ export interface AdminUsersDeps {
|
|||
principalType: PrincipalType;
|
||||
principalId: string | Types.ObjectId;
|
||||
}) => Promise<void>;
|
||||
/**
|
||||
* 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<boolean>;
|
||||
}
|
||||
|
||||
export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
||||
|
|
@ -47,7 +55,14 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
searchUsers: (req: ServerRequest, res: Response) => Promise<Response>;
|
||||
deleteUser: (req: ServerRequest, res: Response) => Promise<Response>;
|
||||
} {
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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<ISchedule> = { ...parsed.data } as Partial<ISchedule>;
|
||||
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' });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue