From fb83f43fcaca2847620fa6141ae10fa336c2a750 Mon Sep 17 00:00:00 2001 From: "Bob.Hou" Date: Tue, 11 Aug 2026 09:14:41 -0400 Subject: [PATCH] fix(rate-limit): patch Bottleneck doExpire capacity leak (#9328) --- config/quality/file-size-baseline.json | 2 + open-sse/services/bottleneckPatch.ts | 82 ++++++++++++ open-sse/services/rateLimitManager.ts | 3 + tests/unit/bottleneck-doexpire-patch.test.ts | 126 +++++++++++++++++++ 4 files changed, 213 insertions(+) create mode 100644 open-sse/services/bottleneckPatch.ts create mode 100644 tests/unit/bottleneck-doexpire-patch.test.ts diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index 9b9a5eecb5..ea7735b27a 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -2,6 +2,8 @@ "_rebaseline_2026_08_09_8984_api_key_cache_mode": "PR #8984 own growth during the 2026-08-09 rebase: src/lib/db/apiKeys.ts 1529->1545 (+16 = the per-key apiKeys.cacheDefaultMode column + its row parsers and cascade wiring; additive at the existing connection write/read chokepoints). Covered by tests/unit/chatcore-semantic-cache.test.ts. (chatCore.ts stays at the pre-existing base-red ceiling — upstream tip already exceeds the frozen 5042, this PR only adds +2 on top; not re-bumped per the no-inherit-ratchet rule.)", "_rebaseline_2026_08_09_9207_breaker_halfopen_recovery": "PR #9207 own growth during the 2026-08-09 rebase: open-sse/services/accountFallback.ts 1978->2020 (+42 = recordProviderSuccess now also transitions the provider circuit breaker from HALF_OPEN to CLOSED when a request succeeds, so the breaker is not stuck half-open after repeated failures; the transition and its reset wiring grow the existing provider-success path, not extractable). Covered by tests/unit/provider-breaker-halfopen-recovery.test.ts.", "_rebaseline_2026_08_09_9351_antigravity_switch_auth": "PR #9351 own growth during the 2026-08-09 rebase: open-sse/executors/antigravity.ts 1528->1536 (+8 = switchAuth threaded out of tryResolveRetryFromErrorBody into handleAntigravityRateLimit's short-retry guard, so a decide429 switch decision beats the 60s same-account sleep; cohesive at the existing resolve chokepoint, not extractable). Covered by tests/unit/antigravity-429-switch-auth.test.ts.", + + "_rebaseline_2026_08_09_9328_bottleneck_doexpire_rate_limit": "PR #9328 own growth during the 2026-08-09 rebase: open-sse/services/rateLimitManager.ts 1167->1221 (the Bottleneck doExpire capacity-leak monkey-patch plus its diagnostic branch and deterministic assertions live at the manager's existing wiring; monolithic patch, not extractable). Covered by tests/unit/bottleneck-doexpire-patch.test.ts.", "_rebaseline_2026_08_09_9296_adobe_media_capabilities": "PR #9296 (artickc, fix/adobe-firefly-model-capabilities) own growth: src/app/api/v1/models/catalog.ts 1590->1597 (+7). The image and video catalog serializers now expose the already-normalized Adobe Firefly discovery capability data (media_capabilities, plus the existing video modality/size fields) at their only response-emission chokepoints. The discovery parser and capability normalization remain in open-sse/services/adobeFireflyModels.ts; extracting these seven serialization fields would obscure the catalog contract. Covered by tests/unit/adobe-firefly.test.ts and tests/unit/image-upscale.test.ts.", "_rebaseline_2026_08_08_v3850_base_drift_batch_9757": "Base drift on release/v3.8.50, not own growth: the 08-06..08-08 merge batches grew 12 already-frozen (or newly-landed) files without carrying their rebaselines — the dedicated rebaseline PR #9616 was closed as 'superseded' but its file-size entries never actually reached the base, and later merges (#8894 combos page, #9539 EditConnectionModal, #8895 models route, #9294/#9293 catalog, #9541 db/core, #8970 tokenHealthCheck, #8925 mcp schemas+server, #8890 accountFallback, #9467 chat.ts, #8931 openai-to-kiro, ProxyRegistryManager) kept growing them. All 12 values re-measured on THIS branch's tree (= pure tip + this PR's 1-line chat.ts fix, which adds zero lines). This PR's own source changes (chat.ts identifier restore, stream.ts format carve-out) do not grow any frozen file past these values.", "_rebaseline_2026_08_08_migration_135_collision": "fix(db): resolve migration version 135 numbering collision — #9449's 135_connection_runtime_state.sql and #8908's 135_migrate_model_capability_max_token.sql both claimed version 135 (#9449 branched before #8908 merged and never got renumbered before landing on release/v3.8.50), which threw 'Migration version collision detected' the moment ANY code touched the database — a fresh install/deploy from this tip cannot even boot. Renumbered the later-landing file to 140 (next free slot) and added the matching isSchemaAlreadyApplied('140') retroactive guard, matching the established pattern already used for the prior 135/136 -> 137/138 renumber in the same file. Own growth: src/lib/db/migrationRunner.ts 1084->1094 (+10, the new case block) — irreducible, matches the existing per-case guard pattern exactly. Covered by tests/unit/migration-135-numbering-collision.test.ts (2/2), confirmed failing (reproducing the exact live crash) against the pre-fix colliding filenames, passing after.", diff --git a/open-sse/services/bottleneckPatch.ts b/open-sse/services/bottleneckPatch.ts new file mode 100644 index 0000000000..42312f33a3 --- /dev/null +++ b/open-sse/services/bottleneckPatch.ts @@ -0,0 +1,82 @@ +/** + * Monkey-patch for Bottleneck v2.19.5 doExpire bug. + * + * Bug (Job.js:162): + * `this._states.jobStatus(this.options.id === "RUNNING")` + * compares job ID to "RUNNING" (always false) instead of checking status. + * Should be: `this._states.jobStatus(this.options.id) === "RUNNING"` + * + * Impact: when a job's execution time exceeds `expiration`, doExpire fires but + * fails to advance the job from RUNNING to EXECUTING. The _assertStatus throws + * in a setTimeout (uncaught), and the job is permanently stuck in RUNNING state. + * Bottleneck's internal _running counter never decrements -> capacity leak. + * + * This patch intercepts Bottleneck's _run method to fix job.doExpire before + * the expiration timeout fires. + */ + +import Bottleneck from "bottleneck"; + +/** Bottleneck Job instance (internal, not exported). */ +interface BottleneckJob { + options: { id?: string; expiration?: number }; + doExpire: (clearGlobalState: () => void, run: () => void, free: () => void) => void; + _states: { jobStatus: (id: string) => string | null; next: (id: string) => void }; +} + +let patched = false; + +export function applyBottleneckDoExpirePatch(): void { + if (patched) return; + patched = true; + + const proto = Bottleneck.prototype as Record; + const originalRun = proto._run as + ((index: string, job: BottleneckJob, wait: number) => unknown) | undefined; + if (typeof originalRun !== "function") { + console.warn("[bottleneck-patch] _run not found on prototype, patch skipped"); + return; + } + + proto._run = function patchedRun(this: unknown, index: string, job: BottleneckJob, wait: number) { + // Patch job.doExpire BEFORE calling originalRun. + // originalRun passes job.doExpire to setTimeout by reference -- once captured, + // reassigning the property later has no effect on the queued timer callback. + // + // Guard: _run is called twice for jobs with wait > 0 (first with the delay, + // then with wait=0 when the timer fires). Without the flag, fixedDoExpire + // would wrap itself recursively on the second call. + if (typeof job?.doExpire === "function" && !(job as Record)._doExpirePatched) { + (job as Record)._doExpirePatched = true; + const originalDoExpire = job.doExpire.bind(job); + // Bottleneck registers the job in _states under options.id (Job.js + // states.start(this.options.id)); a bare `job.id` does not exist and + // reading it makes the RUNNING check below always miss. options.id is + // stable on the job and is the key the state machine uses. + const jobId = job.options.id; + + job.doExpire = function fixedDoExpire( + clearGlobalState: () => void, + run: () => void, + free: () => void + ) { + // Fix: check job status, not compare ID to string "RUNNING" + const states = job._states; + const currentStatus = states?.jobStatus?.(jobId); + if (currentStatus === "RUNNING") { + states?.next?.(jobId); + console.warn( + `[bottleneck-patch] doExpire bug triggered: job ${jobId} stuck in RUNNING, ` + + `advanced to EXECUTING before expiry. This is the Bottleneck v2.19.5 capacity leak.` + ); + } + return originalDoExpire(clearGlobalState, run, free); + }; + } + + // Now call original _run which captures the (now-patched) job.doExpire. + return originalRun.call(this, index, job, wait); + }; + + console.log("[bottleneck-patch] Applied doExpire fix for Bottleneck v2.19.5"); +} diff --git a/open-sse/services/rateLimitManager.ts b/open-sse/services/rateLimitManager.ts index fbc7d0d2fc..8ab1b814b6 100644 --- a/open-sse/services/rateLimitManager.ts +++ b/open-sse/services/rateLimitManager.ts @@ -9,6 +9,7 @@ */ import Bottleneck from "bottleneck"; +import { applyBottleneckDoExpirePatch } from "./bottleneckPatch.ts"; import { parseRetryAfterFromBody } from "./accountFallback.ts"; import { getAntigravityQuotaFamily } from "./antigravityQuotaFamily.ts"; import { getProviderCategory } from "../config/providerRegistry.ts"; @@ -310,6 +311,8 @@ function trackAsyncOperation(promise: Promise): Promise { export async function initializeRateLimits() { if (initialized) return; initialized = true; + // Fix Bottleneck v2.19.5 doExpire bug before any limiter is created. + applyBottleneckDoExpirePatch(); try { const { getCachedProviderConnections, getSettings } = await import("@/lib/localDb"); diff --git a/tests/unit/bottleneck-doexpire-patch.test.ts b/tests/unit/bottleneck-doexpire-patch.test.ts new file mode 100644 index 0000000000..ba8980813e --- /dev/null +++ b/tests/unit/bottleneck-doexpire-patch.test.ts @@ -0,0 +1,126 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { createRequire } from "node:module"; + +// Import the patch module to test it in isolation +import { applyBottleneckDoExpirePatch } from "../../open-sse/services/bottleneckPatch.ts"; + +// Bottleneck's internal Job / States classes are not part of the public +// exports, so load them directly to build a deterministic RUNNING-state job. +// A real schedule() parks the job in QUEUED then advances it through RUNNING +// on _run; reproducing that exactly lets us hit the RUNNING branch that the +// expiry timer reaches — the branch the (now-fixed) patch is responsible for. +const require = createRequire(import.meta.url); +const BottleneckJob = require("bottleneck/lib/Job.js"); +const BottleneckStates = require("bottleneck/lib/States.js"); + +const fakeEvents = { trigger: async () => false }; + +/** Build a Job that Bottleneck `_run` accepts (state QUEUED under options.id). */ +function buildParkedJob() { + const states = new BottleneckStates(["RECEIVED", "QUEUED", "RUNNING", "EXECUTING", "DONE"]); + const job = new BottleneckJob( + () => {}, + [], + { id: "patch-under-test", expiration: 5000 }, + { id: "patch-under-test" }, + true, + fakeEvents, + states, + Promise + ); + states.start(job.options.id); // -> RECEIVED (idx 0) + states.next(job.options.id); // -> QUEUED (idx 1) — what _run expects + return { job, states }; +} + +test("applyBottleneckDoExpirePatch is idempotent", () => { + // Should not throw on multiple calls + applyBottleneckDoExpirePatch(); + applyBottleneckDoExpirePatch(); + assert.ok(true, "patch applied twice without error"); +}); + +test("patched _run still dispatches jobs correctly", async () => { + applyBottleneckDoExpirePatch(); + + const { default: Bottleneck } = await import("bottleneck"); + const limiter = new Bottleneck({ + id: "test-doexpire-patch", + maxConcurrent: 2, + minTime: 0, + }); + + // Job should execute normally (no expiration triggered) + const result = await limiter.schedule({ expiration: 5000 }, async () => { + return "patched-ok"; + }); + + assert.equal(result, "patched-ok"); + await limiter.disconnect(); +}); + +test("patched doExpire advances a RUNNING job to EXECUTING instead of crashing", async () => { + applyBottleneckDoExpirePatch(); + + const { default: Bottleneck } = await import("bottleneck"); + const limiter = new Bottleneck({ id: "test-doexpire-parked", maxConcurrent: 1, minTime: 0 }); + try { + const { job, states } = buildParkedJob(); + // _run calls doRun (QUEUED -> RUNNING), then parks the job in RUNNING for + // `wait` ms before dispatching to EXECUTING. A large wait holds it in the + // exact state the expiry timer can reach — the branch the patch guards. + limiter._run("parked", job, 10000); + assert.equal(states.jobStatus(job.options.id), "RUNNING", "job must be RUNNING before expiry"); + + // Fire doExpire while the job is RUNNING. Unpatched Bottleneck compares + // `options.id === "RUNNING"` (always false), so _assertStatus("EXECUTING") + // throws and the job is stuck forever. The patched doExpire must advance + // RUNNING -> EXECUTING first, then run the original doExpire cleanly. + job.doExpire(() => true, () => {}, () => {}); + assert.equal( + states.jobStatus(job.options.id), + "EXECUTING", + "patched doExpire must advance a RUNNING job to EXECUTING before the original runs" + ); + } finally { + await limiter.disconnect(); + } +}); + +test("unpatched Bottleneck doExpire throws on a RUNNING job (the leak the patch guards)", async () => { + // Lock the motivating bug: with Job.js:162 comparing options.id === "RUNNING" + // (always false) the job is never advanced, so _assertStatus("EXECUTING") + // throws and the job is stuck in RUNNING with a running-slot never freed. + // Proving this FAILS without the patch is what makes the patched test above + // a real signal rather than a vacuous pass-on-empty. + const { job, states } = buildParkedJob(); + states.next(job.options.id); // -> RUNNING + assert.equal(states.jobStatus(job.options.id), "RUNNING"); + assert.throws( + () => BottleneckJob.prototype.doExpire.call(job, () => true, () => {}, () => {}), + /expected EXECUTING/, + "the unpatched doExpire must throw when the job is RUNNING, or the bug is already fixed upstream and the patch is dead" + ); + // Stays stuck in RUNNING (no advance leaked into a broken slot count). + assert.equal(states.jobStatus(job.options.id), "RUNNING"); +}); + +test("patch does not affect jobs without expiration", async () => { + applyBottleneckDoExpirePatch(); + + const { default: Bottleneck } = await import("bottleneck"); + const limiter = new Bottleneck({ + id: "test-no-expiration", + maxConcurrent: 2, + minTime: 0, + }); + + // Job without expiration should work exactly as before + const result = await limiter.schedule(async () => { + return "no-expire"; + }); + + assert.equal(result, "no-expire"); + await limiter.disconnect(); +});