fix: close five card-projection, cleanup and admission gaps

Two are regressions from this session's own ordered-projection work:

- The pause path called projectLastRun WITHOUT the revision filter the terminal
  path passes, so an owner edit landing between fire and approval got the old
  config's pause stamped on it — and since the later terminal write IS fenced, it
  could never replace that status and the card stuck on "Needs approval".
- recordSkippedRun wrote lastRun directly, dropping the `scheduledFor` ordering
  marker projectLastRun keys off. The next projection then read the marker as
  absent and let an older occurrence overwrite the newer skip. Skips now go
  through the same ordered helper.

The rest:

- The success path's completeJob returns early for a job that is no longer
  `running` — exactly what a schedule delete leaves behind (aborted, retained
  without completedAt). With the run terminal, reconcile never scans it again, so
  the job leaked. It is now cleared explicitly, identity- and generation-fenced,
  mirroring the abort route.
- A boolean role/user override could re-enable a base `interface.schedules:
  false`: both values are booleans so neither runtime-field fold applied and the
  plain fallback won. The service reads the base value and keeps refusing writes,
  so this only produced a panel whose actions all fail. A global stop may now be
  narrowed, never widened.
- Admission did not require `enabled`. A policy auto-disable flips it without
  touching configRevision, so the revision fence could not see it and an
  occurrence already in the claim-to-controller window still started a billed
  generation. Automatic fires now require an enabled schedule; Run Now stays
  allowed on a disabled one, matching fireScheduleNow.
This commit is contained in:
Danny Avila 2026-07-27 01:01:16 -04:00
parent d79efa0aba
commit c05a8f59d3
7 changed files with 182 additions and 12 deletions

View file

@ -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 {

View file

@ -391,7 +391,7 @@ describe('loopback self URL', () => {
describe('admission revision fence', () => {
const noRuns = () => jest.fn<Promise<ActiveRun[]>, [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);

View file

@ -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<boolean>;
isScheduleLive: (
scheduleId: string,
expectedConfigRevision?: number,
options?: { automatic?: boolean },
) => Promise<boolean>;
/** Soft-deletes an owner's schedule: stop claims, abort active runs, drain, erase. */
deleteScheduleForOwner: (scheduleId: string, userId: string) => Promise<boolean>;
/**
@ -578,6 +582,7 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer
async function isScheduleLive(
scheduleId: string,
expectedConfigRevision?: number,
options?: { automatic?: boolean },
): Promise<boolean> {
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

View file

@ -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<string, Record<string, unknown>>;
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<string, Record<string, unknown>>
).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.

View file

@ -196,6 +196,15 @@ function deepMerge<T extends AnyObject>(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;
}

View file

@ -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

View file

@ -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;
}