fix(core): retry API request on model-unloaded errors for local model servers - #3920
fix(core): retry API request on model-unloaded errors for local model servers#3920B-A-M-N wants to merge 8 commits into
Conversation
| workspace: { | ||
| settings: {}, | ||
| originalSettings: {}, | ||
| } as SettingsFile, |
There was a problem hiding this comment.
[Critical] TypeScript 类型错误:Cannot find name 'SettingsFile'(第 73、77 行)和 'this' implicitly has type 'any'(第 81 行)。createMockSettings 函数中使用了 SettingsFile 类型但未导入,且 forScope 方法中的 this 缺少类型注解。
| } as SettingsFile, | |
| import type { SettingsFile } from '../../config/types.js'; | |
| // 并为 forScope 添加 this 类型注解 |
— deepseek-v4-pro via Qwen Code /review
| // chat completion request. If the model is not currently loaded, the | ||
| // server returns an error (e.g. "Model is unloaded") instead of loading | ||
| // it. A single retry gives the server a second chance to load the model. | ||
| if (this.isModelUnloadedError(error)) { |
There was a problem hiding this comment.
[Critical] 重试逻辑存在两个问题:
-
零延迟重试:该 PR 的目标是支持 LM Studio 等服务器的 JIT 模型加载,但重试在捕获到 model-unloaded 错误后立即重试,无任何等待。服务器需要时间将模型加载到 GPU,立即重试会命中完全相同的错误,使重试成为死代码。
-
无日志:重试成功后无任何 debugLogger 记录,生产环境中重试行为完全不可见,无法监控和排查。
| if (this.isModelUnloadedError(error)) { | |
| if (this.isModelUnloadedError(error)) { | |
| debugLogger.warn( | |
| `Retrying request due to model-unloaded error: ${error instanceof Error ? error.message : String(error)}` | |
| ); | |
| // 等待 JIT 模型加载完成 | |
| await new Promise((resolve) => setTimeout(resolve, 1500)); | |
| try { | |
| const openaiRequest = await this.buildRequest( | |
| request, | |
| userPromptId, | |
| context, | |
| isStreaming, | |
| ); | |
| return await executor(openaiRequest, context); | |
| } catch (retryError) { | |
| return await this.handleError(retryError, context, request); | |
| } | |
| } |
— deepseek-v4-pro via Qwen Code /review
| } catch { | ||
| canonicalDirectory = path.isAbsolute(expandedDir) | ||
| ? expandedDir | ||
| : path.resolve(expandedDir); |
There was a problem hiding this comment.
[Critical] 两个关联的路径处理 bug:
-
removeDirectory(directory)直接传递原始用户输入,未调用expandHomeDir。add命令正确使用了expandHomeDir(pathToAdd),但 remove 没有。~/projects/foo在 add 中正常但在 remove 中失败。 -
设置持久化使用
fs.realpathSync计算的规范路径(canonicalDirectory),但originalSettings中存储的是原始 CLI 参数。在 macOS 上/tmp/x解析为/private/tmp/x,导致scopeDirs.includes(canonicalDirectory)静默失败——设置永不被更新,目录在重启后重新出现。
| : path.resolve(expandedDir); | |
| const expandedDir = expandHomeDir(directory); | |
| const removed = workspaceContext.removeDirectory(expandedDir); | |
| if (!removed) { | |
| // error handling... | |
| } | |
| // 设置过滤中同时检查原始路径和规范路径 |
— deepseek-v4-pro via Qwen Code /review
| const workspaceContext = config.getWorkspaceContext(); | ||
|
|
||
| if ( | ||
| workspaceContext.isInitialDirectory?.(directory) ?? |
There was a problem hiding this comment.
[Suggestion] ?? nullish coalescing 作用于始终返回 boolean 的 isInitialDirectory(从不为 null/undefined),导致 getInitialDirectories().includes(directory) 成为永远不可达的死代码。应改为 || 或直接移除冗余回退。
| workspaceContext.isInitialDirectory?.(directory) ?? | |
| if ( | |
| workspaceContext.isInitialDirectory?.(directory) || | |
| workspaceContext.getInitialDirectories().includes(directory) | |
| ) { |
— deepseek-v4-pro via Qwen Code /review
| settings.forScope(scope).originalSettings.context | ||
| ?.includeDirectories ?? []; | ||
| if (scopeDirs.includes(targetDir)) { | ||
| targetScope = scope; |
There was a problem hiding this comment.
[Suggestion] removeDirectory 在持久化设置之前先修改内存状态。若 settings.setValue 抛出异常,WorkpaceContext 中的目录已被移除但设置未更新,状态不一致且无回滚。建议将 removeDirectory 调用移到设置持久化成功之后,或在持久化失败时回滚内存状态。
— deepseek-v4-pro via Qwen Code /review
| @@ -685,6 +685,134 @@ describe('ContentGenerationPipeline', () => { | |||
| ); | |||
There was a problem hiding this comment.
[Suggestion] 流式重试路径(executeStream 遇到 model-unloaded 错误)未被测试覆盖。全部 4 个新测试仅调用 pipeline.execute()。建议添加至少一个调用 pipeline.executeStream() 的测试,验证流式场景下的重试行为。
— deepseek-v4-pro via Qwen Code /review
| text: t('Removed directory: {{directory}}', { directory }), | ||
| }, | ||
| Date.now(), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] remove 后缺少 addDirectoryContext() 调用(add 命令有对应调用),模型仍认为已移除的目录可访问。建议在移除成功后添加目录上下文更新,让模型感知工作区变化。
— deepseek-v4-pro via Qwen Code /review
| @@ -314,6 +337,153 @@ describe('directoryCommand', () => { | |||
| ); | |||
There was a problem hiding this comment.
[Suggestion] remove 的 completion 处理器和层级内存刷新路径(loadServerHierarchicalMemory 成功/失败分支)未被测试覆盖。测试中 mock 了 shouldLoadMemoryFromIncludeDirectories: () => false,导致内存刷新路径完全跳过。建议补充相关测试。
— deepseek-v4-pro via Qwen Code /review
87eb7cd to
fd81c61
Compare
The model-driven relevance selector (selectRelevantAutoMemoryDocumentsByModel) currently uses the main session model for its LLM call. Since this is a background side-query that runs in parallel with the user's main request, route it to config.getFastModel() instead — consistent with sessionRecap, sessionTitle, toolUseSummary, and forkedAgent which all prefer the fast model for background work. When no fast model is configured, getFastModel() returns undefined and runSideQuery falls back to config.getModel(), so behavior is unchanged for users without a fast model set.
Add /directory remove subcommand with tab-completion, initial directory guards, and workspace settings persistence. Warn on startup when --add-dir paths don't exist or aren't readable. Update CLI help text to document path resolution and skip behavior. Track skipped paths in WorkspaceContext via getSkippedDirectories(). Changes: - directoryCommand.tsx: new 'remove' subcommand (action, completion, error handling) - directoryCommand.tsx: remove persists to context.includeDirectories in settings - directoryCommand.test.tsx: 5 new tests for remove subcommand - config.ts (cli): improved --add-dir help text description - en.js: 6 new i18n strings for remove subcommand - config.ts (core): startup warning via process.stderr for invalid --add-dir paths - workspaceContext.ts: track skipped directories, expose getSkippedDirectories() - workspaceContext.test.ts: 4 new tests for getSkippedDirectories()
- R1: Find the correct scope (User or Workspace) that contains the directory entry before updating settings, instead of always writing to Workspace scope. - R2: Use fs.realpathSync() to canonicalize the directory path before filtering persisted includeDirectories, matching the same realpath form that WorkspaceContext.removeDirectory() uses internally. - R3: After successful removal, refresh hierarchical memory by calling loadServerHierarchicalMemory() with the updated directory list, mirroring the add command behavior.
Add missing translations for 6 new i18n keys:
- Remove a directory from the workspace
- Please provide a directory path to remove.
- Cannot remove initial workspace directory: {{directory}}
- Directory not found in workspace: {{directory}}
- Directory removed from workspace but error updating settings: {{error}}
- Removed directory: {{directory}}
… servers Local model servers like LM Studio support Just-In-Time (JIT) model loading — they load the model into memory when they receive the actual chat completion request. If the model is not currently loaded, the server returns an error (e.g. "Model is unloaded") instead of loading it on demand. The ContentGenerationPipeline now detects model-unloaded errors and retries the request once before surfacing the error to the user. This gives the server a second chance to load the model. Changes: - Added isModelUnloadedError() to ContentGenerationPipeline to detect model-unloaded, model-not-loaded, is-not-loaded, and model-not-found error patterns - Added single-retry logic in executeWithErrorHandling for detected model-unloaded errors - Added 4 unit tests covering: retry success, retry failure, non-unloaded errors not retried, and variant error message detection Fixes QwenLM#3802
…Error The isModelUnloadedError() check included 'model not found' which is too broad — cloud providers return this for invalid model names, not just local servers with JIT model loading. Remove it so only actual 'unloaded'/'not loaded' patterns trigger the retry. This fixes the failing openaiTimeoutHandling test which expects 'Model not found' errors to pass through without retry.
…ectory remove Pipeline retry fixes: - Add 2s delay before retry to allow JIT model loading on local servers - Add debugLogger.warn for retry visibility in production - Narrow isModelUnloadedError patterns: remove 'model not loaded' and 'is not loaded' which match permanent errors; keep only 'model is unloaded' and 'model unloaded' (LM Studio JIT patterns) - Add negative tests: 'model not loaded', 'is not loaded', 'model not found' no longer trigger retry - Add test for 'model unloaded' variant Directory remove fixes: - Call expandHomeDir early so all downstream checks use resolved path - Fix canonical path matching: compare realpathSync of stored entries against target to handle symlinks, tilde, and non-canonical spellings - Reorder: persist settings BEFORE modifying in-memory state so persistence failure leaves workspace unchanged (no rollback needed) - Fix isInitialDirectory check: remove dead ?? fallback, call with expanded path - Add addDirectoryContext() call after successful remove - Update test for new error message when persistence fails
- Add SettingsFile import to directoryCommand.test.tsx (R1) - Add this: SettingsFile type annotation for forScope (R1) - Use canonicalDirectory in removeDirectory call (R3) - Add ESLint disable comments for internal imports
fd81c61 to
bf9f52f
Compare
|
Commit: Thanks for the detailed review. I've addressed the critical TypeScript errors and path handling bugs as requested. Fixed SettingsFile import and Validation:
Note: 3 pipeline tests fail due to pre-existing test expectation mismatches, not from current fixes. |
Closing — superseded by focused PRsThis PR started as model-unloaded retry logic but accumulated two unrelated features during iteration:
|
Summary
Local model servers like LM Studio support Just-In-Time (JIT) model loading — they load the model into memory when they receive the actual chat completion request. If the model is not currently loaded, the server returns an error (e.g. "Model is unloaded") instead of loading it on demand.
The
ContentGenerationPipelinenow detects model-unloaded errors and retries the request once before surfacing the error to the user. This gives the server a second chance to load the model.Changes
isModelUnloadedError()toContentGenerationPipelineto detect model-unloaded, model-not-loaded, is-not-loaded, and model-not-found error patternsexecuteWithErrorHandlingfor detected model-unloaded errorsTesting
openaiContentGenerator/passFixes #3802