mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: fence loopback admission on the claimed config revision
B6. The loopback POST carried only scheduleId/scheduledFor/clientRequestId/ newConversationId, and the admission boundary validated EXISTENCE only (isScheduleLive). So an owner edit landing between the claim and persistence was not fenced where it matters: the fire had already been dispatched with the OLD prompt/agent, and the run would then persist into the edited schedule's history as though it had run under the new config. Nothing downstream caught it, because by then the schedule legitimately exists. The fire now carries `scheduleConfigRevision` (the generation it was CLAIMED under) and isScheduleLive revalidates it BEFORE any message is written, refusing the fire when the owner has since edited. This composes with the existing claimToken/lease fencing rather than duplicating it: claimToken rotates on every lease acquisition and so cannot distinguish an owner edit from a normal re-claim, whereas configRevision moves only on owner config mutation. Absent on either side disables the fence, so pre-existing schedules and in-flight fires from before this change keep working with no migration. Tests: the revision is present on the POST body; admission admits a matching revision, REFUSES one an owner edit has moved on, refuses a deleted schedule regardless, and stays permissive when either side has no revision.
This commit is contained in:
parent
94d400e3ae
commit
17534d13af
5 changed files with 87 additions and 4 deletions
|
|
@ -231,6 +231,7 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit
|
|||
responseMessageId: editedResponseMessageId = null,
|
||||
scheduleId: bodyScheduleId = null,
|
||||
scheduledFor: bodyScheduledFor = null,
|
||||
scheduleConfigRevision: bodyScheduleConfigRevision = null,
|
||||
} = req.body;
|
||||
|
||||
// Only honor schedule bookkeeping fields on a server-minted scheduled fire
|
||||
|
|
@ -247,6 +248,9 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit
|
|||
req._isScheduledFire = isScheduledFire;
|
||||
const scheduleId = isScheduledFire ? bodyScheduleId : null;
|
||||
const scheduledFor = isScheduledFire ? bodyScheduledFor : null;
|
||||
const scheduleConfigRevision = isScheduledFire
|
||||
? (bodyScheduleConfigRevision ?? undefined)
|
||||
: undefined;
|
||||
|
||||
const userId = req.user.id;
|
||||
|
||||
|
|
@ -404,7 +408,7 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit
|
|||
// affects it.
|
||||
if (scheduleId) {
|
||||
await GenerationJobManager.updateMetadata(streamId, { scheduleId, scheduledFor });
|
||||
if (!(await isScheduleLive(scheduleId))) {
|
||||
if (!(await isScheduleLive(scheduleId, scheduleConfigRevision))) {
|
||||
logger.info(
|
||||
`[AgentController] Scheduled fire aborted before start; schedule ${scheduleId} no longer active`,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -222,6 +222,22 @@ describe('fireSchedule', () => {
|
|||
expect([...runs.values()][0].status).toBe('started');
|
||||
});
|
||||
|
||||
it('carries the claimed config revision on the loopback POST', async () => {
|
||||
const { methods } = makeMethods();
|
||||
mockFetch(async () => okResponse());
|
||||
await fireSchedule(
|
||||
makeDeps(methods),
|
||||
makeSchedule({ configRevision: 7 } as never),
|
||||
LIMITS,
|
||||
dueAt(),
|
||||
);
|
||||
const body = JSON.parse((global.fetch as unknown as jest.Mock).mock.calls[0][1].body as string);
|
||||
// The admission boundary revalidates this before persisting anything, so an owner
|
||||
// edit landing in the claim -> persistence window is refused rather than written
|
||||
// into the edited schedule's history.
|
||||
expect(body.scheduleConfigRevision).toBe(7);
|
||||
});
|
||||
|
||||
it('records a definite HTTP rejection as error', async () => {
|
||||
const { methods, calls } = makeMethods();
|
||||
mockFetch(async () => ({ ok: false, status: 500, text: async () => 'boom' }) as Response);
|
||||
|
|
|
|||
|
|
@ -130,6 +130,13 @@ async function postChatMessageInner(
|
|||
// conversation's job by this id instead of mislabeling the run an orphan.
|
||||
newConversationId: conversationId,
|
||||
clientRequestId: buildFireClientRequestId(schedule.id, scheduledFor),
|
||||
// The owner-config generation this fire was CLAIMED under. The admission
|
||||
// boundary revalidates it before persisting anything, so an owner edit landing
|
||||
// in the claim -> persistence window cannot have its old prompt/agent written
|
||||
// into the edited schedule's history.
|
||||
...(typeof schedule.configRevision === 'number'
|
||||
? { scheduleConfigRevision: schedule.configRevision }
|
||||
: {}),
|
||||
...(files.length > 0 ? { files } : {}),
|
||||
}),
|
||||
});
|
||||
|
|
|
|||
|
|
@ -148,3 +148,39 @@ describe('global kill switch', () => {
|
|||
expect(await service.engineDeps.isGloballyDisabled()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('admission revision fence', () => {
|
||||
const noRuns = () => jest.fn<Promise<ActiveRun[]>, [string]>().mockResolvedValue([]);
|
||||
|
||||
function serviceWithSchedule(schedule: { configRevision?: number } | null) {
|
||||
const service = makeService(noRuns());
|
||||
(
|
||||
service.engineDeps.methods as unknown as {
|
||||
getScheduleById: jest.Mock;
|
||||
}
|
||||
).getScheduleById = jest.fn(async () => schedule);
|
||||
return service;
|
||||
}
|
||||
|
||||
it('admits when the claimed revision still matches', async () => {
|
||||
const service = serviceWithSchedule({ configRevision: 3 });
|
||||
expect(await service.isScheduleLive('sched-1', 3)).toBe(true);
|
||||
});
|
||||
|
||||
it('REFUSES when an owner edit moved the revision on after the claim', async () => {
|
||||
// The fire was claimed under revision 3; the owner edited since (now 4). Persisting
|
||||
// would write the OLD prompt/agent into the edited schedule's history.
|
||||
const service = serviceWithSchedule({ configRevision: 4 });
|
||||
expect(await service.isScheduleLive('sched-1', 3)).toBe(false);
|
||||
});
|
||||
|
||||
it('refuses a schedule that is gone regardless of revision', async () => {
|
||||
const service = serviceWithSchedule(null);
|
||||
expect(await service.isScheduleLive('sched-1', 3)).toBe(false);
|
||||
});
|
||||
|
||||
it('stays permissive when either side has no revision (pre-existing rows)', async () => {
|
||||
expect(await serviceWithSchedule({}).isScheduleLive('sched-1', 3)).toBe(true);
|
||||
expect(await serviceWithSchedule({ configRevision: 4 }).isScheduleLive('sched-1')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -121,7 +121,7 @@ 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) => Promise<boolean>;
|
||||
isScheduleLive: (scheduleId: string, expectedConfigRevision?: number) => Promise<boolean>;
|
||||
/**
|
||||
* Acquires the durable RESUMING lease for a HITL resume, BEFORE the approval claim
|
||||
* (so a deferral leaves the approval claimable). Per-schedule overlap is enforced by
|
||||
|
|
@ -532,11 +532,31 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer
|
|||
return false;
|
||||
}
|
||||
|
||||
async function isScheduleLive(scheduleId: string): Promise<boolean> {
|
||||
async function isScheduleLive(
|
||||
scheduleId: string,
|
||||
expectedConfigRevision?: number,
|
||||
): Promise<boolean> {
|
||||
if (!scheduleId) {
|
||||
return false;
|
||||
}
|
||||
return (await methods.getScheduleById(scheduleId)) != null;
|
||||
const schedule = await methods.getScheduleById(scheduleId);
|
||||
if (schedule == null) {
|
||||
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
|
||||
// catch it because the run persists under the NEW schedule. Refuse before any
|
||||
// message is written. Absent on either side disables the fence, so pre-existing
|
||||
// schedules and older fires keep working.
|
||||
if (
|
||||
expectedConfigRevision != null &&
|
||||
typeof schedule.configRevision === 'number' &&
|
||||
schedule.configRevision !== expectedConfigRevision
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue