Added LSPUnavailableRetryDelay option to configure period when LSPs will be checked again#3096
Open
gmit3 wants to merge 8 commits into
Open
Added LSPUnavailableRetryDelay option to configure period when LSPs will be checked again#3096gmit3 wants to merge 8 commits into
gmit3 wants to merge 8 commits into
Conversation
…ill be checked again
…ill be checked again
… into checking-LSPs-slowdown
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a configurable backoff delay for retrying unavailable LSP servers, wiring a new config option into the LSP Manager.
Changes:
- Introduces
Options.LSPUnavailableRetryDelay(seconds) and assigns a default in config loading. - Stores the computed retry delay on
Managerand uses it inrecentlyUnavailable. - Updates the existing backoff test to use the new default constant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| internal/lsp/manager.go | Computes and applies a configurable unavailable-retry delay. |
| internal/lsp/manager_test.go | Updates test setup to align with the new Manager field/constant. |
| internal/config/load.go | Sets a default for the new config option during defaulting. |
| internal/config/config.go | Adds the new lsp_unavailable_retry_delay option to the schema/model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DisableMetrics bool `json:"disable_metrics,omitempty" jsonschema:"description=Disable sending metrics,default=false"` | ||
| InitializeAs string `json:"initialize_as,omitempty" jsonschema:"description=Name of the context file to create/update during project initialization,default=AGENTS.md,example=AGENTS.md,example=CRUSH.md,example=CLAUDE.md,example=docs/LLMs.md"` | ||
| AutoLSP *bool `json:"auto_lsp,omitempty" jsonschema:"description=Automatically setup LSPs based on root markers,default=true"` | ||
| LSPUnavailableRetryDelay int `json:"lsp_unavailable_retry_delay,omitempty" jsonschema:"description=Seconds to wait before retrying an unavailable LSP server (0 to disable retry),default=999999,minimum=0"` |
Comment on lines
+491
to
+493
| if c.Options.LSPUnavailableRetryDelay <= 0 { | ||
| c.Options.LSPUnavailableRetryDelay = 999999 | ||
| } |
Comment on lines
+24
to
+25
| // defaultUnavailableRetryDelay is the fallback when config doesn't specify one. | ||
| const defaultUnavailableRetryDelay = 999999 * time.Second |
Comment on lines
+65
to
+68
| retryDelay := defaultUnavailableRetryDelay | ||
| if cfg != nil && cfg.Config() != nil && cfg.Config().Options != nil && cfg.Config().Options.LSPUnavailableRetryDelay > 0 { | ||
| retryDelay = time.Duration(cfg.Config().Options.LSPUnavailableRetryDelay) * time.Second | ||
| } |
| return false | ||
| } | ||
| if s.now().Sub(lastUnavailableAt) < unavailableRetryDelay { | ||
| if s.now().Sub(lastUnavailableAt) < s.unavailableRetry { |
Comment on lines
17
to
21
| manager := &Manager{ | ||
| unavailable: csync.NewMap[string, time.Time](), | ||
| now: func() time.Time { return now }, | ||
| unavailableRetry: defaultUnavailableRetryDelay, | ||
| } |
Author
|
I have read the Contributor License Agreement (CLA) and hereby sign the CLA. |
Author
|
recheck |
Author
|
recheck |
Comment on lines
+24
to
+25
| // defaultUnavailableRetryDelay is the fallback when config doesn't specify one. | ||
| const defaultUnavailableRetryDelay = 999999 * time.Second |
Comment on lines
+65
to
+68
| retryDelay := defaultUnavailableRetryDelay | ||
| if cfg != nil && cfg.Config() != nil && cfg.Config().Options != nil && cfg.Config().Options.LSPUnavailableRetryDelay != nil { | ||
| retryDelay = time.Duration(*cfg.Config().Options.LSPUnavailableRetryDelay) * time.Second | ||
| } |
Comment on lines
+280
to
+285
| // LSPUnavailableRetryDelay is the number of seconds to wait before | ||
| // retrying an LSP server that was marked unavailable (e.g. binary not | ||
| // found). 0 means no backoff — the server is retried immediately on | ||
| // every file access. When unset (nil), the default is very large | ||
| // (effectively infinite). | ||
| LSPUnavailableRetryDelay *int `json:"lsp_unavailable_retry_delay,omitempty" jsonschema:"description=Seconds to wait before retrying an unavailable LSP server. 0 disables backoff and retries immediately. When unset, defaults to 999999,default=999999,minimum=0"` |
| manager *powernapconfig.Manager | ||
| callback func(name string, client *Client) | ||
| now func() time.Time | ||
| unavailableRetry time.Duration |
Comment on lines
+68
to
+75
| retryDelay := defaultUnavailableRetryDelay | ||
| if cfg != nil && cfg.Config() != nil && cfg.Config().Options != nil && cfg.Config().Options.LSPUnavailableRetryDelay != nil { | ||
| val := *cfg.Config().Options.LSPUnavailableRetryDelay | ||
| if val >= 0 { | ||
| retryDelay = time.Duration(val) * time.Second | ||
| } | ||
| // val < 0 (including -1) means infinite backoff. | ||
| } |
Comment on lines
+71
to
+73
| if val >= 0 { | ||
| retryDelay = time.Duration(val) * time.Second | ||
| } |
Comment on lines
+25
to
+27
| // defaultUnavailableRetryDelay is used when the config leaves the field unset | ||
| // or explicitly sets it to a negative value. It is math.MaxInt64 nanoseconds | ||
| // (~292 years), which is effectively infinite. |
Comment on lines
+73
to
+76
| const maxSeconds = int(math.MaxInt64 / int64(time.Second)) | ||
| if val > maxSeconds { | ||
| val = maxSeconds | ||
| } |
Comment on lines
+25
to
+28
| // defaultUnavailableRetryDelay is the effective retry delay when the configured | ||
| // value is negative (including the -1 default set by setDefaults). It is | ||
| // math.MaxInt64 nanoseconds (~292 years), which is effectively infinite. | ||
| const defaultUnavailableRetryDelay = time.Duration(math.MaxInt64) |
Comment on lines
+68
to
+81
| retryDelay := defaultUnavailableRetryDelay | ||
| if cfg != nil { | ||
| if conf := cfg.Config(); conf != nil && conf.Options != nil && conf.Options.LSPUnavailableRetryDelay != nil { | ||
| val := *conf.Options.LSPUnavailableRetryDelay | ||
| if val >= 0 { | ||
| const maxSeconds = int(math.MaxInt64 / int64(time.Second)) | ||
| if val > maxSeconds { | ||
| val = maxSeconds | ||
| } | ||
| retryDelay = time.Duration(val) * time.Second | ||
| } | ||
| // val < 0 (including -1) means infinite backoff. | ||
| } | ||
| } |
| manager *powernapconfig.Manager | ||
| callback func(name string, client *Client) | ||
| now func() time.Time | ||
| unavailableRetry time.Duration |
Comment on lines
+68
to
+78
| retryDelay := defaultUnavailableRetryDelay | ||
| if cfg != nil { | ||
| if conf := cfg.Config(); conf != nil && conf.Options != nil && conf.Options.LSPUnavailableRetryDelay != nil { | ||
| val := *conf.Options.LSPUnavailableRetryDelay | ||
| if val >= 0 { | ||
| const maxSeconds = int(math.MaxInt64 / int64(time.Second)) | ||
| if val > maxSeconds { | ||
| val = maxSeconds | ||
| } | ||
| retryDelay = time.Duration(val) * time.Second | ||
| } |
| manager *powernapconfig.Manager | ||
| callback func(name string, client *Client) | ||
| now func() time.Time | ||
| unavailableRetry time.Duration |
Comment on lines
+33
to
35
| // Clearing should make it available immediately. | ||
| manager.clearUnavailable("gopls") | ||
| require.False(t, manager.recentlyUnavailable("gopls")) |
Comment on lines
+18
to
+27
| negOne := -1 | ||
| zero := 0 | ||
| five := 5 | ||
| huge := int(math.MaxInt64) | ||
|
|
||
| require.Equal(t, defaultUnavailableRetryDelay, parseUnavailableRetryDelay(nil)) | ||
| require.Equal(t, defaultUnavailableRetryDelay, parseUnavailableRetryDelay(&negOne)) | ||
| require.Equal(t, time.Duration(0), parseUnavailableRetryDelay(&zero)) | ||
| require.Equal(t, 5*time.Second, parseUnavailableRetryDelay(&five)) | ||
| require.Equal(t, time.Duration(math.MaxInt64), parseUnavailableRetryDelay(&huge)) |
Comment on lines
+103
to
+106
| const maxSeconds = math.MaxInt64 / int64(time.Second) | ||
| if int64(v) > maxSeconds { | ||
| return time.Duration(math.MaxInt64) | ||
| } |
Comment on lines
+280
to
+285
| // LSPUnavailableRetryDelay is the number of seconds to wait before | ||
| // retrying an LSP server that was marked unavailable (e.g. binary not | ||
| // found). -1 or unset means infinite backoff (never retry). 0 means | ||
| // no backoff — the server is retried immediately on every file access. | ||
| // Positive values are interpreted as seconds. | ||
| LSPUnavailableRetryDelay *int `json:"lsp_unavailable_retry_delay,omitempty" jsonschema:"description=Seconds to wait before retrying an unavailable LSP server. -1 or unset means infinite backoff (default). 0 disables backoff and retries immediately. Positive values are seconds,default=-1,minimum=-1"` |
Comment on lines
+80
to
82
| // Clearing should make it available immediately. | ||
| manager.clearUnavailable("gopls") | ||
| require.False(t, manager.recentlyUnavailable("gopls")) |
Comment on lines
+38
to
+39
| store, err := config.Load(dir, "", false) | ||
| require.NoError(t, err) |
Comment on lines
+94
to
+108
| // parseUnavailableRetryDelay converts a config value (in seconds) to a | ||
| // time.Duration for the LSP unavailable backoff. nil or negative values mean | ||
| // infinite backoff (defaultUnavailableRetryDelay). Non-negative values are | ||
| // clamped to math.MaxInt64 to avoid overflow. | ||
| func parseUnavailableRetryDelay(val *int) time.Duration { | ||
| if val == nil || *val < 0 { | ||
| return defaultUnavailableRetryDelay | ||
| } | ||
| v := *val | ||
| const maxSeconds = math.MaxInt64 / int64(time.Second) | ||
| if int64(v) > maxSeconds { | ||
| return defaultUnavailableRetryDelay | ||
| } | ||
| return time.Duration(v) * time.Second | ||
| } |
Author
|
I'm done fighting with Copilot comments... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In Issue #3072, it was observed that Crush often spends a significant amount of time during View/Edit tool executions.
Root cause: Nearly all of this time is spent determining whether an LSP server exists or should start.
How expensive is the check?
lsps.json contains ~300 LSP server definitions
For each server, ~12 file extensions are checked
Each combination is checked against every folder in PATH (70 folders on my system)
This results in roughly 250,000 checks
Impact:
On my system, this takes about 1 minute total (~200µs per check, which is reasonable). There's a default 30-second cooldown before LSPs are checked again, but since the scan takes longer than that, the cooldown rarely helps. This causes tool calls to be stuck unnecessarily.
Fix:
I've made the LSP timeout configurable and set the default to 999,999 seconds. This effectively means Crush needs to be restarted if an LSP appears on PATH. This is acceptable since Crush already requires a restart when the config file is edited.