From 166ff08bb6cfe58dc3ab061561b3242afb4eed15 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 25 Jul 2026 01:54:08 -0400 Subject: [PATCH] fix: a message-limiter refusal is a skip, not a schedule failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/api/src/schedules/fire.spec.ts | 22 ++++++++++++++++++++++ packages/api/src/schedules/fire.ts | 25 ++++++++++++++++++++++++- packages/api/src/schedules/handlers.ts | 9 +++++++-- packages/api/src/schedules/types.ts | 1 + 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/packages/api/src/schedules/fire.spec.ts b/packages/api/src/schedules/fire.spec.ts index 76bf1a5787..db328db297 100644 --- a/packages/api/src/schedules/fire.spec.ts +++ b/packages/api/src/schedules/fire.spec.ts @@ -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); diff --git a/packages/api/src/schedules/fire.ts b/packages/api/src/schedules/fire.ts index 65bc3ec547..bfdfbb5e1f 100644 --- a/packages/api/src/schedules/fire.ts +++ b/packages/api/src/schedules/fire.ts @@ -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({ diff --git a/packages/api/src/schedules/handlers.ts b/packages/api/src/schedules/handlers.ts index cb040635c5..9dd88ebed7 100644 --- a/packages/api/src/schedules/handlers.ts +++ b/packages/api/src/schedules/handlers.ts @@ -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; diff --git a/packages/api/src/schedules/types.ts b/packages/api/src/schedules/types.ts index f3d03c83df..0708b26d60 100644 --- a/packages/api/src/schedules/types.ts +++ b/packages/api/src/schedules/types.ts @@ -138,6 +138,7 @@ export interface FireResult { | 'user_missing' | 'user_deleting' | 'permission_revoked' + | 'rate_limited' | 'disabled'; error?: string; }