fix(pool): an [lsp] restart can no longer land on a superseded config

CHANGELOG.md

@@ -18,6 +18,15 @@
   example a hook-less desktop chat sharing a connector. Its calls can then be
   credited to the session that did declare, so the report remains a proxy.
 
+- **An `[lsp]` restart can no longer land on a superseded config.** A
+  reconfigure that resolved its config just before a newer one was published
+  re-resolves instead of restarting the server back onto the old one, the
+  config and the project file's validity now come from one resolution, and a
+  restart round that already has a newer change waiting no longer wakes its
+  server for the config about to be replaced (costly for a server such as
+  jdtls). A resync after lost file events is now logged as one, not as a config
+  change.
+
 - **The file watcher leaves linked git worktrees out.** The OS watcher a
   workspace's topology index and language servers share now drops every event
   inside a linked worktree of the workspace's repository (or of one of its

internal/cli/pool.go

@@ -63,8 +63,11 @@ type workspacePool struct {
 	langs   []langConfig
 	langsMu sync.RWMutex
 
-	// langsGen counts the times enableLanguage has widened the effective set. It
-	// is the signal a live connection watches to know its primary language may be
+	// langsGen is the pool's language-config generation, bumped AFTER every
+	// publish of a new effective set (enableLanguage, applyGlobalLSP,
+	// invalidateLanguageConfig). reconfigureEntry reads it before its unlocked
+	// resolve and re-checks it under p.mu, so a resolve that straddled a publish
+	// is repeated rather than acted on. It is also the signal a live connection watches to know its primary language may be
 	// resolvable now when it was not at attach time: `enable-lsp` deliberately
 	// leaves existing pool entries and pinned workspaces untouched, so without
 	// this a session that attached as LanguageNone would report "none attached"
@@ -89,6 +92,10 @@ type workspacePool struct {
 	// the cost, since the walk and the stats run on a hit as well. The guard would
 	// then pass while the regression it exists to catch was back.
 	langResolves atomic.Uint64
+	// reconfigureResolved, when set, runs in reconfigureEntry and wakeHeld
+	// between the unlocked resolve and the locked decision: a test seam for the
+	// window the config generation check closes. Nil in production.
+	reconfigureResolved func(*poolEntry)
 
 	baseConfig     config.Config // global base for per-workspace LSP overrides
 	languageConfig languageConfigState

internal/cli/pool_config.go

@@ -14,3 +14,12 @@ func (p *workspacePool) cfgFor(language string) (config.LSPConfig, bool) {
 func (p *workspacePool) cfgForWorkspace(root, language string) (config.LSPConfig, bool) {
 	return cfgAmong(p.effectiveLanguages(root), language)
 }
+
+// entryConfig is e's effective config, whether its language is enabled, and
+// whether the project config governing it fails to parse — all from one
+// resolution (resolveLanguages).
+func (p *workspacePool) entryConfig(e *poolEntry) (cfg config.LSPConfig, enabled, invalid bool) {
+	langs, invalid := p.resolveLanguages(e.root)
+	cfg, enabled = cfgAmong(langs, e.language)
+	return cfg, enabled, invalid
+}

internal/cli/pool_languages.go

@@ -62,7 +62,7 @@ type cachedWorkspaceLanguages struct {
 	stamp languageConfigStamp
 	langs []langConfig
 	// invalid marks a project config that did not parse, so langs is the
-	// global fallback (see projectConfigInvalid).
+	// global fallback (see resolveLanguages).
 	invalid bool
 }
 
@@ -197,31 +197,40 @@ func languageBoundaryKnownHome(dir string, atHome bool) bool {
 // LoadProject would fail validation. LogLevel is the sentinel for that, matching
 // what cfgForWorkspace used before this became a shared resolver.
 func (p *workspacePool) effectiveLanguages(root string) []langConfig {
+	langs, _ := p.resolveLanguages(root)
+	return langs
+}
+
+// resolveLanguages is effectiveLanguages plus whether the project config
+// governing root fails to parse (the languages are then the global fallback),
+// both from ONE resolution: asking twice could straddle an edit to the file and
+// pair one state's languages with the other's verdict (PLAN-539 follow-up).
+func (p *workspacePool) resolveLanguages(root string) ([]langConfig, bool) {
 	p.langResolves.Add(1)
 	// LogLevel is written once at construction and never mutated, so it needs no
 	// lock; the maps and slices behind it do (see langsSnapshot). Safe only while
 	// nothing assigns p.baseConfig WHOLE — .LSP is replaced field-wise by
 	// enableLanguage for exactly this reason.
 	if p.baseConfig.LogLevel == "" {
-		return p.langsSnapshot()
+		return p.langsSnapshot(), false
 	}
 	policyRoot := languagePolicyRoot(root)
 	if policyRoot == "" {
-		return p.langsSnapshot()
+		return p.langsSnapshot(), false
 	}
 	stamp := projectLanguageStamp(policyRoot)
 
 	p.languageConfig.mu.RLock()
 	cached, ok := p.languageConfig.cache[policyRoot]
 	p.languageConfig.mu.RUnlock()
 	if ok && cached.stamp == stamp {
-		return cached.langs
+		return cached.langs, cached.invalid
 	}
 
 	p.languageConfig.mu.Lock()
 	defer p.languageConfig.mu.Unlock()
 	if cached, ok = p.languageConfig.cache[policyRoot]; ok && cached.stamp == stamp {
-		return cached.langs
+		return cached.langs, cached.invalid
 	}
 	langs, err := p.resolveProjectLanguages(policyRoot)
 	invalid := err != nil
@@ -245,22 +254,7 @@ func (p *workspacePool) effectiveLanguages(root string) []langConfig {
 		p.languageConfig.cache = make(map[string]cachedWorkspaceLanguages)
 	}
 	p.languageConfig.cache[policyRoot] = resolved
-	return resolved.langs
-}
-
-// projectConfigInvalid reports whether the project config governing root's
-// languages fails to parse — the pool is then running on the global fallback,
-// which is a degradation rather than an edit, so a reconfigure must not act on
-// it (PLAN-539). Resolving first keeps the answer current for the file's stamp.
-func (p *workspacePool) projectConfigInvalid(root string) bool {
-	p.effectiveLanguages(root)
-	policyRoot := languagePolicyRoot(root)
-	if policyRoot == "" {
-		return false
-	}
-	p.languageConfig.mu.RLock()
-	defer p.languageConfig.mu.RUnlock()
-	return p.languageConfig.cache[policyRoot].invalid
+	return resolved.langs, resolved.invalid
 }
 
 // resolveProjectLanguages merges the project config at policyRoot onto the base

internal/cli/pool_reconfigure.go

@@ -116,13 +116,36 @@ func (p *workspacePool) reconfigureEntries(selected func(*poolEntry) bool) int {
 // instead of finishing on the one it started with. An entry governed by a
 // project config that does not parse is left as it is until the file is fixed
 // — the pool's fallback to the global config is a degradation, not an edit.
+//
+// The config is resolved outside p.mu, so it is checked against the pool's
+// config generation under the lock: one that moved means a newer config was
+// published while this one resolved, and the resolve is repeated rather than
+// acting on a stale want — which could otherwise restart a server back onto
+// an old config just after a newer restart finished.
 func (p *workspacePool) reconfigureEntry(e *poolEntry) bool {
-	if p.projectConfigInvalid(e.root) {
-		return false
+	for {
+		gen := p.langsGen.Load()
+		want, enabled, invalid := p.entryConfig(e)
+		if invalid {
+			return false
+		}
+		if p.reconfigureResolved != nil {
+			p.reconfigureResolved(e)
+		}
+		p.mu.Lock()
+		if p.langsGen.Load() != gen {
+			p.mu.Unlock()
+			continue
+		}
+		started := p.startReconfigureLocked(e, want, enabled)
+		p.mu.Unlock()
+		return started
 	}
-	want, enabled := p.cfgForWorkspace(e.root, e.language)
-	p.mu.Lock()
-	defer p.mu.Unlock()
+}
+
+// startReconfigureLocked starts e's restart onto want, or marks a running one
+// to run again. Caller holds p.mu.
+func (p *workspacePool) startReconfigureLocked(e *poolEntry, want config.LSPConfig, enabled bool) bool {
 	if e.reconfiguring {
 		e.reconfigureAgain = true
 		return false
@@ -131,7 +154,7 @@ func (p *workspacePool) reconfigureEntry(e *poolEntry) bool {
 		return false
 	}
 	e.reconfiguring = true
-	go p.restartWithConfig(e, want, enabled)
+	go p.restartWithConfig(e, want, enabled, restartReasonConfig)
 	return true
 }
 
@@ -160,13 +183,29 @@ func serverChange(a, b config.LSPConfig) bool {
 	return !reflect.DeepEqual(a, b)
 }
 
+// Why an entry is restarted in place, for its log lines: a changed config
+// (PLAN-539), or a resync after its file watcher lost events (PLAN-538), which
+// restarts it on the config it already has.
+const (
+	restartReasonConfig = "its new config"
+	restartReasonResync = "a resync after lost file events"
+)
+
 // restartWithConfig is one entry's in-place restart: hibernate it once no call
 // holds it, swap in a supervisor for cfg, and wake it when a session holds it.
-func (p *workspacePool) restartWithConfig(e *poolEntry, cfg config.LSPConfig, enabled bool) {
+// A round that ends with another already requested does not wake the server it
+// built — that would start it on a config about to be replaced (expensive for a
+// server such as jdtls) — so the wake is left to the round that follows, or, if
+// none needs a restart after all, done here at the end.
+func (p *workspacePool) restartWithConfig(e *poolEntry, cfg config.LSPConfig, enabled bool, reason string) {
 	for {
-		p.restartOnce(e, cfg, enabled)
+		deferred := p.restartOnce(e, cfg, enabled, reason)
+		reason = restartReasonConfig // a further round is always a config change's
 		var again bool
 		if cfg, enabled, again = p.nextRestart(e); !again {
+			if deferred {
+				p.wakeHeld(e)
+			}
 			return
 		}
 	}
@@ -187,10 +226,10 @@ func (p *workspacePool) nextRestart(e *poolEntry) (config.LSPConfig, bool, bool)
 		}
 		e.reconfigureAgain = false
 		p.mu.Unlock()
-		if p.projectConfigInvalid(e.root) {
+		want, enabled, invalid := p.entryConfig(e)
+		if invalid {
 			continue // re-checks the flag; ends the round unless a request came in
 		}
-		want, enabled := p.cfgForWorkspace(e.root, e.language)
 		p.mu.Lock()
 		restart := needsRestartLocked(e, want, enabled)
 		p.mu.Unlock()
@@ -201,24 +240,25 @@ func (p *workspacePool) nextRestart(e *poolEntry) (config.LSPConfig, bool, bool)
 }
 
 // restartOnce is one restart round: hibernate e once no call holds it, swap in
-// a supervisor for cfg, and wake it when a session holds it.
-func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled bool) {
+// a supervisor for cfg, and wake it when a session holds it — unless another
+// round is already requested, when it reports the wake deferred instead.
+func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled bool, reason string) (wakeDeferred bool) {
 	var env []string
 	var goWorkOff string
 	if enabled {
 		env, goWorkOff = goLSPEnv(e.language, e.root, cfg, envFor(cfg))
 	}
 	if err := p.hibernateWhenIdle(e); err != nil {
-		slog.Warn("pool: could not restart a language server for its new config", "root", e.root, "language", e.language, "err", err)
-		return
+		slog.Warn("pool: could not restart a language server for "+reason, "root", e.root, "language", e.language, "err", err)
+		return false
 	}
 	// p.mu is held from here (hibernateWhenIdle returns with it).
 	if p.closed || p.entries[poolKey{e.root, e.language}] != e || !enabled {
 		p.mu.Unlock()
 		if !enabled {
 			slog.Info("pool: language disabled by config — its server stays stopped", "root", e.root, "language", e.language)
 		}
-		return
+		return false
 	}
 	e.lspCfg = cfg
 	e.goWorkOff = goWorkOff
@@ -228,20 +268,65 @@ func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled
 		e.cache.Clear()
 	}
 	held := e.refs > 0
+	if held && e.reconfigureAgain {
+		p.mu.Unlock()
+		slog.Info("pool: restarted a language server for "+reason+"; its wake waits for the newer config", "root", e.root, "language", e.language)
+		return true
+	}
 	var readyCh <-chan error
 	var err error
 	if held {
 		readyCh, err = p.wakeLocked(e)
 	}
 	p.mu.Unlock()
-	slog.Info("pool: restarted a language server for its new config", "root", e.root, "language", e.language, "woken", held && err == nil)
+	slog.Info("pool: restarted a language server for "+reason, "root", e.root, "language", e.language, "woken", held && err == nil)
+	p.afterWake(e, held, readyCh, err)
+	return false
+}
+
+// wakeHeld wakes e when a session still holds it and it is hibernated, as the
+// last round of a restart that deferred its wake leaves it. An entry torn down,
+// woken already, or taken by a newer restart (which wakes it itself) is left.
+//
+// It resolves the config first: the request that deferred the wake may have
+// DISABLED the language, and a disabled language's server stays stopped. The
+// resolve is checked against the config generation under p.mu, as in
+// reconfigureEntry, so a disable published in between is not missed.
+func (p *workspacePool) wakeHeld(e *poolEntry) {
+	for {
+		gen := p.langsGen.Load()
+		if _, enabled, invalid := p.entryConfig(e); !enabled && !invalid {
+			return
+		}
+		if p.reconfigureResolved != nil {
+			p.reconfigureResolved(e)
+		}
+		p.mu.Lock()
+		if p.langsGen.Load() == gen {
+			break // p.mu stays held
+		}
+		p.mu.Unlock()
+	}
+	if p.closed || p.entries[poolKey{e.root, e.language}] != e || e.reconfiguring || e.refs == 0 || e.state != poolHibernated {
+		p.mu.Unlock()
+		return
+	}
+	readyCh, err := p.wakeLocked(e)
+	p.mu.Unlock()
+	slog.Info("pool: woke a restarted language server once its config settled", "root", e.root, "language", e.language, "woken", err == nil)
+	p.afterWake(e, true, readyCh, err)
+}
+
+// afterWake finishes a restart's wake outside p.mu: it reports a failed wake
+// (the next acquire retries), and otherwise reattaches the file watcher
+// hibernation dropped — the wake does not rebuild it (attachWatcher, PLAN-536)
+// — and watches for a late start failure.
+func (p *workspacePool) afterWake(e *poolEntry, woke bool, readyCh <-chan error, err error) {
 	if err != nil {
 		slog.Warn("pool: waking a reconfigured language server failed; the next acquire retries", "root", e.root, "language", e.language, "err", err)
 		return
 	}
-	if held {
-		// The wake does not rebuild the file watcher hibernation dropped; it is
-		// attached here, after the unlock (attachWatcher, PLAN-536).
+	if woke {
 		p.attachWatcher(e)
 	}
 	if readyCh != nil {

internal/cli/pool_reconfigure_test.go

@@ -311,7 +311,7 @@ func TestRestartWithConfig_DoesNotReviveAfterClose(t *testing.T) {
 	p.close()
 	g := cfg.LSP["go"]
 	g.Args = []string{"35"}
-	p.restartWithConfig(e, g, true)
+	p.restartWithConfig(e, g, true, restartReasonConfig)
 	if now, c, state := entryNow(p, e); now != sup || slices.Equal(c.Args, []string{"35"}) || state.running() {
 		t.Fatalf("a closed pool's entry was restarted (new supervisor %v, args %v, state %s)", now != sup, c.Args, state)
 	}