mirror of
https://github.com/lukaszraczylo/filepuff-mcp.git
synced 2026-10-12 04:54:27 +00:00
fix(lsp): detect, evict, and respawn crashed language servers
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.
This commit is contained in:
+19
-1
@@ -304,7 +304,12 @@ func (c *Client) send(msg interface{}) error {
|
|||||||
// On exit (for any reason), it drains all pending Call waiters with a
|
// On exit (for any reason), it drains all pending Call waiters with a
|
||||||
// synthetic error so that goroutines blocked in Call are unblocked.
|
// synthetic error so that goroutines blocked in Call are unblocked.
|
||||||
func (c *Client) readLoop() {
|
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.drainPending()
|
||||||
|
defer c.setRunning(false)
|
||||||
|
|
||||||
reader := bufio.NewReader(c.stdout)
|
reader := bufio.NewReader(c.stdout)
|
||||||
|
|
||||||
@@ -380,13 +385,19 @@ func (c *Client) drainPending() {
|
|||||||
defer c.mu.Unlock()
|
defer c.mu.Unlock()
|
||||||
|
|
||||||
for id, ch := range c.pending {
|
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",
|
JSONRPC: "2.0",
|
||||||
ID: id,
|
ID: id,
|
||||||
Error: &ResponseError{
|
Error: &ResponseError{
|
||||||
Code: -32603, // InternalError
|
Code: -32603, // InternalError
|
||||||
Message: "LSP client readLoop terminated",
|
Message: "LSP client readLoop terminated",
|
||||||
},
|
},
|
||||||
|
}:
|
||||||
|
default:
|
||||||
}
|
}
|
||||||
delete(c.pending, id)
|
delete(c.pending, id)
|
||||||
}
|
}
|
||||||
@@ -398,3 +409,10 @@ func (c *Client) IsRunning() bool {
|
|||||||
defer c.runningMu.RUnlock()
|
defer c.runningMu.RUnlock()
|
||||||
return c.running
|
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()
|
||||||
|
}
|
||||||
|
|||||||
@@ -1,10 +1,48 @@
|
|||||||
package lsp
|
package lsp
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"context"
|
||||||
|
"os/exec"
|
||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"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
|
// drainStderr retains a bounded tail of server stderr for diagnostics; capture
|
||||||
// must keep recent bytes and never grow past StderrCaptureMax.
|
// must keep recent bytes and never grow past StderrCaptureMax.
|
||||||
func TestClientStderrCapture(t *testing.T) {
|
func TestClientStderrCapture(t *testing.T) {
|
||||||
|
|||||||
+14
-2
@@ -128,7 +128,11 @@ func (m *Manager) GetServer(ctx context.Context, lang protocol.Language) (*Manag
|
|||||||
srv, exists := m.servers[lang]
|
srv, exists := m.servers[lang]
|
||||||
m.mu.RUnlock()
|
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
|
// Update lastUsed with server's own lock to avoid race condition
|
||||||
srv.mu.Lock()
|
srv.mu.Lock()
|
||||||
srv.lastUsed = time.Now()
|
srv.lastUsed = time.Now()
|
||||||
@@ -141,13 +145,21 @@ func (m *Manager) GetServer(ctx context.Context, lang protocol.Language) (*Manag
|
|||||||
defer m.mu.Unlock()
|
defer m.mu.Unlock()
|
||||||
|
|
||||||
// Double-check after acquiring write lock
|
// 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.mu.Lock()
|
||||||
srv.lastUsed = time.Now()
|
srv.lastUsed = time.Now()
|
||||||
srv.mu.Unlock()
|
srv.mu.Unlock()
|
||||||
return srv, nil
|
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
|
// Check if server config exists
|
||||||
config, ok := DefaultServerConfigs[lang]
|
config, ok := DefaultServerConfigs[lang]
|
||||||
if !ok {
|
if !ok {
|
||||||
|
|||||||
Reference in New Issue
Block a user