feat(core): cap concurrent in-flight requests per provider (#3409) - #3636
feat(core): cap concurrent in-flight requests per provider (#3409)#3636JahanzaibTayyab wants to merge 2 commits into
Conversation
Several users hit "Error: 429 Too many concurrent requests for this model" against rate-limited backends, especially when sub-agents fan out or when /compress runs alongside the main turn. There was no way to gate this on the client; the only available knob was retry-on-429, which both hides the symptom and burns latency. This PR adds a per-provider request-concurrency cap that translates upstream rate limits into client-side back-pressure. Excess callers queue FIFO behind a semaphore; behavior is identical to today when no cap is configured. Wiring: - ContentGeneratorConfig.requestConcurrency -- new optional field (provider-scoped via modelProviders[].generationConfig). - QWEN_REQUEST_CONCURRENCY env var as a fallback when the per-provider field is unset, so the cap can be applied process-wide without editing settings.json. - 0 / undefined / negative => unlimited (default; no behavior change). Implementation: - packages/core/src/utils/concurrencyLimiter.ts -- minimal FIFO counting semaphore. acquire/release pair plus a runExclusive helper. Release is idempotent so accidental double-release does not over-credit. Capacity <= 0 short-circuits to a no-op. - packages/core/src/core/rateLimitedContentGenerator.ts -- wrapper implementing ContentGenerator. Gates generateContent and generateContentStream; intentionally does NOT gate countTokens or embedContent (cheap, often local). For streaming the slot is held until the consumer fully drains the iterator (or it errors, throws during init, or unwinds early via break/return) -- the rate limit the upstream actually counts is concurrent open connections, not request initiation. - packages/core/src/core/contentGenerator.ts -- createContentGenerator applies RateLimitedContentGenerator before LoggingContentGenerator so rate-limit wait time is visible inside the logged request span. Tests - 6 cases in concurrencyLimiter.test.ts (cap respected, FIFO, success + failure release, double-release idempotency, NaN/Infinity coerced to unlimited). - 7 cases in rateLimitedContentGenerator.test.ts (peak in-flight, per-call slot release on rejection, stream slot held until drain, release on init throw, release on consumer-side break, no gating for countTokens/embedContent, useSummarizedThinking pass-through). - 6 cases extending contentGenerator.test.ts pin the resolution precedence: not wrapped by default, wrapped when configured, env fallback, config-over-env, garbage env values treated as unlimited, fractional config values floored. Affected suite: 6,093 core tests pass; full TypeScript build clean. Prepared with assistance from Claude (Anthropic) under human review.
There was a problem hiding this comment.
Pull request overview
Adds a configurable, per-model/provider concurrency cap to turn upstream “429 too many concurrent requests” failures into client-side back-pressure (FIFO queuing), with schema/docs updates and thorough unit tests.
Changes:
- Introduces a FIFO counting semaphore (
ConcurrencyLimiter) and aRateLimitedContentGeneratorwrapper to gategenerateContent/generateContentStream. - Wires the limiter into
createContentGenerator(applied before logging) and addsrequestConcurrencyconfiguration (withQWEN_REQUEST_CONCURRENCYenv fallback). - Updates CLI/VS Code settings schemas and user docs, and adds test coverage across the new components.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode-ide-companion/schemas/settings.schema.json | Adds requestConcurrency to the VS Code companion JSON schema. |
| packages/core/src/utils/concurrencyLimiter.ts | Implements FIFO counting semaphore with idempotent release and runExclusive. |
| packages/core/src/utils/concurrencyLimiter.test.ts | Tests semaphore capacity, FIFO ordering, and release semantics. |
| packages/core/src/core/rateLimitedContentGenerator.ts | Wraps ContentGenerator to enforce concurrency caps (including stream-drain release). |
| packages/core/src/core/rateLimitedContentGenerator.test.ts | Tests gating behavior for sync + streaming paths and non-gated methods. |
| packages/core/src/core/contentGenerator.ts | Adds requestConcurrency to config and wraps generator with limiter before logging. |
| packages/core/src/core/contentGenerator.test.ts | Tests wrapping behavior and env/config precedence. |
| packages/cli/src/config/settingsSchema.ts | Exposes requestConcurrency in CLI settings schema. |
| docs/users/configuration/settings.md | Documents the new setting and provides an example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Maximum number of concurrent in-flight requests against the model API. | ||
| * Use this when the upstream returns ``429 Too many concurrent requests`` | ||
| * for the configured model -- excess callers wait FIFO instead of erroring | ||
| * (#3409). ``0`` / ``undefined`` / negative => no limit (default). | ||
| */ | ||
| requestConcurrency?: number; |
There was a problem hiding this comment.
requestConcurrency is added to ContentGeneratorConfig and used in createContentGenerator, but it won’t be populated from user settings / modelProviders because the unified model resolver only copies fields listed in MODEL_GENERATION_CONFIG_FIELDS (see packages/core/src/models/constants.ts). Since requestConcurrency isn’t in that list (and also isn’t in ModelGenerationConfig), settings.model.generationConfig.requestConcurrency (and any modelProviders generationConfig value) will be ignored and the limiter won’t activate as documented. Add requestConcurrency to the generation-config field allowlist (and the modelProviders generationConfig type/pick) so the config knob actually takes effect.
wenshao
left a comment
There was a problem hiding this comment.
[Critical] requestConcurrency is added to ContentGeneratorConfig and documented under generationConfig, but it is not included in MODEL_GENERATION_CONFIG_FIELDS or ModelGenerationConfig. Users setting model.generationConfig.requestConcurrency or modelProviders[].generationConfig.requestConcurrency will have that value silently ignored, so the advertised config path does not enable the limiter. Please add 'requestConcurrency' to MODEL_GENERATION_CONFIG_FIELDS and include it in ModelGenerationConfig in packages/core/src/models/types.ts.
— gpt-5.5 via Qwen Code /review
| // Apply per-provider concurrency limit (#3409) before logging so rate-limit | ||
| // back-pressure is visible to the logger as ``in flight`` time, not as a | ||
| // separate phase outside of the request span. | ||
| const limit = resolveRequestConcurrency(generatorConfig.requestConcurrency); |
There was a problem hiding this comment.
[Critical] This creates a fresh limiter for every createContentGenerator() call, so the cap is scoped to a generator instance rather than to the provider. Main config, subagents, and in-process backends can each create separate generators for the same provider, allowing aggregate concurrency to exceed the configured provider cap and still trigger upstream 429s.
Please share limiter instances by resolved provider identity, for example auth type/base URL/model, through a process-level or Config-owned registry, and add a cross-generator test proving two generators for the same provider share the cap.
— gpt-5.5 via Qwen Code /review
| * 2. ``QWEN_REQUEST_CONCURRENCY`` env var. | ||
| * Anything <= 0, NaN, or otherwise non-numeric is treated as "unlimited" (0). | ||
| */ | ||
| function resolveRequestConcurrency(configured: number | undefined): number { |
There was a problem hiding this comment.
[Critical] resolveRequestConcurrency() normalizes explicit 0 or negative config values to 0, then treats that the same as an unset config and falls back to QWEN_REQUEST_CONCURRENCY. That contradicts the documented precedence: env should only be a fallback when the provider value is unset, and explicit 0/negative should mean unlimited.
One way to preserve the opt-out semantics is:
function resolveRequestConcurrency(configured: number | undefined): number {
if (configured !== undefined) {
return normalizeConcurrency(configured);
}
const envRaw = process.env['QWEN_REQUEST_CONCURRENCY'];
if (envRaw === undefined) {
return 0;
}
const parsed = Number.parseInt(envRaw.trim(), 10);
return normalizeConcurrency(parsed);
}— gpt-5.5 via Qwen Code /review
| * the limiter is unlimited (``capacity <= 0``) this resolves immediately | ||
| * with a no-op release. | ||
| */ | ||
| async acquire(): Promise<() => void> { |
There was a problem hiding this comment.
[Suggestion] Queued acquisitions are not abort-aware. generateContent() and generateContentStream() wait for acquire() before the wrapped generator sees request.config?.abortSignal, so a cancelled queued request remains in waiters until a slot opens and can then start stale upstream work.
Please consider accepting an AbortSignal in acquire()/runExclusive(), removing the queued waiter on abort, and failing fast when the signal is already aborted.
— gpt-5.5 via Qwen Code /review
|
This pull request has had no activity for 60 days and is being marked as stale. It will be closed in another 30 days if no further activity occurs. To keep it open, push a new commit or leave a comment. Maintainers may apply |
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review — requestConcurrency silently dropped from user settings
The concurrency cap works when set via the QWEN_REQUEST_CONCURRENCY env var. However, the settings-file path is silently broken: requestConcurrency was added to ContentGeneratorConfig and to the JSON schema, but was not added to the two structures that control which fields flow from settings.json into the content generator.
Finding 1 — MODEL_GENERATION_CONFIG_FIELDS missing 'requestConcurrency'
packages/core/src/models/constants.ts, the MODEL_GENERATION_CONFIG_FIELDS array. This array is iterated by resolveGenerationConfig() in modelConfigResolver.ts (and the four call sites that invoke it: modelsConfig.ts lines ~607, ~867; content-generator-config.ts lines ~65, ~165). Any field absent from this array is silently skipped — the value is read from settings.json but never copied into the result ContentGeneratorConfig. Because 'requestConcurrency' is not in this array, modelProviders[].generationConfig.requestConcurrency and model.generationConfig.requestConcurrency are both silently dropped at all four call sites.
Finding 2 — ModelGenerationConfig Pick missing requestConcurrency
packages/core/src/models/types.ts, the ModelGenerationConfig = Pick<ContentGeneratorConfig, ...> definition. This typed alias is used throughout ModelConfig, ResolvedModelConfig, and RuntimeModelSnapshot. Because requestConcurrency is absent from the Pick, TypeScript will reject the field at compile time on any typed model-config object — meaning the value can never flow through the typed pipeline even if Finding 1 is fixed independently.
Minimal fix:
// packages/core/src/models/constants.ts
export const MODEL_GENERATION_CONFIG_FIELDS = [
// … existing fields …
'requestConcurrency', // ← add
] as const satisfies ReadonlyArray<keyof ContentGeneratorConfig>;
// packages/core/src/models/types.ts
export type ModelGenerationConfig = Pick<
ContentGeneratorConfig,
| /* … existing fields … */
| 'requestConcurrency' // ← add
>;Both additions are required. The env-var fallback (QWEN_REQUEST_CONCURRENCY) is unaffected and works today.
Generated by Claude Code
|
@qen-code /triage |
DragonnZhang
left a comment
There was a problem hiding this comment.
LGTM. The concurrency limiter is a correct FIFO semaphore with double-release protection. The RateLimitedContentGenerator properly holds slots through the full stream lifecycle (drain, error, early abort, consumer return()). Wrapping order (base -> rate-limited -> logging) is correct so queue wait shows as in-flight time. Configuration resolution (config > env var > unlimited) and normalization (floor, reject non-finite/negative) are clean. Test coverage is thorough -- cap enforcement, error-path release, stream abort mid-flight, and init failure are all covered.
|
This pull request has had no activity for 60 days and is being marked as stale. It will be closed in another 30 days if no further activity occurs. To keep it open, push a new commit or leave a comment. Maintainers may apply |
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root cause
Textual or semanticSchema/docs conflicts were textual: adjacent inserts, resolution keeps both ( return new LazyContentGenerator(async () => {
const [baseGenerator, { LoggingContentGenerator }] = await Promise.all([
loadBaseGenerator(),
import('./loggingContentGenerator/index.js'),
]);
const limit = resolveRequestConcurrency(generatorConfig.requestConcurrency);
const wrapped =
limit > 0
? new RateLimitedContentGenerator(baseGenerator, new ConcurrencyLimiter(limit))
: baseGenerator;
return new LoggingContentGenerator(wrapped, config, generatorConfig);
});The PR's six wrapping tests cast the result to Load-bearing
Could not verify — follow-up neededNo build/tests run here. Known breakage in a NON-conflicted PR file: 中文说明冲突根源:① #7512 把 文本还是语义:schema/文档为文本冲突,两边条目都保留。 关键点: 未能验证:此处未运行构建/测试。未冲突的 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — precheck-pr was skipped in CI and the full packages/core suite did not run locally (blocked by the PR's compile failure).
Test Plan (not a blocker): npm run build — exit 1.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| countTokens(request: CountTokensParameters): Promise<CountTokensResponse> { | ||
| return this.wrapped.countTokens(request); | ||
| } |
There was a problem hiding this comment.
[Critical] R1-1: packages/core does not compile at this commit. RateLimitedContentGenerator forwards countTokens (line 69) and useSummarizedThinking (line 77) on this.wrapped, but the ContentGenerator interface (contentGenerator.ts:38) declares only generateContent, generateContentStream, and embedContent, so npm run build / npm run typecheck fail with TS2339 at both sites and every downstream workspace build is blocked. No production generator implements either method and no production caller invokes them — the forwarders are dead code that breaks the build; the unit tests stay green only because vitest/esbuild strips types without checking and the test FakeGenerator happens to implement both. Delete the two forwarders (and the now-unused CountTokensParameters/CountTokensResponse imports) and trim the two corresponding cases in rateLimitedContentGenerator.test.ts ('does not gate countTokens / embedContent' down to embedContent, and 'forwards useSummarizedThinking without blocking').
Witness:
src/core/rateLimitedContentGenerator.ts(69,25): error TS2339: Property 'countTokens' does not exist on type 'ContentGenerator'.
src/core/rateLimitedContentGenerator.ts(77,25): error TS2339: Property 'useSummarizedThinking' does not exist on type 'ContentGenerator'.
The fix must respect the interface as declared at packages/core/src/core/contentGenerator.ts:38 — only generateContent, generateContentStream, and embedContent, at both the merge base and HEAD; widening it would force implementations onto LazyContentGenerator, LoggingContentGenerator, and every provider generator. Acceptance check: npm run build --workspace="packages/core" exits 0 after the removal — re-adding either forwarder without extending the interface reproduces TS2339.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const limit = resolveRequestConcurrency( | ||
| generatorConfig.requestConcurrency, | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-2: [certifies-falsely] [new-surface] The documented requestConcurrency setting never reaches this read site. The field was added to ContentGeneratorConfig, both schemas, and the docs, but not to MODEL_GENERATION_CONFIG_FIELDS (packages/core/src/models/constants.ts) or the ModelGenerationConfig Pick (packages/core/src/models/types.ts) — and every settings→generator propagation site iterates that allowlist (resolveGenerationConfig in modelConfigResolver.ts:387, mergeSettingsGenerationConfig and the provider-sync loop in modelsConfig.ts, both loops in content-generator-config.ts). A user following the docs this PR adds and setting model.generationConfig.requestConcurrency: 4 in settings.json has the value silently dropped with no warning, no limiter is wrapped, and the upstream 429 Too many concurrent requests for this model keeps firing — the exact incident of issue #3409, unchanged; only the env var works. The new tests stay green because they inject the field directly into createContentGenerator, bypassing every resolver. The same omission also leaks the cap across providers: buildAgentContentGeneratorConfig clears only allowlisted fields on provider switch, so a configured cap survives the ...parentConfig spread into sub-agent generators targeting a different provider, unlike timeout/maxRetries. This re-confirms the standing blocker from the April review (review 4177606870 and comment 3143164406). Add 'requestConcurrency' to MODEL_GENERATION_CONFIG_FIELDS in packages/core/src/models/constants.ts and to the ModelGenerationConfig Pick in packages/core/src/models/types.ts.
Witness:
FAIL probe R1-2 > allowlist contains requestConcurrency
expected [ 'samplingParams', 'timeout', …(18) ] to include 'requestConcurrency'
FAIL probe R1-2 > resolveModelConfig propagates settings.generationConfig.requestConcurrency
AssertionError: expected undefined to be 4 (sibling timeout: 60000 propagated on the same call)
The fix must enroll the field in both the constant and the Pick — the loops at modelsConfig.ts:915 and content-generator-config.ts:178 index the ModelGenerationConfig Pick with every entry of the list, so adding the constant without extending the Pick is a compile error; membership also flows the field into PROVIDER_SOURCED_FIELDS (constants.ts:61) and the cross-provider clear in content-generator-config.ts:73-78. Acceptance check: add a case in packages/core/src/models/modelConfigResolver.test.ts asserting resolveModelConfig with settings.generationConfig.requestConcurrency: 4 yields config.requestConcurrency === 4 (plus the modelProvider.generationConfig variant), and confirm removing the allowlist entry turns it red.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| ? new RateLimitedContentGenerator( | ||
| baseGenerator, | ||
| new ConcurrencyLimiter(limit), | ||
| ) |
There was a problem hiding this comment.
[Critical] R1-3: [certifies-falsely] [new-surface] The limiter is constructed per createContentGenerator call, so the cap is scoped to a generator instance rather than to the provider. The process holds several generators for the same provider — dedicated per-subagent generators (subagent-manager.ts), Arena agents (InProcessBackend.ts:636), forks (forkedAgent.ts:292), and baseLlmClient's perModelGeneratorCache — so aggregate in-flight traffic reaches #generators × limit. With requestConcurrency: 1 and two subagents whose model selectors resolve to the same provider, the upstream still sees three concurrent requests and still returns 429 — the exact multi-agent scenario this PR targets ('inherit' subagents share the parent generator and do hold the cap; the shortfall is dedicated-generator topologies). This re-confirms the standing blocker (comment 3144320778): share one limiter per resolved provider identity (auth type / base URL / model) through a process-level registry consulted here instead of constructing one per call.
Witness:
PROBE-R1-3 limiters identical: false, limits: 1/1
PROBE-R1-3 observed: started=2, peakInFlight=2, limitersShared=false (cap=1)
AssertionError: expected 2 to be less than or equal to 1
A registry key must not assume only the main generator carries a configured value — buildAgentContentGeneratorConfig spreads {...parentConfig} (content-generator-config.ts:67-70) into every per-agent generator — and it must not alter forked agents' same-model/same-auth dedup back to the ambient generator (forkedAgent.ts:285-290). Acceptance check: add the cross-generator test the standing blocker requests — two generators for the same provider config with requestConcurrency: 1, concurrent generateContent across both, aggregate peak in-flight ≤ 1; it is red while the limiter is per-instance.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| function resolveRequestConcurrency(configured: number | undefined): number { | ||
| const fromConfig = normalizeConcurrency(configured); | ||
| if (fromConfig > 0) { |
There was a problem hiding this comment.
[Critical] R1-4: [fails-closed] [new-surface] An explicit requestConcurrency: 0 (documented as "no limit") cannot opt out of the env-var cap. normalizeConcurrency collapses an explicit 0/negative config value to 0, which this function then treats identically to unset and falls through to QWEN_REQUEST_CONCURRENCY — contradicting the docs this PR adds ("honored as a fallback when the provider-level value is unset") and the PR's own wiring table ("(unset / 0 / negative) → No limit"). An operator who exports QWEN_REQUEST_CONCURRENCY=4 host-wide and sets requestConcurrency: 0 on one provider to opt it out is still capped at 4, with requests serializing unexpectedly. Currently latent on the settings path (R1-2 drops the value first), fully live once that is fixed. This re-confirms the standing blocker (comment 3144320779).
Witness:
PROBE-R1-4 observed: wrapped=true, limit=6 (env=6, config=0)
AssertionError: expected RateLimitedContentGenerator{ …(2) } to not be an instance of RateLimitedContentGenerator
| function resolveRequestConcurrency(configured: number | undefined): number { | |
| const fromConfig = normalizeConcurrency(configured); | |
| if (fromConfig > 0) { | |
| function resolveRequestConcurrency(configured: number | undefined): number { | |
| if (configured !== undefined) { | |
| return normalizeConcurrency(configured); | |
| } | |
| const fromConfig = normalizeConcurrency(configured); | |
| if (fromConfig > 0) { |
Keep the env-side semantics pinned by this PR's own tests — 'treats env var %s as unlimited' for '0'/'-1'/'nope'/' ' and 'falls back to QWEN_REQUEST_CONCURRENCY env var when config is unset' in contentGenerator.test.ts. Acceptance check: add a case with env QWEN_REQUEST_CONCURRENCY=6 and config requestConcurrency: 0 asserting logging.getWrapped() is not a RateLimitedContentGenerator; removing the !== undefined guard turns it red.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * thin pass-through with a single ``await Promise.resolve()`` worth of | ||
| * overhead. | ||
| */ | ||
| export class RateLimitedContentGenerator implements ContentGenerator { |
There was a problem hiding this comment.
[Critical] R1-5: [fails-closed] [new-surface] All four new files (rateLimitedContentGenerator.ts, rateLimitedContentGenerator.test.ts, packages/core/src/utils/concurrencyLimiter.ts, concurrencyLimiter.test.ts) are camelCase and are not in eslint.legacy-filenames.mjs, but eslint.config.js applies check-file/filename-naming-convention with KEBAB_CASE to packages/core/src/**/*.ts — so npm run lint fails with four errors and the CI lint gate is red. Rename to rate-limited-content-generator.ts / rate-limited-content-generator.test.ts / concurrency-limiter.ts / concurrency-limiter.test.ts, updating the imports in contentGenerator.ts, contentGenerator.test.ts, and the two test files in the same commit.
Witness:
rateLimitedContentGenerator.test.ts error The filename "rateLimitedContentGenerator.test.ts" does not match the "KEBAB_CASE" pattern check-file/filename-naming-convention
rateLimitedContentGenerator.ts error The filename "rateLimitedContentGenerator.ts" does not match the "KEBAB_CASE" pattern check-file/filename-naming-convention
concurrencyLimiter.test.ts error The filename "concurrencyLimiter.test.ts" does not match the "KEBAB_CASE" pattern check-file/filename-naming-convention
concurrencyLimiter.ts error The filename "concurrencyLimiter.ts" does not match the "KEBAB_CASE" pattern check-file/filename-naming-convention
✖ 4 problems (4 errors, 0 warnings)
The allowlist eslint.legacy-filenames.mjs covers pre-existing files only — neither new name is in it. Acceptance check: ESLint on the four renamed files reports zero check-file/filename-naming-convention errors; renaming back to camelCase re-creates them.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| export class ConcurrencyLimiter { | ||
| private active = 0; | ||
| private readonly waiters: Array<() => void> = []; |
There was a problem hiding this comment.
[Suggestion] R1-6: This 96-line hand-rolled FIFO counting semaphore duplicates Semaphore from async-mutex, already a packages/core dependency (^0.5.0) and imported in six core files (e.g. utils/jsonl-utils.ts, agents/team/mailbox.ts, memory/writeContextFile.ts). The installed library provides every behavior concurrencyLimiter.test.ts pins — FIFO dispatch, acquire() resolving to a release function, runExclusive with release-in-finally, and idempotent release — so the package now carries two semaphore implementations whose semantics must be kept in sync by hand; any future fix (e.g. queue cancellation on shutdown) must be made twice. The only differences are the capacity <= 0 pass-through (never constructed in production: contentGenerator.ts builds the limiter only when limit > 0) and the inFlight/queued getters (in-flight is derivable as limit - semaphore.getValue()). Back RateLimitedContentGenerator with async-mutex's Semaphore, keeping at most a thin adapter if the test-facing counters are wanted.
Witness:
acquire shape: weight = 1 , releaser is function: true
runExclusive exists: true
FIFO order: [2,3]
double release: no throw
after double-release, second waiter parked (cap holds): true
getValue() after 1 of 2 acquired: 1 (inFlight derivable as limit - getValue())
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const parsed = Number.parseInt(envRaw.trim(), 10); | ||
| return normalizeConcurrency(parsed); |
There was a problem hiding this comment.
[Suggestion] R1-7: This hand-rolls positive-integer env-var parsing when packages/core already exports parsePositiveIntegerEnv (packages/core/src/utils/env.ts) — the same helper used for the analogous QWEN_CODE_MAX_TOOL_CONCURRENCY cap in coreToolScheduler.ts — plus a near-identical parsePositiveIntegerEnvValue in tokenLimits.ts. A third inline parser drifts: for the tested cases ('0', '-1', 'nope', ' ', '6') the helper returns exactly what this code returns, but for fractional env values the behaviors diverge — '3.7' floors to a cap of 3 here while the helper's ^\d+$ regex would read it as unset/unlimited — a case pinned by neither docs nor tests. Use the shared helper (import it from packages/core/src/utils/env.ts); if flooring fractional env values is actually intended, document it in settings.md instead.
Witness:
prEnv37: {wrapped: true, limit: 3} // parseInt floors '3.7'
HELPER('3.7') = 0 // parsePositiveIntegerEnv -> unlimited
fixed tree: prEnv37: {wrapped: false, limit: 0}, pinned contentGenerator tests 36/36 pass
| const parsed = Number.parseInt(envRaw.trim(), 10); | |
| return normalizeConcurrency(parsed); | |
| return parsePositiveIntegerEnv(envRaw, 0); |
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const parsed = Number.parseInt(envRaw.trim(), 10); | ||
| return normalizeConcurrency(parsed); |
There was a problem hiding this comment.
[Suggestion] R1-8: Malformed QWEN_REQUEST_CONCURRENCY values are silently swallowed — 'x4' parses to NaN → 0 → unlimited with zero diagnostics, and parseInt tolerates trailing junk ('4x' → 4, silently truncated) — so a typo'd knob silently stays unlimited while the 429s this feature exists to prevent keep firing. The sibling resolver resolveStreamGuardMs (packages/core/src/core/stream-guards.ts) handles the same class of knob with strict /^\d+$/ validation and console.warn('[qwen-code] Ignoring invalid ${envName}="..."'), pinned by tests, and its env names are exported constants (e.g. QWEN_STREAM_IDLE_TIMEOUT_MS_ENV) where this one is an inline literal repeated across implementation and tests. Validate strictly, warn on malformed input in the same shape, and hoist the env name to an exported constant.
Witness:
prEnvX4: {wrapped: false, matchingWarns: 0, totalWarns: 0, warnMessages: []}
prEnv4x: {wrapped: true, limit: 4, matchingWarns: 0}
fixed tree: prEnvX4.matchingWarns=1 '[qwen-code] Ignoring invalid QWEN_REQUEST_CONCURRENCY="x4" ...'; prEnv4x: {wrapped: false, limit: 0, matchingWarns: 1}
Match the sibling warning shape at packages/core/src/core/stream-guards.ts ('[qwen-code] Ignoring invalid ${envName}="${raw}"' with /^\d+$/-style validation). Acceptance check: with QWEN_REQUEST_CONCURRENCY='x4' and a vi.spyOn(console, 'warn'), generator creation yields no wrapper but the spy is called with a message naming the variable — removing the warn branch turns it red.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const start = Date.now(); | ||
| await Promise.all([ | ||
| gen.countTokens({} as CountTokensParameters), | ||
| gen.embedContent({} as EmbedContentParameters), | ||
| ]); | ||
| expect(Date.now() - start).toBeLessThan(50); |
There was a problem hiding this comment.
[Suggestion] R1-9: This test cannot distinguish gated from ungated: the slot is held by the default FakeGenerator.generateContent, which holds it for only 5ms (setTimeout(resolve, 5)), while the oracle allows 50ms — routing countTokens/embedContent through the limiter would add ~5ms and still pass. We ran the mutation: with the calls rerouted through limiter.runExclusive, the suite stays 7/7 green, so the only guard over the documented exemption certifies that regression green. The wall-clock oracle is also load-sensitive in reverse: an ungated call exceeding 50ms on a busy runner fails spuriously. Replace the timing oracle with a control-flow one: hold the slot with a generateImpl that resolves only via a test-owned deferred (never within the assertion window), await Promise.all([gen.countTokens(...), gen.embedContent(...)]), and assert the inner call counts — if the calls were gated, the Promise.all would hang and fail by timeout.
Witness:
mutant + original test: Test Files 1 passed (1) / Tests 7 passed (7)
mutant + fixed test: × RateLimitedContentGenerator > does not gate countTokens / embedContent 60004ms … Tests 1 failed | 6 passed
intact + fixed test: Tests 7 passed (7)
The shared 5ms default hold is used by the other tests in this file, so the never-resolving hold must be supplied per-test via inner.generateImpl rather than by changing the default. Acceptance check: the rewritten test must time out (go red) when countTokens or embedContent is routed through limiter.runExclusive.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| return new Promise<() => void>((resolve) => { | ||
| this.waiters.push(() => { | ||
| this.active++; | ||
| resolve(this.makeRelease()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-10: Queue waits behind the cap are silent — no log line, heartbeat, or timeout when callers park in waiters, and the queued/inFlight getters are read only by tests. Waiting behind this cap is the feature's documented core behavior ("excess callers wait FIFO"), so every parked caller experiences total silence; with requestConcurrency: 1 and a slow primary stream (or timeout: 0, documented as "disable"), the user sees a frozen CLI with no output and oncall has no log distinguishing "queued behind the semaphore" from "dead" — while the retry path at least prints heartbeats ('[qwen-code] Waiting for API capacity...'). Log via debugLogger (utils/debugLogger.ts) when a caller parks — with queue depth and the cap — and when it is granted after waiting; consider a stderr heartbeat after N seconds, mirroring retryWithBackoff's heartbeatFn. (The abort-awareness half of queued waits is already tracked in the open thread at concurrencyLimiter.ts:52.)
Witness:
{parkedStillWaiting: true, queued: 1, consoleCalls: {log: 0, warn: 0, error: 0, info: 0}, messages: []}
fixed tree: {warn: 1, messages: ["[qwen-code] Waiting for request capacity... queued=1, limit=1"]}
Acceptance check: a case in concurrencyLimiter.test.ts asserting the log fires when a second caller parks behind a full limiter (spy on debugLogger); removing the log statement turns it red.
— qwen3.8-max via Qwen Code /review (v0.23.0)
doudouOUC
left a comment
There was a problem hiding this comment.
Review at head 88de7ffb342f8671b114a1494815bec2dd5dcf61 — verdict: 4 Criticals, 2 Suggestions. Core-gate: yes (packages/core/src/core/**, utils/**; cross-package with packages/cli config). Prod logic ≈ 259 lines. Mergeable: CONFLICTING (DIRTY), 284 commits behind main.
Staleness first, as the review rules require. Unlike some other large stale PRs, this diff is small and still applies conceptually to main — the allowlist, the resolver and the call sites are all unchanged there — so I re-verified each finding against current main, not just the PR's merge base.
Critical 1 — the documented setting is a dead switch
packages/cli/src/config/settingsSchema.ts adds model.generationConfig.requestConcurrency, and docs/users/configuration/settings.md documents it. But resolveGenerationConfig copies only allowlisted keys:
// packages/core/src/models/modelConfigResolver.ts:378-402
for (const field of MODEL_GENERATION_CONFIG_FIELDS) {
if (authType && modelProviderConfig && field in modelProviderConfig) { (result as any)[field] = modelProviderConfig[field]; … }
else if (settingsConfig && field in settingsConfig) { (result as any)[field] = settingsConfig[field]; … }
}requestConcurrency is absent from MODEL_GENERATION_CONFIG_FIELDS (packages/core/src/models/constants.ts:21-41) at current main and at the PR's merge base, and this PR does not touch that file. I confirmed it directly: the constant holds 20 entries (samplingParams … toolResultContentFormat) and requestConcurrency is not among them; a repo-wide grep for the symbol on origin/main returns nothing.
So the value never reaches ContentGeneratorConfig, and resolveRequestConcurrency(generatorConfig.requestConcurrency) always sees undefined. Only the QWEN_REQUEST_CONCURRENCY env fallback works. The new tests hide this by injecting the field straight into a ContentGeneratorConfig, bypassing the resolver.
Critical 2 — the cap is per generator instance, not per provider
This contradicts the PR title. createContentGenerator (packages/core/src/core/contentGenerator.ts:489) is not memoized and constructs new ConcurrencyLimiter(limit) on every call. Three call sites create independent generators against the same upstream:
config/config.ts:3676core/baseLlmClient.ts:747(its ownperModelGeneratorCache, keyed per model)models/content-generator-config.ts:136(createRuntimeContentGeneratorView, one per subagent runtime view)
Effective concurrency is limit × number of live generators, so the 429 this PR targets still fires in any session with subagents or a fallback model.
Critical 3 — an explicit requestConcurrency: 0 cannot opt out of the env cap
normalizeConcurrency collapses 0, negatives and undefined all to 0, and resolveRequestConcurrency then treats 0 as "unset" and reads QWEN_REQUEST_CONCURRENCY. So with the env var exported, the documented "0 = unlimited" per-model escape hatch is unreachable — the config value cannot beat the env value it is supposed to override.
Critical 4 — all four new files violate the repo's filename lint and are not allow-listed
eslint.config.js:255-272 enforces check-file/filename-naming-convention KEBAB_CASE over packages/core/src/**/*.ts and packages/cli/src/**/*.ts, ignoring only entries in eslint.legacy-filenames.mjs (569 lines). These four are camelCase and absent from that allowlist, so the CI lint gate fails:
rateLimitedContentGenerator.tsrateLimitedContentGenerator.test.tsutils/concurrencyLimiter.tsutils/concurrencyLimiter.test.ts
Rename to rate-limited-content-generator.ts and concurrency-limiter.ts.
Suggestions
- Stream permit release depends on the caller iterating.
generateContentStreamacquires the permit eagerly, butgateStream'sfinallyonly runs once the returned async generator is started; a caller that obtains the stream and never callsnext()leaks the slot permanently, andConcurrencyLimiterhas no timeout, so withlimit: 1the next request waits forever. I traced the real consumers (geminiChat.ts:3894insideretryWithBackoff,:3966 processStreamResponse, consumed at:2778-2816and:3984-3996;loggingContentGenerator.ts:421-488) and found no reachable non-iterating path today — the stream call passes noshouldRetryOnContent, soretry.ts:299-327's result-discarding content-retry arm never fires here. So this is a fragility note, not a Critical. Acquiring insidegateStream, or releasing on aqueueMicrotaskwatchdog, would make it structural. - No abort/cancel leak found:
generateContentStreamreleases in thecatcharound initialization,for awaitforwards.return()intogateStream'sfinallyon consumer unwind, andrunExclusivereleases on throw. Worth a test for the "consumer breaks out of the for-await mid-stream" case, which the new tests do not cover.
Pre-existing Criticals re-checked at head
7 unresolved Critical threads. My independent findings above match R1-2 (dead switch), R1-3 (per-instance limiter), R1-4 (explicit 0) and R1-5 (kebab-case) — all four still stand at head.
| Thread | Status at head |
|---|---|
R1-1 packages/core does not compile — this.wrapped.countTokens / useSummarizedThinking not on ContentGenerator |
fixed by main's drift, not by the PR — at the merge base 60161cb64a the interface has only 3 members, so the claim was correct there; current contentGenerator.ts:38-54 declares all five, so rateLimitedContentGenerator.ts:68-78 type-checks against main. Re-verify after the required rebase. |
| Duplicate "fresh limiter per call" thread (marked outdated) | superseded by R1-3, still stands |
| R1-2 / R1-3 / R1-4 / R1-5 | still stand (evidence above) |
Given CONFLICTING plus a dead switch on the PR's only user-facing control, the next step is a rebase onto main and adding requestConcurrency to MODEL_GENERATION_CONFIG_FIELDS, before another review round.
Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.
Closes #3409.
What
Adds a per-provider request-concurrency cap so users hitting
Error: 429 Too many concurrent requests for this modelcan translate the upstream rate limit into client-side back-pressure instead of an exception. Excess callers (sub-agents fanning out,/compressinterleaving with the main turn, parallel tool calls) queue FIFO behind a semaphore.Why
The current behavior is an exception.
maxRetriesonly retries the same 429 — it doesn't gate concurrency, so under sub-agent load the same request fires again and gets the same 429 back. The user explicitly asked: ?How can I set concurrent requests limit in my config?? ? this PR is that knob.Wiring
modelProviders[].generationConfig.requestConcurrencyQWEN_REQUEST_CONCURRENCYenv var0/ negative)Implementation
packages/core/src/utils/concurrencyLimiter.ts? minimal FIFO counting semaphore.acquire/releaseplus arunExclusivehelper. Release is idempotent so accidental double-release does not over-credit.capacity <= 0short-circuits to a no-op.packages/core/src/core/rateLimitedContentGenerator.ts? wrapper implementingContentGenerator. GatesgenerateContentandgenerateContentStream; intentionally does not gatecountTokens/embedContent(cheap, often local). For streaming the slot is held until the consumer fully drains the iterator (or it errors, throws during init, or unwinds early viabreak/return) ? what the upstream actually rate-limits is concurrent open connections, not request initiation.packages/core/src/core/contentGenerator.ts?createContentGeneratorappliesRateLimitedContentGeneratorbeforeLoggingContentGeneratorso rate-limit wait time is visible inside the logged request span (instead of as a phase outside it).Areas needing careful review
for awaitunwinds early (viatry/finallyinside an async generator). I added an explicit "consumer breaks early" test; the alternative (release on first yield) would let the model server count the request as still open even though we've released the slot, defeating the purpose.0/ negative all evaluate to "unlimited". This matters because theLoggingContentGeneratorwrapping check usesinstanceof RateLimitedContentGenerator? a default-false config means we can ship without breaking any existing user observability.Tests
concurrencyLimiter.test.ts? cap respected, FIFO ordering, success + failure release, double-release idempotency, NaN/Infinity coerced to unlimited.rateLimitedContentGenerator.test.ts? peak in-flight enforcement, slot release on rejection, stream slot held until drain, release on init throw, release on consumer-sidebreak, no gating forcountTokens/embedContent,useSummarizedThinkingpass-through.contentGenerator.test.ts? not wrapped by default, wrapped when configured, env fallback, config wins over env, garbage env values treated as unlimited, fractional config values floored.Testing
npx vitest runagainstpackages/core)npm run buildclean (full TypeScript build across the monorepo)docs/users/configuration/settings.md)Prepared with assistance from Claude (Anthropic) under human review.