diff --git a/docs/memory-leak-audit.md b/docs/memory-leak-audit.md index 8b336d481..7501e35ae 100644 --- a/docs/memory-leak-audit.md +++ b/docs/memory-leak-audit.md @@ -18,6 +18,7 @@ - [x] #11 LRU 缓存键保留大 JSON — **已确认完整实现**：FileStateCache 使用 LRU 双重限制（max 100 条目 + maxSize 25MB）+ sizeCalculation，22 tests - [x] #12 QueryEngine.mutableMessages 不收缩 — **已修复**：实现 snipCompactIfNeeded（按 removedUuids 过滤）+ snipProjection（边界检测 + 视图投影），28 tests - [x] #18 Permission Polling Interval 泄漏 — **已修复**：inProcessRunner 权限响应后未调用 cleanup()，导致 setInterval 永远运行 + abort listener 挂载，6 tests +- [x] #17 LSP Opened Files Map 不收缩 — **已修复**：LSPServerManager 添加 closeAllFiles() 方法，postCompactCleanup 集成调用，compaction 后释放 openedFiles Map，5 tests ## 总览 --- @@ -567,10 +568,71 @@ if (snipResult !== undefined) { --- +## 17. LSP Opened Files Map 不收缩 + +**状态：已修复** + +**代码注释描述**：`closeFile()` 存在但未与 compact 流程集成（`LSPServerManager.ts:373-375` 显式标注为 TODO） + +### 实现位置 + +- `src/services/lsp/LSPServerManager.ts:414-428` — `closeAllFiles()` 方法 +- `src/services/compact/postCompactCleanup.ts:81-88` — 集成调用 + +### 问题详情 + +`LSPServerManager` 中的 `openedFiles: Map` 追踪所有通过 `didOpen` 打开的文件。`closeFile()` 方法存在可以发送 `didClose` 通知并清理 Map 条目，但代码注释明确标注： + +``` +NOTE: Currently available but not yet integrated with compact flow. +TODO: Integrate with compact - call closeFile() when compact removes files from context +``` + +长时间会话中，每次读取/编辑文件都会通过 `openFile()` 添加条目，但 compaction 不会清理这些条目，导致 Map 无限增长。 + +### 修复方式 + +1. **添加 `closeAllFiles()` 方法**：遍历 `openedFiles` Map，对每个文件发送 `didClose` 通知，然后清空 Map。Best-effort 错误处理。 + +```typescript +async function closeAllFiles(): Promise { + const entries = [...openedFiles.entries()] + openedFiles.clear() + for (const [fileUri, serverName] of entries) { + const server = servers.get(serverName) + if (!server || server.state !== 'running') continue + try { + await server.sendNotification('textDocument/didClose', { + textDocument: { uri: fileUri }, + }) + } catch { + // Best-effort — server may have stopped + } + } +} +``` + +2. **集成到 `postCompactCleanup`**：在 compaction 后自动调用 `closeAllFiles()`，释放所有 LSP 服务器端的文件状态。 + +```typescript +// postCompactCleanup.ts +try { + const lspManager = getLspServerManager() + if (lspManager) { + await lspManager.closeAllFiles() + } +} catch { + // LSP module may not be available in all environments +} +``` + +--- + ## 总结 ``` -确认已实现 (12): #1 图片 #2 /usage #3 进度消息 #4 空闲渲染 #5 虚拟滚动器 #6 管道输出 #7 语法加载 #8 NO_FLICKER #9 RC权限 #10 MCP缓冲区 #11 LRU缓存键 #12 snipCompact +确认已实现 (12): #1 图片 #2 /usage #3 进度消息 #4 空闲渲染 #5 虚拟滚动器 #6 管道输出 #10 MCP缓冲区 +已修复 (7): #7 语法加载 #8 NO_FLICKER #9 RC权限 #11 LRU缓存键 #12 snipCompact #17 LSP文件追踪 #18 Permission Polling ### 测试覆盖 @@ -583,7 +645,8 @@ if (snipResult !== undefined) { | #11 FileStateCache | `src/utils/__tests__/fileStateCache.test.ts` | 22 | | #7 语言注册 | `packages/color-diff-napi/src/__tests__/language-registration.test.ts` | 7 | | #18 Permission Polling | `src/hooks/__tests__/swarmPermissionPoller.test.ts` | 6 | -| **总计** | **7 个测试文件** | **78** | +| #17 LSP Opened Files | `src/services/lsp/__tests__/closeAllFiles.test.ts` | 5 | +| **总计** | **8 个测试文件** | **83** | ``` ### 需要关注的优先级 @@ -593,3 +656,4 @@ if (snipResult !== undefined) { 3. ~~**P2 — NO_FLICKER 流状态**~~ **已修复** 4. ~~**P2 — 空闲渲染循环**~~ **已确认完整** 5. ~~**P2 — Permission Polling Interval**~~ **已修复** +6. ~~**P2 — LSP Opened Files Map**~~ **已修复**：closeAllFiles() 集成到 postCompactCleanup diff --git a/src/services/compact/postCompactCleanup.ts b/src/services/compact/postCompactCleanup.ts index aa789f05b..50cbfd617 100644 --- a/src/services/compact/postCompactCleanup.ts +++ b/src/services/compact/postCompactCleanup.ts @@ -7,6 +7,7 @@ import { clearClassifierApprovals } from '../../utils/classifierApprovals.js' import { resetGetMemoryFilesCache } from '../../utils/claudemd.js' import { clearSessionMessagesCache } from '../../utils/sessionStorage.js' import { clearBetaTracingState } from '../../utils/telemetry/betaSessionTracing.js' +import { getLspServerManager } from '../../services/lsp/manager.js' import { resetMicrocompactState } from './microCompact.js' /** @@ -28,7 +29,7 @@ import { resetMicrocompactState } from './microCompact.js' * pass querySource — undefined is only safe for callers that are * genuinely main-thread-only (/compact, /clear). */ -export function runPostCompactCleanup(querySource?: QuerySource): void { +export async function runPostCompactCleanup(querySource?: QuerySource): Promise { // Subagents (agent:*) run in the same process and share module-level // state with the main thread. Only reset main-thread module-level state // (context-collapse, memory file cache) for main-thread compacts. @@ -74,4 +75,15 @@ export function runPostCompactCleanup(querySource?: QuerySource): void { ) } clearSessionMessagesCache() + // Close all LSP-tracked files so servers release state for files no longer + // in the active context after compaction. Best-effort — LSP may not be + // initialized, and closeAllFiles catches per-file errors internally. + try { + const lspManager = getLspServerManager() + if (lspManager) { + await lspManager.closeAllFiles() + } + } catch { + // LSP module may not be available in all environments + } } diff --git a/src/services/lsp/LSPServerManager.ts b/src/services/lsp/LSPServerManager.ts index cca207a06..50b8234d4 100644 --- a/src/services/lsp/LSPServerManager.ts +++ b/src/services/lsp/LSPServerManager.ts @@ -40,6 +40,8 @@ export type LSPServerManager = { closeFile(filePath: string): Promise /** Check if a file is already open on a compatible LSP server */ isFileOpen(filePath: string): boolean + /** Close all tracked open files (sends didClose for each) */ + closeAllFiles(): Promise } /** @@ -404,6 +406,27 @@ export function createLSPServerManager(): LSPServerManager { return openedFiles.has(fileUri) } + /** + * Close all tracked open files. Called after compaction to release LSP + * server state for files that are no longer in the active context. + * Sends didClose for each file and clears the tracking Map. + */ + async function closeAllFiles(): Promise { + const entries = [...openedFiles.entries()] + openedFiles.clear() + for (const [fileUri, serverName] of entries) { + const server = servers.get(serverName) + if (!server || server.state !== 'running') continue + try { + await server.sendNotification('textDocument/didClose', { + textDocument: { uri: fileUri }, + }) + } catch { + // Best-effort — server may have stopped + } + } + } + return { initialize, shutdown, @@ -415,6 +438,7 @@ export function createLSPServerManager(): LSPServerManager { changeFile, saveFile, closeFile, + closeAllFiles, isFileOpen, } } diff --git a/src/services/lsp/__tests__/closeAllFiles.test.ts b/src/services/lsp/__tests__/closeAllFiles.test.ts new file mode 100644 index 000000000..7d91dea3a --- /dev/null +++ b/src/services/lsp/__tests__/closeAllFiles.test.ts @@ -0,0 +1,137 @@ +import { describe, expect, test, mock } from 'bun:test' +import { createLSPServerManager } from '../LSPServerManager.js' + +// Mock config loading to avoid real filesystem/LSP server access +mock.module('../config.js', () => ({ + getAllLspServers: async () => ({ + servers: { + 'test-server': { + command: ['test-lsp'], + extensionToLanguage: { + '.ts': 'typescript', + '.js': 'javascript', + }, + }, + }, + }), +})) + +// Mock LSPServerInstance to avoid spawning real processes +const sendNotificationMock = mock(() => Promise.resolve()) +mock.module('../LSPServerInstance.js', () => ({ + createLSPServerInstance: (name: string, config: any) => ({ + name, + config, + state: 'running', + start: mock(async () => { + /* no-op */ + }), + stop: mock(async () => { + /* no-op */ + }), + sendRequest: mock(async () => undefined), + sendNotification: sendNotificationMock, + onRequest: mock(() => {}), + }), +})) + +// Mock log modules with side effects +mock.module('../../../utils/log.js', () => ({ + logError: mock(() => {}), +})) + +mock.module('../../../utils/debug.js', () => ({ + logForDebugging: mock(() => {}), +})) + +describe('LSPServerManager closeAllFiles', () => { + test('closeAllFiles is a no-op when no files are open', async () => { + const manager = createLSPServerManager() + await manager.initialize() + // Should not throw + await manager.closeAllFiles() + }) + + test('closeAllFiles sends didClose for each open file', async () => { + const manager = createLSPServerManager() + await manager.initialize() + + // Open some files via the public API. + // Since createLSPServerInstance is mocked with state='running', + // openFile should track them and send didOpen. + sendNotificationMock.mockClear() + await manager.openFile('/project/a.ts', 'content-a') + await manager.openFile('/project/b.js', 'content-b') + + // Verify files are tracked as open + expect(manager.isFileOpen('/project/a.ts')).toBe(true) + expect(manager.isFileOpen('/project/b.js')).toBe(true) + + // Now close all + sendNotificationMock.mockClear() + await manager.closeAllFiles() + + // didClose should have been sent for both files + expect(sendNotificationMock).toHaveBeenCalledTimes(2) + const calls = sendNotificationMock.mock.calls.map((c: any[]) => c) + const uris = calls.map((c) => (c[1] as any)?.textDocument?.uri as string) + expect(uris).toEqual( + expect.arrayContaining([ + expect.stringContaining('a.ts'), + expect.stringContaining('b.js'), + ]), + ) + + // Files should no longer be tracked + expect(manager.isFileOpen('/project/a.ts')).toBe(false) + expect(manager.isFileOpen('/project/b.js')).toBe(false) + }) + + test('closeAllFiles clears tracking even if server notification fails', async () => { + const manager = createLSPServerManager() + await manager.initialize() + + await manager.openFile('/project/x.ts', 'content-x') + expect(manager.isFileOpen('/project/x.ts')).toBe(true) + + // Make sendNotification throw + sendNotificationMock.mockRejectedValueOnce(new Error('server gone')) + + // Should not throw, and file tracking should be cleared + await manager.closeAllFiles() + expect(manager.isFileOpen('/project/x.ts')).toBe(false) + }) + + test('closeAllFiles handles double invocation gracefully', async () => { + const manager = createLSPServerManager() + await manager.initialize() + + await manager.openFile('/project/y.ts', 'content-y') + await manager.closeAllFiles() + expect(manager.isFileOpen('/project/y.ts')).toBe(false) + + // Second call should be a no-op (no files to close) + sendNotificationMock.mockClear() + await manager.closeAllFiles() + expect(sendNotificationMock).not.toHaveBeenCalled() + }) + + test('closeAllFiles skips servers that are not running', async () => { + // Create manager and manually register a server with 'stopped' state + const manager = createLSPServerManager() + await manager.initialize() + + // Open a file first (mocked server is running) + await manager.openFile('/project/z.ts', 'content-z') + expect(manager.isFileOpen('/project/z.ts')).toBe(true) + + // If we manually stop the server (simulating server crash), + // closeAllFiles should skip it gracefully. + // Since we can't easily change the mock state, we verify that + // closeAllFiles at least clears tracking regardless. + sendNotificationMock.mockClear() + await manager.closeAllFiles() + // Tracking cleared regardless of server state + expect(manager.isFileOpen('/project/z.ts')).toBe(false) + }) +})