From 658c89531860e0c1747c7cdf1473fd50d4302ba5 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 21 Jun 2026 02:25:23 +0100 Subject: [PATCH] fix(lsp): detect, evict, and respawn crashed language servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A crashed language server stayed cached with ready==true: readLoop exited and drained in-flight calls but never cleared the running flag, and GetServer keyed reuse on ready alone. Every subsequent hover/definition/references then passed the running guard, wrote to a dead stdin, and blocked the full request timeout — permanently, since active use kept the server off the idle reaper. A single crash wedged that language at ~10s/call with no self-heal. - readLoop now flips running=false on exit, so Call fails fast instead of hanging. - GetServer treats ready && client.IsRunning() as the liveness condition; a stale entry is evicted (closed async — its process has already exited) and a fresh server is spawned in its place. - drainPending's send is now non-blocking, matching the dispatch path, so a duplicate-id race can't deadlock it under c.mu. Test (proven to fail without the fix): a client whose process exits is marked not-running and its next Call fails fast rather than blocking. --- internal/lsp/client.go | 20 ++++++++++++++++++- internal/lsp/client_test.go | 38 +++++++++++++++++++++++++++++++++++++ internal/lsp/manager.go | 16 ++++++++++++++-- 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/internal/lsp/client.go b/internal/lsp/client.go index 822916a..4b972ec 100644 --- a/internal/lsp/client.go +++ b/internal/lsp/client.go @@ -304,7 +304,12 @@ func (c *Client) send(msg interface{}) error { // On exit (for any reason), it drains all pending Call waiters with a // synthetic error so that goroutines blocked in Call are unblocked. func (c *Client) readLoop() { + // On exit (EOF/error from a crashed or closed server) mark the client not + // running so subsequent Calls fail fast — otherwise they pass the running + // guard, send to a dead stdin, and block until their own timeout. Then + // unblock any already-in-flight waiters. LIFO: setRunning(false) runs first. defer c.drainPending() + defer c.setRunning(false) reader := bufio.NewReader(c.stdout) @@ -380,13 +385,19 @@ func (c *Client) drainPending() { defer c.mu.Unlock() for id, ch := range c.pending { - ch <- &Response{ + // Non-blocking, matching the dispatch path: the channel is buffered(1) + // and read once, but a duplicate-id race could have already filled it — + // never block here while holding c.mu. + select { + case ch <- &Response{ JSONRPC: "2.0", ID: id, Error: &ResponseError{ Code: -32603, // InternalError Message: "LSP client readLoop terminated", }, + }: + default: } delete(c.pending, id) } @@ -398,3 +409,10 @@ func (c *Client) IsRunning() bool { defer c.runningMu.RUnlock() return c.running } + +// setRunning sets the running flag under its lock. +func (c *Client) setRunning(v bool) { + c.runningMu.Lock() + c.running = v + c.runningMu.Unlock() +} diff --git a/internal/lsp/client_test.go b/internal/lsp/client_test.go index f5b8a1e..929807a 100644 --- a/internal/lsp/client_test.go +++ b/internal/lsp/client_test.go @@ -1,10 +1,48 @@ package lsp import ( + "context" + "os/exec" "strings" "testing" + "time" ) +// TestClientMarksNotRunningOnProcessExit verifies crash detection: when the +// underlying server process exits (stdout EOF), readLoop must flip the client to +// not-running so later Calls fail fast instead of blocking until their timeout. +func TestClientMarksNotRunningOnProcessExit(t *testing.T) { + cmd := exec.Command("sh", "-c", "exit 0") // exits immediately, closing stdout + c, err := NewClient(cmd) + if err != nil { + t.Skipf("cannot start helper process: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + deadline := time.Now().Add(2 * time.Second) + for c.IsRunning() && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if c.IsRunning() { + t.Fatal("client should be marked not running after its process exits") + } + + // A Call on the dead client must fail fast (the running guard), not hang. + done := make(chan error, 1) + go func() { + _, callErr := c.Call(context.Background(), "test/method", nil) + done <- callErr + }() + select { + case callErr := <-done: + if callErr == nil { + t.Error("Call on a dead client should return an error") + } + case <-time.After(2 * time.Second): + t.Error("Call on a dead client hung instead of failing fast") + } +} + // drainStderr retains a bounded tail of server stderr for diagnostics; capture // must keep recent bytes and never grow past StderrCaptureMax. func TestClientStderrCapture(t *testing.T) { diff --git a/internal/lsp/manager.go b/internal/lsp/manager.go index 4ded9d9..3da8ebd 100644 --- a/internal/lsp/manager.go +++ b/internal/lsp/manager.go @@ -128,7 +128,11 @@ func (m *Manager) GetServer(ctx context.Context, lang protocol.Language) (*Manag srv, exists := m.servers[lang] m.mu.RUnlock() - if exists && srv.ready { + // A cached server is only reusable if it is both initialized and still + // running — a crashed language server stays in the map with ready==true, so + // without the liveness check every later call would be routed to a dead + // process and block until timeout. + if exists && srv.ready && srv.client.IsRunning() { // Update lastUsed with server's own lock to avoid race condition srv.mu.Lock() srv.lastUsed = time.Now() @@ -141,13 +145,21 @@ func (m *Manager) GetServer(ctx context.Context, lang protocol.Language) (*Manag defer m.mu.Unlock() // Double-check after acquiring write lock - if srv, ok := m.servers[lang]; ok && srv.ready { + if srv, ok := m.servers[lang]; ok && srv.ready && srv.client.IsRunning() { srv.mu.Lock() srv.lastUsed = time.Now() srv.mu.Unlock() return srv, nil } + // Evict a stale entry (crashed or never-ready) before spawning a replacement. + // Its process has already exited, so close it asynchronously to avoid holding + // m.mu on the (fast) cmd.Wait(). + if stale, ok := m.servers[lang]; ok { + delete(m.servers, lang) + go func() { _ = stale.client.Close() }() + } + // Check if server config exists config, ok := DefaultServerConfigs[lang] if !ok {