mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
fix: round eighteen: admin deletion commitment, cached-hit tombstone check, erasure idempotency tombstone, shutdown dispatch gate
- The admin deletion handler commits to automatic completion BEFORE raising the barrier, mirroring the self-service controller: an uncommitted barrier (worker exit, unretried 503) locked the account with its data retained forever since the sweep only finishes committed rows. Ordering and refusal are spec-locked. - OpenID cache hits check the deletion tombstone before counting as a conclusive fence: a hit read in the barrier's stamp-to-sweep window authenticated a pre-barrier document. Tombstoned hits fall through to the fresh lookup, whose own checks refuse the barriered user. - Erasing a schedule that carries a create-idempotency key leaves a content-free tombstone (24h TTL) instead of vanishing: a create retry whose response was lost otherwise recreated recurring work the owner had already deleted. The replay path resolves the tombstone through the existing deleting-row 410; sweeps exclude tombstones. - fireSchedule steps aside at the dispatch boundary once shutdown begins (the coordinator closes the listener before pre-drain tasks run), and a refused connection during shutdown classifies as ambiguous instead of a definite error — no auto-disable creep from restarts. The engine threads its stop flag to the fire path.
This commit is contained in:
parent
396bf61571
commit
d50d64e63d
12 changed files with 197 additions and 9 deletions
|
|
@ -20,6 +20,7 @@ const handlers = createAdminUsersHandlers({
|
|||
deleteAclEntries: db.deleteAclEntries,
|
||||
quiesceUserSchedules,
|
||||
markUserDeleting: db.markUserDeleting,
|
||||
markUserDeletionCommitted: db.markUserDeletionCommitted,
|
||||
deleteSchedulesByUser: db.deleteSchedulesByUser,
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ const {
|
|||
getOpenIdIssuer,
|
||||
normalizeOpenIdIssuer,
|
||||
buildAuthUserDocCacheKey,
|
||||
buildAuthUserDocTombstoneKey,
|
||||
getAuthUserDocCacheMode,
|
||||
getCachedAuthUserDoc,
|
||||
invalidateCachedAuthUserDoc,
|
||||
|
|
@ -116,10 +117,33 @@ const openIdJwtLogin = (openIdConfig) => {
|
|||
const authUserCacheMode = getAuthUserDocCacheMode();
|
||||
const authUserCacheStore =
|
||||
authUserCacheMode !== 'off' && authUserCacheKey ? getAuthUserDocCacheStore() : undefined;
|
||||
const cachedUser =
|
||||
let cachedUser =
|
||||
authUserCacheMode !== 'off' && authUserCacheStore && authUserCacheKey
|
||||
? await getCachedAuthUserDoc(authUserCacheStore, authUserCacheKey)
|
||||
: undefined;
|
||||
// A hit is only a conclusive deletion fence if the user's tombstone is
|
||||
// ABSENT: the barrier stamps Mongo, writes the tombstone, then sweeps keys,
|
||||
// so a hit read in the stamp-to-sweep window would otherwise authenticate a
|
||||
// pre-barrier document during the cascade. A tombstoned hit falls through
|
||||
// to the fresh lookup, whose own checks refuse the barriered user. Costs
|
||||
// one extra cache read per hit; entries expire in seconds, so past the
|
||||
// tombstone's window a hit cannot be pre-barrier.
|
||||
if (cachedUser != null && authUserCacheStore) {
|
||||
const cachedUserId = cachedUser._id ?? cachedUser.id;
|
||||
const tombstoned =
|
||||
cachedUserId != null
|
||||
? await authUserCacheStore
|
||||
.get(buildAuthUserDocTombstoneKey(String(cachedUserId)))
|
||||
.catch(() => true)
|
||||
: true;
|
||||
if (tombstoned != null && tombstoned !== false) {
|
||||
await invalidateCachedAuthUserDoc(authUserCacheStore, {
|
||||
userId: cachedUserId != null ? String(cachedUserId) : undefined,
|
||||
cacheKey: authUserCacheKey,
|
||||
});
|
||||
cachedUser = undefined;
|
||||
}
|
||||
}
|
||||
|
||||
const servedCachedUser = authUserCacheMode === 'on' && cachedUser != null;
|
||||
const lookupResult = servedCachedUser
|
||||
|
|
|
|||
|
|
@ -35,6 +35,7 @@ jest.mock('@librechat/api', () => ({
|
|||
getOpenIdIssuer: jest.fn(() => 'https://issuer.example.com'),
|
||||
normalizeOpenIdIssuer: jest.requireActual('@librechat/api').normalizeOpenIdIssuer,
|
||||
buildAuthUserDocCacheKey: jest.fn(() => 'auth-user-doc-key'),
|
||||
buildAuthUserDocTombstoneKey: jest.requireActual('@librechat/api').buildAuthUserDocTombstoneKey,
|
||||
getAuthUserDocCacheMode: jest.fn(() => 'off'),
|
||||
getCachedAuthUserDoc: jest.fn(),
|
||||
invalidateCachedAuthUserDoc: jest.fn(),
|
||||
|
|
|
|||
|
|
@ -61,6 +61,7 @@ function createDeps(overrides: Partial<AdminUsersDeps> = {}): AdminUsersDeps {
|
|||
deleteAclEntries: jest.fn().mockResolvedValue(undefined),
|
||||
quiesceUserSchedules: jest.fn().mockResolvedValue(true),
|
||||
markUserDeleting: jest.fn().mockResolvedValue(new Date()),
|
||||
markUserDeletionCommitted: jest.fn().mockResolvedValue(undefined),
|
||||
deleteSchedulesByUser: jest.fn().mockResolvedValue(undefined),
|
||||
...overrides,
|
||||
};
|
||||
|
|
@ -511,6 +512,27 @@ describe('createAdminUsersHandlers', () => {
|
|||
const barrier = (deps.markUserDeleting as jest.Mock).mock.invocationCallOrder[0];
|
||||
const quiesce = (deps.quiesceUserSchedules as jest.Mock).mock.invocationCallOrder[0];
|
||||
expect(barrier).toBeLessThan(quiesce);
|
||||
// COMMITMENT precedes the barrier: the barrier refuses authentication and the
|
||||
// sweep only finishes committed deletions, so an uncommitted barrier (worker
|
||||
// exit, unretried 503) locked the account with its data retained forever.
|
||||
const committed = (deps.markUserDeletionCommitted as jest.Mock).mock.invocationCallOrder[0];
|
||||
expect(committed).toBeLessThan(barrier);
|
||||
});
|
||||
|
||||
it('refuses the delete when the commitment cannot be recorded', async () => {
|
||||
const deps = createDeps({
|
||||
markUserDeletionCommitted: 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);
|
||||
// A barrier without a commitment is the unrecoverable lockout; refusing before
|
||||
// the barrier leaves the account fully functional for a clean retry.
|
||||
expect(deps.markUserDeleting).not.toHaveBeenCalled();
|
||||
expect(deps.deleteUserById).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('refuses the delete when the barrier cannot be raised', async () => {
|
||||
|
|
|
|||
|
|
@ -52,6 +52,8 @@ export interface AdminUsersDeps {
|
|||
* the quiesce is a one-shot scan, and only the barrier refuses admission to work
|
||||
* created after it. */
|
||||
markUserDeleting: (userId: string) => Promise<Date | null>;
|
||||
/** Commits the deletion to automatic completion; only committed rows are swept. */
|
||||
markUserDeletionCommitted: (userId: string) => Promise<void>;
|
||||
/** 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>;
|
||||
|
|
@ -70,6 +72,7 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
deleteAclEntries,
|
||||
quiesceUserSchedules,
|
||||
markUserDeleting,
|
||||
markUserDeletionCommitted,
|
||||
deleteSchedulesByUser,
|
||||
} = deps;
|
||||
|
||||
|
|
@ -170,7 +173,27 @@ export function createAdminUsersHandlers(deps: AdminUsersDeps): {
|
|||
}
|
||||
}
|
||||
|
||||
// Raise the durable barrier FIRST, exactly as the self-service controller does.
|
||||
// COMMIT to automatic completion BEFORE the barrier, exactly as the
|
||||
// self-service controller orders it: the barrier refuses authentication, and
|
||||
// the background sweep only finishes COMMITTED deletions — an uncommitted
|
||||
// barrier (worker exit here, or an unretried 503 below) locked the account
|
||||
// with its data retained indefinitely. Committed-without-barrier is inert.
|
||||
let committed = false;
|
||||
for (let attempt = 1; attempt <= 3 && !committed; attempt++) {
|
||||
committed = await markUserDeletionCommitted(id).then(
|
||||
() => true,
|
||||
(error) => {
|
||||
logger.error(`[adminUsers] Failed to commit deletion (attempt ${attempt}/3)`, error);
|
||||
return false;
|
||||
},
|
||||
);
|
||||
}
|
||||
if (!committed) {
|
||||
res.set('Retry-After', '30');
|
||||
return res.status(503).json({ error: 'Could not start deletion. Please retry shortly.' });
|
||||
}
|
||||
|
||||
// Raise the durable barrier next, 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
|
||||
|
|
|
|||
|
|
@ -402,9 +402,18 @@ export function startScheduleEngine(deps: ScheduleEngineDeps): ScheduleEngine {
|
|||
return false;
|
||||
}
|
||||
try {
|
||||
const result = await fireSchedule(deps, schedule, limits, scheduledFor, {
|
||||
dbNow: new Date(dbNow),
|
||||
});
|
||||
const result = await fireSchedule(
|
||||
// The engine's stop flag reaches the dispatch boundary: a pass in flight
|
||||
// when shutdown begins releases its claim instead of POSTing at the
|
||||
// closing listener.
|
||||
{ ...deps, isShuttingDown: () => stopped },
|
||||
schedule,
|
||||
limits,
|
||||
scheduledFor,
|
||||
{
|
||||
dbNow: new Date(dbNow),
|
||||
},
|
||||
);
|
||||
if (result.fired) {
|
||||
fired += 1;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -161,9 +161,12 @@ async function postChatMessageInner(
|
|||
// (reset after send, request timeout) is genuinely ambiguous: the generation may
|
||||
// already be running, so leave the run reconcilable.
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
// During shutdown a refused connection is the CLOSING listener, not a broken
|
||||
// SCHEDULES_SELF_URL: classify it ambiguous so the row stays reconcilable and
|
||||
// no failure is booked against the schedule for a restart.
|
||||
throw new ScheduleFireError(
|
||||
`Fire POST network failure: ${message}`,
|
||||
!isDefiniteConnectFailure(error),
|
||||
!isDefiniteConnectFailure(error) || deps.isShuttingDown?.() === true,
|
||||
);
|
||||
}
|
||||
if (!response.ok) {
|
||||
|
|
@ -435,6 +438,17 @@ export async function fireSchedule(
|
|||
return stepAsideSuperseded();
|
||||
}
|
||||
|
||||
// SHUTDOWN gate at the dispatch boundary: the coordinator closes the listener
|
||||
// BEFORE the engine's pre-drain task runs, so a pass already past its preflight
|
||||
// would POST at a refusing socket — a definite connect failure that terminalizes
|
||||
// as `error` and walks a healthy schedule toward auto-disable for nothing more
|
||||
// than a restart. Nothing is reserved yet, so stepping aside leaves the
|
||||
// occurrence due for the restarted process (within the misfire grace).
|
||||
if (deps.isShuttingDown?.() === true) {
|
||||
logger.info(`[schedules] shutdown in progress; releasing claim on ${schedule.id}`);
|
||||
return stepAsideSuperseded();
|
||||
}
|
||||
|
||||
// Pre-generate the conversation id and reserve the run row up front. The
|
||||
// loopback POST reuses it (streamId === conversationId), so reconciliation can
|
||||
// ALWAYS locate this occurrence's job — even if the post-accept detail write
|
||||
|
|
|
|||
|
|
@ -144,6 +144,8 @@ export interface ScheduleEngineDeps {
|
|||
* the conversationId but strips scheduleId/scheduledFor) before trusting the status.
|
||||
*/
|
||||
getJobStatus: (conversationId: string) => Promise<JobState | null>;
|
||||
/** True once graceful shutdown began; fires step aside instead of dispatching. */
|
||||
isShuttingDown?: () => boolean;
|
||||
/**
|
||||
* Aborts the loopback generation for a scheduled occurrence, identity-guarded so
|
||||
* it never signals/clobbers a replacement turn that reused the conversationId.
|
||||
|
|
|
|||
|
|
@ -1544,6 +1544,52 @@ describe('deleteScheduleRun conversation fence', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('erasure leaves an idempotency tombstone', () => {
|
||||
it('retains the create key so a late retry cannot resurrect deleted work', async () => {
|
||||
const user = new mongoose.Types.ObjectId();
|
||||
const schedule = await methods.createSchedule(
|
||||
scheduleData({
|
||||
user,
|
||||
deleting: true,
|
||||
nextRunAt: undefined,
|
||||
clientRequestId: 'late-retry-key',
|
||||
clientRequestDigest: 'digest-1',
|
||||
}),
|
||||
);
|
||||
|
||||
await expect(methods.eraseScheduleIfDrained(schedule.id)).resolves.toBe(true);
|
||||
|
||||
// The content is gone, the identity is not: a create retry with this key must
|
||||
// resolve to the tombstone (the handler answers 410 on deleting rows) instead
|
||||
// of missing entirely and recreating the recurring work the owner deleted.
|
||||
const tombstone = await methods.getScheduleByClientRequestId(user, 'late-retry-key');
|
||||
expect(tombstone).not.toBeNull();
|
||||
expect(tombstone?.erased).toBe(true);
|
||||
expect(tombstone?.deleting).toBe(true);
|
||||
expect(tombstone?.clientRequestDigest).toBe('digest-1');
|
||||
expect(tombstone?.prompt).toBeUndefined();
|
||||
expect(tombstone?.name).toBeUndefined();
|
||||
|
||||
// Tombstones are inert to every sweep: re-erasing or re-deleting them forever
|
||||
// would pin the bounded windows.
|
||||
const sweepRows = await methods.getDeletingSchedules(50);
|
||||
expect(sweepRows.some((row) => row.id === schedule.id)).toBe(false);
|
||||
const retryIds = await methods.getDeletingScheduleIds(user, 50);
|
||||
expect(retryIds).not.toContain(schedule.id);
|
||||
});
|
||||
|
||||
it('hard-deletes rows that never carried a create key', async () => {
|
||||
const user = new mongoose.Types.ObjectId();
|
||||
const schedule = await methods.createSchedule(
|
||||
scheduleData({ user, deleting: true, nextRunAt: undefined }),
|
||||
);
|
||||
|
||||
await expect(methods.eraseScheduleIfDrained(schedule.id)).resolves.toBe(true);
|
||||
const gone = await mongoose.models.Schedule.findOne({ id: schedule.id }).lean();
|
||||
expect(gone).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('deletion quiescing (soft-delete, drain, erase)', () => {
|
||||
it('markScheduleDeleting hides + un-claims; erase waits for active runs to drain', async () => {
|
||||
const schedule = await methods.createScheduleWithSlot(scheduleData(), 10);
|
||||
|
|
|
|||
|
|
@ -1530,7 +1530,7 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche
|
|||
* leases) starved every row behind it out of the sweep indefinitely. */
|
||||
async function getDeletingSchedules(limit: number): Promise<ISchedule[]> {
|
||||
return Schedule()
|
||||
.find({ deleting: true })
|
||||
.find({ deleting: true, erased: { $ne: true } })
|
||||
.sort({ eraseAttemptedAt: 1 })
|
||||
.limit(limit)
|
||||
.lean<ISchedule[]>();
|
||||
|
|
@ -1572,7 +1572,7 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche
|
|||
// the ones beyond it never get their deletion re-driven. Callers stamp
|
||||
// markEraseAttempted after each attempt to rotate the window.
|
||||
const rows = await Schedule()
|
||||
.find({ user: userId, deleting: true })
|
||||
.find({ user: userId, deleting: true, erased: { $ne: true } })
|
||||
.sort({ eraseAttemptedAt: 1 })
|
||||
.select('id')
|
||||
.limit(limit)
|
||||
|
|
@ -1614,7 +1614,37 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche
|
|||
return false;
|
||||
}
|
||||
await ScheduleRun().deleteMany({ scheduleId: id });
|
||||
await Schedule().deleteOne({ id, deleting: true });
|
||||
// Rows carrying a create-idempotency key leave a content-free TOMBSTONE instead
|
||||
// of vanishing: a create whose response was lost retries with the same key, and
|
||||
// if the owner deleted the schedule before that retry arrived, a hard delete
|
||||
// would let the retry recreate the recurring work they just removed. The
|
||||
// tombstone keeps only the identity fields (key, digest, owner) for a bounded
|
||||
// window (TTL on erasedAt); the replay path answers "deleted" against it.
|
||||
const tombstoned = await Schedule().updateOne(
|
||||
{ id, deleting: true, clientRequestId: { $exists: true } },
|
||||
{
|
||||
$set: { erased: true, erasedAt: new Date(), enabled: false },
|
||||
$unset: {
|
||||
name: 1,
|
||||
prompt: 1,
|
||||
agent_id: 1,
|
||||
cadence: 1,
|
||||
timezone: 1,
|
||||
file_ids: 1,
|
||||
lastRun: 1,
|
||||
nextRunAt: 1,
|
||||
leaseUntil: 1,
|
||||
leaseBy: 1,
|
||||
disabledReason: 1,
|
||||
countedFor: 1,
|
||||
},
|
||||
// Not a config edit; the tombstone must not surface in updated-time listings.
|
||||
},
|
||||
{ timestamps: false },
|
||||
);
|
||||
if (tombstoned.matchedCount === 0) {
|
||||
await Schedule().deleteOne({ id, deleting: true });
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -118,6 +118,15 @@ const scheduleSchema: Schema<IScheduleDocument> = new Schema(
|
|||
type: Boolean,
|
||||
default: false,
|
||||
},
|
||||
/** Erased tombstone: content is gone, only the create-idempotency identity
|
||||
* remains for a bounded retry window (TTL below). A delayed create retry that
|
||||
* matches this key must answer "deleted", not resurrect the recurring work. */
|
||||
erased: {
|
||||
type: Boolean,
|
||||
},
|
||||
erasedAt: {
|
||||
type: Date,
|
||||
},
|
||||
/**
|
||||
* Per-user occupancy slot in [0, maxPerUser). Assigned atomically via the
|
||||
* partial unique index below so concurrent creates cannot exceed the cap: two
|
||||
|
|
@ -196,6 +205,11 @@ const scheduleSchema: Schema<IScheduleDocument> = new Schema(
|
|||
},
|
||||
);
|
||||
|
||||
// Idempotency tombstones expire after the bounded retry window; live rows never match.
|
||||
scheduleSchema.index(
|
||||
{ erasedAt: 1 },
|
||||
{ expireAfterSeconds: 24 * 60 * 60, partialFilterExpression: { erased: true } },
|
||||
);
|
||||
scheduleSchema.index({ id: 1, tenantId: 1 }, { unique: true });
|
||||
scheduleSchema.index({ enabled: 1, nextRunAt: 1 });
|
||||
// Atomic per-user create cap: a live (non-deleting) schedule occupies a unique
|
||||
|
|
|
|||
|
|
@ -28,6 +28,8 @@ export interface ISchedule {
|
|||
/** Owner-config generation; bumped only by an owner edit. */
|
||||
configRevision?: number;
|
||||
deleting?: boolean;
|
||||
erased?: boolean;
|
||||
erasedAt?: Date;
|
||||
slot?: number;
|
||||
/** Client-supplied idempotency key of the create that produced this row. */
|
||||
clientRequestId?: string;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue