mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: a message-limiter refusal is a skip, not a schedule failure
Codex review 4778591637. A consequence of 7b9aab364, which deliberately stopped Run Now
inheriting the automatic fire's limiter exemption: the loopback can now come back 429,
and every non-2xx was classified as a definite fire rejection and recorded as
`status: 'error'`. So an owner who is merely over their own message quota could click
Run Now five times and auto-disable a perfectly healthy schedule as `too_many_failures`,
with no generation ever started.
A 429 says something about the CALLER's quota, not the schedule's health, so it is now
its own classification alongside the controller's pre-start fence. Only a manual run can
reach it, since automatic occurrences remain exempt.
The two pre-start refusals differ in one way that matters: the controller fence already
recorded an outcome for the occurrence, but a limiter denial never reaches the
controller, so this path must roll the reservation back — otherwise the run row keeps
its global capacity slot and blocks the schedule's overlap guard until the orphan sweep.
Run Now also answers 429 rather than burying it in the generic 409, since it is the
caller's quota rather than a conflicting schedule state.
The new case fails against the pre-fix fire path; all 22 existing fire cases pass
unchanged on both sides.
This commit is contained in:
parent
6164b05e10
commit
166ff08bb6
4 changed files with 54 additions and 3 deletions
|
|
@ -281,6 +281,28 @@ describe('fireSchedule', () => {
|
|||
expect(calls.advance).toBe(1);
|
||||
});
|
||||
|
||||
it('treats a message-limiter 429 as a skip, not a schedule failure', async () => {
|
||||
const { methods, runs } = makeMethods();
|
||||
mockFetch(
|
||||
async () =>
|
||||
({
|
||||
ok: false,
|
||||
status: 429,
|
||||
text: async () => '{"message":"Too many requests"}',
|
||||
}) as Response,
|
||||
);
|
||||
const result = await fireSchedule(makeDeps(methods), makeSchedule(), LIMITS, dueAt(), {
|
||||
manual: true,
|
||||
});
|
||||
expect(result.skipped).toBe('rate_limited');
|
||||
// Counting this as a failure would let an owner merely over their message quota
|
||||
// auto-disable a healthy schedule by clicking Run Now enough times.
|
||||
expect(methods.recordRunOutcome).not.toHaveBeenCalled();
|
||||
// Nothing reached the controller, so no outcome was recorded for the occurrence and
|
||||
// the reservation must not be left holding its capacity slot.
|
||||
expect([...runs.values()].filter((r) => r.status === 'started')).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('records a definite HTTP rejection as error', async () => {
|
||||
const { methods, calls } = makeMethods();
|
||||
mockFetch(async () => ({ ok: false, status: 500, text: async () => 'boom' }) as Response);
|
||||
|
|
|
|||
|
|
@ -27,6 +27,11 @@ class ScheduleFireError extends Error {
|
|||
* controller already recorded the occurrence's outcome — so this is a SKIP, not a
|
||||
* fault, and must not count toward auto-disable. */
|
||||
readonly preStartAbort = false,
|
||||
/** The server's own message limiter refused the fire before it reached the
|
||||
* controller. Reachable for a manual Run Now, which is deliberately NOT exempt from
|
||||
* the interactive limiters. Nothing started and the schedule is not at fault, so
|
||||
* this must not count toward auto-disable either. */
|
||||
readonly throttled = false,
|
||||
) {
|
||||
super(message);
|
||||
}
|
||||
|
|
@ -162,10 +167,15 @@ async function postChatMessageInner(
|
|||
}
|
||||
if (!response.ok) {
|
||||
const body = await response.text().catch(() => '');
|
||||
// A received error response is a definite rejection (nothing started).
|
||||
// A received error response is a definite rejection (nothing started). 429 is the
|
||||
// exception: the server's OWN message limiter refused the request, which says
|
||||
// nothing about the schedule's health. Only a manual Run Now can reach it, since
|
||||
// automatic occurrences are exempt.
|
||||
throw new ScheduleFireError(
|
||||
`Fire POST failed (${response.status}): ${body.slice(0, 300)}`,
|
||||
false,
|
||||
false,
|
||||
response.status === 429,
|
||||
);
|
||||
}
|
||||
// The accept path always answers with JSON ({ streamId, conversationId, status }).
|
||||
|
|
@ -530,6 +540,19 @@ export async function fireSchedule(
|
|||
await advance();
|
||||
return { fired: false, skipped: 'superseded' as const };
|
||||
}
|
||||
if (error instanceof ScheduleFireError && error.throttled) {
|
||||
// The server's own message limiter refused this before it reached the
|
||||
// controller, so nothing was billed and the SCHEDULE is not at fault. Counting
|
||||
// it as a failure would let an owner who is merely over their message quota
|
||||
// auto-disable a perfectly healthy schedule by clicking Run Now enough times.
|
||||
// Unlike the controller fence above, nothing recorded an outcome for this
|
||||
// occurrence, so the reservation has to be rolled back or it holds its capacity
|
||||
// slot and blocks overlap until the orphan sweep.
|
||||
logger.info(`[schedules] fire refused by the message limiter for ${schedule.id}`);
|
||||
await rollbackReservation();
|
||||
await advance();
|
||||
return { fired: false, skipped: 'rate_limited' as const };
|
||||
}
|
||||
// Definite rejection (an error response was received): nothing started.
|
||||
logger.error(`[schedules] fire rejected for ${schedule.id}:`, error);
|
||||
await methods.recordRunOutcome({
|
||||
|
|
|
|||
|
|
@ -412,8 +412,13 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH
|
|||
return;
|
||||
}
|
||||
if (!result.fired) {
|
||||
res.status(409).json({
|
||||
error: result.error ?? `Run skipped (${result.skipped ?? 'unknown'})`,
|
||||
// A limiter refusal is the caller's own quota, not a conflicting schedule state,
|
||||
// so answer 429 rather than burying it in the generic 409.
|
||||
res.status(result.skipped === 'rate_limited' ? 429 : 409).json({
|
||||
error:
|
||||
result.skipped === 'rate_limited'
|
||||
? 'Too many messages. Try running this schedule again shortly.'
|
||||
: (result.error ?? `Run skipped (${result.skipped ?? 'unknown'})`),
|
||||
skipped: result.skipped,
|
||||
});
|
||||
return;
|
||||
|
|
|
|||
|
|
@ -138,6 +138,7 @@ export interface FireResult {
|
|||
| 'user_missing'
|
||||
| 'user_deleting'
|
||||
| 'permission_revoked'
|
||||
| 'rate_limited'
|
||||
| 'disabled';
|
||||
error?: string;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue