diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ad5f374bd..5fbf4826b 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator. +- Fixed a cancelled `lsp reload` reporting `Restarted ` while killing the server with no replacement: `reloadServer` swallowed `ToolAbortError`/tool timeout in both fallback `catch` blocks and fell through to `proc.kill()`. Cancellation and timeout now propagate; the rust-analyzer fallback only triggers on genuine method-not-found; and a wedged-connection teardown removes the client by identity and awaits confirmed process exit before claiming a restart (surfacing a truthful teardown error if the process outlives the kill). ([#6369](https://github.com/can1357/oh-my-pi/issues/6369)) - Fixed Ctrl-clicking a wrapped OAuth authorization URL opening only the clicked row's truncated fragment by preserving the complete hyperlink target on every rendered row. - Fixed used-only absolute usage amounts across output surfaces: CLI now renders `$123.45 used`; the TUI shows a neutral, width-bounded amount instead of a pending/dotted/account-count placeholder; and ACP preserves `123.45 usd used` while suppressing duplicate window suffixes such as `— extra`. ([#5575](https://github.com/can1357/oh-my-pi/issues/5575)) diff --git a/packages/coding-agent/src/lsp/client.ts b/packages/coding-agent/src/lsp/client.ts index d5f0f0c34..dab1e47ea 100644 --- a/packages/coding-agent/src/lsp/client.ts +++ b/packages/coding-agent/src/lsp/client.ts @@ -1181,9 +1181,19 @@ async function waitForExit(client: LspClient, timeoutMs: number): Promise { +export async function shutdownClientInstance(client: LspClient): Promise { + if (clients.get(client.name) === client) clients.delete(client.name); + const err = new Error("LSP client shutdown"); for (const pending of Array.from(client.pendingRequests.values())) { pending.reject(err); @@ -1196,21 +1206,23 @@ async function shutdownClientInstance(client: LspClient): Promise { ); if (shutdownCompleted) { await sendNotification(client, "exit", undefined).catch(() => {}); - if (await waitForExit(client, EXIT_TIMEOUT_MS)) return; + if (await waitForExit(client, EXIT_TIMEOUT_MS)) return true; } client.proc.kill(); - await waitForExit(client, EXIT_TIMEOUT_MS); + return await waitForExit(client, EXIT_TIMEOUT_MS); } /** * Shutdown a specific client by key. + * + * @returns `true` when the client is gone (already absent or confirmed exited), + * `false` if a live process outlived the shutdown budget. */ -export async function shutdownClient(key: string): Promise { +export async function shutdownClient(key: string): Promise { const client = clients.get(key); - if (!client) return; - clients.delete(key); - await shutdownClientInstance(client); + if (!client) return true; + return await shutdownClientInstance(client); } // ============================================================================= diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 12dc7dd9b..d1683620f 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -29,6 +29,7 @@ import { sendNotification, sendRequest, setIdleTimeout, + shutdownClientInstance, supportsDocumentDiagnostics, syncContent, WARMUP_TIMEOUT_MS, @@ -507,12 +508,18 @@ function isMethodNotFoundError(err: unknown): boolean { } async function reloadServer(client: LspClient, serverName: string, signal?: AbortSignal): Promise { - // rust-analyzer exposes a real reload request. + throwIfAborted(signal); + // rust-analyzer exposes a real reload request. Every other server rejects it + // with method-not-found — that alone justifies the generic fallback. A caller + // cancel or tool timeout must propagate, never be mistaken for an unsupported + // method and swallowed into a bogus "Restarted" (issue #6369). try { await sendRequest(client, "rust-analyzer/reloadWorkspace", null, signal); return `Reloaded ${serverName}`; - } catch { - // Method not supported — fall through. + } catch (err) { + throwIfAborted(signal); + if (!isMethodNotFoundError(err)) throw err; + // Method not supported — fall through to the generic reload. } // workspace/didChangeConfiguration is a notification per spec; sending it // as a request hangs until the tool deadline on servers that route it to @@ -520,8 +527,16 @@ async function reloadServer(client: LspClient, serverName: string, signal?: Abor try { await sendNotification(client, "workspace/didChangeConfiguration", { settings: {} }, signal); return `Reloaded ${serverName}`; - } catch { - client.proc.kill(); + } catch (err) { + throwIfAborted(signal); + // The reload notification could not be delivered — the connection is + // wedged or the process already died. Tear the client down (removing it + // from the registry by identity and awaiting confirmed process exit) so + // the next request cold-starts a fresh client. A kill that never confirms + // exit is not a restart: surface the teardown failure truthfully. + if (!(await shutdownClientInstance(client))) { + throw new Error(`Failed to restart ${serverName}: server process did not exit after kill`); + } return `Restarted ${serverName}`; } } diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index bf06804f4..44ed62bd9 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -44,6 +44,7 @@ import { } from "@oh-my-pi/pi-coding-agent/lsp/utils"; import { getThemeByName } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { ToolAbortError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; import { clampTimeout } from "@oh-my-pi/pi-coding-agent/tools/tool-timeouts"; import * as piUtils from "@oh-my-pi/pi-utils"; import { sanitizeText, TempDir } from "@oh-my-pi/pi-utils"; @@ -86,7 +87,7 @@ type FakeLspHandler = (message: RpcMessage, server: FakeLspServer) => void | Pro // no real-clock latency. Installed by spying on the shared `ptree` namespace // object (NOT `mock.module`, which would leak across files); the suite's // `afterEach` `vi.restoreAllMocks()` removes it. -function installFakeLsp(handler: FakeLspHandler): FakeLspServer { +function installFakeLsp(handler: FakeLspHandler, options?: { killResolvesExit?: boolean }): FakeLspServer { const encoder = new TextEncoder(); const received: RpcMessage[] = []; const waiters: Array<{ @@ -191,7 +192,7 @@ function installFakeLsp(handler: FakeLspHandler): FakeLspServer { peekStderr: () => "", kill() { killed = true; - server.exit(0); + if (options?.killResolvesExit !== false) server.exit(0); }, } as unknown as LspClient["proc"]; @@ -2772,6 +2773,163 @@ describe("lsp regressions", () => { } }); + describe("reload cancellation and truthful teardown (#6369)", () => { + // A JSON-RPC error response the client maps to isMethodNotFoundError, so a + // non-rust server falls through from `rust-analyzer/reloadWorkspace` to the + // generic `workspace/didChangeConfiguration` reload. + const methodNotFound = (id: RpcMessage["id"]): RpcMessage => ({ + jsonrpc: "2.0", + id, + error: { code: -32_601, message: "method not found" }, + }); + + it("propagates cancellation of the reload request instead of reporting Restarted", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-cancel-req-"); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + // rust-analyzer/reloadWorkspace is left pending: only the caller + // signal decides its fate. + }); + const config: ServerConfig = { command: "fake-reload-cancel-req", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const controller = new AbortController(); + const pending = tool.execute("reload-cancel-req", { action: "reload", file: "*" }, controller.signal); + await server.waitFor(m => m.method === "rust-analyzer/reloadWorkspace"); + controller.abort(new ToolAbortError()); + + // Pre-fix, the bare `catch` swallowed the abort, fell through to the + // notification (also aborted), hit the second bare `catch`, killed + // the process, and returned "Restarted". + await expect(pending).rejects.toBeInstanceOf(ToolAbortError); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("propagates cancellation that arrives during the notification fallback", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-cancel-fallback-"); + const controller = new AbortController(); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "rust-analyzer/reloadWorkspace") { + // Fall through to the generic reload, then cancel before it lands. + srv.send(methodNotFound(message.id)); + controller.abort(new ToolAbortError()); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-reload-cancel-fb", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const pending = tool.execute("reload-cancel-fb", { action: "reload", file: "*" }, controller.signal); + + await expect(pending).rejects.toBeInstanceOf(ToolAbortError); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("still falls back to the generic reload on method-not-found without killing the server", async () => { + const tempDir = TempDir.createSync("@omp-lsp-reload-fallback-ok-"); + try { + const server = installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "rust-analyzer/reloadWorkspace") { + srv.send(methodNotFound(message.id)); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-reload-fallback", fileTypes: [".ts"], rootMarkers: [] }; + vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: { fake: config }, idleTimeoutMs: undefined }); + + const tool = new LspTool(makeLspSession(tempDir.path())); + const result = await tool.execute("reload-fallback", { action: "reload", file: "*" }); + + expect(textResult(result)).toContain("Reloaded fake"); + expect(server.killed).toBe(false); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("shutdownClientInstance removes the client by identity and confirms process exit", async () => { + const tempDir = TempDir.createSync("@omp-lsp-teardown-confirm-"); + try { + installFakeLsp((message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } else if (message.method === "exit") { + srv.exit(0); + } + }); + const config: ServerConfig = { command: "fake-teardown-confirm", fileTypes: [".ts"], rootMarkers: [] }; + const client = await lspClient.getOrCreateClient(config, tempDir.path()); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(true); + + const exited = await lspClient.shutdownClientInstance(client); + expect(exited).toBe(true); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(false); + } finally { + vi.restoreAllMocks(); + await lspClient.shutdownAll(); + tempDir.removeSync(); + } + }); + + it("shutdownClientInstance reports a failed teardown when the process outlives the kill", async () => { + const tempDir = TempDir.createSync("@omp-lsp-teardown-delayed-"); + try { + installFakeLsp( + (message, srv) => { + if (message.method === "initialize") { + srv.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } }); + } else if (message.method === "shutdown") { + srv.send({ jsonrpc: "2.0", id: message.id, result: null }); + } + // `exit` notification and `kill()` never resolve `proc.exited`. + }, + { killResolvesExit: false }, + ); + const config: ServerConfig = { command: "fake-teardown-delayed", fileTypes: [".ts"], rootMarkers: [] }; + const client = await lspClient.getOrCreateClient(config, tempDir.path()); + + const exited = await lspClient.shutdownClientInstance(client); + expect(exited).toBe(false); + expect(lspClient.getActiveClients().some(s => s.name === config.command)).toBe(false); + } finally { + vi.restoreAllMocks(); + tempDir.removeSync(); + } + }, 15_000); + }); + // #3962 — LSP cold-start and notification writes must honor the tool's // combined timeout/caller abort signal. Before the fix, a wedged server // hung past the tool's advertised deadline: `initialize` fell back to the