fix(coding-agent): propagated cancellation from lsp reload instead of false restart
reloadServer caught every error from both fallback mechanisms in bare catch blocks, so a caller cancel or tool timeout was swallowed and fell through to `proc.kill(); return "Restarted"` -- reporting a successful restart while killing the server with no replacement. - Propagate ToolAbortError/timeout from both the rust-analyzer request and the didChangeConfiguration notification fallback. - Gate the fallback on genuine method-not-found via isMethodNotFoundError instead of any error. - Replace the blind proc.kill with shutdownClientInstance: remove the client from the registry by identity and await confirmed process exit, surfacing a truthful teardown error when the process outlives the kill. Fixes #6369
This commit is contained in:
roboomp
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user