Merge PR #5137: perf(coding-agent): avoid cold LSP startup on format-only writes (@wolfiesch)

This commit is contained in:
can1357
2026-07-23 11:37:13 +02:00
7 changed files with 409 additions and 13 deletions
+3
View File
@@ -99,6 +99,9 @@
### Fixed
- Fixed Portkey/gateway custom models whose ids start with `@` (e.g. `@modal/GLM-5-2-FP8`) being rewritten to unrelated bundled wire ids (e.g. `glm-5-2`), which caused `400` responses requiring `x-portkey-config` or `x-portkey-provider`.
### Changed
- Reduced format-on-write latency by avoiding cold language-server startup when diagnostics are disabled.
## [17.0.6] - 2026-07-20
+23
View File
@@ -846,6 +846,29 @@ export async function getOrCreateClient(
return clientPromise;
}
/** Return an active or already-starting client without starting a language server. */
export async function getActiveOrPendingClient(
config: ServerConfig,
cwd: string,
signal?: AbortSignal,
): Promise<LspClient | undefined> {
throwIfAborted(signal);
const client = clients.get(`${config.command}:${cwd}`);
if (client) {
client.lastActivity = Date.now();
return client;
}
const pending = clientLocks.get(`${config.command}:${cwd}`);
if (!pending) return undefined;
try {
return await untilAborted(signal, pending);
} catch {
throwIfAborted(signal);
return undefined;
}
}
/**
* Ensure a file is opened in the LSP client.
* Sends didOpen notification if the file is not already tracked.
@@ -2,7 +2,7 @@
* Biome CLI-based linter client.
* Uses Biome's CLI with JSON output instead of LSP (which has stale diagnostics issues).
*/
import path from "node:path";
import * as path from "node:path";
import { logger } from "@oh-my-pi/pi-utils";
import type { Diagnostic, DiagnosticSeverity, LinterClient, ServerConfig } from "../../lsp/types";
@@ -141,14 +141,13 @@ export class BiomeClient implements LinterClient {
) {}
async format(filePath: string, content: string): Promise<string> {
// Write content to file first
// Keep the standalone LinterClient contract: callers supply the content to
// format, regardless of what is currently on disk.
await Bun.write(filePath, content);
// Run biome format --write
const result = await runBiome(["format", "--write", filePath], this.cwd, this.config.resolvedCommand);
if (result.success) {
// Read back formatted content
return await Bun.file(filePath).text();
}
+25 -7
View File
@@ -20,6 +20,7 @@ import {
ensureFileOpen,
FileChangeType,
getActiveClients,
getActiveOrPendingClient,
getOrCreateClient,
type LspServerStatus,
notifySaved,
@@ -211,6 +212,7 @@ async function syncFileContent(
cwd: string,
servers: Array<[string, ServerConfig]>,
signal?: AbortSignal,
createMissing = true,
): Promise<void> {
throwIfAborted(signal);
await Promise.allSettled(
@@ -219,11 +221,15 @@ async function syncFileContent(
if (serverConfig.createClient) {
return;
}
const client = await getOrCreateClient(serverConfig, cwd, undefined, signal);
const client = createMissing
? await getOrCreateClient(serverConfig, cwd, undefined, signal)
: await getActiveOrPendingClient(serverConfig, cwd, signal);
if (!client) return;
throwIfAborted(signal);
await syncContent(client, absolutePath, content, signal);
}),
);
throwIfAborted(signal);
}
/**
@@ -239,6 +245,7 @@ async function notifyFileSaved(
cwd: string,
servers: Array<[string, ServerConfig]>,
signal?: AbortSignal,
createMissing = true,
): Promise<void> {
throwIfAborted(signal);
await Promise.allSettled(
@@ -247,10 +254,14 @@ async function notifyFileSaved(
if (serverConfig.createClient) {
return;
}
const client = await getOrCreateClient(serverConfig, cwd, undefined, signal);
const client = createMissing
? await getOrCreateClient(serverConfig, cwd, undefined, signal)
: await getActiveOrPendingClient(serverConfig, cwd, signal);
if (!client) return;
await notifySaved(client, absolutePath, signal);
}),
);
throwIfAborted(signal);
}
// Cache config per cwd to avoid repeated file I/O
@@ -1333,7 +1344,8 @@ async function runLspWritethrough(
// Capture diagnostic versions BEFORE syncing to detect stale diagnostics
// Bound client creation by the writethrough budget: a hung/broken server
// must not add its full init wait (30s default) to every edit.
const minVersions = enableDiagnostics ? await captureDiagnosticVersions(cwd, servers, 5_000, signal) : undefined;
const minVersionsPromise = enableDiagnostics ? captureDiagnosticVersions(cwd, servers, 5_000, signal) : undefined;
let minVersions = useCustomFormatter ? undefined : await minVersionsPromise;
let expectedDocumentVersions: ServerVersionMap | undefined;
let formatter: FileFormatResult | undefined;
@@ -1353,13 +1365,19 @@ async function runLspWritethrough(
operationSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal;
await untilAborted(operationSignal, async () => {
if (useCustomFormatter) {
// Custom linters (e.g. Biome CLI) require on-disk input.
// Custom linters operate on on-disk input; the shared pre-write also
// supports implementations that inspect the file before formatting.
await writeContent(content);
finalContent = await formatContent(dst, content, cwd, customLinterServers, operationSignal);
const [formattedContent, capturedVersions] = await Promise.all([
formatContent(dst, content, cwd, customLinterServers, operationSignal),
minVersionsPromise,
]);
finalContent = formattedContent;
minVersions = capturedVersions;
formatter = finalContent !== content ? FileFormatResult.FORMATTED : FileFormatResult.UNCHANGED;
await writeContent(finalContent);
await notifyWriteCommitted(operationSignal);
await syncFileContent(dst, finalContent, cwd, lspServers, operationSignal);
await syncFileContent(dst, finalContent, cwd, lspServers, operationSignal, enableDiagnostics);
} else {
// 1. Sync original content to LSP servers
await syncFileContent(dst, content, cwd, lspServers, operationSignal);
@@ -1385,7 +1403,7 @@ async function runLspWritethrough(
}
// 5. Notify saved to LSP servers
await notifyFileSaved(dst, cwd, lspServers, operationSignal);
await notifyFileSaved(dst, cwd, lspServers, operationSignal, !useCustomFormatter || enableDiagnostics);
});
synced = true;
} catch {
@@ -0,0 +1,132 @@
import { afterEach, describe, expect, test } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import { BiomeClient } from "../src/lsp/clients/biome-client";
import type { ServerConfig } from "../src/lsp/types";
const tempDirs: string[] = [];
const tempRoots: string[] = [];
const repoRoot = path.resolve(import.meta.dir, "../../..");
function resolveRepoBiome(): string {
const platformPackages: Partial<Record<NodeJS.Platform, Partial<Record<NodeJS.Architecture, string[]>>>> = {
darwin: { arm64: ["cli-darwin-arm64"], x64: ["cli-darwin-x64"] },
linux: {
arm64: ["cli-linux-arm64", "cli-linux-arm64-musl"],
x64: ["cli-linux-x64", "cli-linux-x64-musl"],
},
win32: { arm64: ["cli-win32-arm64"], x64: ["cli-win32-x64"] },
};
const executable = process.platform === "win32" ? "biome.exe" : "biome";
for (const packageName of platformPackages[process.platform]?.[process.arch] ?? []) {
try {
return Bun.resolveSync(`@biomejs/${packageName}/${executable}`, repoRoot);
} catch {}
}
throw new Error(`No repository Biome binary for ${process.platform}/${process.arch}`);
}
const repoBiome = resolveRepoBiome();
afterEach(async () => {
await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { force: true, recursive: true })));
await Promise.all(tempRoots.splice(0).map(dir => fs.rm(dir, { force: true, recursive: true })));
});
async function makeTempDir(): Promise<string> {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-biome-client-test-"));
tempDirs.push(dir);
return dir;
}
async function createFakeBiomeCommand(
tempDir: string,
expectedInput: string,
formattedOutput: string,
): Promise<string> {
const command = path.join(tempDir, "biome");
const expectedInputPath = path.join(tempDir, "expected-input.ts");
const formattedOutputPath = path.join(tempDir, "formatted-output.ts");
await Bun.write(expectedInputPath, expectedInput);
await Bun.write(formattedOutputPath, formattedOutput);
await Bun.write(
command,
`#!/bin/sh
test "$1" = "format" || exit 7
test "$2" = "--write" || exit 8
test "$3" = "${path.join(tempDir, "example.ts")}" || exit 10
cmp -s "$3" "${expectedInputPath}" || exit 9
cp "${formattedOutputPath}" "$3"
exit 0
`,
);
await fs.chmod(command, 0o755);
return command;
}
function biomeConfig(command: string): ServerConfig {
return {
command: "biome",
fileTypes: [".ts"],
rootMarkers: [],
resolvedCommand: command,
};
}
describe("BiomeClient format", () => {
test("formats the supplied content instead of stale on-disk content", async () => {
const tempDir = await makeTempDir();
const targetFile = path.join(tempDir, "example.ts");
const unformatted = "export const value:number=1\n";
const formatted = "export const value: number = 1;\n";
await Bun.write(targetFile, "export const stale = true;\n");
const command = await createFakeBiomeCommand(tempDir, unformatted, formatted);
const result = await new BiomeClient(biomeConfig(command), tempDir).format(targetFile, unformatted);
expect(result).toBe(formatted);
expect(await Bun.file(targetFile).text()).toBe(formatted);
});
test("formats configured TypeScript with the repository Biome", async () => {
const scratchDir = await fs.mkdtemp(
path.join(repoRoot, "packages", "coding-agent", "src", "__biome_client_test__-"),
);
tempDirs.push(scratchDir);
const targetFile = path.join(scratchDir, "configured.ts");
const unformatted = "export const configured:number=1\n";
await Bun.write(targetFile, unformatted);
const result = await new BiomeClient(biomeConfig(repoBiome), repoRoot).format(targetFile, unformatted);
expect(result).toBe("export const configured: number = 1;\n");
});
test("leaves config-excluded content unchanged with the repository Biome", async () => {
const excludedRoot = path.join(repoRoot, ".perf");
const createdRoot = await fs.mkdir(excludedRoot, { recursive: true });
if (createdRoot) tempRoots.push(excludedRoot);
const scratchDir = await fs.mkdtemp(path.join(excludedRoot, "biome-client-test-"));
tempDirs.push(scratchDir);
const targetFile = path.join(scratchDir, "excluded.ts");
const unformatted = "export const excluded:number=1\n";
await Bun.write(targetFile, unformatted);
const result = await new BiomeClient(biomeConfig(repoBiome), repoRoot).format(targetFile, unformatted);
expect(result).toBe(unformatted);
});
test("returns the original content when Biome fails", async () => {
const tempDir = await makeTempDir();
const command = path.join(tempDir, "biome-failure");
await Bun.write(command, "#!/bin/sh\ncat >/dev/null\nexit 1\n");
await fs.chmod(command, 0o755);
const targetFile = path.join(tempDir, "example.ts");
const content = "export const value = 1;\n";
const result = await new BiomeClient(biomeConfig(command), tempDir).format(targetFile, content);
expect(result).toBe(content);
});
});
@@ -1,10 +1,10 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test";
import * as path from "node:path";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { createLspWritethrough, type FileDiagnosticsResult } from "@oh-my-pi/pi-coding-agent/lsp";
import { createLspWritethrough, type FileDiagnosticsResult, FileFormatResult } from "@oh-my-pi/pi-coding-agent/lsp";
import * as lspClient from "@oh-my-pi/pi-coding-agent/lsp/client";
import * as lspConfig from "@oh-my-pi/pi-coding-agent/lsp/config";
import type { Diagnostic, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types";
import type { Diagnostic, LinterClient, LspClient, ServerConfig } from "@oh-my-pi/pi-coding-agent/lsp/types";
import { fileToUri } from "@oh-my-pi/pi-coding-agent/lsp/utils";
import type { DeferredDiagnosticsEntry, ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
import { WriteTool } from "@oh-my-pi/pi-coding-agent/tools/write";
@@ -16,6 +16,19 @@ const TEST_SERVER: ServerConfig = {
rootMarkers: [],
};
function createFormatter(format: (filePath: string, content: string) => Promise<string>): ServerConfig {
return {
command: "test-formatter",
fileTypes: ["ts"],
rootMarkers: [],
createClient: () =>
({
format,
lint: async () => [],
}) satisfies LinterClient,
};
}
function createDiagnostic(message: string): Diagnostic {
return {
message,
@@ -163,6 +176,137 @@ describe("LSP diagnostics freshness", () => {
expect(getOrCreate).not.toHaveBeenCalled();
});
it("does not cold-start an LSP server for custom formatting when diagnostics are disabled", async () => {
const filePath = path.join(tempDir.path(), "formatted.ts");
const formatter = createFormatter(async () => "export const value = 1;\n");
vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined });
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([
["test-lsp", TEST_SERVER],
["formatter", formatter],
]);
const getOrCreate = vi
.spyOn(lspClient, "getOrCreateClient")
.mockRejectedValue(new Error("format-only writes must not cold-start an LSP server"));
vi.spyOn(lspClient, "getActiveOrPendingClient").mockResolvedValue(undefined);
const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue();
const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue();
vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue();
const writethrough = createLspWritethrough(tempDir.path(), {
enableFormat: true,
enableDiagnostics: false,
});
const result = await writethrough(filePath, "export const value=1\n");
expect(result?.formatter).toBe(FileFormatResult.FORMATTED);
expect(await Bun.file(filePath).text()).toBe("export const value = 1;\n");
expect(getOrCreate).not.toHaveBeenCalled();
expect(sync).not.toHaveBeenCalled();
expect(notifySaved).not.toHaveBeenCalled();
});
it("keeps an already-running LSP client synchronized after custom formatting", async () => {
const filePath = path.join(tempDir.path(), "formatted.ts");
const client = createClient(tempDir.path(), TEST_SERVER);
const formatter = createFormatter(async () => "export const value = 1;\n");
vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined });
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([
["test-lsp", TEST_SERVER],
["formatter", formatter],
]);
const getOrCreate = vi.spyOn(lspClient, "getOrCreateClient");
vi.spyOn(lspClient, "getActiveOrPendingClient").mockResolvedValue(client);
const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue();
const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue();
vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue();
const writethrough = createLspWritethrough(tempDir.path(), {
enableFormat: true,
enableDiagnostics: false,
});
await writethrough(filePath, "export const value=1\n");
expect(getOrCreate).not.toHaveBeenCalled();
expect(sync).toHaveBeenCalledWith(client, filePath, "export const value = 1;\n", expect.any(AbortSignal));
expect(notifySaved).toHaveBeenCalledWith(client, filePath, expect.any(AbortSignal));
});
it("waits for an already-starting LSP client without cold-starting another one", async () => {
const filePath = path.join(tempDir.path(), "formatted.ts");
const client = createClient(tempDir.path(), TEST_SERVER);
const pendingClient = Promise.withResolvers<LspClient | undefined>();
const formatter = createFormatter(async () => "export const value = 1;\n");
vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined });
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([
["test-lsp", TEST_SERVER],
["formatter", formatter],
]);
const getOrCreate = vi.spyOn(lspClient, "getOrCreateClient");
const getActiveOrPending = vi
.spyOn(lspClient, "getActiveOrPendingClient")
.mockImplementation(async () => pendingClient.promise);
const sync = vi.spyOn(lspClient, "syncContent").mockResolvedValue();
const notifySaved = vi.spyOn(lspClient, "notifySaved").mockResolvedValue();
vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue();
const writethrough = createLspWritethrough(tempDir.path(), {
enableFormat: true,
enableDiagnostics: false,
});
const resultPromise = writethrough(filePath, "export const value=1\n");
await Bun.sleep(0);
expect(sync).not.toHaveBeenCalled();
pendingClient.resolve(client);
await resultPromise;
expect(getOrCreate).not.toHaveBeenCalled();
expect(getActiveOrPending).toHaveBeenNthCalledWith(1, TEST_SERVER, tempDir.path(), expect.any(AbortSignal));
expect(getActiveOrPending).toHaveBeenNthCalledWith(2, TEST_SERVER, tempDir.path(), expect.any(AbortSignal));
expect(sync).toHaveBeenCalledWith(client, filePath, "export const value = 1;\n", expect.any(AbortSignal));
expect(notifySaved).toHaveBeenCalledWith(client, filePath, expect.any(AbortSignal));
});
it("starts cold diagnostic initialization before custom formatting completes", async () => {
const filePath = path.join(tempDir.path(), "formatted.ts");
const uri = fileToUri(filePath);
const client = createClient(tempDir.path(), TEST_SERVER);
const init = Promise.withResolvers<LspClient>();
let initStarted = false;
const formatter = createFormatter(async () => {
expect(initStarted).toBe(true);
init.resolve(client);
return "export const value = 1;\n";
});
const clock = new VirtualClock(Date.now());
installVirtualTime(clock);
vi.spyOn(lspConfig, "loadConfig").mockReturnValue({ servers: {}, idleTimeoutMs: undefined });
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([
["test-lsp", TEST_SERVER],
["formatter", formatter],
]);
vi.spyOn(lspClient, "getOrCreateClient").mockImplementation(() => {
initStarted = true;
return init.promise;
});
vi.spyOn(lspClient, "syncContent").mockImplementation(async mockClient => {
mockClient.openFiles.set(uri, { version: 1, languageId: "typescript" });
});
vi.spyOn(lspClient, "notifySaved").mockImplementation(async mockClient => {
publishDiagnostics(mockClient, uri, [], 1);
});
vi.spyOn(lspClient, "notifyWorkspaceWatchedFiles").mockResolvedValue();
const writethrough = createLspWritethrough(tempDir.path(), {
enableFormat: true,
enableDiagnostics: true,
});
const result = await writethrough(filePath, "export const value=1\n");
expect(result?.formatter).toBe(FileFormatResult.FORMATTED);
expect(result?.messages).toEqual([]);
expect(await Bun.file(filePath).text()).toBe("export const value = 1;\n");
});
it("announces batched sibling writes before syncing the diagnostic target", async () => {
const stylesPath = path.join(tempDir.path(), "probe.module.scss");
const tsPath = path.join(tempDir.path(), "probe.tsx");
@@ -315,6 +315,83 @@ describe("lsp regressions", () => {
}
});
it("returns an already-starting client without creating a second client", async () => {
const tempDir = TempDir.createSync("@omp-lsp-pending-client-");
const initialize = Promise.withResolvers<void>();
try {
const server = installFakeLsp(async (message, srv) => {
if (message.method === "initialize") {
await initialize.promise;
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-pending-lsp",
fileTypes: ["ts"],
rootMarkers: [],
};
const startingClient = lspClient.getOrCreateClient(config, tempDir.path(), 1_000);
await server.waitFor(message => message.method === "initialize");
const existingClient = lspClient.getActiveOrPendingClient(config, tempDir.path());
let settled = false;
void existingClient.then(() => {
settled = true;
});
await Bun.sleep(0);
expect(settled).toBe(false);
initialize.resolve();
expect(await existingClient).toBe(await startingClient);
expect(server.received.filter(message => message.method === "initialize")).toHaveLength(1);
} finally {
initialize.resolve();
await lspClient.shutdownAll();
tempDir.removeSync();
}
});
it("stops waiting for a pending client on caller abort without cancelling its initialization", async () => {
const tempDir = TempDir.createSync("@omp-lsp-pending-abort-");
const initialize = Promise.withResolvers<void>();
try {
const server = installFakeLsp(async (message, srv) => {
if (message.method === "initialize") {
await initialize.promise;
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-abort-pending-lsp",
fileTypes: ["ts"],
rootMarkers: [],
};
const startingClient = lspClient.getOrCreateClient(config, tempDir.path(), 1_000);
await server.waitFor(message => message.method === "initialize");
const controller = new AbortController();
const waitingClient = lspClient.getActiveOrPendingClient(config, tempDir.path(), controller.signal);
controller.abort();
await expect(waitingClient).rejects.toThrow();
initialize.resolve();
expect(await startingClient).toBeDefined();
expect(server.received.filter(message => message.method === "initialize")).toHaveLength(1);
} finally {
initialize.resolve();
await lspClient.shutdownAll();
tempDir.removeSync();
}
});
it("advertises workspace folder support during LSP initialization", async () => {
const tempDir = TempDir.createSync("@omp-lsp-workspace-folders-");
try {