fix: Codex round 11 + three self-review findings on the schedule seams

Codex round 11:
- admin user deletion refuses (503, retryable) when the schedule-row hard delete
  fails, instead of deleting the user doc and making the leftover prompt rows
  permanently unretryable.
- isGloballyDisabled reads both stop shapes via the shared isRuntimeDisabled
  (now exported): the object form { use: false } previously kept the engine
  claiming while getLimits refused fires, so a maintenance stop dropped every
  occurrence it covered instead of freezing them.
- Both entrypoints keep schedule DELETE open when the engine never arms;
  stranding users with schedules they can see but never remove was the
  clustered gate's behavior and the standard gate's forever-503.
- The abort replacement check fails closed (503) when the job store read
  throws: unknown is not absence, and falling through pruned the
  conversation-wide checkpoint a live replacement depends on.

Self-review (adversarial workflow on b830ea127):
- A failed revalidation never advances: advance() is a no-op in every true
  supersession (token rotated) and only ever landed on PURE lease expiry,
  where it moved nextRunAt past an occurrence nothing fired. stepAsideSuperseded
  releases leases only; manual run-now keeps its release-only semantics.
- The preStartAbort branch releases the fire's lease explicitly: an owner edit
  rotates the claim token (advance no-ops) but never touches lease fields,
  leaving Run Now and the next claim blocked for the 5-minute TTL.
- The pre-start fence abort in the chat controller releases the pending-request
  slot and idempotency claim (manual Run Now is not limiter-exempt), mirroring
  the init-error cleanup; leaving them held 429'd the user's next messages.

Every fix carries a test verified by neutering the fix (test fails) and
restoring it (test passes). Also precises the delete-user.js barrier comment:
the in-process cache topology bounds the residual window at the 5s TTL and the
fire guard reads Mongo directly.
This commit is contained in:
Danny Avila 2026-07-28 08:23:22 -04:00
parent b830ea1278
commit 1c3f53bd4a
15 changed files with 247 additions and 32 deletions

View file

@ -11,6 +11,7 @@ const mockGenerationJobManager = {
createJob: jest.fn(),
emitError: jest.fn(),
completeJob: jest.fn(),
abortJob: jest.fn(),
getResumeState: jest.fn(),
updateMetadata: jest.fn(),
claimGeneration: jest.fn(),
@ -880,6 +881,40 @@ describe('ResumableAgentController resume metadata', () => {
expect(mockGenerationJobManager.createJob).not.toHaveBeenCalled();
});
it('releases the pending slot and idempotency claim when the pre-start fence aborts', async () => {
// Manual Run Now is NOT limiter-exempt, so the controller incremented the pending
// counter and claimed the clientRequestId. The fence abort skips the whole
// generation, so nothing downstream releases either — this exit must, or the
// user's next interactive messages 429 until the counter's TTL expires.
const schedules = require('~/server/services/Schedules');
schedules.isScheduleLive.mockResolvedValueOnce(false);
mockGenerationJobManager.claimGeneration.mockResolvedValue({ claimed: true });
mockGenerationJobManager.abortJob.mockResolvedValue({ success: true });
const req = {
user: { id: 'user-123' },
_isScheduledFire: true,
_isManualScheduledFire: true,
body: {
text: 'Scheduled prompt',
messageId: 'user-msg',
clientRequestId: 'req-abc',
conversationId: 'conversation-123',
scheduleId: 'sched-1',
scheduledFor: '2026-07-28T12:00:00.000Z',
endpointOption: { endpoint: 'agents', modelOptions: { model: 'gpt-4.1' } },
},
config: {},
};
const res = createResumableResponse();
await AgentController(req, res, jest.fn(), jest.fn(), null);
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ status: 'aborted' }));
expect(mockCheckAndIncrementPendingRequest).toHaveBeenCalledWith('user-123');
expect(mockDecrementPendingRequest).toHaveBeenCalledWith('user-123');
expect(mockGenerationJobManager.releaseGeneration).toHaveBeenCalledWith('user-123', 'req-abc');
});
it('does not finalize an unscoped generation when job creation rejects before returning', async () => {
mockGenerationJobManager.claimGeneration.mockResolvedValue({ claimed: true });
mockGenerationJobManager.createJob.mockRejectedValue(new Error('create failed before return'));

View file

@ -477,6 +477,15 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit
preserveForReconcile: !outcomeRecorded,
expectedCreatedAt: jobCreatedAt,
}).catch(() => undefined);
// Mirror the init-error cleanup: this return skips the whole generation, so the
// pending-request slot (manual Run Now is NOT limiter-exempt) and the
// idempotency claim it acquired must be released here — nothing downstream
// will. Leaving them held 429'd the user's next interactive messages until the
// counter's TTL expired.
if (ownsIdempotencyClaim) {
await GenerationJobManager.releaseGeneration(userId, clientRequestId).catch(() => {});
}
await finishResumableRequest(req, userId);
startupTelemetry?.end('aborted');
return res.json({ streamId, conversationId, status: 'aborted' });
}

