fix(lsp): stop respawn storm on failed server start + support TypeScript 7 native server - #1134
fix(lsp): stop respawn storm on failed server start + support TypeScript 7 native server#1134cast-vytautas wants to merge 4 commits into
Conversation
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 83/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — A focused regression test file 📝 Found 2 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 83/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — A focused regression test file src/extensions/lsp/lsp-file-sync.test.ts was added. It verifies that a server that fails to start is logged once as a single-line message, is not respawned on later file operations, is retried after a new session_start, and that LSP tools surface an actionable session-scoped error. The new getRegisteredTool mock helper is also tested indirectly.
📝 Found 2 issue(s). See inline comments for details.
| // same way, and re-spawning on each file op re-prints the same error each | ||
| // time. Reset on session_start — the failure is often fixed between | ||
| // sessions (e.g. by running the package installer), so a new session | ||
| // retries. |
There was a problem hiding this comment.
failedClients is a module-level let that is reassigned in session_start and session_end, but the eager getOrCreateClient(...).catch((err) => noteClientFailure(...)) callbacks and noteClientFailure itself close over the variable binding, not the Set value. If a stale async rejection from a previous session fires after a new session has reset failedClients, the old failure is recorded in the new session's set and can incorrectly suppress that server for the rest of the new session.
💡 Suggestion: Capture the active failure set at session creation time and have noteClientFailure operate on that stable reference. For example, assign const sessionFailedClients = failedClients inside session_start and pass sessionFailedClients to the helpers, or keep the set in a session-scoped object instead of a top-level let.
|
|
||
| /** Like getOrCreateClient, but skips server+root pairs that already failed | ||
| * this session instead of re-spawning a doomed server. Throws a | ||
| * human-readable error so tool handlers surface something actionable. */ |
There was a problem hiding this comment.
startClient records a failure in failedClients but does not call noteClientFailure, so when an LSP tool is the first code path to trigger a start failure the status bar is not updated and no one-line console message is emitted. This makes the tool path inconsistent with the file-sync and eager-start paths and leaves the user without the status feedback the comment promises.
💡 Suggestion: Make startClient reuse noteClientFailure by passing the UI context into it, e.g. change the catch block to noteClientFailure(server, root, err, ui) instead of manually calling failedClients.add(clientKey(server, root)).
When an LSP server binary is on PATH but the session cwd has no resolvable TypeScript installation — fresh worktrees without node_modules, or projects on TypeScript 7 which ships no tsserver.js — typescript-language-server rejects initialize and exits. Each file operation re-spawned the server, and logging the rejected Error object dumped Bun's bundled-source code frame into the TUI every time. - Cache failed server+root pairs per session and skip re-spawn; reset on session start/shutdown so a new session retries. - Log one line (LSP file sync failed: <message>) instead of the Error object. - Surface the failure in the status bar: LSP: <server> failed to start. - Route the five lsp_* tools through a startClient guard that errors fast with an actionable message for known-failed servers. Adds regression tests for the respawn storm, eager-start failure, session-restart retry, and the tool-side guard. Closes getkimchi#1133 Co-Authored-By: Kimchi <noreply@kimchi.dev>
- Unify failure recording: startClient routes through noteClientFailure so tool-path failures get the status bar update and one-line log. - Track failure phase (start vs mid-session sync) so a crashed server reports "failed" instead of the misleading "failed to start". - Keep failure indicators when a healthy server syncs afterwards — mixed-repo status no longer erases a failed server. - Phase-aware tool guard message; tests use mock getHandler generics. Co-Authored-By: Kimchi <noreply@kimchi.dev>
19df98d to
3022a56
Compare
TypeScript 7 ships no tsserver.js — its language service is a plain LSP server inside the native binary (tsc --lsp --stdio), so typescript-language-server can never serve a TS7 workspace; it failed initialize with the classic Could not find a valid TypeScript installation error in every session. - servers.ts: recognize the native package signature (bin/tsc present, lib/tsserver.js absent) via resolveTsNativeServerPath, with workspace-flavor precedence — the workspace's own typescript package decides (classic wins even if the main repo is native), worktrees fall back to the main repo's bin. In a TS7 workspace the native server is preferred over an installed typescript-language-server and also works with no global install. - client.ts: skip the projectLoaded wait for servers that never emit $/progress startup cycles (previously a fixed 15s stall on every spawn), and add pullDiagnostics for the LSP 3.17 textDocument/diagnostic pull model since the native server has no publishDiagnostics push. - lsp.ts: tool_result sync and lsp_diagnostics pull diagnostics for pull-model servers; push flow unchanged for classic servers. Verified live against the real TS7 server: initialize, didOpen, hover, definition, documentSymbol, and pull diagnostics all work; push diagnostics confirmed absent. Co-Authored-By: Kimchi <noreply@kimchi.dev>
cast-vytautas
changed the title
- tool_result sync resolves the project root via findRoot() like the lsp_* tools do, so the failure cache and the spawned client are keyed by the same root whichever path sees the file first. Spawn suppression stays per server+root (distinct monorepo roots are distinct server processes), but the console line is now reported once per server per session (reportedFailures) — the issue's "one human-readable line per session" even in monorepos. - The failure log label reflects the phase: "LSP: <server> failed to start:" for spawn/initialize failures (including tool-initiated starts, previously mislabeled as file sync failures) and the original "LSP file sync failed:" line for mid-session sync breaks. - servers.ts: hoist the typescript-language-server entry to a named constant referenced by identity — removes the non-null `as` cast on SERVERS.find() and the repeated name string across detection branches. Co-Authored-By: Kimchi <noreply@kimchi.dev>
1 participant
Linked issue
Closes #1133
What does this PR do?
Part 1 — quiet, session-scoped failure handling
When a language server fails to start (e.g.
typescript-language-serverwith no resolvabletsserver.js— fresh worktrees withoutnode_modules, or projects on TypeScript 7 which ships none), the session previously re-spawned the doomed server on every file operation and dumped Bun's bundled-source code frame into the TUI each time.src/extensions/lsp.tsremembers failedserver+rootpairs for the session (failedClients);tool_resultand the eagersession_startregister failures and skip re-spawn. Reset onsession_start/session_shutdown, so a new session retries (the failure is often fixed between sessions, e.g. by running the package installer). Failures are recorded with a phase (startvs mid-sessionsync) so the status bar reports them accurately.LSP file sync failed: <message>) instead of logging theErrorobject, whose Bun rendering produced the code-frame dump.LSP: <server> failed to start, kept even when other servers sync fine.lsp_*tools go through astartClienthelper that fails fast with an actionable message for known-failed servers instead of re-spawning.Reproduced with a fixture matrix (main repo vs worktree × with/without resolvable
typescript, realtypescript-language-server6.0.0 + TS 5.9.3 / 7.0.2): red exactly in the no-resolvable-tsserver variants with the same error and stack frame as production, green elsewhere.Part 2 — TypeScript 7 native server support
TS7 workspaces turned out to be exactly the environments that fail most often, and no harness-side fallback can fix them the old way:
typescript@7ships nolib/tsserver.js— its language service is a plain LSP server inside the native binary (tsc --lsp --stdio), sotypescript-language-servercan never start there. This branch now activates the native server directly:src/extensions/lsp/servers.ts) —resolveTsNativeServerPathrecognizes the native package signature (bin/tscpresent,lib/tsserver.jsabsent). The workspace's own typescript package decides the flavor (a classic TS≤6 package wins even if the worktree's main repo is native); worktrees without their own install fall back to the main repo's bin. In a TS7 workspace the native server is preferred over an installedtypescript-language-server, and it works with no global install at all.client.ts) —skipProjectLoadWaitskips theprojectLoadedwait for servers that never emit$/progressstartup cycles (otherwise a fixed 15-second stall on every spawn).client.ts,lsp.ts) — the native server has nopublishDiagnosticspush, so pull-model servers use the LSP 3.17textDocument/diagnosticrequest in the tool_result sync path and inlsp_diagnostics. Push flow unchanged for classic servers.Verified live against the real TS7 server in an actual TS7 workspace: initialize, didOpen, hover, definition, documentSymbol, and pull diagnostics all work; push diagnostics confirmed absent. Previously these sessions emitted the code-frame dump storm from Part 1; now they get working LSP, and if a server still can't start, the quiet one-line failure plus status entry applies.
Checklist
pnpm run test) — 140 LSP tests green including regression tests for the respawn/failure paths (Part 1) and the native-server matrix: signature resolution, flavor precedence, worktree fallback, detection preference order, missing-candidate exclusion, and pull-vs-push sync/tool paths (Part 2). Full suite green except pre-existing environment-dependent failures inpi-package-lookup(leaks local~/.config/kimchi/harness/npmpackages) andweb-fetchintegration (network) — both fail identically without this branchpnpm run check)