From d50d64e63db7e22c4c72f1665933abde8bcd0d3b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 31 Jul 2026 00:16:08 -0400 Subject: [PATCH] fix: round eighteen: admin deletion commitment, cached-hit tombstone check, erasure idempotency tombstone, shutdown dispatch gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- api/server/routes/admin/users.js | 1 + api/strategies/openIdJwtStrategy.js | 26 ++++++++++- api/strategies/openIdJwtStrategy.spec.js | 1 + packages/api/src/admin/users.spec.ts | 22 +++++++++ packages/api/src/admin/users.ts | 25 +++++++++- packages/api/src/schedules/engine.ts | 15 ++++-- packages/api/src/schedules/fire.ts | 16 ++++++- packages/api/src/schedules/types.ts | 2 + .../src/methods/schedule.methods.spec.ts | 46 +++++++++++++++++++ packages/data-schemas/src/methods/schedule.ts | 36 +++++++++++++-- packages/data-schemas/src/schema/schedule.ts | 14 ++++++ packages/data-schemas/src/types/schedule.ts | 2 + 12 files changed, 197 insertions(+), 9 deletions(-) diff --git a/api/server/routes/admin/users.js b/api/server/routes/admin/users.js index 2ef5dece72..1bcd4c1483 100644 --- a/api/server/routes/admin/users.js +++ b/api/server/routes/admin/users.js @@ -20,6 +20,7 @@ const handlers = createAdminUsersHandlers({ deleteAclEntries: db.deleteAclEntries, quiesceUserSchedules, markUserDeleting: db.markUserDeleting, + markUserDeletionCommitted: db.markUserDeletionCommitted, deleteSchedulesByUser: db.deleteSchedulesByUser, }); diff --git a/api/strategies/openIdJwtStrategy.js b/api/strategies/openIdJwtStrategy.js index 530441eee7..1c407674cc 100644 --- a/api/strategies/openIdJwtStrategy.js +++ b/api/strategies/openIdJwtStrategy.js @@ -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 diff --git a/api/strategies/openIdJwtStrategy.spec.js b/api/strategies/openIdJwtStrategy.spec.js index 21c0c471ac..8448683f5e 100644 --- a/api/strategies/openIdJwtStrategy.spec.js +++ b/api/strategies/openIdJwtStrategy.spec.js @@ -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(), diff --git a/packages/api/src/admin/users.spec.ts b/packages/api/src/admin/users.spec.ts index f1627016fa..9fb05c43ca 100644 --- a/packages/api/src/admin/users.spec.ts +++ b/packages/api/src/admin/users.spec.ts @@ -61,6 +61,7 @@ function createDeps(overrides: Partial = {}): 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 () => { diff --git a/packages/api/src/admin/users.ts b/packages/api/src/admin/users.ts index 6dc1cae018..63041bb763 100644 --- a/packages/api/src/admin/users.ts +++ b/packages/api/src/admin/users.ts @@ -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; + /** Commits the deletion to automatic completion; only committed rows are swept. */ + markUserDeletionCommitted: (userId: string) => Promise; /** 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; @@ -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 diff --git a/packages/api/src/schedules/engine.ts b/packages/api/src/schedules/engine.ts index 45f7ca580a..ecf18d0e7e 100644 --- a/packages/api/src/schedules/engine.ts +++ b/packages/api/src/schedules/engine.ts @@ -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; } diff --git a/packages/api/src/schedules/fire.ts b/packages/api/src/schedules/fire.ts index e53bd897a7..1598d21216 100644 --- a/packages/api/src/schedules/fire.ts +++ b/packages/api/src/schedules/fire.ts @@ -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 diff --git a/packages/api/src/schedules/types.ts b/packages/api/src/schedules/types.ts index 6ceef30ac1..8c264039c0 100644 --- a/packages/api/src/schedules/types.ts +++ b/packages/api/src/schedules/types.ts @@ -144,6 +144,8 @@ export interface ScheduleEngineDeps { * the conversationId but strips scheduleId/scheduledFor) before trusting the status. */ getJobStatus: (conversationId: string) => Promise; + /** 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. diff --git a/packages/data-schemas/src/methods/schedule.methods.spec.ts b/packages/data-schemas/src/methods/schedule.methods.spec.ts index 72577e1072..07311508d9 100644 --- a/packages/data-schemas/src/methods/schedule.methods.spec.ts +++ b/packages/data-schemas/src/methods/schedule.methods.spec.ts @@ -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); diff --git a/packages/data-schemas/src/methods/schedule.ts b/packages/data-schemas/src/methods/schedule.ts index 4961d98cbe..e84dd83f85 100644 --- a/packages/data-schemas/src/methods/schedule.ts +++ b/packages/data-schemas/src/methods/schedule.ts @@ -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 { return Schedule() - .find({ deleting: true }) + .find({ deleting: true, erased: { $ne: true } }) .sort({ eraseAttemptedAt: 1 }) .limit(limit) .lean(); @@ -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; } diff --git a/packages/data-schemas/src/schema/schedule.ts b/packages/data-schemas/src/schema/schedule.ts index c032823276..7cdd5e8908 100644 --- a/packages/data-schemas/src/schema/schedule.ts +++ b/packages/data-schemas/src/schema/schedule.ts @@ -118,6 +118,15 @@ const scheduleSchema: Schema = 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 = 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 diff --git a/packages/data-schemas/src/types/schedule.ts b/packages/data-schemas/src/types/schedule.ts index 9eb21c9101..57d7cfb14a 100644 --- a/packages/data-schemas/src/types/schedule.ts +++ b/packages/data-schemas/src/types/schedule.ts @@ -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;