fix(core): parse API timeout env strictly - #5602
Conversation
|
@qwen-code /triage |
✅ Verification — strict
|
| Piece | Detail |
|---|---|
| Build | head fd61a982 → npm install + npm run bundle → real dist/cli.js |
| Probe | drive the real exported resolveModelConfig via tsx across the env-value matrix |
| E2E | real dist/cli.js -p … --approval-mode yolo under tmux; a mock that hangs every /v1/chat/completions and logs how long the client kept the socket open before aborting — that elapsed time is the effective OpenAI-client timeout |
1) Direct probe of resolveModelConfig — 22/22 (the real resolver)
settings.generationConfig.timeout = 30000 is the lower-priority fallback; QWEN_CODE_API_TIMEOUT_MS is varied:
| Env value | Resolved config.timeout |
Source |
|---|---|---|
300000, 45000 |
the value | env ✅ |
300000 (whitespace) |
300000 (trimmed) |
env ✅ |
1.5e5 |
30000 (settings) |
not env ✅ — old Number() = 150000 |
0x2BF20 |
30000 (settings) |
not env ✅ — old Number() = 180000 |
12345.67 |
30000 (settings) |
not env ✅ — old Math.floor = 12345 |
99999999999999999999 (unsafe int) |
30000 (settings) |
not env ✅ — old = 1e20 |
3000.5, +300000, 300_000 |
30000 (settings) |
not env ✅ |
-100, 0, not-a-number, ``, |
30000 (settings) |
not env ✅ (unchanged) |
Precedence preserved: modelProvider 60000 wins over env 900000 (source: modelProviders); a valid env applies only when modelProvider sets no timeout; a malformed env is ignored even then.
2) PR's own suite — modelConfigResolver.test.ts: 51/51 passed
3) End-to-end — the resolved timeout reaches the real OpenAI client
A mock hangs the chat request; the client aborts at exactly the effective timeout. Same dist/cli.js, only QWEN_CODE_API_TIMEOUT_MS differs:
CASE A env=3000 (valid) → client ABORTED after 2996ms ← valid value honored, reaches the client
CASE B env=3000.5 (FIX) → NO abort within 11s ← ignored → falls back to the 120000ms default
CASE C env=3000.5 (no-fix*) → client ABORTED after 2994ms ← old Number()→Math.floor coerced it to 3000
* CASE C is a counterfactual: I reverted just the parse to the old Number()/Math.floor coercion, rebuilt core + rebundled, and re-ran the same 3000.5 input. The B-vs-C contrast — measured at the real client's abort time — is decisive: with the fix a malformed *_MS value no longer silently sets a (coerced) timeout; without it, 3000.5 quietly became a 3 s timeout. (CASE A confirms valid values still flow through to new OpenAI({ timeout }).)
📝 Notes for merge (non-blocking)
- Behavior change is intentional and now correct for a
*_MSvar —1.5e5/0x2BF20/12345.67/ unsafe integers are ignored (fall back to the configured/default timeout) rather than coerced; the PR documents this in Risk & Scope and it matches the stricter sharedparsePositiveIntegerEnvused by nearby knobs. - No silent breakage of normal use — plain decimal ms (
300000), whitespace trimming, and the modelProvider > env > settings precedence are all preserved (probe §1 + §3 CASE A).
Verdict (verification side): the resolver now parses QWEN_CODE_API_TIMEOUT_MS strictly (22/22 probe + 51/51 unit), and the resolved value demonstrably drives the real OpenAI client's request timeout — with a malformed value correctly falling back instead of being coerced. Looks merge-ready from here; final call is the maintainers'.
🇨🇳 中文版(点击展开)
✅ 验证 —— QWEN_CODE_API_TIMEOUT_MS 严格解析,在 resolver 与真实请求上均确认
我在 fd61a982 上本地(Linux)验证了该修复(fixes #5596)。三层结果一致,并且我比 PR 自身范围多走一步:跑了一次真实端到端请求,确认解析后的 timeout 确实到达 OpenAI 客户端(PR 自述「未运行端到端模型请求」)。
方法
| 部分 | 细节 |
|---|---|
| 构建 | head fd61a982 → npm install + npm run bundle → 真实 dist/cli.js |
| 探针 | 用 tsx 驱动真实导出的 resolveModelConfig,跨 env 取值矩阵 |
| E2E | tmux 下真实 dist/cli.js -p … --approval-mode yolo;mock 挂起所有 /v1/chat/completions,并记录客户端在中止前保持 socket 多久 —— 这个耗时即生效的 OpenAI 客户端 timeout |
1) 直接探针打 resolveModelConfig —— 22/22(真实 resolver)
以 settings.generationConfig.timeout = 30000 作为低优先级回退;变化 QWEN_CODE_API_TIMEOUT_MS:
| Env 值 | 解析得到的 config.timeout |
来源 |
|---|---|---|
300000、45000 |
原值 | env ✅ |
300000 (空白) |
300000(已 trim) |
env ✅ |
1.5e5 |
30000(settings) |
非 env ✅ —— 旧 Number() = 150000 |
0x2BF20 |
30000(settings) |
非 env ✅ —— 旧 Number() = 180000 |
12345.67 |
30000(settings) |
非 env ✅ —— 旧 Math.floor = 12345 |
99999999999999999999(不安全整数) |
30000(settings) |
非 env ✅ —— 旧 = 1e20 |
3000.5、+300000、300_000 |
30000(settings) |
非 env ✅ |
-100、0、not-a-number、``、 |
30000(settings) |
非 env ✅(行为不变) |
优先级保留:modelProvider 60000 胜过 env 900000(source: modelProviders);仅当 modelProvider 未设 timeout 时合法 env 才生效;此时 malformed env 仍被忽略。
2) PR 自带套件 —— modelConfigResolver.test.ts:51/51 通过
3) 端到端 —— 解析后的 timeout 到达真实 OpenAI 客户端
mock 挂起 chat 请求;客户端会恰好在生效 timeout 处中止。同一 dist/cli.js,仅 QWEN_CODE_API_TIMEOUT_MS 不同:
CASE A env=3000 (合法) → 客户端 ABORTED 于 2996ms ← 合法值被采用,到达客户端
CASE B env=3000.5 (修复) → 11s 内无中止 ← 被忽略 → 回退到 120000ms 默认
CASE C env=3000.5 (无修复*) → 客户端 ABORTED 于 2994ms ← 旧 Number()→Math.floor 把它转成 3000
* CASE C 是反事实:我只把解析回退为旧的 Number()/Math.floor,重编 core + 重打包,再用相同的 3000.5 跑一次。B 与 C 的对比 —— 在真实客户端中止时刻测得 —— 是决定性的:有修复时,malformed 的 *_MS 不再静默设置(转换后的)timeout;无修复时,3000.5 悄悄变成了 3 秒 timeout。(CASE A 确认合法值仍会流入 new OpenAI({ timeout })。)
📝 合并参考(非阻塞)
- 行为变更是有意的,且对
*_MS变量更正确 ——1.5e5/0x2BF20/12345.67/ 不安全整数被忽略(回退到已配置/默认 timeout)而非转换;PR 已在 Risk & Scope 说明,且与附近配置项采用的更严格的共享parsePositiveIntegerEnv一致。 - 不破坏正常用法 —— 纯十进制毫秒(
300000)、空白 trim、以及 modelProvider > env > settings 的优先级都保留(探针 §1 + §3 CASE A)。
结论(验证视角): resolver 现在严格解析 QWEN_CODE_API_TIMEOUT_MS(探针 22/22 + 单测 51/51),且解析结果确实驱动真实 OpenAI 客户端的请求 timeout —— malformed 值正确回退而非被转换。从这里看具备合并条件;最终决定权在维护者。
|
Thanks for the PR, @tt-a1i — and good catch on the silent coercion bug. Template ✓ all required headings present, bilingual body, risk/scope disclosed. Direction — this is squarely in scope. Approach — minimal and clean. The whole change is: swap One observation (non-blocking): Moving on to code review + testing. 🔍 中文说明感谢 @tt-a1i 的 PR —— 也感谢你揪出了这个静默强制转换的 bug。 模板 ✓ 所有必需标题齐全、双语正文、风险/范围都已披露。 方向 —— 完全在范围内。 方案 —— 最小且干净。全部改动:把 一个观察(非阻塞): 进入代码审查 + 测试。🔍 — Qwen Code · qwen3.7-max |
2a. Code reviewIndependently, before reading the diff I would have: (1) swapped the The PR does exactly that. No surprises. Critical blockers: none. The new code path ( Reuse check: pass. The helper is reused, not duplicated. Pre-existing sibling Clear AGENTS.md violations: none. ESM imports, strict typing, test collocated with source, file naming ( One tiny stylistic observation (non-blocking): the new test block names the 2b. Real-scenario testingDrove the real exported Unit tests also run from the worktree via
Before (installed build / old parser,
|
|
Stepping back: this is exactly the kind of fix I want to see land. The bug is real (silently coercing My independent proposal from Stage 2a was "reuse The pre-existing near-duplicate Merge-ready from my side. Approving. ✅ 中文说明退一步看:这正是我希望看到合并的那类修复。bug 真实存在(把 我在 Stage 2a 的独立方案是"复用
从我这边看可以合并。批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
This PR makes
QWEN_CODE_API_TIMEOUT_MSuse the same strict positive-integer env parsing rule as the newer shared env parser. Decimal positive integer values still override the lower-priority timeout, while malformed values are ignored.It also updates the timeout env override tests so fractional, scientific-notation, hex, and unsafe-integer values fall back to the existing settings timeout instead of being coerced.
Why it's needed
The previous parser used JavaScript number coercion, so values like
1.5e5,0x2BF20, and12345.67were accepted as valid millisecond timeouts. That is surprising for a*_MSenv var and inconsistent with the stricter env parsing now used by nearby configuration knobs.Keeping timeout env parsing strict avoids silently changing user intent while preserving the current precedence order: model provider config wins, then env, then settings/defaults.
Reviewer Test Plan
How to verify
Reviewers can confirm that
QWEN_CODE_API_TIMEOUT_MSaccepts decimal positive integers such as300000, still trims surrounding whitespace, and ignores malformed values such as1.5e5,0x2BF20,12345.67, and unsafe integers.Commands run locally:
(cd packages/core && npx vitest run src/models/modelConfigResolver.test.ts) npx prettier --check packages/core/src/models/modelConfigResolver.ts packages/core/src/models/modelConfigResolver.test.ts npx eslint packages/core/src/models/modelConfigResolver.ts packages/core/src/models/modelConfigResolver.test.ts npm run build --workspace @qwen-code/qwen-code-core npm run typecheck --workspace @qwen-code/qwen-code-core git diff --checkEvidence (Before & After)
Before:
QWEN_CODE_API_TIMEOUT_MS=1.5e5,QWEN_CODE_API_TIMEOUT_MS=0x2BF20, andQWEN_CODE_API_TIMEOUT_MS=12345.67were accepted/coerced as env timeouts.After: those malformed values are ignored and the lower-priority configured timeout is preserved. This is covered by the updated
modelConfigResolverunit tests.No UI/TUI evidence is attached because this is a config parsing change covered by unit tests.
Tested on
Environment (optional)
Local Node/npm workspace on macOS.
npm installcompleted successfully before running the focused validation commands.Risk & Scope
QWEN_CODE_API_TIMEOUT_MSvalues are now ignored instead of coerced.Linked Issues
Fixes #5596
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
这个 PR 让
QWEN_CODE_API_TIMEOUT_MS使用和较新的共享 env parser 一致的严格正整数解析规则。十进制正整数仍然会覆盖低优先级 timeout,格式不合法的值会被忽略。同时更新了 timeout env override 的测试,确保小数、科学计数法、十六进制和不安全整数会回退到已有 settings timeout,而不是被静默转换。
Why it's needed
之前的解析逻辑使用 JavaScript number coercion,因此
1.5e5、0x2BF20、12345.67这类值都会被当成有效毫秒 timeout。对于一个*_MSenv var 来说这比较意外,也和附近配置项现在采用的严格 env 解析不一致。把 timeout env 解析收紧可以避免静默改变用户意图,同时保留当前优先级顺序:model provider 配置优先,其次是 env,然后是 settings/defaults。
Reviewer Test Plan
How to verify
Reviewer 可以确认
QWEN_CODE_API_TIMEOUT_MS仍然接受300000这样的十进制正整数,也仍然会 trim 前后空白;同时会忽略1.5e5、0x2BF20、12345.67和不安全整数。本地已运行命令:
(cd packages/core && npx vitest run src/models/modelConfigResolver.test.ts) npx prettier --check packages/core/src/models/modelConfigResolver.ts packages/core/src/models/modelConfigResolver.test.ts npx eslint packages/core/src/models/modelConfigResolver.ts packages/core/src/models/modelConfigResolver.test.ts npm run build --workspace @qwen-code/qwen-code-core npm run typecheck --workspace @qwen-code/qwen-code-core git diff --checkEvidence (Before & After)
Before:
QWEN_CODE_API_TIMEOUT_MS=1.5e5、QWEN_CODE_API_TIMEOUT_MS=0x2BF20和QWEN_CODE_API_TIMEOUT_MS=12345.67会被接受/转换成 env timeout。After:这些格式不合法的值会被忽略,并保留低优先级的已配置 timeout。这个行为已由更新后的
modelConfigResolver单元测试覆盖。这里没有附 UI/TUI 证据,因为这是配置解析变更,已由单元测试覆盖。
Tested on
Environment (optional)
本地 macOS Node/npm workspace。运行 focused validation commands 前,
npm install已成功完成。Risk & Scope
QWEN_CODE_API_TIMEOUT_MS现在会被忽略,而不是被转换。Linked Issues
Fixes #5596
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.