Merge PR #7042: fix(lsp): handle quick exits before reader teardown (@roboomp)
This commit is contained in:
can1357
@@ -28,6 +28,7 @@
|
||||
- Reduced bash, grep, and glob tool guidance while preserving supported internal-URL routes and making shell/eval boundaries and broad-search timeout avoidance explicit.
|
||||
- Fixed the spawn-based models config validator laziness test flaking under CPU contention by giving its two sequential probe processes an explicit 60s per-test timeout instead of sharing bun's 5s default ([#7018](https://github.com/can1357/oh-my-pi/issues/7018)).
|
||||
- Coalesced the models configuration resource probe into one child process to avoid startup contention while preserving retained-resource coverage.
|
||||
- Fixed quick LSP server exits being misreported as reader failures and explicit reloads being blocked by the initialization backoff ([#7041](https://github.com/can1357/oh-my-pi/issues/7041)).
|
||||
|
||||
## [17.1.8] - 2026-07-28
|
||||
|
||||
|
||||
@@ -27,6 +27,7 @@ const fileOperationLocks = new Map<string, Promise<void>>();
|
||||
/** Negative cache of recent init failures so a broken server fails fast instead of re-spawning per call. */
|
||||
const INIT_FAILURE_BACKOFF_MS = 3 * 60 * 1000;
|
||||
const initFailures = new Map<string, { at: number; message: string }>();
|
||||
const READER_EXIT_GRACE_MS = 100;
|
||||
|
||||
// Idle timeout configuration (disabled by default)
|
||||
let idleTimeoutMs: number | null = null;
|
||||
@@ -303,6 +304,7 @@ async function startMessageReader(client: LspClient): Promise<void> {
|
||||
|
||||
const framer = new MessageFramer(Buffer.from(client.messageBuffer));
|
||||
|
||||
let readerFailed = false;
|
||||
try {
|
||||
while (true) {
|
||||
const { done, value } = await reader.read();
|
||||
@@ -391,6 +393,7 @@ async function startMessageReader(client: LspClient): Promise<void> {
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
readerFailed = true;
|
||||
// Connection closed or error - reject all pending requests
|
||||
for (const pending of Array.from(client.pendingRequests.values())) {
|
||||
pending.reject(new Error(`LSP connection closed: ${err}`));
|
||||
@@ -401,6 +404,9 @@ async function startMessageReader(client: LspClient): Promise<void> {
|
||||
client.messageBuffer = framer.remainder();
|
||||
reader.releaseLock();
|
||||
client.isReading = false;
|
||||
if (!readerFailed && client.proc.exitCode === null) {
|
||||
await waitForExit(client, READER_EXIT_GRACE_MS);
|
||||
}
|
||||
// Reader exited while the server process is still alive (unrecoverable
|
||||
// read error or bad stream state): nothing will route responses anymore,
|
||||
// so tear the client down — the next call respawns instead of timing out.
|
||||
@@ -676,6 +682,15 @@ const PROJECT_LOAD_TIMEOUT_MS = 15_000;
|
||||
const SHUTDOWN_TIMEOUT_MS = 5_000;
|
||||
const EXIT_TIMEOUT_MS = 1_000;
|
||||
|
||||
function clientKey(config: ServerConfig, cwd: string): string {
|
||||
return `${config.command}:${cwd}`;
|
||||
}
|
||||
|
||||
/** Allow an explicit user reload to retry a matching initialization failure immediately. */
|
||||
export function clearInitializationFailure(config: ServerConfig, cwd: string): void {
|
||||
initFailures.delete(clientKey(config, cwd));
|
||||
}
|
||||
|
||||
/**
|
||||
* Get or create an LSP client for the given server configuration and working directory.
|
||||
* @param config - Server configuration
|
||||
@@ -692,7 +707,7 @@ export async function getOrCreateClient(
|
||||
initTimeoutMs?: number,
|
||||
signal?: AbortSignal,
|
||||
): Promise<LspClient> {
|
||||
const key = `${config.command}:${cwd}`;
|
||||
const key = clientKey(config, cwd);
|
||||
|
||||
// Check if client already exists
|
||||
const existingClient = clients.get(key);
|
||||
@@ -865,13 +880,13 @@ export async function getActiveOrPendingClient(
|
||||
signal?: AbortSignal,
|
||||
): Promise<LspClient | undefined> {
|
||||
throwIfAborted(signal);
|
||||
const client = clients.get(`${config.command}:${cwd}`);
|
||||
const client = clients.get(clientKey(config, cwd));
|
||||
if (client) {
|
||||
client.lastActivity = Date.now();
|
||||
return client;
|
||||
}
|
||||
|
||||
const pending = clientLocks.get(`${config.command}:${cwd}`);
|
||||
const pending = clientLocks.get(clientKey(config, cwd));
|
||||
if (!pending) return undefined;
|
||||
try {
|
||||
return await untilAborted(signal, pending);
|
||||
|
||||
@@ -17,6 +17,7 @@ import { formatPathRelativeToCwd, resolveToCwd } from "../tools/path-utils";
|
||||
import { ToolAbortError, ToolError, throwIfAborted } from "../tools/tool-errors";
|
||||
import { clampTimeout } from "../tools/tool-timeouts";
|
||||
import {
|
||||
clearInitializationFailure,
|
||||
ensureFileOpen,
|
||||
FileChangeType,
|
||||
getActiveClients,
|
||||
@@ -2345,6 +2346,7 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
|
||||
const outputs: string[] = [];
|
||||
for (const [workspaceServerName, workspaceServerConfig] of servers) {
|
||||
throwIfAborted(signal);
|
||||
clearInitializationFailure(workspaceServerConfig, this.session.cwd);
|
||||
try {
|
||||
const workspaceClient = await getOrCreateClient(
|
||||
workspaceServerConfig,
|
||||
@@ -2377,6 +2379,8 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
|
||||
|
||||
const [serverName, serverConfig] = serverInfo;
|
||||
|
||||
if (action === "reload") clearInitializationFailure(serverConfig, this.session.cwd);
|
||||
|
||||
try {
|
||||
const client = await getOrCreateClient(serverConfig, this.session.cwd, undefined, signal);
|
||||
const targetFile = resolvedFile;
|
||||
|
||||
@@ -73,6 +73,8 @@ interface FakeLspServer {
|
||||
send(message: RpcMessage): void;
|
||||
/** Resolve the process `exited` promise and close stdout. */
|
||||
exit(code?: number): void;
|
||||
/** Fail stdout without exiting the process. */
|
||||
failStdout(error: Error): void;
|
||||
/** Whether the client invoked `proc.kill()` (production's hard-kill fallback). */
|
||||
readonly killed: boolean;
|
||||
/** Resolve once a received message matches `predicate` (already-seen or future). */
|
||||
@@ -81,13 +83,19 @@ interface FakeLspServer {
|
||||
|
||||
type FakeLspHandler = (message: RpcMessage, server: FakeLspServer) => void | Promise<void>;
|
||||
|
||||
interface FakeLspOptions {
|
||||
killResolvesExit?: boolean;
|
||||
stderr?: string;
|
||||
stdoutClosesBeforeExit?: boolean;
|
||||
}
|
||||
|
||||
// In-memory LSP transport fake. Replaces the real subprocess (`ptree.spawn`)
|
||||
// with an in-process JSON-RPC peer so the initialize / shutdown / exit and
|
||||
// workspace-folder handshakes resolve deterministically -- no subprocess spawn,
|
||||
// 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, options?: { killResolvesExit?: boolean }): FakeLspServer {
|
||||
function installFakeLsp(handler: FakeLspHandler, options?: FakeLspOptions): FakeLspServer {
|
||||
const encoder = new TextEncoder();
|
||||
const received: RpcMessage[] = [];
|
||||
const waiters: Array<{
|
||||
@@ -97,6 +105,7 @@ function installFakeLsp(handler: FakeLspHandler, options?: { killResolvesExit?:
|
||||
}> = [];
|
||||
let exitCode: number | null = null;
|
||||
let killed = false;
|
||||
let stdoutStopped = false;
|
||||
let controller: ReadableStreamDefaultController<Uint8Array> | null = null;
|
||||
const { promise: exited, resolve: resolveExited } = Promise.withResolvers<number>();
|
||||
|
||||
@@ -114,14 +123,29 @@ function installFakeLsp(handler: FakeLspHandler, options?: { killResolvesExit?:
|
||||
const server: FakeLspServer = {
|
||||
received,
|
||||
send(message) {
|
||||
if (controller && exitCode === null) controller.enqueue(frame(message));
|
||||
if (controller && exitCode === null && !stdoutStopped) controller.enqueue(frame(message));
|
||||
},
|
||||
exit(code = 0) {
|
||||
if (exitCode !== null) return;
|
||||
if (!stdoutStopped) {
|
||||
stdoutStopped = true;
|
||||
controller?.close();
|
||||
}
|
||||
if (options?.stdoutClosesBeforeExit) {
|
||||
queueMicrotask(() => {
|
||||
exitCode = code;
|
||||
resolveExited(code);
|
||||
});
|
||||
return;
|
||||
}
|
||||
exitCode = code;
|
||||
controller?.close();
|
||||
resolveExited(code);
|
||||
},
|
||||
failStdout(error) {
|
||||
if (stdoutStopped) return;
|
||||
stdoutStopped = true;
|
||||
controller?.error(error);
|
||||
},
|
||||
get killed() {
|
||||
return killed;
|
||||
},
|
||||
@@ -189,7 +213,7 @@ function installFakeLsp(handler: FakeLspHandler, options?: { killResolvesExit?:
|
||||
end: async () => 0,
|
||||
},
|
||||
stdout,
|
||||
peekStderr: () => "",
|
||||
peekStderr: () => options?.stderr ?? "",
|
||||
kill() {
|
||||
killed = true;
|
||||
if (options?.killResolvesExit !== false) server.exit(0);
|
||||
@@ -3010,6 +3034,102 @@ describe("lsp regressions", () => {
|
||||
}
|
||||
}, 15_000);
|
||||
});
|
||||
describe("reader exit ordering and initialization backoff (#7041)", () => {
|
||||
it("surfaces the process diagnostic when stdout closes before exit publication", async () => {
|
||||
installFakeLsp(
|
||||
(message, server) => {
|
||||
if (message.method === "initialize") server.exit(23);
|
||||
},
|
||||
{
|
||||
stdoutClosesBeforeExit: true,
|
||||
stderr: "simulated rust-analyzer crash",
|
||||
},
|
||||
);
|
||||
const tempDir = TempDir.createSync("@omp-lsp-quick-exit-");
|
||||
try {
|
||||
const config: ServerConfig = {
|
||||
command: "fake-lsp-quick-exit",
|
||||
fileTypes: [".rs"],
|
||||
rootMarkers: [],
|
||||
};
|
||||
|
||||
await expect(lspClient.getOrCreateClient(config, tempDir.path())).rejects.toThrow(
|
||||
"LSP server exited (code 23): simulated rust-analyzer crash",
|
||||
);
|
||||
} finally {
|
||||
await lspClient.shutdownAll();
|
||||
tempDir.removeSync();
|
||||
}
|
||||
});
|
||||
|
||||
it("kills and evicts a client whose stdout reader fails while the process is alive", async () => {
|
||||
const server = installFakeLsp((message, fake) => {
|
||||
if (message.method === "initialize") fake.failStdout(new Error("simulated reader failure"));
|
||||
});
|
||||
const tempDir = TempDir.createSync("@omp-lsp-reader-failure-");
|
||||
try {
|
||||
const config: ServerConfig = {
|
||||
command: "fake-lsp-reader-failure",
|
||||
fileTypes: [".ts"],
|
||||
rootMarkers: [],
|
||||
};
|
||||
|
||||
await expect(lspClient.getOrCreateClient(config, tempDir.path())).rejects.toThrow(
|
||||
"LSP connection closed: Error: simulated reader failure",
|
||||
);
|
||||
expect(server.killed).toBe(true);
|
||||
expect(lspClient.getActiveClients().some(client => client.name === config.command)).toBe(false);
|
||||
} finally {
|
||||
await lspClient.shutdownAll();
|
||||
tempDir.removeSync();
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps ordinary backoff but lets explicit reload retry immediately", async () => {
|
||||
installFakeLsp((message, server) => {
|
||||
if (message.method === "initialize") server.exit(23);
|
||||
});
|
||||
const tempDir = TempDir.createSync("@omp-lsp-reload-init-failure-");
|
||||
const config: ServerConfig = {
|
||||
command: "fake-lsp-reload-init-failure",
|
||||
fileTypes: [".ts"],
|
||||
rootMarkers: [],
|
||||
};
|
||||
try {
|
||||
await expect(lspClient.getOrCreateClient(config, tempDir.path())).rejects.toBeInstanceOf(Error);
|
||||
await expect(lspClient.getOrCreateClient(config, tempDir.path())).rejects.toThrow(
|
||||
"failed to initialize recently",
|
||||
);
|
||||
|
||||
vi.restoreAllMocks();
|
||||
const retryServer = installFakeLsp((message, server) => {
|
||||
if (message.method === "initialize") {
|
||||
server.send({ jsonrpc: "2.0", id: message.id, result: { capabilities: {} } });
|
||||
} else if (message.method === "rust-analyzer/reloadWorkspace") {
|
||||
server.send({ jsonrpc: "2.0", id: message.id, result: null });
|
||||
} else if (message.method === "shutdown") {
|
||||
server.send({ jsonrpc: "2.0", id: message.id, result: null });
|
||||
} else if (message.method === "exit") {
|
||||
server.exit(0);
|
||||
}
|
||||
});
|
||||
vi.spyOn(lspConfig, "loadConfig").mockReturnValue({
|
||||
servers: { fake: config },
|
||||
idleTimeoutMs: undefined,
|
||||
});
|
||||
|
||||
const tool = new LspTool(makeLspSession(tempDir.path()));
|
||||
const result = await tool.execute("reload-init-failure", { action: "reload", file: "*" });
|
||||
|
||||
expect(textResult(result)).toContain("Reloaded fake");
|
||||
expect(retryServer.received.some(message => message.method === "initialize")).toBe(true);
|
||||
} finally {
|
||||
vi.restoreAllMocks();
|
||||
await lspClient.shutdownAll();
|
||||
tempDir.removeSync();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// #3962 — LSP cold-start and notification writes must honor the tool's
|
||||
// combined timeout/caller abort signal. Before the fix, a wedged server
|
||||
|
||||
Reference in New Issue
Block a user