feat(lsp): remove internal watcher (#1062)

* feat(lsp): remove internal watcher

It was only ever useful if the user edits files through their editor,
but we don't really need it assuming we only care about edits done by
Crush itself.

Basically, we lose that functionality, but save a bunch of file
descriptors and have improved perf.

Damn good deal if you ask me.

Signed-off-by: Carlos Alexandro Becker <[email protected]>

* fix: cleanup

Signed-off-by: Carlos Alexandro Becker <[email protected]>

* fix: remove rlimit

Signed-off-by: Carlos Alexandro Becker <[email protected]>

* fix: more cleanup

Signed-off-by: Carlos Alexandro Becker <[email protected]>

---------

Signed-off-by: Carlos Alexandro Becker <[email protected]>

Author: caarlos0

go.mod

@@ -114,7 +114,6 @@ require (
 	github.com/ncruces/julianday v1.0.0 // indirect
 	github.com/pierrec/lz4/v4 v4.1.22 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	github.com/raphamorim/notify v0.9.4
 	github.com/rivo/uniseg v0.4.7
 	github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
 	github.com/sethvargo/go-retry v0.3.0 // indirect

go.sum

@@ -237,8 +237,6 @@ github.com/pressly/goose/v3 v3.25.0 h1:6WeYhMWGRCzpyd89SpODFnCBCKz41KrVbRT58nVjG
 github.com/pressly/goose/v3 v3.25.0/go.mod h1:4hC1KrritdCxtuFsqgs1R4AU5bWtTAf+cnWvfhf2DNY=
 github.com/qjebbs/go-jsons v0.0.0-20221222033332-a534c5fc1c4c h1:kmzxiX+OB0knCo1V0dkEkdPelzCdAzCURCfmFArn2/A=
 github.com/qjebbs/go-jsons v0.0.0-20221222033332-a534c5fc1c4c/go.mod h1:wNJrtinHyC3YSf6giEh4FJN8+yZV7nXBjvmfjhBIcw4=
-github.com/raphamorim/notify v0.9.4 h1:JXAGOzeR/cnclKkRCZINKS4EtB47O5TD1N1iCkkarTM=
-github.com/raphamorim/notify v0.9.4/go.mod h1:3FXSIPyrunV10GCnLGPrpSxoY/Dxi+saeQb9hf+TDSo=
 github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE=
 github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=
 github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc=
@@ -370,7 +368,6 @@ golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sync v0.17.0 h1:l60nONMj9l5drqw6jlhIELNv9I0A4OFgRsG9k2oT9Ug=
 golang.org/x/sync v0.17.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI=
-golang.org/x/sys v0.0.0-20180926160741-c2ed4eda69e7/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

internal/app/app.go

@@ -7,14 +7,11 @@ import (
 	"fmt"
 	"log/slog"
 	"maps"
-	"os/exec"
-	"strings"
 	"sync"
 	"time"
 
 	tea "github.com/charmbracelet/bubbletea/v2"
 	"github.com/charmbracelet/crush/internal/config"
-	"github.com/charmbracelet/crush/internal/csync"
 	"github.com/charmbracelet/crush/internal/db"
 	"github.com/charmbracelet/crush/internal/format"
 	"github.com/charmbracelet/crush/internal/history"
@@ -23,7 +20,6 @@ import (
 	"github.com/charmbracelet/crush/internal/pubsub"
 
 	"github.com/charmbracelet/crush/internal/lsp"
-	"github.com/charmbracelet/crush/internal/lsp/watcher"
 	"github.com/charmbracelet/crush/internal/message"
 	"github.com/charmbracelet/crush/internal/permission"
 	"github.com/charmbracelet/crush/internal/session"
@@ -41,9 +37,6 @@ type App struct {
 
 	clientsMutex sync.RWMutex
 
-	watcherCancelFuncs *csync.Slice[context.CancelFunc]
-	lspWatcherWG       sync.WaitGroup
-
 	config *config.Config
 
 	serviceEventsWG *sync.WaitGroup
@@ -56,16 +49,6 @@ type App struct {
 	cleanupFuncs []func() error
 }
 
-// isGitRepo checks if the current directory is a git repository
-func isGitRepo() bool {
-	bts, err := exec.CommandContext(
-		context.Background(),
-		"git", "rev-parse",
-		"--is-inside-work-tree",
-	).CombinedOutput()
-	return err == nil && strings.TrimSpace(string(bts)) == "true"
-}
-
 // New initializes a new applcation instance.
 func New(ctx context.Context, conn *sql.DB, cfg *config.Config) (*App, error) {
 	q := db.New(conn)
@@ -89,24 +72,13 @@ func New(ctx context.Context, conn *sql.DB, cfg *config.Config) (*App, error) {
 
 		config: cfg,
 
-		watcherCancelFuncs: csync.NewSlice[context.CancelFunc](),
-
 		events:          make(chan tea.Msg, 100),
 		serviceEventsWG: &sync.WaitGroup{},
 		tuiWG:           &sync.WaitGroup{},
 	}
 
 	app.setupEvents()
 
-	// Start the global watcher only if this is a git repository
-	if isGitRepo() {
-		if err := watcher.Start(); err != nil {
-			return nil, fmt.Errorf("app: %w", err)
-		}
-	} else {
-		slog.Warn("Not starting global watcher: not a git repository")
-	}
-
 	// Initialize LSP clients in the background.
 	app.initLSPClients(ctx)
 
@@ -352,13 +324,6 @@ func (app *App) Shutdown() {
 		app.CoderAgent.CancelAll()
 	}
 
-	for cancel := range app.watcherCancelFuncs.Seq() {
-		cancel()
-	}
-
-	// Wait for all LSP watchers to finish.
-	app.lspWatcherWG.Wait()
-
 	// Get all LSP clients.
 	app.clientsMutex.RLock()
 	clients := make(map[string]*lsp.Client, len(app.LSPClients))
@@ -374,9 +339,6 @@ func (app *App) Shutdown() {
 		cancel()
 	}
 
-	// Shutdown the global watcher
-	watcher.Shutdown()
-
 	// Call call cleanup functions.
 	for _, cleanup := range app.cleanupFuncs {
 		if cleanup != nil {

internal/app/lsp.go

@@ -6,9 +6,7 @@ import (
 	"time"
 
 	"github.com/charmbracelet/crush/internal/config"
-	"github.com/charmbracelet/crush/internal/log"
 	"github.com/charmbracelet/crush/internal/lsp"
-	"github.com/charmbracelet/crush/internal/lsp/watcher"
 )
 
 // initLSPClients initializes LSP clients.
@@ -77,64 +75,8 @@ func (app *App) createAndStartLSPClient(ctx context.Context, name string, config
 
 	slog.Info("LSP client initialized", "name", name)
 
-	// Create a child context that can be canceled when the app is shutting
-	// down.
-	watchCtx, cancelFunc := context.WithCancel(ctx)
-
-	// Create the workspace watcher.
-	workspaceWatcher := watcher.New(name, lspClient)
-
-	// Store the cancel function to be called during cleanup.
-	app.watcherCancelFuncs.Append(cancelFunc)
-
 	// Add to map with mutex protection before starting goroutine
 	app.clientsMutex.Lock()
 	app.LSPClients[name] = lspClient
 	app.clientsMutex.Unlock()
-
-	// Run workspace watcher.
-	app.lspWatcherWG.Add(1)
-	go app.runWorkspaceWatcher(watchCtx, name, workspaceWatcher)
-}
-
-// runWorkspaceWatcher executes the workspace watcher for an LSP client.
-func (app *App) runWorkspaceWatcher(ctx context.Context, name string, workspaceWatcher *watcher.Client) {
-	defer app.lspWatcherWG.Done()
-	defer log.RecoverPanic("LSP-"+name, func() {
-		// Try to restart the client.
-		app.restartLSPClient(ctx, name)
-	})
-
-	workspaceWatcher.Watch(ctx, app.config.WorkingDir())
-	slog.Info("Workspace watcher stopped", "client", name)
-}
-
-// restartLSPClient attempts to restart a crashed or failed LSP client.
-func (app *App) restartLSPClient(ctx context.Context, name string) {
-	// Get the original configuration.
-	clientConfig, exists := app.config.LSP[name]
-	if !exists {
-		slog.Error("Cannot restart client, configuration not found", "client", name)
-		return
-	}
-
-	// Clean up the old client if it exists.
-	app.clientsMutex.Lock()
-	oldClient, exists := app.LSPClients[name]
-	if exists {
-		// Remove from map before potentially slow shutdown.
-		delete(app.LSPClients, name)
-	}
-	app.clientsMutex.Unlock()
-
-	if exists && oldClient != nil {
-		// Try to shut down client gracefully, but don't block on errors.
-		shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-		_ = oldClient.Close(shutdownCtx)
-		cancel()
-	}
-
-	// Create a new client using the shared function.
-	app.createAndStartLSPClient(ctx, name, clientConfig)
-	slog.Info("Successfully restarted LSP client", "client", name)
 }

internal/csync/maps.go

@@ -70,10 +70,10 @@ func (m *Map[K, V]) GetOrSet(key K, fn func() V) V {
 
 // Take gets an item and then deletes it.
 func (m *Map[K, V]) Take(key K) (V, bool) {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	v, ok := m.inner[key]
-	delete(m.inner, key)
+	v, ok := m.Get(key)
+	if ok {
+		m.Del(key)
+	}
 	return v, ok
 }
 

internal/csync/versionedmap.go

@@ -0,0 +1,35 @@
+package csync
+
+import (
+	"sync/atomic"
+)
+
+// NewVersionedMap creates a new versioned, thread-safe map.
+func NewVersionedMap[K comparable, V any]() *VersionedMap[K, V] {
+	return &VersionedMap[K, V]{
+		Map: NewMap[K, V](),
+	}
+}
+
+// VersionedMap is a thread-safe map that keeps track of its version.
+type VersionedMap[K comparable, V any] struct {
+	*Map[K, V]
+	v atomic.Uint64
+}
+
+// Set sets the value for the specified key in the map and increments the version.
+func (m *VersionedMap[K, V]) Set(key K, value V) {
+	m.Map.Set(key, value)
+	m.v.Add(1)
+}
+
+// Del deletes the specified key from the map and increments the version.
+func (m *VersionedMap[K, V]) Del(key K) {
+	m.Map.Del(key)
+	m.v.Add(1)
+}
+
+// Version returns the current version of the map.
+func (m *VersionedMap[K, V]) Version() uint64 {
+	return m.v.Load()
+}

internal/csync/versionedmap_test.go

@@ -0,0 +1,89 @@
+package csync
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestVersionedMap_Set(t *testing.T) {
+	t.Parallel()
+
+	vm := NewVersionedMap[string, int]()
+	require.Equal(t, uint64(0), vm.Version())
+
+	vm.Set("key1", 42)
+	require.Equal(t, uint64(1), vm.Version())
+
+	value, ok := vm.Get("key1")
+	require.True(t, ok)
+	require.Equal(t, 42, value)
+}
+
+func TestVersionedMap_Del(t *testing.T) {
+	t.Parallel()
+
+	vm := NewVersionedMap[string, int]()
+	vm.Set("key1", 42)
+	initialVersion := vm.Version()
+
+	vm.Del("key1")
+	require.Equal(t, initialVersion+1, vm.Version())
+
+	_, ok := vm.Get("key1")
+	require.False(t, ok)
+}
+
+func TestVersionedMap_VersionIncrement(t *testing.T) {
+	t.Parallel()
+
+	vm := NewVersionedMap[string, int]()
+	initialVersion := vm.Version()
+
+	// Setting a value should increment the version
+	vm.Set("key1", 42)
+	require.Equal(t, initialVersion+1, vm.Version())
+
+	// Deleting a value should increment the version
+	vm.Del("key1")
+	require.Equal(t, initialVersion+2, vm.Version())
+
+	// Deleting a non-existent key should still increment the version
+	vm.Del("nonexistent")
+	require.Equal(t, initialVersion+3, vm.Version())
+}
+
+func TestVersionedMap_ConcurrentAccess(t *testing.T) {
+	t.Parallel()
+
+	vm := NewVersionedMap[int, int]()
+	const numGoroutines = 100
+	const numOperations = 100
+
+	// Initial version
+	initialVersion := vm.Version()
+
+	// Perform concurrent Set and Del operations
+	for i := range numGoroutines {
+		go func(id int) {
+			for j := range numOperations {
+				key := id*numOperations + j
+				vm.Set(key, key*2)
+				vm.Del(key)
+			}
+		}(i)
+	}
+
+	// Wait for operations to complete by checking the version
+	// This is a simplified check - in a real test you might want to use sync.WaitGroup
+	expectedMinVersion := initialVersion + uint64(numGoroutines*numOperations*2)
+
+	// Allow some time for operations to complete
+	for vm.Version() < expectedMinVersion {
+		// Busy wait - in a real test you'd use proper synchronization
+	}
+
+	// Final version should be at least the expected minimum
+	require.GreaterOrEqual(t, vm.Version(), expectedMinVersion)
+	require.Equal(t, 0, vm.Len())
+}