mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: raise the barrier before admin quiescing, and settle a stale claimed resume
P1 — the admin delete quiesced without first raising the durable deletion barrier, which the self-service path does. The quiesce is a one-shot disable plus active-run scan, so a schedule create or Run Now overlapping the delete could pass its own admission check, land after the scan, and arm or dispatch while the user document was being removed. Only the barrier refuses that admission for the rest of the cascade. Raised first now, refusing with 503 if it cannot be. Admin deletion also now hard-deletes the schedule rows. I had argued the quiesce marks them `deleting` and the reconciler erases them — but the clustered experimental.js entrypoint never arms the engine, so in that topology nothing sweeps and the removed user's prompt text would persist indefinitely. The post-CAS resume guard aborted the claimed turn without recording an outcome. The CAS had already promoted the run out of `requires_action` and the abort deletes the job, so the row was left active with no generation and no evidence, holding a capacity slot until the orphan sweep. It settles as `interrupted` now. .env.example claimed USE_REDIS_STREAMS alone gives replicas a shared job store. It does not: the Redis client it needs is only constructed when USE_REDIS is also true, so stream-only Redis silently leaves the store process-local — which is exactly the topology the scheduler's arming gate treats as safe.
This commit is contained in:
parent
4fa1f74eda
commit
a61198d0df
5 changed files with 96 additions and 2 deletions
|
|
@ -1096,8 +1096,10 @@ OPENWEATHER_API_KEY=
|
|||
# SCHEDULES_DISABLED=true
|
||||
|
||||
# v1 runs the scheduler IN-PROCESS, and the standard server arms it in EVERY replica.
|
||||
# That is only safe when replicas can see each other's generations. With USE_REDIS_STREAMS
|
||||
# they share a job store and the scheduler arms on its own. Without it the job store is
|
||||
# That is only safe when replicas can see each other's generations. With USE_REDIS=true AND
|
||||
# USE_REDIS_STREAMS=true they share a job store and the scheduler arms on its own — the
|
||||
# stream flag alone is NOT enough, because the Redis client it needs is only constructed
|
||||
# when USE_REDIS is also on. Without a shared store the job store is
|
||||
# process-local, so a peer would reconcile runs whose jobs it cannot see and could mark a
|
||||
# still-running generation interrupted. A process cannot count its own replicas, so set
|
||||
# this to assert you run exactly ONE. If neither holds, the scheduler refuses to start and
|
||||
|
|
|
|||
|
|
@ -689,6 +689,17 @@ const ResumeAgentController = async (req, res, next, initializeClient, addTitle)
|
|||
await GenerationJobManager.abortJob(streamId, { expectedCreatedAt: job.createdAt }).catch(
|
||||
(err) => logger.warn('[ResumeAgentController] Failed to abort a superseded resume', err),
|
||||
);
|
||||
// Settle the run too. The CAS already promoted it out of `requires_action`, and
|
||||
// the abort deletes the job, so without this the row stays active with no
|
||||
// generation and no evidence — holding a capacity slot and blocking deletion
|
||||
// until the orphan sweep.
|
||||
await recordScheduleOutcome({
|
||||
scheduleId: job.metadata.scheduleId,
|
||||
scheduledFor: job.metadata.scheduledFor,
|
||||
status: 'interrupted',
|
||||
conversationId,
|
||||
error: 'Schedule was no longer active when the approval was answered',
|
||||
});
|
||||
await decrementPendingRequest(userId);
|
||||
return res.status(409).json({ error: 'This scheduled run is no longer active' });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -19,6 +19,8 @@ const handlers = createAdminUsersHandlers({
|
|||
deleteConfig: db.deleteConfig,
|
||||
deleteAclEntries: db.deleteAclEntries,
|
||||
quiesceUserSchedules,
|
||||
markUserDeleting: db.markUserDeleting,
|
||||
deleteSchedulesByUser: db.deleteSchedulesByUser,
|
||||
});
|
||||
|
||||
router.use(requireJwtAuth, requireAdminAccess);
|
||||
|
|
|
|||
|
|
@ -60,6 +60,8 @@ function createDeps(overrides: Partial<AdminUsersDeps> = {}): AdminUsersDeps {
|
|||
deleteConfig: jest.fn().mockResolvedValue(null),
|
||||
deleteAclEntries: jest.fn().mockResolvedValue(undefined),
|
||||
quiesceUserSchedules: jest.fn().mockResolvedValue(true),
|
||||
markUserDeleting: jest.fn().mockResolvedValue(new Date()),
|
||||
deleteSchedulesByUser: jest.fn().mockResolvedValue(undefined),
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
|
@ -496,6 +498,49 @@ describe('createAdminUsersHandlers', () => {
|
|||
* are not dormant data: an in-flight fire keeps persisting messages and billing
|
||||
* after the user document is gone.
|
||||
*/
|
||||
it('raises the deletion barrier BEFORE quiescing', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminUsersHandlers(deps);
|
||||
const { req, res } = createReqRes({ params: { id: validUserId } });
|
||||
|
||||
await handlers.deleteUser(req, res);
|
||||
|
||||
// The quiesce is a one-shot scan; only the durable barrier refuses admission to
|
||||
// work created after it, so a create/Run Now racing the delete must hit the
|
||||
// barrier rather than slip past the scan.
|
||||
const barrier = (deps.markUserDeleting as jest.Mock).mock.invocationCallOrder[0];
|
||||
const quiesce = (deps.quiesceUserSchedules as jest.Mock).mock.invocationCallOrder[0];
|
||||
expect(barrier).toBeLessThan(quiesce);
|
||||
});
|
||||
|
||||
it('refuses the delete when the barrier cannot be raised', async () => {
|
||||
const deps = createDeps({
|
||||
markUserDeleting: 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.quiesceUserSchedules).not.toHaveBeenCalled();
|
||||
expect(deps.deleteUserById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('hard-deletes the schedule rows rather than trusting the reconciler sweep', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminUsersHandlers(deps);
|
||||
const { req, res } = createReqRes({ params: { id: validUserId } });
|
||||
|
||||
await handlers.deleteUser(req, res);
|
||||
|
||||
// The clustered entrypoint never arms the engine, so nothing would sweep them.
|
||||
expect(deps.deleteSchedulesByUser).toHaveBeenCalledWith(validUserId);
|
||||
const rows = (deps.deleteSchedulesByUser as jest.Mock).mock.invocationCallOrder[0];
|
||||
const userDel = (deps.deleteUserById as jest.Mock).mock.invocationCallOrder[0];
|
||||
expect(rows).toBeLessThan(userDel);
|
||||
});
|
||||
|
||||
it('quiesces the user schedules before removing the user', async () => {
|
||||
const deps = createDeps();
|
||||
const handlers = createAdminUsersHandlers(deps);
|
||||
|
|
|
|||
|
|
@ -48,6 +48,13 @@ export interface AdminUsersDeps {
|
|||
* confirmed, in which case deletion must be refused rather than proceed.
|
||||
*/
|
||||
quiesceUserSchedules: (userId: string) => Promise<boolean>;
|
||||
/** Raises the durable, one-way account-deletion barrier. Must run BEFORE the quiesce:
|
||||
* the quiesce is a one-shot scan, and only the barrier refuses admission to work
|
||||
* created after it. */
|
||||
markUserDeleting: (userId: string) => Promise<Date | null>;
|
||||
/** Hard-deletes the user's Schedule/ScheduleRun rows. Not left to the reconciler's
|
||||
* `deleting` sweep, which the clustered entrypoint never runs. */
|
||||
deleteSchedulesByUser: (userId: string) => Promise<void>;
|
||||
}
|
||||
|
||||
export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
||||
|
|
@ -62,6 +69,8 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
deleteConfig,
|
||||
deleteAclEntries,
|
||||
quiesceUserSchedules,
|
||||
markUserDeleting,
|
||||
deleteSchedulesByUser,
|
||||
} = deps;
|
||||
|
||||
async function listUsersHandler(req: ServerRequest, res: Response) {
|
||||
|
|
@ -161,6 +170,23 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
}
|
||||
}
|
||||
|
||||
// Raise the durable barrier FIRST, exactly as the self-service controller does.
|
||||
// The quiesce below is a one-shot disable + active-run scan, so a schedule create
|
||||
// or Run Now that overlaps it can pass its own admission check, land after the
|
||||
// scan, and arm or dispatch while the user document is being removed. Only the
|
||||
// barrier refuses that admission for the rest of the cascade.
|
||||
const barrierRaised = await markUserDeleting(id).then(
|
||||
() => true,
|
||||
(error) => {
|
||||
logger.error('[adminUsers] Failed to raise the deletion barrier', error);
|
||||
return false;
|
||||
},
|
||||
);
|
||||
if (!barrierRaised) {
|
||||
res.set('Retry-After', '30');
|
||||
return res.status(503).json({ error: 'Could not start deletion. Please retry shortly.' });
|
||||
}
|
||||
|
||||
// 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
|
||||
|
|
@ -177,6 +203,14 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
});
|
||||
}
|
||||
|
||||
// Hard-delete the schedule rows rather than relying on the reconciler's
|
||||
// `deleting` sweep: the clustered `experimental.js` entrypoint never arms the
|
||||
// engine, so in that topology nothing would ever erase them and the deleted
|
||||
// user's prompt text would persist indefinitely.
|
||||
await deleteSchedulesByUser(id).catch((error) => {
|
||||
logger.error('[adminUsers] Failed to delete schedules for the removed user', error);
|
||||
});
|
||||
|
||||
const result = await deleteUserById(id);
|
||||
|
||||
if (result.deletedCount === 0) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue