-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): support per-provider stream idle timeout #9795
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ describe('buildAgentContentGeneratorConfig', () => { | |
| samplingParams: { temperature: 0.7, top_p: 0.9 }, | ||
| reasoning: { effort: 'high' as const }, | ||
| timeout: 30000, | ||
| streamIdleTimeoutMs: 300000, | ||
| maxRetries: 3, | ||
| contextWindowSize: 128000, | ||
| extra_body: { custom: 'value' }, | ||
|
|
@@ -68,6 +69,7 @@ describe('buildAgentContentGeneratorConfig', () => { | |
| expect(result.samplingParams).toEqual({ temperature: 0.7, top_p: 0.9 }); | ||
| expect(result.reasoning).toEqual({ effort: 'high' }); | ||
| expect(result.timeout).toBe(30000); | ||
| expect(result.streamIdleTimeoutMs).toBe(300000); | ||
| expect(result.maxRetries).toBe(3); | ||
| expect(result.contextWindowSize).toBe(128000); | ||
| expect(result.extra_body).toEqual({ custom: 'value' }); | ||
|
|
@@ -101,6 +103,7 @@ describe('buildAgentContentGeneratorConfig', () => { | |
| expect(result.samplingParams).toBeUndefined(); | ||
| expect(result.reasoning).toBeUndefined(); | ||
| expect(result.timeout).toBeUndefined(); | ||
| expect(result.streamIdleTimeoutMs).toBeUndefined(); | ||
| expect(result.maxRetries).toBeUndefined(); | ||
| expect(result.contextWindowSize).toBeUndefined(); | ||
| expect(result.extra_body).toBeUndefined(); | ||
|
|
@@ -152,6 +155,7 @@ describe('buildAgentContentGeneratorConfig', () => { | |
| envKey: 'REGISTRY_API_KEY', | ||
| generationConfig: { | ||
| samplingParams: { temperature: 0.5 }, | ||
| streamIdleTimeoutMs: 600000, | ||
| contextWindowSize: 200000, | ||
|
Comment on lines
157
to
159
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-2: The registry-overlay path in Evidence (mutation probe in an isolated scratch tree): baseline 18/18; with the mutation, 153/153 passed across Suggested fix — pin the falsy branch with a sibling case in the it('should apply a falsy registry-resolved streamIdleTimeoutMs', () => {
const config = createMockConfig(parentConfig, {
...resolvedModel,
generationConfig: {
...resolvedModel.generationConfig,
streamIdleTimeoutMs: 0,
},
});
const result = buildAgentContentGeneratorConfig(
config,
'registry-model-id',
{ authType: 'anthropic' },
);
expect(result.streamIdleTimeoutMs).toBe(0);
});中文说明
证据(在隔离 scratch tree 中进行的变异探针):基线 18/18;应用变异后, 建议修复 —— 在 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||
| reasoning: { effort: 'medium' as const }, | ||
| }, | ||
|
|
@@ -182,6 +186,7 @@ describe('buildAgentContentGeneratorConfig', () => { | |
| expect(result.apiKeyEnvKey).toBe('REGISTRY_API_KEY'); | ||
| // Registry generation config applied | ||
| expect(result.samplingParams).toEqual({ temperature: 0.5 }); | ||
| expect(result.streamIdleTimeoutMs).toBe(600000); | ||
| expect(result.contextWindowSize).toBe(200000); | ||
| expect(result.reasoning).toEqual({ effort: 'medium' }); | ||
| // Fields not in registry stay cleared (cross-provider) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-1: The settings knob this PR adds takes precedence over
QWEN_STREAM_IDLE_TIMEOUT_MS(perresolveStreamGuardMsinpackages/core/src/core/openaiContentGenerator/pipeline.tsand the docs added here), but theStreamInactivityTimeoutErrormessage in that same file (lines 192-196) still tells users to set the env var. That advice is silently ineffective whenever the timeout came from settings/modelProviders: an operator who setsmodel.generationConfig.streamIdleTimeoutMs: 300000(the exact configuration the new docs recommend) and then hits a long SSE silence gets "Set QWEN_STREAM_IDLE_TIMEOUT_MS to increase this window (or 0 to disable it)" on every retry attempt; exporting that env var (even to0) changes nothing because the explicit config field wins, and nothing hints that a settings value is overriding it. In a daemon deployment this turns a one-step fix into an open-ended debugging session.Evidence: at the reviewed commit,
npx vitest run src/core/openaiContentGenerator/pipeline.test.ts -t "stream is silent past the idle timeout|explicit streamIdleTimeoutMs config take precedence"inpackages/core→Tests 2 passed | 158 skipped (160)— one test asserts the env-only advice string fires from an explicit-config timeout, and the other proves config beats env (env stubbed1000vs config5000: the guard does not trip at 1000 ms and trips at 5000 ms).Fix location:
packages/core/src/core/openaiContentGenerator/pipeline.ts(~line 192) — name the settings knobs in the message, or make it source-aware (the pipeline resolvesconfig.streamIdleTimeoutMsin its constructor and knows whether it was explicit):中文说明
本 PR 新增的设置项优先级高于
QWEN_STREAM_IDLE_TIMEOUT_MS(见packages/core/src/core/openaiContentGenerator/pipeline.ts中的resolveStreamGuardMs以及本 PR 新增的文档),但同一文件中的StreamInactivityTimeoutError错误信息(第 192-196 行)仍然只提示用户设置环境变量。当超时值来自 settings/modelProviders 时,该提示会静默失效:运维人员按新文档推荐设置model.generationConfig.streamIdleTimeoutMs: 300000后,若遇到较长的 SSE 静默,每次重试都会看到 "Set QWEN_STREAM_IDLE_TIMEOUT_MS to increase this window (or 0 to disable it)";此时导出该环境变量(即使设为0)也不会有任何效果,因为显式配置优先于环境变量,而错误信息中没有任何线索表明有设置值在覆盖它。在守护进程部署场景下,这会把一步就能解决的问题变成无期限的排查。证据:在被审提交上运行
npx vitest run src/core/openaiContentGenerator/pipeline.test.ts -t "stream is silent past the idle timeout|explicit streamIdleTimeoutMs config take precedence"(位于packages/core)→Tests 2 passed | 158 skipped (160)—— 其中一个测试断言仅提及环境变量的提示文案会在显式配置超时场景下出现,另一个测试证明配置优先于环境变量(env 设为1000、配置为5000:守卫不在 1000 ms 触发,而在 5000 ms 触发)。修复位置:
packages/core/src/core/openaiContentGenerator/pipeline.ts(约第 192 行)—— 在错误信息中同时提及设置项,或让提示感知来源(pipeline 在构造函数中解析config.streamIdleTimeoutMs,知道该值是否为显式配置),参考上方代码块。— qwen3.8-max via Qwen Code /review (v0.22.0)