diff --git a/api/server/controllers/agents/request.js b/api/server/controllers/agents/request.js index 5c3249ef70..9ecef679ba 100644 --- a/api/server/controllers/agents/request.js +++ b/api/server/controllers/agents/request.js @@ -25,7 +25,11 @@ const { } = require('~/server/services/MCPRequestContext'); const { handleAbortError } = require('~/server/middleware'); const { logViolation } = require('~/cache'); -const { recordScheduleOutcome, isScheduleLive } = require('~/server/services/Schedules'); +const { + recordScheduleOutcome, + isScheduleLive, + clearScheduledJob, +} = require('~/server/services/Schedules'); const { saveMessage, getMessages, getConvo } = require('~/models'); function createCloseHandler(abortController) { @@ -427,7 +431,11 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit // terminalize the run and tear the job down BEFORE any messages are persisted. // Scoped to scheduled fires, so interactive chat never pays for it. if (scheduleId) { - if (!(await isScheduleLive(scheduleId, scheduleConfigRevision))) { + if ( + !(await isScheduleLive(scheduleId, scheduleConfigRevision, { + automatic: req._isManualScheduledFire !== true, + })) + ) { logger.info( `[AgentController] Scheduled fire aborted before start; schedule ${scheduleId} no longer active`, ); @@ -1099,6 +1107,16 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit // as reconcile evidence, so request cleanup or process exit must not outrun it. if (!scheduleOutcomeRecorded) { await completion; + } else if (scheduleId) { + // completeJob returns early for a job that is no longer `running` — which is + // exactly the state a schedule delete leaves behind (aborted, retained + // without completedAt). With the run now terminal, reconcile never scans it + // again, so nothing else would ever reap that job. Identity- and + // generation-fenced, and a no-op when completeJob already deleted it. + await completion; + await clearScheduledJob(streamId, { scheduleId, scheduledFor }).catch((err) => + logger.warn('[ResumableAgentController] Failed to clear reconciled job', err), + ); } await finishResumableRequest(req, userId); } else { diff --git a/packages/api/src/schedules/service.spec.ts b/packages/api/src/schedules/service.spec.ts index b257610ada..7348daaae4 100644 --- a/packages/api/src/schedules/service.spec.ts +++ b/packages/api/src/schedules/service.spec.ts @@ -391,7 +391,7 @@ describe('loopback self URL', () => { describe('admission revision fence', () => { const noRuns = () => jest.fn, [string]>().mockResolvedValue([]); - function serviceWithSchedule(schedule: { configRevision?: number } | null) { + function serviceWithSchedule(schedule: { configRevision?: number; enabled?: boolean } | null) { const service = makeService(noRuns()); ( service.engineDeps.methods as unknown as { @@ -413,6 +413,28 @@ describe('admission revision fence', () => { expect(await service.isScheduleLive('sched-1', 3)).toBe(false); }); + /** + * A policy auto-disable flips `enabled` WITHOUT touching configRevision (an older + * paused occurrence can resume, fail and cross the threshold while a newer occurrence + * is already in the claim-to-controller window), so the revision fence cannot see it. + */ + it('refuses an AUTOMATIC fire once the schedule was disabled', async () => { + const service = serviceWithSchedule({ configRevision: 3, enabled: false }); + expect(await service.isScheduleLive('sched-1', 3, { automatic: true })).toBe(false); + }); + + it('still admits Run Now on a disabled schedule', async () => { + // An explicit user action, matching fireScheduleNow's own relaxation. + const service = serviceWithSchedule({ configRevision: 3, enabled: false }); + expect(await service.isScheduleLive('sched-1', 3, { automatic: false })).toBe(true); + expect(await service.isScheduleLive('sched-1', 3)).toBe(true); + }); + + it('admits an automatic fire while the schedule is still enabled', async () => { + const service = serviceWithSchedule({ configRevision: 3, enabled: true }); + expect(await service.isScheduleLive('sched-1', 3, { automatic: true })).toBe(true); + }); + it('refuses a schedule that is gone regardless of revision', async () => { const service = serviceWithSchedule(null); expect(await service.isScheduleLive('sched-1', 3)).toBe(false); diff --git a/packages/api/src/schedules/service.ts b/packages/api/src/schedules/service.ts index eaef781fd2..8c88bcb41d 100644 --- a/packages/api/src/schedules/service.ts +++ b/packages/api/src/schedules/service.ts @@ -136,7 +136,11 @@ export interface SchedulesService { * the reservation row exists but the job did not yet, so the deletion's abort * missed it) — aborting before any messages are persisted. */ - isScheduleLive: (scheduleId: string, expectedConfigRevision?: number) => Promise; + isScheduleLive: ( + scheduleId: string, + expectedConfigRevision?: number, + options?: { automatic?: boolean }, + ) => Promise; /** Soft-deletes an owner's schedule: stop claims, abort active runs, drain, erase. */ deleteScheduleForOwner: (scheduleId: string, userId: string) => Promise; /** @@ -578,6 +582,7 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer async function isScheduleLive( scheduleId: string, expectedConfigRevision?: number, + options?: { automatic?: boolean }, ): Promise { if (!scheduleId) { return false; @@ -586,6 +591,15 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer if (schedule == null) { return false; } + // An AUTOMATIC fire must still be wanted. A policy auto-disable (too many failures, + // insufficient balance) flips `enabled` WITHOUT touching configRevision, so the + // revision fence below cannot see it — an occurrence already in the claim-to- + // controller window would otherwise start a billed generation for a schedule that + // has just been switched off. Run Now is an explicit user action and stays allowed + // on a disabled schedule, matching fireScheduleNow. + if (options?.automatic === true && schedule.enabled === false) { + return false; + } // REVISION FENCE at the admission boundary. Existence alone is not enough: an owner // edit landing between the claim and this point means the dispatched prompt/agent // came from a config the owner has since replaced, and nothing downstream would diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 72bafe708e..015a6e0426 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -65,6 +65,28 @@ describe('mergeConfigOverrides', () => { expect(dIface.schedules).toEqual({ use: false, maxPerUser: 50, minIntervalMinutes: 5 }); }); + it('does not let a boolean override re-enable a globally-disabled feature', () => { + // Both values are booleans, so neither fold above applies and the plain fallback + // used to replace the base `false`. The service reads the BASE value and keeps + // refusing every write, so the client would render a panel whose create, edit and + // run actions all fail. A global stop may be narrowed, never widened. + const base = { interfaceConfig: { schedules: false } } as unknown as AppConfig; + const merged = mergeConfigOverrides(base, [ + fakeConfig({ interface: { schedules: true } }, 10), + ]) as unknown as Record>; + expect(merged.interfaceConfig.schedules).toBe(false); + + // An enabled base still honours a boolean override in both directions. + const enabledBase = { interfaceConfig: { schedules: true } } as unknown as AppConfig; + expect( + ( + mergeConfigOverrides(enabledBase, [ + fakeConfig({ interface: { schedules: false } }, 10), + ]) as unknown as Record> + ).interfaceConfig.schedules, + ).toBe(false); + }); + it('folds an object schedules override onto a boolean base, inheriting the enable state', () => { // Enabled base, object override that only TUNES a limit (no `use`): the enable // state must be inherited from the boolean base, not silently dropped. diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 853b61171d..002eb3adbb 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -196,6 +196,15 @@ function deepMerge(target: T, source: AnyObject, depth = 0, // explicitly — otherwise setting a limit on a globally-disabled feature // (`schedules: false`) would silently re-enable it. result[key] = { use: targetVal, ...(sourceVal as AnyObject) }; + } else if ( + typeof sourceVal === 'boolean' && + typeof targetVal === 'boolean' && + RUNTIME_CONFIG_INTERFACE_FIELDS.has(key) + ) { + // Both booleans: a base `false` is a GLOBAL stop that an override may narrow but + // never widen. The service reads the base value and keeps refusing writes, so + // letting the override win here only produces a panel whose actions all fail. + result[key] = targetVal === false ? false : sourceVal; } else { result[key] = sourceVal; } diff --git a/packages/data-schemas/src/methods/schedule.methods.spec.ts b/packages/data-schemas/src/methods/schedule.methods.spec.ts index 1f6c62ce7b..502ea8865a 100644 --- a/packages/data-schemas/src/methods/schedule.methods.spec.ts +++ b/packages/data-schemas/src/methods/schedule.methods.spec.ts @@ -937,6 +937,80 @@ describe('claim-token fencing (stale worker cannot mutate an edited/deleted sche }); }); +describe('card projection fences (regressions in the ordered projection)', () => { + /** + * The terminal path passes the row's revision to projectLastRun; the pause path did + * not. An owner edit landing between fire and approval therefore got the OLD config's + * pause stamped on it, and because the later terminal write IS revision-fenced it + * could never replace that status — the card stuck on "Needs approval". + */ + it('does not stamp a pause onto a schedule edited after the run started', async () => { + const schedule = await methods.createSchedule(scheduleData()); + const scheduledFor = new Date('2026-07-26T10:00:00.000Z'); + const runRevision = (await getSchedule(schedule.id)).configRevision; + await methods.insertScheduleRun( + runData(schedule, { scheduledFor, configRevision: runRevision }), + ); + await methods.updateScheduleById(schedule.id, schedule.user, { name: 'renamed' }); + + await methods.recordRunOutcome({ + scheduleId: schedule.id, + scheduledFor, + status: 'requires_action', + conversationId: 'convo-1', + autoDisableAfterFailures: 3, + }); + + // The row still pauses (the generation really is waiting) but the edited + // schedule's card is not stamped by the superseded config. + expect(await getRun(schedule.id, scheduledFor)).toMatchObject({ status: 'requires_action' }); + expect((await getSchedule(schedule.id)).lastRun).toBeUndefined(); + }); + + /** + * A skip wrote `lastRun` directly, dropping the `scheduledFor` ordering marker that + * projectLastRun keys off — so the NEXT projection saw an absent marker and let an + * older occurrence's result overwrite the newer skip. + */ + it('keeps the occurrence marker when recording a skip', async () => { + const schedule = await methods.createSchedule(scheduleData()); + const older = new Date('2026-07-26T10:00:00.000Z'); + const newer = new Date('2026-07-26T11:00:00.000Z'); + await methods.insertScheduleRun(runData(schedule, { scheduledFor: older })); + await methods.recordRunOutcome({ + scheduleId: schedule.id, + scheduledFor: older, + status: 'requires_action', + conversationId: 'convo-older', + autoDisableAfterFailures: 3, + }); + + // The newer occurrence is skipped for balance while the older one sits paused. + await methods.recordSkippedRun( + { + scheduleId: schedule.id, + scheduledFor: newer, + user: schedule.user, + status: 'skipped_balance', + firedAt: new Date(), + }, + 5, + ); + expect((await getSchedule(schedule.id)).lastRun?.status).toBe('skipped_balance'); + + // The older occurrence then resumes and finishes. It must NOT roll the card back. + await methods.recordRunOutcome({ + scheduleId: schedule.id, + scheduledFor: older, + status: 'success', + conversationId: 'convo-older', + autoDisableAfterFailures: 3, + }); + + expect((await getSchedule(schedule.id)).lastRun?.status).toBe('skipped_balance'); + }); +}); + describe('auto-disable carries the run config generation', () => { /** * The auto-disable policy re-reads the schedule and then writes, so the decision and diff --git a/packages/data-schemas/src/methods/schedule.ts b/packages/data-schemas/src/methods/schedule.ts index 141ac7007c..b0a08d3166 100644 --- a/packages/data-schemas/src/methods/schedule.ts +++ b/packages/data-schemas/src/methods/schedule.ts @@ -691,10 +691,15 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche if (paused == null) { return; } + // Revision-fenced like the terminal path: an owner edit landing between the fire + // and the approval must not have the OLD config's pause stamped on it. Without + // this the later terminal write (which IS fenced) could never replace the stale + // status, so the card stuck on "Needs approval". await projectLastRun( params.scheduleId, { conversationId: params.conversationId, status: params.status, firedAt }, params.scheduledFor, + paused.configRevision != null ? { configRevision: paused.configRevision } : {}, ); return; } @@ -780,15 +785,21 @@ export function createScheduleMethods(mongoose: typeof import('mongoose')): Sche // Surface the skip on the card (its chip reads schedule.lastRun). An overlap // skip is an intervening non-balance outcome, so it BREAKS the balance-skip // streak (the counter is for CONSECUTIVE balance skips). - await Schedule().updateOne( - { id: data.scheduleId, ...skipRevisionFilter }, - { - $set: { - lastRun: { status: data.status, firedAt }, - ...(data.status !== 'skipped_balance' ? { balanceSkipCount: 0 } : {}), - }, - }, + // Through the ORDERED projection: writing `lastRun` directly dropped the + // `scheduledFor` marker, after which the next projection read the marker as absent + // and let an older occurrence's outcome overwrite this newer skip. + await projectLastRun( + data.scheduleId, + { status: data.status, firedAt }, + data.scheduledFor, + skipRevisionFilter, ); + if (data.status !== 'skipped_balance') { + await Schedule().updateOne( + { id: data.scheduleId, ...skipRevisionFilter }, + { $set: { balanceSkipCount: 0 } }, + ); + } if (data.status !== 'skipped_balance' || balanceSkipDisableThreshold == null) { return; }