feat(core): QWEN_STREAM_IDLE_TIMEOUT_MS env knob for the stream idle timeout - #5845
Merged
Conversation
…idle timeout The streaming inactivity timeout was only programmatically configurable via ContentGeneratorConfig.streamIdleTimeoutMs (default 120s). Add a deployment knob so a daemon deployment can tune it without code, the same way the QWEN_SERVE_* params are set. resolveStreamIdleTimeoutMs precedence: explicit config field (wins, including 0 to disable) > QWEN_STREAM_IDLE_TIMEOUT_MS env > default. A malformed env value is ignored with a debug warning rather than failing the request. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
|
Re-triage of PR #5845. Template: uses custom headings ( Direction: solid. Daemon deployments needing to tune the stream idle timeout without code changes is a legitimate ops need. The env-var knob pattern matches how daemon deployments already set Scope: tight. One resolver function (~40 lines), two constants, one constructor-level resolution replacing an inline expression. Every line serves the stated goal. 336 additions are almost entirely tests (11 new test cases covering all edge paths) plus the minimal implementation. No drive-by refactors, no scope creep. Approach: env precedence (config → env → default) follows an established pattern in the codebase. The validation choices — strict decimal integer, JS timer ceiling ( Moving on to code review and tests. 🔍 中文说明重新审查 PR #5845。 模板：使用了自定义标题（ 方向：合理。Daemon 部署需要在不改代码的情况下调整流空闲超时——这是正当的运维需求。环境变量旋钮模式与 范围：紧凑。一个解析函数（约40行）、两个常量、构造函数级别的一次性解析替换内联表达式。每一行都服务于目标。336行新增几乎全是测试（11个新测试用例覆盖所有边界）加最小实现。无顺手重构，无范围蔓延。 方案：环境变量优先级（config → env → default）与代码库已有模式一致。校验选择——严格十进制整数、JS 计时器上限（ 进入代码审查和测试。🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal (before reading the diff): I'd add a The PR matches this proposal almost exactly — same function location, same precedence, same validation approach (with one improvement: strict decimal-only regex rejecting hex/scientific notation, which I hadn't considered but is clearly better). Correctness: clean. Config Reuse check: the env-reading pattern follows Style: the code is comment-heavy relative to the project's "default to none" convention, but every comment explains a why (timer ceiling overflow behavior, regex strictness rationale, cascade semantics). Appropriate for infrastructure code with subtle failure modes. Unit TestsAll 80 tests pass (69 pre-existing + 11 new). TypeScript typecheck: clean. Real-Scenario Testing (tmux)N/A — this is a non-UI infrastructure change. The stream idle timeout fires inside the content generator pipeline during long-running streaming responses when the remote server goes silent. There's no practical way to trigger this in a tmux session against the real CLI: it requires either a remote server that stops sending chunks mid-stream, or a mock OpenAI-compatible endpoint. The unit tests with 中文说明代码审查独立方案（看 diff 之前）：在 PR 与此方案高度一致——相同位置、相同优先级、相同校验方式，且有一处改进：严格十进制正则拒绝十六进制/科学计数法，更优。 正确性：无问题。Config 复用检查：env 读取模式与同包的 风格：相对项目"默认无注释"惯例，注释偏多，但每条都在解释 why（计时器天花板溢出行为、正则严格性原因、级联语义）。对于失败模式微妙的基础设施代码，合理。 单元测试80 项全部通过（69 已有 + 11 新增）。TypeScript 类型检查：通过。 真实场景测试（tmux）N/A — 非 UI 的基础设施改动。流空闲超时在远程服务器静默时触发于内容生成管线内部，无法在 tmux 中对真实 CLI 实际触发。需要远程服务器中途停止发送 chunks 或模拟 OpenAI 兼容端点。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a well-executed, tightly-scoped feature PR. Going back to my independent proposal from Stage 2: the PR matches it and improves on it (the strict decimal-only regex for env parsing is a better design choice than a simple The code is straightforward — one function, one responsibility. The 11 new tests aren't excessive; each covers a distinct edge path in the precedence/validation cascade, and they're well-structured with clear arrange/act/assert. The resolve-once-in-constructor choice is correct: the env read and any invalid-value warning happen per pipeline, not per streaming request. The diff is entirely necessary for the stated goal. No unrelated changes, no drive-by refactors. The only non-functional addition is the After seeing the unit tests pass (80/80, including all 11 new edge cases) and the typecheck come back clean, and considering the code follows established patterns in the codebase, this looks ready to ship. Approving. ✅ 中文说明这是一个执行良好、范围紧凑的功能 PR。 回到 Stage 2 的独立方案：PR 与之匹配并有所改进（env 解析的严格十进制正则是比简单 代码直白——一个函数，一个职责。11 个新测试不算多；每个覆盖优先级/校验级联中的一条独立边界路径，结构清晰（arrange/act/assert 分明）。构造函数一次性解析的设计正确：env 读取和无效值警告在 pipeline 级别发生，而非每次流请求。 diff 中的每行改动都服务于目标。无无关变更，无顺手重构。唯一非功能性的新增是 单元测试全部通过（80/80，含 11 个新边界用例），类型检查通过，代码遵循代码库已有模式——可以合并。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Audit follow-ups on QWEN_STREAM_IDLE_TIMEOUT_MS: - Reject values above the JS timer ceiling (2147483647 ms): setTimeout silently compresses larger delays to 1ms, which would make the watchdog trip almost immediately and abort every streaming request. Oversized → default + warning. - Resolve the timeout once in the pipeline constructor instead of per streaming request, so the env read and any invalid-value warning happen once per pipeline rather than on every model call. Adds a test for the oversized-env case. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…lt is used The malformed/oversized env tests only advanced 3000ms and asserted "not tripped" — under fake timers that passes even if the bad value were used (fake timers schedule at the literal delay, with no Node overflow-to-1ms). They now advance to the default and assert the watchdog trips there, which distinguishes "default used" from "bad value scheduled far away". Verified the oversized test fails when the upper bound is removed. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
- Strict decimal-integer env parsing: reject hex ("0x10")/scientific ("1e3")/
float/signed QWEN_STREAM_IDLE_TIMEOUT_MS via /^\d+$/, so a typo can't silently
become a surprising timeout (matches utils/env.ts).
- Validate the explicit config field too: an out-of-range value (above the JS
timer ceiling) would overflow setTimeout to a near-immediate fire; reject it
and fall back instead.
- Test isolation: clear any ambient QWEN_STREAM_IDLE_TIMEOUT_MS in beforeEach so
the default-timeout tests aren't silently overridden by the dev/CI shell.
Adds tests for the non-decimal env and out-of-range config cases (both verified
to fail when their guard is removed).
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
Jun 25, 2026
wenshao
requested changes
Jun 25, 2026
Resolves all 4 review comments on PR #5845: 1. [Critical] Negative config: restore the old `<= 0` disable contract. The resolver now accepts any integer up to the timer ceiling; negatives pass through and the downstream `idleMs > 0` guard skips the watchdog. This fixes a behavioral regression where negative values (previously a valid way to disable) silently became a 120s timeout. 2. [Suggestion] Add a test for `QWEN_STREAM_IDLE_TIMEOUT_MS=0` proving the watchdog is disabled via the env path (regression guard against a regex tightening to `[1-9]\d*`). Also add a test for negative config. 3. [Suggestion] Env-in-pipeline vs config-assembly: acknowledged as an intentional scoping decision — the resolver lives at the layer that enforces the timeout, matching how the sibling `timeout` field works. No code change. 4. [Suggestion] Switch config warnings from debugLogger.warn (off by default) to console.warn so an operator misconfiguring the env gets visible feedback. The resolve-once design means these fire once per pipeline, not per request. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
- Use QWEN_STREAM_IDLE_TIMEOUT_MS_ENV constant in all test stubEnv calls instead of hardcoding the string (comment #5). - Add config→env cascade test: invalid config + valid env → uses the env value, not the default (comment #6). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jun 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
previously approved these changes
Jun 25, 2026
…ce test - Fix JSDoc: says "debug warning" but implementation uses console.warn. - Add test for exact MAX_STREAM_IDLE_TIMEOUT_MS boundary acceptance (guards against an off-by-one changing <= to <). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Clean implementation of the QWEN_STREAM_IDLE_TIMEOUT_MS env override. The resolver's precedence (config > env > default) is correctly implemented, edge cases are well-handled (NaN/Infinity/non-integer rejected, <= 0 disables the watchdog, MAX ceiling guards against setTimeout compression), and the test suite is comprehensive — covering the env path, precedence, malformed values, oversized values, hex rejection, boundary acceptance, config→env cascade, and both disable paths. The single remaining open thread (stray ) typo in a comment) was already flagged by a prior review.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 25, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to the streaming inactivity timeout (#5827). That timeout was only programmatically configurable via
ContentGeneratorConfig.streamIdleTimeoutMs(default 120s) — not settable by a deployment. This adds an env override so a daemon deployment can tune it without code, the same way theQWEN_SERVE_*params are set.Why an env (not settings.json, not a
QWEN_SERVE_*serve param)pipeline.ts), where the field lives and where the openai layer already reads env (constants.ts). AQWEN_SERVE_*serve param would be the wrong layer (serve turn/writer bounds) and need cross-layer plumbing, and would be daemon-only.settings.jsonfield is a poor fit (and the siblingcontentGenerator.timeoutisn't in settings either). The sibling field is programmatic-only with a default; this mirrors it and adds an ops env knob.How
resolveStreamIdleTimeoutMs(config)precedence:ContentGeneratorConfig.streamIdleTimeoutMs(wins — including0to disable)QWEN_STREAM_IDLE_TIMEOUT_MSenv (non-negative integer ms)DEFAULT_STREAM_IDLE_TIMEOUT_MS= 120000)A malformed env value is ignored with a debug warning rather than failing the request.
Tests
3 new tests in
pipeline.test.ts(fake timers +vi.stubEnv): env value is honored when no config is set; an explicit config value takes precedence over the env; a malformed env value is ignored and the default applies. Full pipeline suite: 73/73; core typecheck clean.🤖 Generated with Qwen Code