From 4dc1c9a9e156972fb779eff977656e6dbeeb5341 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 12:19:09 +1100 Subject: [PATCH 1/6] fix(pool): an [lsp] restart can no longer land on a superseded config PLAN-539's three MINOR windows, and PLAN-538's log wording: - reconfigureEntry checks the pool's config generation under p.mu after its unlocked resolve and re-resolves when it moved, so a stale want can no longer restart a server back onto an old config after a newer restart finished; - the effective config and the project file's validity come from one resolution (resolveLanguages / entryConfig) instead of two that could straddle an edit; - a restart round that ends with another already requested defers its wake to that round (wakeHeld wakes it at the end if none needs a restart), so a superseded config's server is never started; - a resync's restart is logged as a resync, not as a config change. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++ internal/cli/pool.go | 4 + internal/cli/pool_config.go | 9 ++ internal/cli/pool_languages.go | 34 +++-- internal/cli/pool_reconfigure.go | 103 +++++++++++++--- internal/cli/pool_reconfigure_test.go | 2 +- internal/cli/pool_reconfigure_window_test.go | 123 +++++++++++++++++++ internal/cli/pool_resync.go | 2 +- internal/cli/pool_resync_test.go | 19 +++ 9 files changed, 263 insertions(+), 41 deletions(-) create mode 100644 internal/cli/pool_reconfigure_window_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c2f5d9b4..25a7c0df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,14 @@ that never identifies itself on a connection is invisible to this rule, for 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 diff --git a/internal/cli/pool.go b/internal/cli/pool.go index dbaf0e94..c34b5c8c 100644 --- a/internal/cli/pool.go +++ b/internal/cli/pool.go @@ -89,6 +89,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 between its + // 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 diff --git a/internal/cli/pool_config.go b/internal/cli/pool_config.go index 5ce6a528..51253c33 100644 --- a/internal/cli/pool_config.go +++ b/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 +} diff --git a/internal/cli/pool_languages.go b/internal/cli/pool_languages.go index c7efc3a6..269a96cf 100644 --- a/internal/cli/pool_languages.go +++ b/internal/cli/pool_languages.go @@ -197,17 +197,26 @@ 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) @@ -215,13 +224,13 @@ func (p *workspacePool) effectiveLanguages(root string) []langConfig { 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 diff --git a/internal/cli/pool_reconfigure.go b/internal/cli/pool_reconfigure.go index 91ed1ac8..40b8863d 100644 --- a/internal/cli/pool_reconfigure.go +++ b/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,28 @@ 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) var again bool if cfg, enabled, again = p.nextRestart(e); !again { + if deferred { + p.wakeHeld(e) + } return } } @@ -187,10 +225,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,16 +239,17 @@ 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 { @@ -218,7 +257,7 @@ func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled 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 +267,46 @@ 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. +func (p *workspacePool) wakeHeld(e *poolEntry) { + p.mu.Lock() + 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() + 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 { diff --git a/internal/cli/pool_reconfigure_test.go b/internal/cli/pool_reconfigure_test.go index 5a50872c..03661c6c 100644 --- a/internal/cli/pool_reconfigure_test.go +++ b/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) } diff --git a/internal/cli/pool_reconfigure_window_test.go b/internal/cli/pool_reconfigure_window_test.go new file mode 100644 index 00000000..78cb4d09 --- /dev/null +++ b/internal/cli/pool_reconfigure_window_test.go @@ -0,0 +1,123 @@ +package cli + +import ( + "context" + "slices" + "strings" + "testing" + "time" + + "github.com/plumbkit/plumb/internal/config" +) + +// pool_reconfigure_window_test.go — the PLAN-539 follow-ups: one resolution +// per reconfigure, a stale resolve never undoing a newer restart, and no wake +// for a config already superseded. + +// waitSettled waits until e has no restart running. +func waitSettled(t *testing.T, p *workspacePool, e *poolEntry) { + t.Helper() + eventually(t, 5*time.Second, "the restart never finished", func() bool { + p.mu.Lock() + defer p.mu.Unlock() + return !e.reconfiguring + }) +} + +// TestReconfigureEntry_ResolvesOnce: the effective config and the project +// config's validity come from one resolution. +func TestReconfigureEntry_ResolvesOnce(t *testing.T) { + p, _ := reconfigPool(t) + e, err := p.acquireLang(context.Background(), markedRoot(t, "go.mod"), "go", true) + if err != nil { + t.Fatal(err) + } + before := p.langResolves.Load() + if p.reconfigureEntry(e) { + t.Fatal("control: an unchanged config started a restart") + } + if n := p.langResolves.Load() - before; n != 1 { + t.Fatalf("one reconfigure resolved the config %d times, want 1", n) + } +} + +// TestReconfigureEntry_AStaleResolveDoesNotUndoANewerRestart: a reconfigure +// that resolved its config just before a newer one was published, and reaches +// its decision after that newer restart finished, re-resolves instead of +// restarting the server back onto the old config. +func TestReconfigureEntry_AStaleResolveDoesNotUndoANewerRestart(t *testing.T) { + p, cfg := reconfigPool(t) + e, err := p.acquireLang(context.Background(), markedRoot(t, "go.mod"), "go", true) + if err != nil { + t.Fatal(err) + } + fired := false // the publish below re-enters this hook; only the outer call acts + p.reconfigureResolved = func(*poolEntry) { + if fired { + return + } + fired = true + // Inside the stale reconfigure's window: publish a newer config and + // let its restart finish. + p.applyGlobalLSP(withGoArgs(cfg, "50")) + waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"50"}) }) + waitSettled(t, p, e) + } + p.reconfigureEntry(e) // resolved the original config, args 30 + waitSettled(t, p, e) + if _, c, _ := entryNow(p, e); !slices.Equal(c.Args, []string{"50"}) { + t.Fatalf("the server ended on args %v; a stale resolve undid the newer restart", c.Args) + } +} + +// heldAndBusy acquires e with a session hold, and starts a call on it that +// keeps a restart waiting until done is called. +func heldAndBusy(t *testing.T, p *workspacePool) (e *poolEntry, root string, done func()) { + t.Helper() + root = markedRoot(t, "go.mod") + e, err := p.acquireLang(context.Background(), root, "go", true) + if err != nil { + t.Fatal(err) + } + _, done, err = p.acquireCall(context.Background(), root, "go") + if err != nil { + t.Fatal(err) + } + return e, root, done +} + +// TestRestartWithConfig_DoesNotWakeASupersededConfig: a round that ends with +// a newer config already requested leaves its server asleep; the round for the +// newer config wakes it. +func TestRestartWithConfig_DoesNotWakeASupersededConfig(t *testing.T) { + logs := captureLogs(t) + p, cfg := reconfigPool(t) + e, _, done := heldAndBusy(t, p) + p.applyGlobalLSP(withGoArgs(cfg, "60")) + p.applyGlobalLSP(withGoArgs(cfg, "61")) + done() + waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"61"}) }) + waitSettled(t, p, e) + if !strings.Contains(logs.String(), "its wake waits for the newer config") { + t.Errorf("the superseded round woke its server; log:\n%s", logs.String()) + } + if _, _, state := entryNow(p, e); !state.running() { + t.Fatalf("the held server ended %s, want running on the newest config", state) + } +} + +// TestRestartWithConfig_ADeferredWakeIsNotLost: when the request that deferred +// the wake turns out to need no restart, the server is still woken. +func TestRestartWithConfig_ADeferredWakeIsNotLost(t *testing.T) { + p, cfg := reconfigPool(t) + e, root, done := heldAndBusy(t, p) + p.applyGlobalLSP(withGoArgs(cfg, "70")) + p.reconfigureRoot(root) // the same config again: marks the round to re-check + done() + waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"70"}) }) + waitSettled(t, p, e) + eventually(t, 5*time.Second, "a held server whose wake was deferred was left asleep", func() bool { + _, _, state := entryNow(p, e) + return state.running() + }) +} diff --git a/internal/cli/pool_resync.go b/internal/cli/pool_resync.go index e55b33fe..665be6dc 100644 --- a/internal/cli/pool_resync.go +++ b/internal/cli/pool_resync.go @@ -76,7 +76,7 @@ func (p *workspacePool) resyncEntry(e *poolEntry) { return } e.reconfiguring = true - go p.restartWithConfig(e, e.lspCfg, true) + go p.restartWithConfig(e, e.lspCfg, true, restartReasonResync) } // resyncInterval is the minimum gap between an entry's resyncs: losses that diff --git a/internal/cli/pool_resync_test.go b/internal/cli/pool_resync_test.go index c2ce3c35..646c24d9 100644 --- a/internal/cli/pool_resync_test.go +++ b/internal/cli/pool_resync_test.go @@ -449,3 +449,22 @@ func TestWatcherLost_ASkippedHibernationKeepsThePendingResync(t *testing.T) { t.Fatal("a refused hibernation dropped the pending resync") } } + +// TestWatcherLost_TheResyncSaysItIsOne: a resync's restart is logged as one, +// not as a config change. +func TestWatcherLost_TheResyncSaysItIsOne(t *testing.T) { + logs := captureLogs(t) + pool, f := resyncPool(t) + root := goModRoot(t) + e, err := pool.acquireLang(t.Context(), root, "go", true) + if err != nil { + t.Fatal(err) + } + sup := supOf(pool, e) + f.latest(root).lost <- struct{}{} + eventually(t, 5*time.Second, "the server was not resynced", func() bool { return supOf(pool, e) != sup }) + out := logs.String() + if !strings.Contains(out, "restarted a language server for a resync after lost file events") || strings.Contains(out, "for its new config") { + t.Fatalf("the resync's restart is not logged as a resync:\n%s", out) + } +} From 93f7cb52e6335c65001e6fdb5aaaa11f377f665a Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 12:23:12 +1100 Subject: [PATCH 2/6] test(pool): the deferred wake leaves an entry a newer restart owns Co-Authored-By: Claude Opus 5.5 --- internal/cli/pool_reconfigure_window_test.go | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/internal/cli/pool_reconfigure_window_test.go b/internal/cli/pool_reconfigure_window_test.go index 78cb4d09..988c261b 100644 --- a/internal/cli/pool_reconfigure_window_test.go +++ b/internal/cli/pool_reconfigure_window_test.go @@ -121,3 +121,30 @@ func TestRestartWithConfig_ADeferredWakeIsNotLost(t *testing.T) { return state.running() }) } + +// TestWakeHeld_LeavesAnEntryANewerRestartOwns: the deferred wake does not wake +// an entry a newer restart has already taken — that restart wakes it itself. +func TestWakeHeld_LeavesAnEntryANewerRestartOwns(t *testing.T) { + p, _ := reconfigPool(t) + e, err := p.acquireLang(context.Background(), markedRoot(t, "go.mod"), "go", true) + if err != nil { + t.Fatal(err) + } + if err := p.hibernateWhenIdle(e); err != nil { + t.Fatal(err) + } + e.reconfiguring = true // hibernateWhenIdle returned holding p.mu + p.mu.Unlock() + p.wakeHeld(e) + if _, _, state := entryNow(p, e); state != poolHibernated { + t.Fatalf("an entry a newer restart owns was woken (%s)", state) + } + p.mu.Lock() + e.reconfiguring = false + p.mu.Unlock() + p.wakeHeld(e) + eventually(t, 5*time.Second, "control: a held, hibernated entry with no restart running was not woken", func() bool { + _, _, state := entryNow(p, e) + return state.running() + }) +} From 5c8150d3f81b096b65c74cd9226b922fe261b861 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 12:48:13 +1100 Subject: [PATCH 3/6] fix(pool): a deferred wake leaves a just-disabled language stopped Review of #701: when the change that deferred a round's wake disabled the language, nextRestart found nothing to restart (the entry was hibernated) and wakeHeld woke the disabled server anyway. wakeHeld now resolves the config and refuses a disabled language. Also: only a restart's first round carries a resync's reason (later rounds are config changes), wakeHeld logs its wake, and the langsGen comment documents its role as the reconfigure staleness token. Tests pin every wakeHeld guard, a single round waking at once, the config reason, and an invalid project file during a waiting round. Co-Authored-By: Claude Opus 5.5 --- internal/cli/pool.go | 7 +- internal/cli/pool_languages.go | 2 +- internal/cli/pool_reconfigure.go | 8 ++ internal/cli/pool_reconfigure_window_test.go | 112 +++++++++++++++++++ internal/cli/pool_resync_test.go | 4 + 5 files changed, 130 insertions(+), 3 deletions(-) diff --git a/internal/cli/pool.go b/internal/cli/pool.go index c34b5c8c..921082cc 100644 --- a/internal/cli/pool.go +++ b/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" diff --git a/internal/cli/pool_languages.go b/internal/cli/pool_languages.go index 269a96cf..64b0078e 100644 --- a/internal/cli/pool_languages.go +++ b/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 } diff --git a/internal/cli/pool_reconfigure.go b/internal/cli/pool_reconfigure.go index 40b8863d..77f53cac 100644 --- a/internal/cli/pool_reconfigure.go +++ b/internal/cli/pool_reconfigure.go @@ -200,6 +200,7 @@ const ( func (p *workspacePool) restartWithConfig(e *poolEntry, cfg config.LSPConfig, enabled bool, reason string) { for { 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 { @@ -286,7 +287,13 @@ func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled // 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. func (p *workspacePool) wakeHeld(e *poolEntry) { + if _, enabled, invalid := p.entryConfig(e); !enabled && !invalid { + return + } p.mu.Lock() if p.closed || p.entries[poolKey{e.root, e.language}] != e || e.reconfiguring || e.refs == 0 || e.state != poolHibernated { p.mu.Unlock() @@ -294,6 +301,7 @@ func (p *workspacePool) wakeHeld(e *poolEntry) { } 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) } diff --git a/internal/cli/pool_reconfigure_window_test.go b/internal/cli/pool_reconfigure_window_test.go index 988c261b..a74520ab 100644 --- a/internal/cli/pool_reconfigure_window_test.go +++ b/internal/cli/pool_reconfigure_window_test.go @@ -2,6 +2,8 @@ package cli import ( "context" + "os" + "path/filepath" "slices" "strings" "testing" @@ -98,6 +100,9 @@ func TestRestartWithConfig_DoesNotWakeASupersededConfig(t *testing.T) { done() waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"61"}) }) waitSettled(t, p, e) + eventually(t, 5*time.Second, "the deferred wake was never logged", func() bool { + return strings.Contains(logs.String(), "once its config settled") || strings.Contains(logs.String(), "woken=true") + }) if !strings.Contains(logs.String(), "its wake waits for the newer config") { t.Errorf("the superseded round woke its server; log:\n%s", logs.String()) } @@ -148,3 +153,110 @@ func TestWakeHeld_LeavesAnEntryANewerRestartOwns(t *testing.T) { return state.running() }) } + +// TestRestartWithConfig_ADeferredWakeLeavesADisabledLanguageStopped: a change +// that disables the language while a restart waits does not have its server +// woken at the end (review of #701). +func TestRestartWithConfig_ADeferredWakeLeavesADisabledLanguageStopped(t *testing.T) { + p, cfg := reconfigPool(t) + e, _, done := heldAndBusy(t, p) + p.applyGlobalLSP(withGoArgs(cfg, "90")) + off := withGoArgs(cfg, "90") + g := off.LSP["go"] + g.Enabled = false + off.LSP["go"] = g + p.applyGlobalLSP(off) + done() + waitSettled(t, p, e) + time.Sleep(100 * time.Millisecond) + if _, _, state := entryNow(p, e); state != poolHibernated { + t.Fatalf("a disabled language's held server ended %s, want hibernated", state) + } +} + +// TestRestartWithConfig_ASingleRoundWakesAtOnce: with no newer change waiting, +// a held server is woken by its own round, and the restart says it was for its +// new config. +func TestRestartWithConfig_ASingleRoundWakesAtOnce(t *testing.T) { + logs := captureLogs(t) + p, cfg := reconfigPool(t) + e, _, done := heldAndBusy(t, p) + p.applyGlobalLSP(withGoArgs(cfg, "91")) + done() + waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"91"}) }) + waitSettled(t, p, e) + eventually(t, 5*time.Second, "the restart was never logged", func() bool { + return strings.Contains(logs.String(), "restarted a language server for") + }) + out := logs.String() + if strings.Contains(out, "its wake waits for the newer config") || strings.Contains(out, "once its config settled") { + t.Errorf("a single round deferred its wake:\n%s", out) + } + if !strings.Contains(out, "restarted a language server for its new config") { + t.Errorf("a config restart is not logged as one:\n%s", out) + } +} + +// TestWakeHeld_WakesOnlyAHeldHibernatedLiveEntry: the deferred wake leaves an +// entry nobody holds, one already running, and one of a closed pool. +func TestWakeHeld_WakesOnlyAHeldHibernatedLiveEntry(t *testing.T) { + logs := captureLogs(t) + p, _ := reconfigPool(t) + root := markedRoot(t, "go.mod") + e, err := p.acquireLang(context.Background(), root, "go", true) + if err != nil { + t.Fatal(err) + } + p.wakeHeld(e) // running already + if strings.Contains(logs.String(), "waking a reconfigured language server failed") || strings.Contains(logs.String(), "once its config settled") { + t.Errorf("a running entry was woken again:\n%s", logs.String()) + } + p.hibernateEntry(e) + p.mu.Lock() + refs := e.refs + e.refs = 0 + p.mu.Unlock() + p.wakeHeld(e) + if _, _, state := entryNow(p, e); state != poolHibernated { + t.Fatalf("an entry nobody holds was woken (%s)", state) + } + p.mu.Lock() + e.refs = refs + p.mu.Unlock() + p.close() + p.wakeHeld(e) + if _, _, state := entryNow(p, e); state != poolHibernated { + t.Fatalf("a closed pool's entry was woken (%s)", state) + } +} + +// TestRestartWithConfig_AnInvalidProjectFileDoesNotMoveAWaitingRound: a +// project file that breaks while a restart waits leaves the server on the +// config that round installed, not the global fallback. +func TestRestartWithConfig_AnInvalidProjectFileDoesNotMoveAWaitingRound(t *testing.T) { + p, cfg := reconfigPool(t) + p.baseConfig.LogLevel = "info" // the project-config resolver is active + root := markedRoot(t, "go.mod") + if err := os.MkdirAll(filepath.Join(root, ".git"), 0o755); err != nil { + t.Fatal(err) + } + writePoolProjectConfig(t, root, "[lsp.go]\ndiagnostics = \"pull\"\n") + e, err := p.acquireLang(context.Background(), root, "go", true) + if err != nil { + t.Fatal(err) + } + _, done, err := p.acquireCall(context.Background(), root, "go") + if err != nil { + t.Fatal(err) + } + p.applyGlobalLSP(withGoArgs(cfg, "92")) + writePoolProjectConfig(t, root, "[lsp.go\ndiagnostics = \n") + p.invalidateLanguageConfig(root) + p.reconfigureRoot(root) // marks the waiting round to re-check + done() + waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"92"}) }) + waitSettled(t, p, e) + if _, c, _ := entryNow(p, e); c.Diagnostics != "pull" { + t.Fatalf("the server moved onto the global fallback (diagnostics %q)", c.Diagnostics) + } +} diff --git a/internal/cli/pool_resync_test.go b/internal/cli/pool_resync_test.go index 646c24d9..da851cb2 100644 --- a/internal/cli/pool_resync_test.go +++ b/internal/cli/pool_resync_test.go @@ -463,6 +463,10 @@ func TestWatcherLost_TheResyncSaysItIsOne(t *testing.T) { sup := supOf(pool, e) f.latest(root).lost <- struct{}{} eventually(t, 5*time.Second, "the server was not resynced", func() bool { return supOf(pool, e) != sup }) + // The line is written after the swap is visible. + eventually(t, 5*time.Second, "the resync's restart was never logged", func() bool { + return strings.Contains(logs.String(), "restarted a language server for") + }) out := logs.String() if !strings.Contains(out, "restarted a language server for a resync after lost file events") || strings.Contains(out, "for its new config") { t.Fatalf("the resync's restart is not logged as a resync:\n%s", out) From dd2cc1af535392c6d637109aa7d1889f2bb988b8 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 12:50:34 +1100 Subject: [PATCH 4/6] test(pool): the invalid-file round test sets its re-check while the file still parses Co-Authored-By: Claude Opus 5.5 --- internal/cli/pool_reconfigure_window_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/cli/pool_reconfigure_window_test.go b/internal/cli/pool_reconfigure_window_test.go index a74520ab..0994d318 100644 --- a/internal/cli/pool_reconfigure_window_test.go +++ b/internal/cli/pool_reconfigure_window_test.go @@ -250,9 +250,11 @@ func TestRestartWithConfig_AnInvalidProjectFileDoesNotMoveAWaitingRound(t *testi t.Fatal(err) } p.applyGlobalLSP(withGoArgs(cfg, "92")) + p.applyGlobalLSP(withGoArgs(cfg, "93")) // marks the waiting round to re-check + // The file breaks before that re-check: it must not move the server onto + // the global fallback (args 93, push diagnostics). writePoolProjectConfig(t, root, "[lsp.go\ndiagnostics = \n") p.invalidateLanguageConfig(root) - p.reconfigureRoot(root) // marks the waiting round to re-check done() waitReconfigured(t, p, e, func(c config.LSPConfig) bool { return slices.Equal(c.Args, []string{"92"}) }) waitSettled(t, p, e) From a6517e722e638497455aea96a030f1ad614bcbb8 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 13:13:48 +1100 Subject: [PATCH 5/6] fix(pool): the deferred wake checks the config generation, as reconfigure does Re-review of #701: wakeHeld resolved the config outside p.mu, so a disable published in between could still be woken. It now re-resolves when the generation moved, exactly as reconfigureEntry does. Co-Authored-By: Claude Opus 5.5 --- internal/cli/pool.go | 6 ++-- internal/cli/pool_reconfigure.go | 20 ++++++++++--- internal/cli/pool_reconfigure_window_test.go | 31 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/internal/cli/pool.go b/internal/cli/pool.go index 921082cc..aad22ea3 100644 --- a/internal/cli/pool.go +++ b/internal/cli/pool.go @@ -92,9 +92,9 @@ 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 between its - // unlocked resolve and the locked decision: a test seam for the window the - // config generation check closes. Nil in production. + // 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 diff --git a/internal/cli/pool_reconfigure.go b/internal/cli/pool_reconfigure.go index 77f53cac..befafe7b 100644 --- a/internal/cli/pool_reconfigure.go +++ b/internal/cli/pool_reconfigure.go @@ -289,12 +289,24 @@ func (p *workspacePool) restartOnce(e *poolEntry, cfg config.LSPConfig, enabled // 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. +// 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) { - if _, enabled, invalid := p.entryConfig(e); !enabled && !invalid { - return + 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() } - p.mu.Lock() if p.closed || p.entries[poolKey{e.root, e.language}] != e || e.reconfiguring || e.refs == 0 || e.state != poolHibernated { p.mu.Unlock() return diff --git a/internal/cli/pool_reconfigure_window_test.go b/internal/cli/pool_reconfigure_window_test.go index 0994d318..3a7ded46 100644 --- a/internal/cli/pool_reconfigure_window_test.go +++ b/internal/cli/pool_reconfigure_window_test.go @@ -262,3 +262,34 @@ func TestRestartWithConfig_AnInvalidProjectFileDoesNotMoveAWaitingRound(t *testi t.Fatalf("the server moved onto the global fallback (diagnostics %q)", c.Diagnostics) } } + +// TestWakeHeld_ADisableDuringItsResolveIsSeen: a disable published between the +// deferred wake's resolve and its decision leaves the server stopped. +func TestWakeHeld_ADisableDuringItsResolveIsSeen(t *testing.T) { + p, cfg := reconfigPool(t) + e, err := p.acquireLang(context.Background(), markedRoot(t, "go.mod"), "go", true) + if err != nil { + t.Fatal(err) + } + p.hibernateEntry(e) + off := cfg + g := cfg.LSP["go"] + g.Enabled = false + off.LSP = map[string]config.LSPConfig{"go": g, "python": cfg.LSP["python"]} + fired := false // the publish re-enters this hook; only the outer call acts + p.reconfigureResolved = func(*poolEntry) { + if fired { + return + } + fired = true + p.applyGlobalLSP(off) + } + p.wakeHeld(e) + if !fired { + t.Fatal("control: the hook never ran") + } + time.Sleep(100 * time.Millisecond) + if _, _, state := entryNow(p, e); state.running() { + t.Fatalf("a disable published during the deferred wake's resolve was missed (%s)", state) + } +} From 16c50a58ae135236f890cf9994691d32a4799246 Mon Sep 17 00:00:00 2001 From: Atlas from Plumb Date: Fri, 16 Oct 2026 13:47:05 +1100 Subject: [PATCH 6/6] docs(changelog): separate the pool entry after the rebase Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25a7c0df..bb158af1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ that never identifies itself on a connection is invisible to this rule, for 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