From 6205e0e4b4f15a8495b3c948b40516aa8ee3cd5d Mon Sep 17 00:00:00 2001 From: karitham Date: Fri, 7 Aug 2026 23:25:00 +0200 Subject: [PATCH] lsp: fix snapshot swap race, view races, and the shared walk guard View.FileChange swapped the snapshot without holding snapshotMu while Snapshot() reads it under the lock: two concurrent FileChanges (the workspace walk plus request handlers, all async) could release the same snapshot twice, panicking the server with a negative WaitGroup counter. The swap, the old snapshot's release, and the release assignment now happen under snapshotMu, and the parse loop and dependent computation run against the snapshot this call created instead of re-reading the (likely newer) view snapshot. IncludeGraph.Get returned live nodes after dropping the RLock; Set and removeWithoutLock mutate those slices in place, racing every reader (Dependents, TokensForFile, referenceFiles). Get now returns a copy. Session.CreateView appended to s.views without viewMu, racing AddView, ViewOf and RemoveView, and duplicated views for the same folder. It is gone; openFile uses AddView. The workspace walk (Initialized) and the per-directory walk (first didOpen) shared one once-guard, so a didOpen racing the Initialized notification permanently skipped scanning the workspace folders. Each walk now has its own sync.Once on the Server. ParsedFile.Tokens lazy-init now uses sync.Once like its siblings, and the no-op Snapshot.Initialize (spawned with ceremony in NewView) is deleted. --- lsp/cache/graph.go | 9 ++++++++- lsp/cache/parse.go | 21 ++++++++++----------- lsp/cache/session.go | 23 ----------------------- lsp/cache/snapshot.go | 3 --- lsp/cache/view.go | 38 +++++++++++++++----------------------- lsp/impl.go | 12 ++++++------ lsp/include_paths_test.go | 2 +- lsp/server.go | 11 ++++++++++- 8 files changed, 50 insertions(+), 69 deletions(-) diff --git a/lsp/cache/graph.go b/lsp/cache/graph.go index 5a65038..ff83cb1 100644 --- a/lsp/cache/graph.go +++ b/lsp/cache/graph.go @@ -56,11 +56,18 @@ func NewIncludeGraph() *IncludeGraph { } } +// Get returns a copy of file's node. The graph's nodes are mutated in place +// by Set and removeWithoutLock, so a live node must never escape the lock. func (g *IncludeGraph) Get(file uri.URI) *IncludeNode { g.mu.RLock() defer g.mu.RUnlock() - return g.mapper[file] + node := g.mapper[file] + if node == nil { + return nil + } + + return node.Clone() } // Set records the include edges of file. resolve maps an include path text diff --git a/lsp/cache/parse.go b/lsp/cache/parse.go index 1b878f6..aefb7b3 100644 --- a/lsp/cache/parse.go +++ b/lsp/cache/parse.go @@ -214,7 +214,8 @@ type ParsedFile struct { errs []syntax.Error // tokens is the identifier set of ast, computed lazily once per parse. - tokens map[string]struct{} + tokensOnce sync.Once + tokens map[string]struct{} // defs and enumValues index the file's definitions, computed lazily // once per parse. A re-parse replaces the whole ParsedFile, so the @@ -241,18 +242,16 @@ func (p *ParsedFile) Errors() []syntax.Error { // reused. A re-parse replaces the whole ParsedFile, so the cache never // goes stale. func (p *ParsedFile) Tokens() map[string]struct{} { - if p.tokens != nil { - return p.tokens - } - - tokens := make(map[string]struct{}) - if p.ast != nil { - collectTokens(p.ast, tokens) - } + p.tokensOnce.Do(func() { + tokens := make(map[string]struct{}) + if p.ast != nil { + collectTokens(p.ast, tokens) + } - p.tokens = tokens + p.tokens = tokens + }) - return tokens + return p.tokens } // Definitions returns the file's top-level definitions indexed by name: diff --git a/lsp/cache/session.go b/lsp/cache/session.go index 85c21d4..117f600 100644 --- a/lsp/cache/session.go +++ b/lsp/cache/session.go @@ -12,11 +12,6 @@ import ( type Session struct { id int64 - // indicates whether initialized with initialize params - // if initialize params doesn't contain any folder. this is false - initializedMu sync.Mutex - initialized bool - // cache is shared global cache *Cache @@ -41,24 +36,6 @@ func NewSession(cache *Cache) *Session { return sess } -func (s *Session) Initialize(fn func()) { - s.initializedMu.Lock() - defer s.initializedMu.Unlock() - - if s.initialized { - return - } - - s.initialized = true - - fn() -} - -func (s *Session) CreateView(folder uri.URI) { - view := NewView(folder.Path(), folder, s.overlayFS, s.cache.IncludePaths) - s.views = append(s.views, view) -} - // AddView registers a view for the workspace folder, returning the // existing view when the folder is already tracked. func (s *Session) AddView(folder uri.URI) *View { diff --git a/lsp/cache/snapshot.go b/lsp/cache/snapshot.go index c1809f6..36c1a6f 100644 --- a/lsp/cache/snapshot.go +++ b/lsp/cache/snapshot.go @@ -188,9 +188,6 @@ func (s *Snapshot) Acquire() func() { return s.refCount.Done } -func (s *Snapshot) Initialize(ctx context.Context) { -} - func (s *Snapshot) Graph() *IncludeGraph { return s.context.graph } diff --git a/lsp/cache/view.go b/lsp/cache/view.go index 007b920..1b5aca6 100644 --- a/lsp/cache/view.go +++ b/lsp/cache/view.go @@ -55,15 +55,6 @@ func NewView(name string, folder uri.URI, fs FileSource, includePaths []string) view.snapshotRelease = view.snapshot.Acquire() - asyncRelease := view.snapshot.Acquire() - go func() { - defer asyncRelease() - - view.snapshotMu.Lock() - view.snapshot.Initialize(context.Background()) - view.snapshotMu.Unlock() - }() - return view } @@ -132,19 +123,22 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. v.MarkFileKnown(change.URI) } - // Swap in the new snapshot. + // Swap in the new snapshot. The pointer and its release are a single + // unit guarded by snapshotMu: FileChange runs concurrently (request + // handlers, the workspace walk), and an unlocked swap could release the + // same snapshot twice (negative WaitGroup panic) or let a reader see a + // released snapshot. + v.snapshotMu.Lock() newSnapshot, release := v.snapshot.clone() v.snapshotRelease() - v.snapshotMu.Lock() - v.snapshot = newSnapshot for _, change := range changes { - v.snapshot.ForgetFile(change.URI) + newSnapshot.ForgetFile(change.URI) } - v.snapshotMu.Unlock() v.snapshotRelease = release + v.snapshotMu.Unlock() - asyncRelease := v.snapshot.Acquire() + asyncRelease := newSnapshot.Acquire() // Re-parse the changed files so the snapshot's include edges and // parsed caches reflect the change before any request observes it. @@ -158,12 +152,12 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. slices.Sort(uris) for _, uri := range uris { - if _, err := v.snapshot.Parse(ctx, uri); err != nil { + if _, err := newSnapshot.Parse(ctx, uri); err != nil { slog.Error("parse error", "err", err) } } - affected := v.affectedFiles(changes) + affected := v.affectedFiles(changes, newSnapshot) go func() { defer asyncRelease() @@ -175,10 +169,10 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. } // affectedFiles returns the changed URIs plus the transitive dependents of -// each, deduped. Dependents are computed on the current snapshot, after the -// changes were applied and re-parsed, so the edges reflect the change. +// each, deduped. Dependents are computed on ss, the snapshot the changes +// were applied to, so the edges reflect the change. // Changes come first, in order; dependents follow, sorted by URI. -func (v *View) affectedFiles(changes []*FileChange) []uri.URI { +func (v *View) affectedFiles(changes []*FileChange, ss *Snapshot) []uri.URI { affected := make([]uri.URI, 0, len(changes)) seen := make(map[uri.URI]struct{}, len(changes)) @@ -190,9 +184,7 @@ func (v *View) affectedFiles(changes []*FileChange) []uri.URI { seen[change.URI] = struct{}{} affected = append(affected, change.URI) - v.snapshotMu.Lock() - deps := v.snapshot.Dependents(change.URI) - v.snapshotMu.Unlock() + deps := ss.Dependents(change.URI) for _, dep := range deps { if _, ok := seen[dep]; ok { diff --git a/lsp/impl.go b/lsp/impl.go index 65d2ba3..3f6dbb8 100644 --- a/lsp/impl.go +++ b/lsp/impl.go @@ -29,7 +29,7 @@ func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocume From: cache.FileChangeTypeDidOpen, } - s.session.Initialize(func() { + s.dirWalkOnce.Do(func() { file := change.URI dirPos := strings.LastIndexByte(string(file), '/') @@ -51,14 +51,14 @@ func (s *Server) openFile(ctx context.Context, change *cache.FileChange) error { } } - if _, err := s.session.ViewOf(change.URI); err != nil { - // create view for this folder + // The file's directory becomes the view when no workspace folder + // covers it (single-file mode); AddView dedups and is concurrency-safe. + view, err := s.session.ViewOf(change.URI) + if err != nil { filename := change.URI.Path() - dir := uri.File(path.Dir(filename)) - s.session.CreateView(dir) + view = s.session.AddView(uri.File(path.Dir(filename))) } - view, _ := s.session.ViewOf(change.URI) view.FileChange(ctx, []*cache.FileChange{change}, s.postDiagnostics(ctx, view)) return nil diff --git a/lsp/include_paths_test.go b/lsp/include_paths_test.go index 0bfa092..eb9c6bf 100644 --- a/lsp/include_paths_test.go +++ b/lsp/include_paths_test.go @@ -26,7 +26,7 @@ func TestServerIncludePathsFlow(t *testing.T) { srv := NewServer(c, nil, formatter.DefaultOptions()) // Views are created per workspace folder at initialization. - srv.session.CreateView(uri.File(dir)) + srv.session.AddView(uri.File(dir)) view, err := srv.session.ViewOf(uri.File(filepath.Join(dir, "app.thrift"))) assert.NoError(t, err) diff --git a/lsp/server.go b/lsp/server.go index de1d133..6289cd5 100644 --- a/lsp/server.go +++ b/lsp/server.go @@ -3,6 +3,7 @@ package lsp import ( "context" "log/slog" + "sync" "go.lsp.dev/protocol" "go.lsp.dev/uri" @@ -23,6 +24,14 @@ type Server struct { // walk starts on the Initialized notification so the initialize // handshake never blocks on parsing the workspace. folders []uri.URI + + // workspaceWalkOnce and dirWalkOnce guard the two independent walks: + // the whole workspace on Initialized, and the opened file's directory + // on the first didOpen (single-file mode). They must not share a guard + // — a didOpen racing the Initialized notification would otherwise + // permanently skip the workspace walk. + workspaceWalkOnce sync.Once + dirWalkOnce sync.Once } func NewServer(c *cache.Cache, client protocol.Client, formatOpts formatter.Options) *Server { @@ -45,7 +54,7 @@ func (s *Server) Initialized(ctx context.Context, params *protocol.InitializedPa // The workspace walk parses every thrift file to warm the cache; it // runs once, off the request path, so the initialize handshake and // early requests never block on it. - s.session.Initialize(func() { + s.workspaceWalkOnce.Do(func() { go func() { for _, folder := range s.folders { s.walkFoldersThriftFile(folder) -- 2.51.2