Skip to content

fix(rate-limit): prevent endless queue with maxWait (#297) #301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
diegosouzapw merged 1 commit into from
Mar 11, 2026
Merged

fix(rate-limit): prevent endless queue with maxWait (#297) #301

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions open-sse/services/rateLimitManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,19 @@ const PERSIST_DEBOUNCE_MS = 60_000; // Debounce persistence to every 60s max
// Track initialization
let initialized = false;

// Max time (ms) a job can wait in queue before failing with a timeout error.
// Prevents infinite queuing when all providers are exhausted after a 429.
// Configurable via RATE_LIMIT_MAX_WAIT_MS env var (default: 2 minutes).
const MAX_WAIT_MS = parseInt(process.env.RATE_LIMIT_MAX_WAIT_MS || "120000", 10);

@kilo-code-bot kilo-code-bot Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Missing validation for invalid or negative values — parseInt returns NaN for non-numeric strings (e.g., RATE_LIMIT_MAX_WAIT_MS=abc), and negative values would be invalid timeouts. Consider adding validation:

const MAX_WAIT_MS = Math.max(0, parseInt(process.env.RATE_LIMIT_MAX_WAIT_MS || "120000", 10));

@gemini-code-assist gemini-code-assist Bot Mar 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current parsing of RATE_LIMIT_MAX_WAIT_MS can result in NaN if a non-numeric string is provided in the environment variable, which could cause unexpected behavior in the Bottleneck limiter. For safer and more consistent parsing, it's better to use the existing toNumber helper function, which is designed to handle such cases gracefully.

Suggested change
const MAX_WAIT_MS = parseInt(process.env.RATE_LIMIT_MAX_WAIT_MS || "120000", 10);
const MAX_WAIT_MS = toNumber(process.env.RATE_LIMIT_MAX_WAIT_MS, 120000);

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_WAIT_MS is derived via parseInt(...) without validating the result. If RATE_LIMIT_MAX_WAIT_MS is set to a non-numeric value (or includes separators like "120_000"), parseInt can yield NaN/partial parses, which then propagates into Bottleneck options and breaks the documented default behavior. Please validate/clamp this env var (e.g., use the existing toNumber helper with a 120000 fallback and enforce >= 0).

Suggested change
const MAX_WAIT_MS = parseInt(process.env.RATE_LIMIT_MAX_WAIT_MS || "120000", 10);
const RAW_MAX_WAIT_MS = toNumber(process.env.RATE_LIMIT_MAX_WAIT_MS, 120000);
const MAX_WAIT_MS = Math.max(0, Math.floor(RAW_MAX_WAIT_MS));

Copilot uses AI. Check for mistakes.

// Default conservative settings (before we learn from headers)
const DEFAULT_SETTINGS = {
maxConcurrent: 10,
minTime: 0, // No throttle by default — let headers teach us
reservoir: null, // No initial reservoir — unlimited until we learn
reservoirRefreshAmount: null,
reservoirRefreshInterval: null,
maxWait: MAX_WAIT_MS, // Fail-fast: don't queue forever on 429 exhaustion

@kilo-code-bot kilo-code-bot Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: maxWait is not a valid Bottleneck option — Bottleneck v2.19.5 does not support this property. It will be silently ignored, and jobs will still queue indefinitely. The intended fail-fast behavior will not work.

Consider using maxQueue to limit queue size (by count), or implementing a custom timeout wrapper using Promise.race() with a timeout promise.

};

/**
Expand Down Expand Up @@ -111,6 +117,7 @@ export async function initializeRateLimits() {
reservoir: rpm,
reservoirRefreshAmount: rpm,
reservoirRefreshInterval: 60 * 1000,
maxWait: MAX_WAIT_MS,

@kilo-code-bot kilo-code-bot Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: maxWait is not a valid Bottleneck option — this will be silently ignored. See comment on line 74.

id: key,
})
);
Expand All @@ -135,6 +142,7 @@ export async function initializeRateLimits() {
reservoir: DEFAULT_API_LIMITS.requestsPerMinute,
reservoirRefreshAmount: DEFAULT_API_LIMITS.requestsPerMinute,
reservoirRefreshInterval: 60 * 1000, // Refresh every minute
maxWait: MAX_WAIT_MS,

@kilo-code-bot kilo-code-bot Bot Mar 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: maxWait is not a valid Bottleneck option — this will be silently ignored. See comment on line 74.

id: key,
})
);
Expand Down
Loading