View file

@ -253,16 +253,19 @@ if (cluster.isMaster) {
let expiredFileSweepOptions = null;
let expiredFileSweepStarted = false;
let schedulesReady = false;
const SCHEDULE_WRITE_READ_ONLY_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']);
const SCHEDULE_ENGINE_OPTIONAL_METHODS = new Set(['GET', 'HEAD', 'OPTIONS', 'DELETE']);
/**
* Refuse schedule WRITES in this clustered entrypoint. v1 supports single-process
* scheduling only, and this process never arms the engine, so accepting a create or
* run-now here would persist a schedule nothing will ever fire. Reads stay open so an
* operator can still inspect existing schedules.
* operator can still inspect existing schedules. DELETE stays open too: erasing a
* stored prompt needs no engine (the handler quiesces through the job store and
* refuses unsafe cases itself), and a deployment switched to clustered mode must not
* strand users with schedules they can see but never remove.
*/
const rejectScheduleWritesUntilReady = (req, res, next) => {
if (schedulesReady || SCHEDULE_WRITE_READ_ONLY_METHODS.has(req.method)) {
if (schedulesReady || SCHEDULE_ENGINE_OPTIONAL_METHODS.has(req.method)) {
return next();
}
return res.status(501).json({

View file

@ -70,7 +70,7 @@ let schedulesReady = false;
const SERVER_NOT_READY_CODE = 'SERVER_NOT_READY';
const SCHEDULES_NOT_READY_CODE = 'SCHEDULES_NOT_READY';
const CHAT_START_RETRY_AFTER_SECONDS = '1';
const READ_ONLY_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']);
const SCHEDULE_ENGINE_OPTIONAL_METHODS = new Set(['GET', 'HEAD', 'OPTIONS', 'DELETE']);
const rejectChatStartsUntilReady = (req, res, next) => {
if (serverReady || req.method !== 'POST' || req.path === '/abort') {
@ -89,10 +89,13 @@ const rejectChatStartsUntilReady = (req, res, next) => {
* indexes) is up. With MONGO_AUTO_INDEX disabled those indexes are created only by
* initializeScheduleEngine, which runs after the server starts listening so a
* create/run-now that lands in that window would persist without duplicate
* protection. Reads stay open; writers get a 503 + Retry-After.
* protection. Reads stay open; writers get a 503 + Retry-After. DELETE stays open
* too: erasure needs none of those indexes, and an engine that REFUSES to arm keeps
* this gate closed forever users must still be able to remove schedules (and their
* stored prompts) they can see.
*/
const rejectScheduleWritesUntilReady = (req, res, next) => {
if (schedulesReady || READ_ONLY_METHODS.has(req.method)) {
if (schedulesReady || SCHEDULE_ENGINE_OPTIONAL_METHODS.has(req.method)) {
return next();
}

View file

@ -160,6 +160,19 @@ describe('Startup readiness wiring', () => {
);
});
it('keeps schedule DELETE open when the engine never arms (both entrypoints)', () => {
// An engine that refuses to arm keeps the gate closed forever; erasure needs no
// engine, and users must not be stranded with schedules they can see but never
// remove. The set name is asserted so a revert to READ_ONLY_METHODS fails here.
const experimental = fs.readFileSync(path.join(__dirname, 'experimental.js'), 'utf8');
for (const entrypoint of [source, experimental]) {
expect(entrypoint).toMatch(
/SCHEDULE_ENGINE_OPTIONAL_METHODS = new Set\(\['GET', 'HEAD', 'OPTIONS', 'DELETE'\]\)/,
);
expect(entrypoint).toContain('SCHEDULE_ENGINE_OPTIONAL_METHODS.has(req.method)');
}
});
it('flips schedulesReady only from the engine-init result (indexes must exist first)', () => {
const engineInitIndex = source.indexOf(
'const scheduleEngine = await initializeScheduleEngine(',

View file

@ -24,6 +24,7 @@ const mockGenerationJobManager = {
};
const mockSaveMessage = jest.fn();
const mockDeleteAgentCheckpoint = jest.fn(async () => undefined);
const mockRecordScheduleOutcome = jest.fn(async () => true);
const mockClearScheduledJob = jest.fn(async () => undefined);
@ -41,6 +42,7 @@ jest.mock('@librechat/api', () => ({
...jest.requireActual('@librechat/api'),
isEnabled: jest.fn().mockReturnValue(false),
GenerationJobManager: mockGenerationJobManager,
deleteAgentCheckpoint: (...args) => mockDeleteAgentCheckpoint(...args),
}));
jest.mock('~/models', () => ({
@ -558,6 +560,32 @@ describe('Agent Abort Endpoint', () => {
// Nothing to damage, so pressing Stop as a turn finishes stays a quiet success.
expect(response.status).toBe(200);
});
it('fails closed when the replacement check cannot read the store', async () => {
// null from getJob means CONFIRMED absent; a THROW means unknown — a
// replacement may be live, and the conversation-wide checkpoint prune below
// would strip its resume state. With the store unreadable, the handler must
// stop before any side effect rather than treat the error as absence.
mockGenerationJobManager.getJob
.mockResolvedValueOnce({
metadata: { userId: 'test-user-123', pendingAction: { payload: {} } },
createdAt: 1000,
})
.mockRejectedValue(new Error('redis gone'));
mockGenerationJobManager.abortJob.mockResolvedValue({
success: false,
jobData: null,
content: [],
});
const response = await request(app)
.post('/api/agents/chat/abort')
.send({ conversationId: 'conv-1' });
expect(response.status).toBe(503);
expect(mockDeleteAgentCheckpoint).not.toHaveBeenCalled();
expect(mockSaveMessage).not.toHaveBeenCalled();
});
});
describe('Scheduled runs', () => {

View file

@ -391,7 +391,20 @@ router.post('/chat/abort', configMiddleware, async (req, res) => {
// is nothing to damage in that case, so it keeps its previous behaviour rather than
// turning a routine stop into an error.
if (!abortResult.success && abortResult.jobData == null) {
const liveJob = await GenerationJobManager.getJob(jobStreamId).catch(() => null);
// FAIL CLOSED on an unreadable store: null means "confirmed absent" (benign),
// but a thrown read means UNKNOWN — a replacement may be live, and everything
// below acts on the conversation as a whole (the checkpoint prune would strip
// its resume state). Nothing of ours was stopped (the abort already failed), so
// refusing here has no side effects and the client simply retries.
let liveJob;
try {
liveJob = await GenerationJobManager.getJob(jobStreamId);
} catch (err) {
logger.error(`[AgentStream] Could not verify abort state: ${jobStreamId}`, err);
return res
.status(503)
.json({ error: 'Could not verify the generation state. Please retry.', aborted: null });
}
if (liveJob != null && liveJob.createdAt !== job.createdAt) {
logger.debug(
`[AgentStream] Abort refused: generation was replaced before it landed: ${jobStreamId}`,

View file

@ -86,11 +86,13 @@ async function gracefulExit(code = 0) {
// boundary (fireSchedule's isOwnerDeleting probe) from this point on, so anything the
// count then misses cannot have started after it.
//
// Through markUserDeleting, never a raw update: the barrier is only in force once no
// CACHED pre-barrier user document can still populate req.user on a live server, and
// that method is what drops the auth cache entry. It fails closed, so a cache it
// cannot reach aborts the deletion rather than proceeding behind a barrier that was
// never actually raised.
// Through markUserDeleting, never a raw update: that method also drops the auth
// user-doc cache entry, and fails closed — a cache it cannot reach aborts the
// deletion rather than proceeding behind a barrier that was never actually raised.
// With a shared (Redis) cache this invalidation reaches the live server. With the
// in-process cache no external script can, but the residual window is bounded by the
// 5s AUTH_USER_DOC_CACHE_TTL_MS, and the schedule-fire guard (isOwnerDeleting) reads
// Mongo directly rather than trusting req.user.
try {
await markUserDeleting(uid);
} catch (err) {

View file

@ -580,6 +580,22 @@ describe('createAdminUsersHandlers', () => {
expect(deps.deleteUserById).not.toHaveBeenCalled();
});
it('refuses the delete when removing the schedule rows fails', async () => {
const deps = createDeps({
deleteSchedulesByUser: jest.fn().mockRejectedValue(new Error('mongo down')),
});
const handlers = createAdminUsersHandlers(deps);
const { req, res, status } = createReqRes({ params: { id: validUserId } });
await handlers.deleteUser(req, res);
// Deleting the user doc first would make this endpoint 404 on retry, so the
// leftover schedule rows (the deleted user's prompt text, no TTL) would become
// unretryable — permanent retention in the clustered topology with no sweep.
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')),

View file

@ -206,10 +206,24 @@ 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);
});
// user's prompt text would persist indefinitely. REFUSE on failure, before the
// user document goes: once it is deleted this endpoint 404s on retry, making the
// leftover rows unretryable — the exact retention this hard delete exists to
// prevent. The barrier is up and quiesce confirmed, so refusing here is safe and
// the admin simply retries. Mirrors the self-service controller's ordering.
const schedulesDeleted = await deleteSchedulesByUser(id).then(
() => true,
(error) => {
logger.error('[adminUsers] Failed to delete schedules for the removed user', error);
return false;
},
);
if (!schedulesDeleted) {
res.set('Retry-After', '30');
return res.status(503).json({
error: 'Could not remove scheduled chats for this user. Please retry shortly.',
});
}
const result = await deleteUserById(id);

View file

@ -287,6 +287,11 @@ describe('fireSchedule', () => {
expect(calls.recordOutcome).toHaveLength(0);
// The occurrence is done, so the schedule still advances past it.
expect(calls.advance).toBe(1);
// The lease is handed back explicitly: an owner EDIT rotates the claim token (so the
// token-fenced advance no-ops) but never touches the lease fields — without this
// release, Run Now and the next claim of the recomputed occurrence stay blocked for
// the full 5-minute lease TTL.
expect(methods.releaseLeaseByHolder).toHaveBeenCalledWith('sched-1', 'inst-1');
});
it('treats a message-limiter 429 as a skip, not a schedule failure', async () => {
@ -484,6 +489,35 @@ describe('fireSchedule', () => {
expect(methods.releaseLeaseByHolder).toHaveBeenCalledWith('sched-1', 'inst-1');
});
/**
* A failed revalidation must NEVER advance. In every true supersession (takeover,
* owner edit) the claim token rotated, so an advance would no-op anyway the only
* case where it can land is a PURE lease expiry with no takeover, where the token
* never rotated. advanceSchedule checks no lease, so that advance moved nextRunAt
* past an occurrence nothing had fired: a slow preflight silently DROPPED it
* instead of leaving it due for the next claim to retry.
*/
it('leaves the occurrence due when only the lease expired (no takeover)', async () => {
const { methods } = makeMethods();
// Same claim token, merely past leaseUntil: revalidateClaim fails on the lease
// predicate alone while the token-fenced advance filter would still MATCH.
(methods.revalidateClaim as jest.Mock).mockResolvedValue(false);
mockFetch(async () => okResponse());
const result = await fireSchedule(makeDeps(methods), makeSchedule(), LIMITS, dueAt());
expect(result.skipped).toBe('superseded');
expect(methods.advanceSchedule).not.toHaveBeenCalled();
expect(global.fetch).not.toHaveBeenCalled();
});
it('never advances from any superseded branch, including post-reserve', async () => {
const { methods } = makeMethods();
(methods.revalidateClaim as jest.Mock).mockResolvedValueOnce(true).mockResolvedValue(false);
mockFetch(async () => okResponse());
const result = await fireSchedule(makeDeps(methods), makeSchedule(), LIMITS, dueAt());
expect(result.skipped).toBe('superseded');
expect(methods.advanceSchedule).not.toHaveBeenCalled();
});
/**
* The lease can expire between the pre-reserve revalidation and the reservation
* itself: the capacity allocator reads occupancy in between, and the preflight

View file

@ -268,6 +268,25 @@ export async function fireSchedule(
}
};
/**
* Steps aside after a failed revalidation WITHOUT advancing. `advance()` here is a
* designed no-op in every true supersession (takeover, owner edit both rotate the
* claim token, so its token-fenced filter misses), which means the only case where it
* actually LANDS is the one it must not: a PURE lease expiry with no takeover, where
* the token never rotated. advanceSchedule checks no lease, so the advance moved
* nextRunAt past an occurrence nothing had fired a preflight outlasting the lease
* silently dropped it instead of leaving it due for the next claim to retry.
* Manual run-now still releases its serialization lease (release-only, no advance)
* so a repeat click isn't met with a stale "already in progress".
*/
const stepAsideSuperseded = async () => {
await releaseSupersededLease();
if (options?.manual) {
await methods.releaseLease(schedule.id, claimToken);
}
return { fired: false, skipped: 'superseded' as const };
};
if (nextRunAt == null) {
await methods.disableSchedule(schedule.id, 'invalid_schedule', claimToken);
await advance();
@ -354,9 +373,7 @@ export async function fireSchedule(
claimToken != null &&
!(await methods.revalidateClaim(schedule.id, claimToken, !options?.manual))
) {
await releaseSupersededLease();
await advance();
return { fired: false, skipped: 'superseded' as const };
return stepAsideSuperseded();
}
// Skip rows carry no `bookkept:false` marker, so the reconciler has no path to
// repair a half-applied skip. If the schedule-side bookkeeping throws, do NOT
@ -411,9 +428,7 @@ export async function fireSchedule(
claimToken != null &&
!(await methods.revalidateClaim(schedule.id, claimToken, !options?.manual))
) {
await releaseSupersededLease();
await advance();
return { fired: false, skipped: 'superseded' as const };
return stepAsideSuperseded();
}
// Pre-generate the conversation id and reserve the run row up front. The
@ -532,9 +547,7 @@ export async function fireSchedule(
!(await methods.revalidateClaim(schedule.id, claimToken, !options?.manual))
) {
await rollbackReservation(conversationId);
await releaseSupersededLease();
await advance();
return { fired: false, skipped: 'superseded' as const };
return stepAsideSuperseded();
}
try {
@ -565,10 +578,14 @@ export async function fireSchedule(
}
if (error instanceof ScheduleFireError && error.preStartAbort) {
// The controller refused this fire at its own liveness/revision fence and has
// already recorded the occurrence's outcome. Advancing is all that's left:
// recording `error` here would count a delete/edit as a schedule FAULT and walk
// it toward auto-disable.
// already recorded the occurrence's outcome. Recording `error` here would count
// a delete/edit as a schedule FAULT and walk it toward auto-disable.
// Release OUR lease explicitly: the fence tripping on an owner EDIT rotated the
// claim token, so the token-fenced advance() below no-ops — but the edit never
// touched the lease fields, and leaving them held blocks Run Now and the next
// claim of the recomputed occurrence for the full 5-minute TTL.
logger.info(`[schedules] fire aborted pre-start for ${schedule.id} (superseded)`);
await releaseSupersededLease();
await advance();
return { fired: false, skipped: 'superseded' as const };
}

View file

@ -431,6 +431,26 @@ describe('global kill switch', () => {
const service = makeService(noRuns(), getAppConfig);
expect(await service.engineDeps.isGloballyDisabled()).toBe(false);
});
it('trips on the object form `{ use: false }` exactly like the boolean stop', async () => {
// Both stop shapes must FREEZE occurrences (engine stops claiming, nothing
// advances). A shape-blind gate left the engine claiming while getLimits refused
// fires, so the disabled path ADVANCED each occurrence — a short maintenance stop
// silently dropped everything it covered instead of leaving it due.
const getAppConfig = jest.fn(async () => ({
interfaceConfig: { schedules: { use: false, maxPerUser: 5 } },
})) as unknown as SchedulesServiceDeps['getAppConfig'];
const service = makeService(noRuns(), getAppConfig);
expect(await service.engineDeps.isGloballyDisabled()).toBe(true);
});
it('does not trip on the object form while `use` stays enabled', async () => {
const getAppConfig = jest.fn(async () => ({
interfaceConfig: { schedules: { use: true, maxPerUser: 5 } },
})) as unknown as SchedulesServiceDeps['getAppConfig'];
const service = makeService(noRuns(), getAppConfig);
expect(await service.engineDeps.isGloballyDisabled()).toBe(false);
});
});
describe('deployment-wide limits', () => {

View file

@ -1,4 +1,4 @@
import { logger, runAsSystem, tenantStorage } from '@librechat/data-schemas';
import { logger, runAsSystem, tenantStorage, isRuntimeDisabled } from '@librechat/data-schemas';
import { getRefillEligibilityDate, Permissions, PermissionTypes } from 'librechat-data-provider';
import type { ScheduleMethods, AppConfig, IBalance } from '@librechat/data-schemas';
import type { TCheckpointerConfig } from 'librechat-data-provider';
@ -463,8 +463,14 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer
// BASE config only: DB principal overrides can narrow availability but must never
// widen past an operator's global stop, so `schedules: false` in librechat.yaml is
// genuinely non-overridable rather than emergent from the override filters.
// isRuntimeDisabled reads BOTH stop shapes (`false` and `{ use: false }`) — the
// same predicate the override merge preserves base stops with. A shape-blind
// check here made the object form disable getLimits but not this gate, so the
// engine kept claiming and fireSchedule ADVANCED each occurrence: a short
// maintenance stop silently dropped every occurrence it covered instead of
// leaving them due.
const base = await deps.getAppConfig({ baseOnly: true });
return base?.interfaceConfig?.schedules === false;
return isRuntimeDisabled(base?.interfaceConfig?.schedules);
},
// Occupancy is read in SYSTEM scope so the cap is global across tenants (the
// owner's tenant context would only see its own runs); the claim itself stays in

View file

@ -280,8 +280,10 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]):
return preserveRuntimeStops(baseConfig, merged);
}
/** Whether a runtime-config interface field reads as OFF in either of its two shapes. */
function isRuntimeDisabled(value: unknown): boolean {
/** Whether a runtime-config interface field reads as OFF in either of its two shapes:
* boolean `false`, or the object form with `use: false`. Exported so runtime gates
* (e.g. the schedule engine's global stop) apply the same semantics as the merge. */
export function isRuntimeDisabled(value: unknown): boolean {
if (value === false) {
return true;
}