diff --git a/api/server/controllers/agents/__tests__/request.resumeMetadata.spec.js b/api/server/controllers/agents/__tests__/request.resumeMetadata.spec.js index d9e96168c8..1eb5d13a58 100644 --- a/api/server/controllers/agents/__tests__/request.resumeMetadata.spec.js +++ b/api/server/controllers/agents/__tests__/request.resumeMetadata.spec.js @@ -107,6 +107,9 @@ jest.mock('@librechat/api', () => ({ buildMessageFiles: jest.fn(() => []), resolveTitleTiming: jest.fn(() => 'immediate'), resolveConversationAnchor: jest.requireActual('@librechat/api').resolveConversationAnchor, + // Real predicate, not a stub: it decides whether this request holds a concurrency + // slot, and a stub here would hide a drift between the acquire and release sites. + exemptFromConcurrencyLimiter: jest.requireActual('@librechat/api').exemptFromConcurrencyLimiter, GenerationJobManager: mockGenerationJobManager, getReferencedQuotes: jest.fn((quotes) => { if (!Array.isArray(quotes)) { diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 71da6ce8e6..bb818adbbd 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -76,6 +76,7 @@ const { resolveYouTubeInjectionConfig, decrementPendingRequest, maybePrewarmCodeSandbox, + exemptFromConcurrencyLimiter, } = require('@librechat/api'); const { Callback, @@ -1525,10 +1526,15 @@ class AgentClient extends BaseClient { // teardown (request.js pause branch / resume.js finally) that would otherwise // release it, and `/resume` 429s under LIMIT_CONCURRENT_MESSAGES. Idempotent via // the flag; if it fails here, the teardown still releases (it checks the flag). - // A scheduled fire never acquired an interactive concurrency slot, so it must - // not release one on pause (that would clear a real user's counter). Mark it + // An AUTOMATIC scheduled fire never acquired an interactive concurrency slot, so it + // must not release one on pause (that would clear a real user's counter). Mark it // released so downstream teardown skips the decrement too. - if (this.options.req?._isScheduledFire) { + // + // Run Now DOES acquire one — it is user-paced and no longer exempt — so it must + // release like any interactive turn. Mirrors `exemptFromConcurrency` at the + // increment site; treating it as exempt here leaked a slot on every paused manual + // fire, and the pause path returns before the teardown that was made manual-aware. + if (exemptFromConcurrencyLimiter(this.options.req)) { this.pendingRequestReleased = true; } else if (!this.pendingRequestReleased) { try { diff --git a/api/server/controllers/agents/request.js b/api/server/controllers/agents/request.js index 2de51bb05b..3f56bc84cb 100644 --- a/api/server/controllers/agents/request.js +++ b/api/server/controllers/agents/request.js @@ -12,6 +12,7 @@ const { decrementPendingRequest, sanitizeMessageForTransmit, checkAndIncrementPendingRequest, + exemptFromConcurrencyLimiter, isUnpersistedPreliminaryParent, isScheduleFireRequest, resolveConversationAnchor, @@ -164,7 +165,7 @@ async function finishResumableRequest(req, userId) { // Mirrors the increment: only an AUTOMATIC fire skipped the counter, so only it // must skip the decrement. Uses the decision captured at request start, not a // re-check of the expiring token. - if (!req._isScheduledFire || req._isManualScheduledFire === true) { + if (!exemptFromConcurrencyLimiter(req)) { await decrementPendingRequest(userId); } } @@ -376,7 +377,9 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit // so exempting it lets one user run unbounded concurrent generations — and unlike an // automatic occurrence, a 429 here is surfaced to the clicking user rather than // booked against the schedule (see the throttled branch in fireSchedule). - const exemptFromConcurrency = isScheduledFire && req._isManualScheduledFire !== true; + // Shared predicate, not a hand-rolled copy: the acquire and both release points + // live in different files, and re-deriving it at each is what let them drift. + const exemptFromConcurrency = exemptFromConcurrencyLimiter(req); if (!exemptFromConcurrency) { const { allowed, pendingRequests, limit } = await checkAndIncrementPendingRequest(userId); if (!allowed) { @@ -920,7 +923,9 @@ const ResumableAgentController = async (req, res, next, initializeClient, addTit // (so a fast /resume isn't 429'd); only release here if that didn't happen. // Always run the MCP request-context cleanup. await cleanupMCPRequestContextForReq(req); - if (!isScheduledFire && !client?.pendingRequestReleased) { + // Mirrors the increment: only an AUTOMATIC fire skipped the counter. A Run Now + // holds a real slot, so it must release here like an interactive turn. + if (!exemptFromConcurrency && !client?.pendingRequestReleased) { await decrementPendingRequest(userId); } if (client) { diff --git a/packages/api/src/crypto/jwt.ts b/packages/api/src/crypto/jwt.ts index 287320f09d..e0afe27198 100644 --- a/packages/api/src/crypto/jwt.ts +++ b/packages/api/src/crypto/jwt.ts @@ -108,3 +108,18 @@ export const exemptFromUserLimiter = (req: ClassifiedFireRequest): boolean => { const claims = classify(req); return claims.scheduled && !claims.manual; }; + +/** + * Whether the interactive CONCURRENT-request limiter should be SKIPPED — and, by the + * same token, whether this request must NOT release a slot on teardown. + * + * Same rule as the user limiter: only AUTOMATIC occurrences are exempt. Run Now is + * user-paced and holds a real slot, so it increments and must decrement. + * + * Exported deliberately rather than re-derived at each site. The acquire and the two + * release points (the HITL pause path and the normal teardown) live in different files, + * and hand-writing the predicate at each let them drift: the acquire became manual-aware + * while a release did not, leaking a slot on every paused Run Now. + */ +export const exemptFromConcurrencyLimiter = (req: ClassifiedFireRequest): boolean => + exemptFromUserLimiter(req); diff --git a/packages/api/src/crypto/limiters.spec.ts b/packages/api/src/crypto/limiters.spec.ts index 49b3317a65..23eedbef69 100644 --- a/packages/api/src/crypto/limiters.spec.ts +++ b/packages/api/src/crypto/limiters.spec.ts @@ -2,6 +2,7 @@ import { exemptFromIpLimiter, SCHEDULE_FIRE_SCOPE, exemptFromUserLimiter, + exemptFromConcurrencyLimiter, SCHEDULE_MANUAL_CLAIM, readScheduleFireClaims, generateShortLivedToken, @@ -72,6 +73,26 @@ describe('message-limiter exemptions for scheduled fires', () => { }); }); + /** + * The ACQUIRE and both RELEASE points live in different files (request.js twice, + * client.js once on the HITL pause path). Re-deriving this rule at each site is what + * let them drift: the acquire became manual-aware while a release did not, so every + * paused Run Now leaked a slot until the counter's idle TTL expired. + */ + describe('concurrency limiter', () => { + it('exempts an automatic occurrence, which never increments', () => { + expect(exemptFromConcurrencyLimiter(request('automatic'))).toBe(true); + }); + + it('does NOT exempt a Run Now, which holds a real slot and must release it', () => { + expect(exemptFromConcurrencyLimiter(request('manual'))).toBe(false); + }); + + it('does not exempt an ordinary interactive turn', () => { + expect(exemptFromConcurrencyLimiter(request('interactive'))).toBe(false); + }); + }); + it('limits rather than exempts a request whose token cannot be verified', () => { const req = { headers: { 'x-lc-scheduled': '1', authorization: 'Bearer not-a-jwt' } }; expect(exemptFromIpLimiter(req)).toBe(false); diff --git a/packages/api/src/schedules/engine.ts b/packages/api/src/schedules/engine.ts index aa8154cac1..2f6398f3ba 100644 --- a/packages/api/src/schedules/engine.ts +++ b/packages/api/src/schedules/engine.ts @@ -411,8 +411,10 @@ export function startScheduleEngine(deps: ScheduleEngineDeps): ScheduleEngine { // listener is closing and the generation manager has already begun refusing new jobs. // A tick in that window claims a due occurrence, fails its loopback POST against a // server that is shutting down, and books the failure against the schedule — walking a - // healthy schedule toward auto-disable for nothing more than a restart. Stopping first - // means the last claim always has a live server to fire at. + // healthy schedule toward auto-disable for nothing more than a restart. This NARROWS + // that window rather than closing it: the listener starts closing before pre-drain + // runs, so a tick already in flight can still lose its POST. Occurrences skipped by + // stopping early are simply still due at restart, within the misfire grace. registerShutdownTask( 'schedule engine', () => { diff --git a/packages/api/src/schedules/handlers.ts b/packages/api/src/schedules/handlers.ts index 27ced87654..72ead7f987 100644 --- a/packages/api/src/schedules/handlers.ts +++ b/packages/api/src/schedules/handlers.ts @@ -262,7 +262,9 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH // Inserted WITHOUT nextRunAt regardless of `enabled`: the engine claims by // nextRunAt, so the row is inert until armed below. That is what makes the // barrier re-check durable — every failure mode leaves a row that cannot fire, - // rather than one that fires for an account already being erased. + // rather than one that fires for an account already being erased. The reconciler's + // unarmed sweep later arms anything left this way, so an inert row is a delay, not + // a permanent state. const created = await deps.methods.createScheduleWithSlot( { ...parsed.data, @@ -285,9 +287,9 @@ export function createSchedulesHandlers(deps: SchedulesHandlersDeps): SchedulesH // Re-checking AFTER the write is what makes the barrier authoritative. if (await deps.isUserDeleting(user.id)) { // Best-effort tidy-up of an unarmed row. Its failure is reported but no longer - // load-bearing: an unarmed schedule is never claimed, so the worst case is an - // inert row the deletion sweep can reap later, not a live one firing billed - // generations for an erased account. + // load-bearing for BILLING: the row is unarmed, and even once the reconciler's + // sweep arms it, the fire path refuses it at the account-deletion barrier + // (isOwnerDeleting). The residual is a retained row, not a billed generation. if (!(await compensateLateCreate(deps, id, user.id))) { res.status(500).json({ error: 'Failed to roll back schedule creation' }); return;