fix(rate-limit): patch Bottleneck doExpire capacity leak (#9328)

This commit is contained in:
Bob.Hou
2026-08-11 09:14:41 -04:00
committed by GitHub
parent a367bf62f5
commit fb83f43fca
4 changed files with 213 additions and 0 deletions

View File

@@ -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.",

View File

@@ -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<string, unknown>;
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<string, unknown>)._doExpirePatched) {
(job as Record<string, unknown>)._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");
}

View File

@@ -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<T>(promise: Promise<T>): Promise<T> {
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");

View File

@@ -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();
});