git.iko.soy/crush
on main help sign in
fix(lsp): mitigate stale diagnostics

* fix(lsp): mitigate stale diagnostics * fix(lsp): log warning when workspace change notification fails 💘 Generated with Crush Assisted-by: GLM-5.1 via Crush <crush@charm.land> * test(lsp): add unit tests for diagnostics settling behavior 💘 Generated with Crush Assisted-by: GLM-5.1 via Crush <crush@charm.land> * refactor(lsp): remove blocking diagnostic wait from post-edit notifications 💘 Generated with Crush Assisted-by: GLM-5.1 via Crush <crush@charm.land> * chore: add async diagnostic waits * Update internal/agent/tools/diagnostics.go Co-authored-by: Andrey Nering <andreynering@users.noreply.github.com> * fix(lsp): stop timer leaks in WaitForDiagnostics Replace time.After with time.NewTimer + defer Stop() to prevent unreclaimable timer allocations on early returns. 💘 Generated with Crush Assisted-by: GLM-5.1 via Crush <crush@charm.land> --------- Co-authored-by: Andrey Nering <andreynering@users.noreply.github.com>

commitd3f6d98ab9c02d560c40b34a2899438f04f9cc53 authorChristian Rocha <christian@rocha.is> dateTue 21 Apr 2026, 17:34 (6 months ago) parentf7beb12
3 files changed+219−9
compare with main
unifiedsplit
internal/agent/tools/diagnostics.go+23−2 internal/lsp/client.go+100−7 internal/lsp/client_test.go+96
internal/agent/tools/diagnostics.go+23−2
@@ -87,25 +87,46 @@ func waitForLSPDiagnostics(
8787
8888// notifyLSPs notifies LSP servers that a file has changed and waits for
8989// updated diagnostics. Use this after edit/multiedit operations.
90+// When filepath is empty, refreshes all open files across all LSP clients
91+// and sends a workspace-level change notification for full re-analysis.
9092func notifyLSPs(
9193 ctx context.Context,
9294 manager *lsp.Manager,
9395 filepath string,
9496) {
95− if filepath == "" || manager == nil {
97+ if manager == nil {
98+ return
99+ }
100+ if filepath == "" {
101+ // No specific file — refresh all open files for all clients.
102+ var wg sync.WaitGroup
103+ for client := range manager.Clients().Seq() {
104+ wg.Go(func() {
105+ client.RefreshOpenFiles(ctx)
106+ if err := client.NotifyWorkspaceChange(ctx); err != nil {
107+ slog.WarnContext(ctx, "Failed to notify workspace change", "error", err)
108+ }
109+ client.WaitForDiagnostics(ctx, 5*time.Second)
110+ })
111+ }
112+ wg.Wait()
96113 return
97114 }
98115
99116 manager.Start(ctx, filepath)
100117
118+ var wg sync.WaitGroup
101119 for client := range manager.Clients().Seq() {
102120 if !client.HandlesFile(filepath) {
103121 continue
104122 }
105123 _ = client.OpenFileOnDemand(ctx, filepath)
106124 _ = client.NotifyChange(ctx, filepath)
107− client.WaitForDiagnostics(ctx, 5*time.Second)
125+ wg.Go(func() {
126+ client.WaitForDiagnostics(ctx, 5*time.Second)
127+ })
108128 }
129+ wg.Wait()
109130}
110131
111132func getDiagnostics(filePath string, manager *lsp.Manager) string {
internal/lsp/client.go+100−7
@@ -530,23 +530,116 @@ func (c *Client) openKeyConfigFiles(ctx context.Context) {
530530 }
531531}
532532
533−// WaitForDiagnostics waits until diagnostics change or the timeout is reached.
534−func (c *Client) WaitForDiagnostics(ctx context.Context, d time.Duration) {
533+// NotifyWorkspaceChange sends a workspace-level file change notification to
534+// trigger re-analysis of all files. This is useful when the overall project
535+// state may have changed (e.g., after a project-wide refactoring) and
536+// diagnostics for files not currently being edited may be stale.
537+func (c *Client) NotifyWorkspaceChange(ctx context.Context) error {
538+ if c == nil {
539+ return nil
540+ }
541+ return c.client.NotifyDidChangeWatchedFiles(ctx, []protocol.FileEvent{
542+ {URI: protocol.DocumentURI(protocol.URIFromPath(c.cwd)), Type: protocol.Changed},
543+ })
544+}
545+
546+// RefreshOpenFiles re-notifies the LSP server about all currently open files,
547+// which triggers re-analysis and fresh diagnostics for the entire project.
548+func (c *Client) RefreshOpenFiles(ctx context.Context) {
535549 if c == nil {
536550 return
537551 }
538− ticker := time.NewTicker(200 * time.Millisecond)
552+ for uri, info := range c.openFiles.Seq2() {
553+ path, err := protocol.DocumentURI(uri).Path()
554+ if err != nil {
555+ slog.Warn("Failed to convert URI to path", "uri", uri, "error", err)
556+ continue
557+ }
558+ content, err := os.ReadFile(path)
559+ if err != nil {
560+ slog.Warn("Failed to read file for refresh", "path", path, "error", err)
561+ continue
562+ }
563+ info.Version++
564+ changes := []protocol.TextDocumentContentChangeEvent{
565+ {
566+ Value: protocol.TextDocumentContentChangeWholeDocument{
567+ Text: string(content),
568+ },
569+ },
570+ }
571+ if err := c.client.NotifyDidChangeTextDocument(ctx, uri, int(info.Version), changes); err != nil {
572+ slog.Warn("Failed to notify file change", "uri", uri, "error", err)
573+ }
574+ }
575+}
576+
577+// WaitForDiagnostics waits until diagnostics stop changing for a settling
578+// period, indicating the LSP server has finished processing. If no
579+// diagnostics change within firstChangeDuration, it returns early since the
580+// server likely isn't going to republish.
581+func (c *Client) WaitForDiagnostics(ctx context.Context, timeout time.Duration) {
582+ if c == nil {
583+ return
584+ }
585+
586+ const (
587+ firstChangeDuration = 1 * time.Second
588+ settleDuration = 300 * time.Millisecond
589+ )
590+
591+ deadline := time.NewTimer(timeout)
592+ defer deadline.Stop()
593+ firstChangeTimer := time.NewTimer(min(timeout, firstChangeDuration))
594+ defer firstChangeTimer.Stop()
595+ previousVersion := c.diagnostics.Version()
596+ ticker := time.NewTicker(100 * time.Millisecond)
539597 defer ticker.Stop()
540− timeout := time.After(d)
541− pv := c.diagnostics.Version()
598+
542599 for {
543600 select {
544601 case <-ctx.Done():
545602 return
546− case <-timeout:
603+ case <-deadline.C:
604+ return
605+ case <-firstChangeTimer.C:
606+ // No change arrived quickly — server isn't republishing.
547607 return
548608 case <-ticker.C:
549− if pv != c.diagnostics.Version() {
609+ currentVersion := c.diagnostics.Version()
610+ if currentVersion != previousVersion {
611+ // Diagnostics changed — now wait for them to settle.
612+ c.waitForDiagnosticsToSettle(ctx, deadline.C, settleDuration)
613+ return
614+ }
615+ }
616+ }
617+}
618+
619+// waitForDiagnosticsToSettle waits until diagnostics version stays the same
620+// for settleDuration, indicating the LSP server has finished publishing.
621+func (c *Client) waitForDiagnosticsToSettle(ctx context.Context, deadline <-chan time.Time, settleDuration time.Duration) {
622+ lastVersion := c.diagnostics.Version()
623+ settleTicker := time.NewTicker(50 * time.Millisecond)
624+ defer settleTicker.Stop()
625+
626+ // Track how long the version has been stable.
627+ stableStart := time.Now()
628+
629+ for {
630+ select {
631+ case <-ctx.Done():
632+ return
633+ case <-deadline:
634+ return
635+ case <-settleTicker.C:
636+ currentVersion := c.diagnostics.Version()
637+ if currentVersion != lastVersion {
638+ // New change detected — reset the stable timer.
639+ lastVersion = currentVersion
640+ stableStart = time.Now()
641+ } else if time.Since(stableStart) >= settleDuration {
642+ // Diagnostics have been stable for the settle duration.
550643 return
551644 }
552645 }
internal/lsp/client_test.go+96
@@ -2,11 +2,14 @@ package lsp
22
33import (
44 "context"
5+ "fmt"
56 "testing"
67 "time"
78
89 "github.com/charmbracelet/crush/internal/config"
10+ "github.com/charmbracelet/crush/internal/csync"
911 "github.com/charmbracelet/crush/internal/env"
12+ "github.com/charmbracelet/x/powernap/pkg/lsp/protocol"
1013 "github.com/stretchr/testify/require"
1114)
1215
@@ -70,3 +73,96 @@ func TestNilClient(t *testing.T) {
7073 require.Nil(t, c.NotifyChange(context.Background(), "/some/file.go"))
7174 c.WaitForDiagnostics(context.Background(), time.Second)
7275}
76+
77+func newTestClient() *Client {
78+ c := &Client{
79+ name: "test",
80+ diagnostics: csync.NewVersionedMap[protocol.DocumentURI, []protocol.Diagnostic](),
81+ openFiles: csync.NewMap[string, *OpenFileInfo](),
82+ }
83+ c.serverState.Store(StateStopped)
84+ return c
85+}
86+
87+func TestWaitForDiagnostics_NoChange(t *testing.T) {
88+ t.Parallel()
89+
90+ c := newTestClient()
91+ start := time.Now()
92+ c.WaitForDiagnostics(t.Context(), 5*time.Second)
93+ elapsed := time.Since(start)
94+
95+ // Should return early via firstChangeDeadline (~1s), not the full timeout.
96+ require.Less(t, elapsed, 2*time.Second, "should return early when no diagnostics change")
97+}
98+
99+func TestWaitForDiagnostics_ImmediateChange(t *testing.T) {
100+ t.Parallel()
101+
102+ c := newTestClient()
103+
104+ go func() {
105+ time.Sleep(100 * time.Millisecond)
106+ c.diagnostics.Set(protocol.DocumentURI("file:///test.go"), nil)
107+ }()
108+
109+ start := time.Now()
110+ c.WaitForDiagnostics(t.Context(), 5*time.Second)
111+ elapsed := time.Since(start)
112+
113+ // Should detect the change and then settle (~300ms settle + overhead).
114+ require.Less(t, elapsed, 2*time.Second, "should return after settling, not full timeout")
115+ require.Greater(t, elapsed, 200*time.Millisecond, "should wait for settle duration")
116+}
117+
118+func TestWaitForDiagnostics_RepeatedChanges(t *testing.T) {
119+ t.Parallel()
120+
121+ c := newTestClient()
122+
123+ // Simulate an LSP server that publishes diagnostics in bursts.
124+ go func() {
125+ for i := range 5 {
126+ time.Sleep(50 * time.Millisecond)
127+ c.diagnostics.Set(protocol.DocumentURI("file:///test.go"), []protocol.Diagnostic{
128+ {Message: fmt.Sprintf("diag-%d", i)},
129+ })
130+ }
131+ }()
132+
133+ start := time.Now()
134+ c.WaitForDiagnostics(t.Context(), 5*time.Second)
135+ elapsed := time.Since(start)
136+
137+ // Should wait for diagnostics to settle after the burst finishes.
138+ // Burst lasts ~250ms, then 300ms settle window, so total ~550ms+.
139+ require.Less(t, elapsed, 2*time.Second, "should return after settling, not full timeout")
140+ require.Greater(t, elapsed, 400*time.Millisecond, "should wait for all changes to settle")
141+}
142+
143+func TestWaitForDiagnostics_ContextCancellation(t *testing.T) {
144+ t.Parallel()
145+
146+ c := newTestClient()
147+ ctx, cancel := context.WithCancel(t.Context())
148+ defer cancel()
149+
150+ go func() {
151+ time.Sleep(200 * time.Millisecond)
152+ cancel()
153+ }()
154+
155+ start := time.Now()
156+ c.WaitForDiagnostics(ctx, 5*time.Second)
157+ elapsed := time.Since(start)
158+
159+ require.Less(t, elapsed, 1*time.Second, "should return shortly after context cancellation")
160+}
161+
162+func TestWaitForDiagnostics_NilClient(t *testing.T) {
163+ t.Parallel()
164+
165+ var c *Client
166+ // Should not panic.
167+ c.WaitForDiagnostics(context.Background(), time.Second)
168+}