diff --git a/docs/design/hot-reload/README.md b/docs/design/hot-reload/README.md new file mode 100644 index 0000000000..6ca2937fc6 --- /dev/null +++ b/docs/design/hot-reload/README.md @@ -0,0 +1,73 @@ +# Hot Reload Overall Plan + +This directory tracks the design work for issue +[#3696](https://github.com/QwenLM/qwen-code/issues/3696): a comprehensive +hot-reload system for skills, extensions, MCP servers, LSP servers, and runtime +configuration. + +## Goal + +Users should be able to update skills, extension state, MCP/LSP configuration, +and supported settings without restarting the current Qwen Code session. The +system should preserve conversation context while making runtime state changes +predictable and visible. + +## Sub-task Breakdown + +The hot-reload plan has **6 top-level sub-tasks**. The current tracking issue +splits sub-task 3 into **3a** and **3b** for implementation clarity, so the +execution checklist contains **7 entries**. + +| Task | Scope | Status | Design document | +| ---- | ---------------------------------------- | ------------------------ | -------------------------------------------------------------------- | +| 1 | Settings file change detection | Done in #4933 | [settings-change-detection.md](./settings-change-detection.md) | +| 2 | Skill hot-reload improvements | Done via #2415 and #3923 | Not in this directory | +| 3a | MCP server runtime re-initialization | In progress via #5561 | [mcp-runtime-reinitialization.md](./mcp-runtime-reinitialization.md) | +| 3b | LSP server runtime re-initialization | In progress | [lsp-runtime-reinitialization.md](./lsp-runtime-reinitialization.md) | +| 4 | Unified refresh/cache orchestration | Not started | Pending | +| 5 | User-facing `/reload` slash command | Not started | Pending | +| 6 | `needsRefresh` app-state/UI notification | Not started | Pending | + +## Document Mapping + +- `settings-change-detection.md` corresponds to **sub-task 1: Settings file + change detection**. It provides the watcher infrastructure: detect supported + `settings.json` changes, reload settings from disk, and notify listeners. It + intentionally does not push updated values into `Config` snapshots or restart + runtime subsystems. +- `mcp-runtime-reinitialization.md` corresponds to **sub-task 3a: MCP server + runtime re-initialization**. It consumes settings change events, updates the + runtime MCP configuration, and incrementally reconciles live MCP connections. + The original issue grouped MCP and LSP under top-level sub-task 3; this + document covers the MCP half only. +- `lsp-runtime-reinitialization.md` corresponds to **sub-task 3b: LSP server + runtime re-initialization**. It watches workspace `.lsp.json` changes, + reuses the existing native LSP client, and incrementally reconciles live LSP + servers. + +## Implementation Order + +1. Keep sub-task 1 as the foundation: settings changes are detected and + dispatched, but consumers decide what to refresh. +2. Complete sub-task 3a so MCP server additions, removals, and configuration + edits can take effect at runtime. +3. Add sub-task 3b for LSP runtime re-initialization using the same principle: + update runtime configuration, stop affected servers, and restart only what + changed. +4. Introduce sub-task 4 as the shared orchestration layer for cache and runtime + refreshes across skills, commands, prompts, extensions, MCP, and LSP. +5. Add sub-task 5 as the manual user entry point: `/reload` should call the + unified orchestration path and report what changed. +6. Add sub-task 6 for background-change UX: set `needsRefresh` when a detected + change cannot or should not be fully applied automatically, then prompt the + user to run `/reload`. + +## Design Principle + +Keep each layer narrow: + +- file watching detects and reports settings changes; +- subsystem reinitialization updates only the affected runtime state; +- unified orchestration sequences existing refresh operations; +- UI commands and notifications expose the behavior without duplicating reload + logic. diff --git a/docs/design/hot-reload/lsp-runtime-reinitialization.md b/docs/design/hot-reload/lsp-runtime-reinitialization.md new file mode 100644 index 0000000000..0b50b17819 --- /dev/null +++ b/docs/design/hot-reload/lsp-runtime-reinitialization.md @@ -0,0 +1,658 @@ +# LSP Runtime Hot Reload Design + +## Background + +This design follows the same layering used by +`mcp-runtime-reinitialization.md`: the CLI decides when to trigger reloads, and +Core decides how to update runtime state. It also reuses the watcher principles +from `settings-change-detection.md`: no filesystem side effects at startup, +debounced changes, semantic diffs, serialized listeners, and listener failures +that do not affect the main session. + +The key difference between LSP and MCP is that LSP server configuration does not +live in `settings.json`. Today the native LSP service uses `LspConfigLoader` to +read the workspace `.lsp.json` and enabled extensions' `lspServers` +declarations, writes the result into the single-session `LspServerManager` via +`NativeLspService.discoverAndPrepare()`, and finally starts all configured +servers with `start()`. Therefore `SettingsWatcher` alone cannot detect changes +to the workspace `.lsp.json`. + +## Current Code Assessment + +- LSP startup is controlled only by `--experimental-lsp` in + `packages/cli/src/config/config.ts`. There is currently no + `--allowed-lsp-server-names` flag or equivalent LSP CLI allow-list parameter; + the existing `--allowed-mcp-server-names` flag is MCP-only. +- `NativeLspService` is constructed once during CLI config loading. The startup + path calls `discoverAndPrepare()`, then `start()`, then wraps the service in + `NativeLspClient` and attaches it to `Config`. +- `Config.setLspClient()` and `Config.setLspInitializationError()` currently + throw after initialization, so runtime hot reload should not replace the + client object. It should keep the existing `NativeLspClient` and only + incrementally reconcile the service behind it. +- `LspConfigLoader` only reads the workspace `.lsp.json` and active extensions' + `lspServers`. The workspace `.lsp.json` overrides extension config by server + name. +- `LspServerManager.setServerConfigs()` currently clears all handles; it does + not yet support incremental reconcile. +- The current repository has no shared pool path for LSP. Each session owns its + own `NativeLspService` and subprocess/socket connections. The design should + leave a boundary for a future shared pool, but v1 only implements + single-session mode. + +## Goals + +Make LSP server configuration changes take effect without restarting the +current Qwen Code session: + +- start a server when it is added; +- stop a server when it is removed, and remove it from status and tool routing; +- restart only the changed server when its config changes; +- keep unchanged servers connected and preserve their warm-up state; +- never start servers that are untrusted or not allowed; +- let LSP tools and `/lsp` status observe the new runtime state through the + existing client object. + +## Non-goals + +- Do not add a shared LSP process pool in this change. +- Do not support toggling `--experimental-lsp` at runtime. If LSP was not + enabled at startup, there is no service to reload. +- Do not fully watch extension install/uninstall changes that affect + `lspServers`; manual `/reload` will cover extension config changes. + +## Design + +### 1. Identify Each LSP Server With a Stable Hash + +Add a small helper near the LSP config code: + +```ts +export function lspServerConfigHash(config: LspServerConfig): string; +``` + +The hash must be stable and based on the normalized runtime config produced by +`LspConfigLoader`: + +- `name` +- `languages` +- `transport` +- `command` +- `args` +- `env` +- `initializationOptions` +- `settings` +- `extensionToLanguage` +- `workspaceFolder` +- `rootUri` +- `startupTimeout` +- `shutdownTimeout` +- `restartOnCrash` +- `maxRestarts` +- `trustRequired` +- `socket` + +Object keys must be sorted so JSON property order does not cause unnecessary +restarts. Array order stays significant because command argument order and +language priority can be meaningful. Do not include runtime fields such as +process id, status, restart count, diagnostics, or warm-up state. + +For future shared-pool compatibility, define the pool identity as: + +```text +lsp::: +``` + +The v1 single-session manager only needs to maintain `serverName -> configHash`, +but the same hash can later be reused directly in the pool key. + +### 2. Add Incremental Reconcile to `LspServerManager` + +Hot reload should not reuse `setServerConfigs()`, which clears every handle. +Add: + +```ts +async reconcileServerConfigs( + configs: LspServerConfig[], +): Promise +``` + +Flow: + +1. Build desired maps: `name -> config` and `name -> hash`. +2. For existing handles whose server no longer exists, call the existing + `stopServer()`, then delete the handle. +3. For existing handles whose hash changed, call `stopServer()`, replace the + handle with `{ config, status: 'NOT_STARTED' }`, then start it. +4. For new servers, create `{ config, status: 'NOT_STARTED' }` and start them. +5. For servers whose hash did not change, do nothing and keep the existing + handle. + +Add a private field: + +```ts +private serverConfigHashes = new Map(); +``` + +Clear it in `stopAll()` and `clearServerHandles()`. + +Return: + +```ts +interface LspReconcileResult { + added: string[]; + removed: string[]; + restarted: string[]; + unchanged: string[]; + failed: string[]; +} +``` + +`skipped` is not part of the `LspServerManager` result. The manager only handles +configs that have passed admission; servers rejected by admission are aggregated +into the service-level result by `NativeLspService.reinitialize()`. + +Concurrency: + +- Add a reconcile queue in either `LspServerManager` or `NativeLspService` so + reconciles run serially. Stopping and starting the same process must not race. +- If a new config arrives while a server is still starting, wait for + `handle.startingPromise` before stopping it. Reuse the existing startup lock + instead of adding an extra per-server lock. +- `stopServer()` itself must await `handle.startingPromise` after setting + `stopRequested`, so `stopAll()`, remove, and restart paths all cover crash + restarts that are still assigning their connection/process. + +Failure behavior: + +- If a newly added or changed server fails to start, keep the handle and mark it + as `FAILED` so `/lsp` can explain the failure. +- Do not count a failed start as `added` or `restarted`; report it in + `failed`. +- Do not cache the config hash for a failed start. A later save with the same + config must retry instead of being classified as `unchanged`. +- If startup fails after a connection or process has been created, release that + connection/process before returning. Failed initialization must not leave a + language server process or socket connection alive behind a `FAILED` handle. +- If startup fails before connection creation, including trust rejection, + unsafe command path, or missing command, clear the cached config hash. A later + reconcile with the same config must retry instead of treating the failed + handle as unchanged. +- If a removed server logs an error during shutdown, still delete it from the + handle map. +- One server's startup failure must not block reconcile for other servers. + +Resource cleanup: + +- `stopServer()` must release both sides of an owned server: gracefully shut down + and end the LSP connection, then kill the spawned process if it is still + alive. This matters for `tcp`/`socket` transports that were launched with a + `command`; closing the socket alone is not enough. +- `process.kill()` must be isolated with its own error handling. A process that + exits during cleanup must not abort the rest of reconcile. +- Graceful shutdown must always have a bounded wait. If the server config does + not specify `shutdownTimeout`, use the default shutdown timeout instead of + awaiting `connection.shutdown()` forever. +- Shutdown timeout timers must be cleared when shutdown completes or fails so a + large timeout does not retain the handle longer than necessary. +- The underlying `shutdown()` promise must be observed even when the timeout + wins the race, so a late server-side rejection cannot surface as an + unhandled rejection. +- `stopAll()` must participate in the same reconcile queue as hot reload. It is + not enough to wait for the current queue and then iterate handles, because a + new reconcile could otherwise enter between the wait and handle cleanup. +- `NativeLspService.stop()` must also cancel any in-flight or queued + `reinitialize()` operation before stopping servers. The implementation uses + cooperative cancellation with `AbortController`: stop marks the service as + stopping, aborts the active reload, and every queued reload checks the signal + before loading config, reconciling, clearing document tracking, replaying + documents, or waiting on replay delays. This prevents shutdown from blocking + indefinitely on a slow reload while also preventing a cancelled reload from + starting new LSP processes after `stopAll()`. +- Crash restarts must also serialize through the reconcile queue, or clear the + hash when they permanently fail. They must not start a replacement process in + parallel with a config-change reconcile. +- Crash restart reset must isolate `connection.end()` and `process.kill()` + errors. Reset runs when the old connection/process may already be broken, and + cleanup failures must not prevent the queued restart from continuing. +- `NativeLspService.stop()` must clear `openedDocuments` and `lastConnections` + after `serverManager.stopAll()` so a stopped service does not retain old + document sets or connection objects. + +### 3. Add `NativeLspService.reinitialize()` + +Add: + +```ts +async reinitialize(): Promise +``` + +Flow: + +1. If `requireTrustedWorkspace` is true and `!config.isTrustedFolder()`, call + `serverManager.stopAll()` and return. This prevents old LSP processes from + continuing after the workspace becomes untrusted. +2. Use the existing `LspConfigLoader` to load the workspace `.lsp.json` and + extension configs. +3. Merge configs using the current precedence. +4. Apply the LSP admission filter before reconcile. +5. Call `serverManager.reconcileServerConfigs(serverConfigs)`. +6. Clear `openedDocuments` and `lastConnections` only for removed and + successfully restarted servers; preserve document state for unchanged and + failed servers. Failed servers keep their document tracking so a later + successful restart can replay the same open documents. +7. For successfully restarted servers, replay `textDocument/didOpen` for + documents that were open before the restart. This gives the replacement + server the same document context without waiting for the next hover, + completion, or diagnostic request to lazily reopen each file. After replaying + one or more documents for a server, wait for the same document-open delay + used by lazy `ensureDocumentOpen()` before reporting reload completion. + +The open-document snapshot must be taken after `reconcileServerConfigs()` +returns and must be scoped to `reconcile.restarted`. Documents opened while +reconcile is pending are then included in the replay snapshot before tracking is +cleared for restarted servers. + +Initial discovery should use the same admission filter before calling +`setServerConfigs()`. This keeps startup and hot reload status consistent for +per-server `trustRequired` filtering in untrusted workspaces. + +`.lsp.json` parse failures need special handling: do not treat parse failure as +empty config. The watcher should report an invalid-config event so the CLI can +show a user-visible error, but it must not call `reinitialize()` for that event. +`reinitialize()` should preserve the old runtime state, skip reconcile, and +write the error to status/logs. Only deleting the file, or parsing a valid empty +JSON config, means the desired config is empty. + +Cold startup and hot reload intentionally use different user-config parsing +strictness: + +- `loadUserConfigs()` stays lenient for startup compatibility. It skips invalid + server entries and returns the valid entries that can be built. +- `loadUserConfigsStrict()` is used by hot reload. If the existing `.lsp.json` + is syntactically valid but contains an invalid top-level shape or a server + entry that cannot be built, it returns an error and `reinitialize()` does not + reconcile. This preserves the currently running LSP state for invalid edits. + The strict path must not introduce field-level validation that cold startup + does not also enforce, because that would make a config valid at startup but + invalid on the next save. Tightening known-field validation should be handled + as a separate compatibility decision for both startup and hot reload. If the + file is missing or is deleted during the strict load, treat that `ENOENT` as a + valid empty user config, because deleting `.lsp.json` is the explicit way to + remove all workspace user LSP servers. + +`NativeLspService.reinitialize()` returns a service-level result: + +```ts +interface LspServiceReinitializeResult { + reconcile: LspReconcileResult; + skipped: Array<{ + name: string; + reason: 'server_trust_required'; + }>; +} +``` + +Add an optional `reinitialize()` method to `NativeLspClient` and delegate to the +service. To avoid opaque type assertions in `Config.reinitializeLsp()`, extend +the `LspClient` interface directly: + +```ts +reinitialize?: () => Promise; +``` + +Add to `Config`: + +```ts +async reinitializeLsp(): Promise +``` + +When LSP is disabled or no client exists, this is a no-op. This method must not +replace the client after `Config.initialize()`. + +Because `setLspInitializationError()` currently rejects calls after +initialization, add a runtime-safe private state setter: + +```ts +private setRuntimeLspInitializationError(error: Error | string | undefined): void +``` + +`reinitializeLsp()` uses it to expose reload failures through +`getLspStatusSnapshot()` without loosening the public post-init client mutation +API. A returned reconcile result with `failed` servers is a partial failure, not +a clean success. `reinitializeLsp()` must set `initializationError` for that +case and only clear the error when the reload has no failed servers. + +### 4. Admission and Permission Boundary + +Current LSP safety checks include: + +- `--experimental-lsp` is the only enablement switch; +- workspace trust is checked before discovery/startup; +- each server's `trustRequired` defaults to true; +- command existence and command path safety are checked before spawn; +- `workspaceFolder` is constrained to the workspace root. + +Hot reload must preserve these checks and complete them before starting a new +server or restarting a changed server. The key rule is: do not spawn first and +decide whether the server is allowed later. + +Workspace `.lsp.json` is workspace-controlled input. User configs must +therefore always be treated as `trustRequired: true`, even if the file +explicitly declares `"trustRequired": false`. Extension-provided LSP configs may +still use their declared `trustRequired` value. This prevents an untrusted +workspace from lowering its own trust boundary. + +Environment variables from `.lsp.json` are also workspace-controlled. Runtime +spawn may merge allowed env overrides, but code-injection variables such as +`NODE_OPTIONS`, `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_INSERT_LIBRARIES`, and +`DYLD_LIBRARY_PATH` must not be overridden by LSP config. `PATH` is allowed for +the actual server process to preserve common toolchain setups. Command +existence probing may keep regular config-provided env values that probes may +need, but it must not use config-provided `PATH` when resolving bare command +names. This prevents a malicious workspace `PATH` from redirecting a probe such +as `clangd --version` to an unintended executable before the real startup path. +Sensitive env key filtering, including the probe-only `PATH` filter, must be +case-insensitive so Windows-style case-insensitive environment names such as +`Path`, `node_options`, or `Ld_PreLoad` cannot bypass the denylist. + +Allow-list boundary: + +- The current repository does not support a CLI allow-list for LSP server names. + I confirmed LSP only has `--experimental-lsp`; allow-list parameters are + MCP-only. +- If this feature adds `--allowed-lsp-server-names`, it must behave like the MCP + startup allow-list and act as an upper bound for the entire session lifetime. + Runtime config may narrow this set, but it must not expand beyond the CLI + startup upper bound. +- Store the startup upper bound in `ConfigParameters.lsp`: + +```ts +cliAllowedLspServerNames?: string[]; +``` + +Expose a getter for it. Do not read the upper bound from mutable settings. + +Admission should be extracted into a pure function: + +```ts +filterLspServerConfigs(configs, { + workspaceTrusted, + requireTrustedWorkspace, + cliAllowedServerNames, +}): { + admitted: LspServerConfig[]; + skipped: Array<{ + name: string; + reason: 'server_trust_required'; + }>; +} +``` + +Even though there is no LSP approval store or CLI allow-list today, this helper +makes the security boundary explicit and leaves room for future hash-based +approval gates. If a future `--allowed-lsp-server-names` flag is added, it +should add a `not_allowed` skipped reason at that time instead of carrying an +unwired allow-list path in v1. + +Trust semantics must match the current startup path: + +- If `requireTrustedWorkspace` is true and the workspace is untrusted, + `NativeLspService.reinitialize()` stops all servers at the service layer and + returns. It does not enter the admission filter and does not preserve old + servers. +- If `requireTrustedWorkspace` is false, the service does not short-circuit + globally, but the admission filter still skips individual servers with + `trustRequired: true`. +- If the workspace is trusted, `trustRequired` does not block the server. + +### 5. Triggering + +Two trigger paths are needed. + +#### Automatic Workspace `.lsp.json` Trigger + +Add a narrow `LspConfigWatcher` in the CLI, modeled after `SettingsWatcher` but +with a smaller responsibility: + +- watch only the workspace root and strictly match basename `.lsp.json`; +- do not create any directory or file; +- debounce for 300 ms; +- compare `.lsp.json` before/after using parse + canonicalize so formatting-only + changes do not trigger reload; +- treat `ENOENT` as deletion; +- distinguish JSON parse failures from other read failures. Both should notify + the listener with a user-visible invalid-config event and preserve the old + runtime state, but the error message must reflect whether the file was invalid + JSON or unreadable; +- file deletion is a separate event and should notify the reload listener, + producing empty workspace config; +- run callbacks serially; +- use listener timeout and failure isolation matching `SettingsWatcher`; +- advance the stored semantic snapshot only after listener notification succeeds. + If the listener throws or times out, retain the previous snapshot so saving the + same content again retries the reload. + +Register the watcher only when `config.isLspEnabled()` and the client supports +`reinitialize()`. On change, call: + +```ts +await config.reinitializeLsp(); +``` + +Then emit an explicit runtime event such as `AppEvent.LspStatusChanged`. +UI surfaces such as `/lsp`, `/about`, or `/status` can subscribe to that event +to refresh. If reconcile returns partial failures, emit the status-changed event +before throwing back to the watcher; this lets the UI observe successfully +restarted servers while the watcher still retains the old semantic snapshot for +retry. On failure, also show a user-visible error through `AppEvent.LogError`; +include the underlying parser/startup error message when available, and do not +only write a debug log. + +#### Manual `/reload` Trigger + +When the future `/reload` command lands, it should call both: + +```ts +await config.reinitializeMcpServers(...); +await config.reinitializeLsp(); +``` + +Manual reload also provides the fallback path for extension `lspServers` +changes, because those changes may not map to a workspace `.lsp.json` file +event. + +## Single Session and Shared Pool + +Current state: only single-session mode exists. The repository has no LSP +equivalent of the MCP transport pool. + +v1: implement incremental reconcile inside `LspServerManager`. Each session owns +its own process and socket. + +Future shared pool: keep `NativeLspService` as the consumer and replace +`LspServerManager` internals with acquire/release of: + +```text +lsp::: +``` + +pool entries. Admission filtering must still happen before acquire, matching the +MCP shared-pool fix, so disallowed or untrusted servers cannot be started +through the pool path. + +## Unit Test Plan + +Prioritize unit tests. Integration tests against real LSP servers are slow and +environment-dependent, so they are not required. + +### Core Tests + +`packages/core/src/lsp/configHash.test.ts` + +- hash ignores object key order; +- changes to command, args order, env, settings, workspace folder, socket, and + trust requirement change the hash; +- hash excludes status/process/runtime fields. + +`packages/core/src/lsp/LspServerManager.test.ts` + +- adding a server starts it exactly once; +- removing a server shuts it down and deletes it from handles; +- hash changes stop the old handle and start a new handle; +- unchanged hash does not stop/start and preserves handle identity; +- startup failure after connection creation releases the connection and owned + process; +- stopping a `tcp`/`socket` server launched by `command` closes the connection + and kills the owned process; +- shutdown timeout timers are cleared when shutdown completes first; +- missing `shutdownTimeout` still uses the default shutdown timeout and cannot + block reconcile forever; +- `stopAll()` waits for in-flight startup before releasing resources; +- `stopAll()` is serialized through the reconcile queue and cannot run + concurrently with a later reconcile; +- `process.kill()` errors are logged and do not abort cleanup; +- one server startup failure does not affect another server's reconcile; +- concurrent reconciles run serially; +- `stopAll()` and `clearServerHandles()` clear the hash map; +- failed starts are reported in `failed`, are not reported as added/restarted, + and do not cache their config hash; +- initial startup failures clear the cached hash so a later reconcile with the + same config retries; +- crash restarts serialize with reconcile and clear cached hashes on permanent + failure; +- crash restart reset ignores connection/process cleanup errors and continues + the queued restart; +- command existence probing keeps regular config-provided env values, but does + not use config-provided `PATH`, and code-injection env overrides are filtered + before spawn; +- reconcile return value contains added/removed/restarted/unchanged/failed, not + admission skipped. + +Mock `createLspConnection`, initialization, and shutdown in tests. Do not start +real language servers. + +`packages/core/src/lsp/NativeLspService.test.ts` + +- `reinitialize()` loads workspace and extension config and passes merged config + to manager reconcile; +- `.lsp.json` parse failure preserves old runtime state and does not call + manager reconcile; +- strict hot reload rejects invalid top-level shapes and server entries that + cannot be built without reconciling, while cold startup keeps loading valid + entries from the same file; +- deleting `.lsp.json` treats workspace config as empty and triggers reconcile; +- strict loading treats `ENOENT` as an empty user config, including the + deletion race where the file disappears between watcher notification and + reload; +- untrusted workspace stops all servers and does not reconcile/start; +- initial discovery applies the same per-server `trustRequired` admission filter + as hot reload; +- workspace `.lsp.json` cannot opt out of `trustRequired`; +- if a CLI allow-list is implemented, the upper bound filters admitted configs; +- service-level return value aggregates admission skipped reasons; +- restarted/removed servers only clear their own document tracking. +- failed servers do not clear document tracking and can replay those documents + after a later successful restart. +- restarted servers replay `textDocument/didOpen` for previously opened + documents after the replacement server is ready, then wait for the + document-open processing delay. +- documents opened while reconcile is pending are included in the replay + snapshot for restarted servers. +- `stop()` cancels in-flight replay delay and queued `reinitialize()` calls + before they can start new servers. +- `stop()` clears document tracking caches after stopping all servers. + +`packages/core/src/config/config.test.ts` + +- `reinitializeLsp()` is a no-op when disabled or no client exists; +- when enabled and the client supports `reinitialize`, it delegates the call; +- when reinitialize throws, the status snapshot exposes the initialization/reload + error. +- when reinitialize returns partial failures, the status snapshot exposes an + initialization/reload error until a later fully successful reload clears it. + +### CLI Tests + +`packages/cli/src/config/lspConfigWatcher.test.ts` + +- does not create `.lsp.json`; +- detects create/modify/delete; +- ignores unrelated files; +- ignores formatting-only changes after canonical parse; +- parse failure emits an invalid-config notification for user-visible feedback + and does not trigger LSP reinitialization; +- non-ENOENT read failure emits a user-visible read-failure message and does not + trigger LSP reinitialization; +- deleting `.lsp.json` triggers the reload listener; +- duplicate file events are debounced; +- slow listeners run serially; +- listener failure does not advance the stored snapshot and the same content can + be retried by a later notification. + +`packages/cli/src/ui/AppContainer.test.tsx` or the corresponding event test + +- `AppEvent.LspStatusChanged` triggers UI refresh; +- reload failure emits a user-visible error through `AppEvent.LogError`. +- partial reconcile failure still emits `AppEvent.LspStatusChanged` before the + listener rejects, so UI state can reflect successful parts of the reload. + +`packages/cli/src/config/config.test.ts` + +- preserve the existing assertion that `--experimental-lsp` constructs and + starts native LSP; +- if `--allowed-lsp-server-names` is added, the parser supports comma-separated + values and repeated flags, and stores them as the startup upper bound. + +`packages/cli/src/ui/commands/lspCommand.test.ts` + +- if `LspStatusSnapshot` exposes skipped reasons, status output can show + skipped/disallowed servers. + +Coverage goals: new pure functions should be near 100%; watcher branch coverage +should be comparable to `SettingsWatcher`; manager reconcile must cover +add/remove/change/unchanged/failure/concurrency. + +## Strict Review + +### Conclusion + +1. **v1 should not use stop-all/start-all.** + That implementation is simplest, but every save would restart unchanged + language servers and lose warm state. The current manager already has + per-server lifecycle methods, and incremental reconcile is a manageable + amount of additional code. + +2. **Do not put `.lsp.json` changes into `SettingsWatcher`.** + `SettingsWatcher` is responsible for settings-scope reloads. Making it watch + arbitrary workspace files would blur the contract and make MCP/settings + behavior harder to reason about. A separate, narrow `.lsp.json` watcher is + clearer. + +3. **Do not replace `NativeLspClient` after initialization.** + `Config.setLspClient()` explicitly forbids post-init mutation. Updating the + service behind the adapter avoids expanding the lifecycle API. + +4. **Admission must happen before process spawn or pool acquire.** + This is the same risk called out in the MCP shared-pool design. Even though + LSP has no pool today, service-level reload results should return pre-start + filtering skipped reasons so a future pool path does not accidentally start a + rejected server. + +5. **A new LSP CLI allow-list is optional, but if added it must be an upper + bound.** + The current code has no LSP allow-list. The design must not allow settings to + expand command-line restrictions at runtime, or it would be weaker than MCP + hot-reload security semantics. + +### Remaining Risks + +- Extension `lspServers` may change without `.lsp.json` changing. The automatic + watcher does not cover all extension filesystem changes; manual `/reload` + covers that path. +- Some language servers do not tolerate rapid restarts well. Serialized + reconcile and debounce reduce the risk, but tests should cover fast + consecutive changes. +- TCP/socket servers may be externally managed daemons. Reconcile should close + the connection, but it should only assume ownership of the process when this + process spawned the server via `command`. diff --git a/packages/cli/src/config/lsp-config-watcher.test.ts b/packages/cli/src/config/lsp-config-watcher.test.ts new file mode 100644 index 0000000000..5b3392d14e --- /dev/null +++ b/packages/cli/src/config/lsp-config-watcher.test.ts @@ -0,0 +1,393 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { LspConfigWatcher } from './lsp-config-watcher.js'; + +// Keep chokidar fully in-process so watcher lifecycle tests can trigger file +// events deterministically without arming real filesystem watchers. +const chokidarMock = vi.hoisted(() => { + const handlers = new Map void>(); + const watcher = { + on: vi.fn((event: string, handler: (...args: unknown[]) => void) => { + handlers.set(event, handler); + return watcher; + }), + close: vi.fn(async () => {}), + }; + return { + handlers, + watch: vi.fn(() => watcher), + watcher, + }; +}); + +const debugLoggerMock = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('chokidar', () => ({ + watch: chokidarMock.watch, +})); + +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => ({ + ...(await importOriginal()), + createDebugLogger: vi.fn(() => debugLoggerMock), +})); + +// The watcher intentionally keeps its refresh pipeline private. These tests +// exercise it directly to cover semantic diffing and timeout behavior without +// waiting on real chokidar scheduling. +type TestableWatcher = { + listener?: (event: unknown) => void | Promise; + handleChange(): Promise; + notifyListener(event: unknown): Promise; +}; + +const tempDirs: string[] = []; + +function makeTempDir(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-watcher-')); + tempDirs.push(dir); + return dir; +} + +describe('LspConfigWatcher', () => { + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + chokidarMock.handlers.clear(); + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does not create .lsp.json during construction', () => { + const dir = makeTempDir(); + new LspConfigWatcher(dir); + + expect(fs.existsSync(path.join(dir, '.lsp.json'))).toBe(false); + }); + + it('notifies on create and suppresses formatting-only changes', async () => { + const dir = makeTempDir(); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + const listener = vi.fn(); + watcher.listener = listener; + + fs.writeFileSync( + path.join(dir, '.lsp.json'), + '{"typescript":{"command":"tsserver"}}', + ); + await watcher.handleChange(); + expect(listener).toHaveBeenCalledWith({ + path: path.join(dir, '.lsp.json'), + changeType: 'created', + }); + expect(debugLoggerMock.info).toHaveBeenCalledWith( + `LSP config changed: created ${path.join(dir, '.lsp.json')}`, + ); + + fs.writeFileSync( + path.join(dir, '.lsp.json'), + JSON.stringify({ typescript: { command: 'tsserver' } }, null, 2), + ); + await watcher.handleChange(); + expect(listener).toHaveBeenCalledTimes(1); + }); + + // Invalid JSON should be visible to the user, but it must not replace the + // current runtime state. A later delete still needs to reconcile to empty. + it('notifies invalid config and notifies on later deletion', async () => { + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + fs.writeFileSync(configPath, '{"typescript":{"command":"tsserver"}}'); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + const listener = vi.fn(); + watcher.listener = listener; + + fs.writeFileSync(configPath, '{'); + await watcher.handleChange(); + expect(listener).toHaveBeenCalledWith({ + path: configPath, + changeType: 'invalid', + error: + 'Invalid JSON in .lsp.json; existing LSP runtime state is unchanged.', + }); + + fs.unlinkSync(configPath); + await watcher.handleChange(); + expect(listener).toHaveBeenCalledWith({ + path: configPath, + changeType: 'deleted', + }); + }); + + it('reports read errors separately from invalid JSON', async () => { + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + const listener = vi.fn(); + watcher.listener = listener; + fs.mkdirSync(configPath); + + await watcher.handleChange(); + + expect(listener).toHaveBeenCalledWith({ + path: configPath, + changeType: 'invalid', + error: + 'Failed to read .lsp.json; existing LSP runtime state is unchanged.', + }); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'Failed to read .lsp.json:', + expect.objectContaining({ code: 'EISDIR' }), + ); + }); + + it('retries the same config content when listener notification fails', async () => { + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + const listener = vi + .fn() + .mockRejectedValueOnce(new Error('reload failed')) + .mockResolvedValueOnce(undefined); + watcher.listener = listener; + + fs.writeFileSync(configPath, '{"typescript":{"command":"tsserver"}}'); + await watcher.handleChange(); + await watcher.handleChange(); + + expect(listener).toHaveBeenCalledTimes(2); + expect(listener).toHaveBeenNthCalledWith(1, { + path: configPath, + changeType: 'created', + }); + expect(listener).toHaveBeenNthCalledWith(2, { + path: configPath, + changeType: 'created', + }); + }); + + it('debounces duplicate filesystem events', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + const watcher = new LspConfigWatcher(dir); + const listener = vi.fn(); + watcher.startWatching(listener); + const onAll = chokidarMock.handlers.get('all'); + expect(onAll).toBeDefined(); + + fs.writeFileSync(configPath, '{"typescript":{"command":"tsserver"}}'); + onAll?.('add', configPath); + onAll?.('change', configPath); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.DEBOUNCE_MS); + + expect(listener).toHaveBeenCalledTimes(1); + await watcher.stopWatching(); + }); + + it('allows retrying startWatching after watcher initialization fails', async () => { + const dir = makeTempDir(); + const watcher = new LspConfigWatcher(dir); + const listener = vi.fn(); + + chokidarMock.watch.mockImplementationOnce(() => { + throw new Error('watch failed'); + }); + + watcher.startWatching(listener); + watcher.startWatching(listener); + + expect(chokidarMock.watch).toHaveBeenCalledTimes(2); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + `Failed to start LSP config watcher for ${dir}:`, + expect.any(Error), + ); + + await watcher.stopWatching(); + expect(chokidarMock.watcher.close).toHaveBeenCalledOnce(); + }); + + it('ignores filesystem events after stopWatching begins', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + let resolveClose: (() => void) | undefined; + chokidarMock.watcher.close.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveClose = resolve; + }), + ); + const watcher = new LspConfigWatcher(dir); + const listener = vi.fn(); + watcher.startWatching(listener); + const onAll = chokidarMock.handlers.get('all'); + expect(onAll).toBeDefined(); + + const stopPromise = watcher.stopWatching(); + onAll?.('change', configPath); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.DEBOUNCE_MS); + + expect(listener).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + + resolveClose?.(); + await stopPromise; + }); + + it('unrefs the debounce timer', async () => { + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + const watcher = new LspConfigWatcher(dir); + watcher.startWatching(vi.fn()); + const onAll = chokidarMock.handlers.get('all'); + expect(onAll).toBeDefined(); + + onAll?.('change', configPath); + + const refreshTimer = ( + watcher as unknown as { refreshTimer: NodeJS.Timeout | null } + ).refreshTimer; + expect(refreshTimer).toBeDefined(); + expect(refreshTimer?.hasRef()).toBe(false); + + await watcher.stopWatching(); + }); + + it('waits for an active listener before stopping', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + let resolveListener: (() => void) | undefined; + const listener = vi.fn( + () => + new Promise((resolve) => { + resolveListener = resolve; + }), + ); + const watcher = new LspConfigWatcher(dir); + watcher.startWatching(listener); + const onAll = chokidarMock.handlers.get('all'); + expect(onAll).toBeDefined(); + + fs.writeFileSync(configPath, '{"typescript":{"command":"tsserver"}}'); + onAll?.('add', configPath); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.DEBOUNCE_MS); + expect(listener).toHaveBeenCalledTimes(1); + + let stopped = false; + const stopPromise = watcher.stopWatching().then(() => { + stopped = true; + }); + await Promise.resolve(); + + expect(stopped).toBe(false); + expect(chokidarMock.watcher.close).not.toHaveBeenCalled(); + + resolveListener?.(); + await stopPromise; + + expect(stopped).toBe(true); + expect(chokidarMock.watcher.close).toHaveBeenCalledOnce(); + }); + + it('does not replace the active drain with a no-op drain', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const configPath = path.join(dir, '.lsp.json'); + let resolveListener: (() => void) | undefined; + const listener = vi.fn( + () => + new Promise((resolve) => { + resolveListener = resolve; + }), + ); + const watcher = new LspConfigWatcher(dir); + watcher.startWatching(listener); + const onAll = chokidarMock.handlers.get('all'); + expect(onAll).toBeDefined(); + + fs.writeFileSync(configPath, '{"typescript":{"command":"tsserver"}}'); + onAll?.('add', configPath); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.DEBOUNCE_MS); + expect(listener).toHaveBeenCalledTimes(1); + + fs.writeFileSync(configPath, '{"typescript":{"command":"pyright"}}'); + onAll?.('change', configPath); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.DEBOUNCE_MS); + + let stopped = false; + const stopPromise = watcher.stopWatching().then(() => { + stopped = true; + }); + await Promise.resolve(); + + expect(stopped).toBe(false); + resolveListener?.(); + await stopPromise; + + expect(stopped).toBe(true); + expect(chokidarMock.watcher.close).toHaveBeenCalledOnce(); + }); + + // Listener timeout is the isolation boundary between file watching and the + // running CLI session: a hung reload callback should be logged and swallowed. + it('times out a hanging listener without throwing', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + watcher.listener = vi.fn(() => new Promise(() => {})); + + const notifyPromise = watcher.notifyListener({ + path: path.join(dir, '.lsp.json'), + changeType: 'modified', + }); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.LISTENER_TIMEOUT_MS); + await expect(notifyPromise).resolves.toBe(false); + + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'LSP config change listener error:', + expect.any(Error), + ); + }); + + it('swallows listener rejection that arrives after timeout', async () => { + vi.useFakeTimers(); + const dir = makeTempDir(); + const watcher = new LspConfigWatcher(dir) as unknown as TestableWatcher; + let rejectListener: ((error: Error) => void) | undefined; + watcher.listener = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectListener = reject; + }), + ); + + const notifyPromise = watcher.notifyListener({ + path: path.join(dir, '.lsp.json'), + changeType: 'modified', + }); + await vi.advanceTimersByTimeAsync(LspConfigWatcher.LISTENER_TIMEOUT_MS); + await expect(notifyPromise).resolves.toBe(false); + + rejectListener?.(new Error('late listener failure')); + await Promise.resolve(); + + expect(debugLoggerMock.warn).toHaveBeenCalledTimes(1); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'LSP config change listener error:', + expect.any(Error), + ); + }); +}); diff --git a/packages/cli/src/config/lsp-config-watcher.ts b/packages/cli/src/config/lsp-config-watcher.ts new file mode 100644 index 0000000000..6638dd4861 --- /dev/null +++ b/packages/cli/src/config/lsp-config-watcher.ts @@ -0,0 +1,303 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { watch as watchFs, type FSWatcher } from 'chokidar'; +import { createDebugLogger } from '@qwen-code/qwen-code-core'; + +const debugLogger = createDebugLogger('LSP_CONFIG_WATCHER'); + +export type LspConfigChangeEvent = + | LspConfigRuntimeChangeEvent + | LspConfigInvalidEvent; + +export interface LspConfigRuntimeChangeEvent { + path: string; + changeType: 'modified' | 'created' | 'deleted'; +} + +export interface LspConfigInvalidEvent { + path: string; + changeType: 'invalid'; + /** User-facing message; invalid configs preserve the current LSP runtime. */ + error: string; +} + +export type LspConfigChangeListener = ( + event: LspConfigChangeEvent, +) => void | Promise; + +interface LspConfigSnapshot { + exists: boolean; + /** Canonical JSON string. Undefined means the file exists but is invalid or unreadable. */ + canonical?: string; + error?: string; +} + +/** + * Watches the workspace `.lsp.json` and reports semantic config changes. + * + * This watcher is intentionally narrow: it never creates files, only considers + * the workspace-root `.lsp.json`, debounces noisy filesystem events, and + * serializes listener calls so LSP reloads cannot overlap. + */ +export class LspConfigWatcher { + private watcher?: FSWatcher; + private listener?: LspConfigChangeListener; + private refreshTimer: NodeJS.Timeout | null = null; + private activeDrain?: Promise; + private processing = false; + private pending = false; + private started = false; + private lastSnapshot: LspConfigSnapshot; + private readonly configPath: string; + + static readonly DEBOUNCE_MS = 300; + static readonly LISTENER_TIMEOUT_MS = 30_000; + + constructor(private readonly workspaceRoot: string) { + this.configPath = path.join(workspaceRoot, '.lsp.json'); + this.lastSnapshot = this.readSnapshot(); + } + + startWatching(listener: LspConfigChangeListener): void { + if (this.started) return; + debugLogger.info(`Starting LSP config watcher for ${this.configPath}`); + try { + const watcher = watchFs(this.workspaceRoot, { + ignoreInitial: true, + depth: 0, + }) + .on('all', (_event: string, changedPath: string) => { + if (path.basename(changedPath) !== '.lsp.json') return; + this.scheduleRefresh(); + }) + .on('error', (error: unknown) => { + debugLogger.warn( + `LSP config watcher error for ${this.workspaceRoot}:`, + error, + ); + }); + this.watcher = watcher; + this.listener = listener; + this.started = true; + } catch (error) { + this.watcher = undefined; + this.listener = undefined; + this.started = false; + debugLogger.warn( + `Failed to start LSP config watcher for ${this.workspaceRoot}:`, + error, + ); + } + } + + async stopWatching(): Promise { + if (!this.started) return; + this.started = false; + debugLogger.info(`Stopping LSP config watcher for ${this.configPath}`); + if (this.refreshTimer) { + clearTimeout(this.refreshTimer); + this.refreshTimer = null; + } + this.pending = false; + + await this.activeDrain; + this.listener = undefined; + + try { + await this.watcher?.close(); + } catch (error) { + debugLogger.warn('LSP config watcher close error:', error); + } + this.watcher = undefined; + } + + private scheduleRefresh(): void { + if (!this.started) return; + this.pending = true; + if (this.refreshTimer) clearTimeout(this.refreshTimer); + this.refreshTimer = setTimeout(() => { + this.refreshTimer = null; + if (this.processing) return; + const activeDrain = this.drainPendingChange().catch((error) => { + debugLogger.warn('LSP config watcher refresh error:', error); + }); + this.activeDrain = activeDrain; + void activeDrain.finally(() => { + if (this.activeDrain === activeDrain) { + this.activeDrain = undefined; + } + }); + }, LspConfigWatcher.DEBOUNCE_MS); + if ( + typeof this.refreshTimer === 'object' && + this.refreshTimer !== null && + 'unref' in this.refreshTimer + ) { + this.refreshTimer.unref(); + } + } + + /** Drains debounced changes one at a time while preserving a trailing update. */ + private async drainPendingChange(): Promise { + if (this.processing) return; + this.processing = true; + try { + while (this.pending) { + this.pending = false; + await this.handleChange(); + } + } finally { + this.processing = false; + } + } + + /** + * Compares the previous and current semantic snapshots. + * + * Invalid JSON emits an `invalid` event for user feedback but does not report + * a runtime config change; the caller must keep the existing LSP state. + */ + private async handleChange(): Promise { + const before = this.lastSnapshot; + const after = this.readSnapshot(); + if (after.exists && after.canonical === undefined) { + debugLogger.warn( + `Invalid .lsp.json; keeping existing LSP runtime state.`, + ); + const notified = await this.notifyListener({ + path: this.configPath, + changeType: 'invalid', + error: + after.error ?? + 'Invalid JSON in .lsp.json; existing LSP runtime state is unchanged.', + }); + if (notified) { + this.lastSnapshot = after; + } + return; + } + + const changed = + before.exists !== after.exists || before.canonical !== after.canonical; + if (!changed) { + this.lastSnapshot = after; + return; + } + + const event: LspConfigChangeEvent = { + path: this.configPath, + changeType: + !before.exists && after.exists + ? 'created' + : before.exists && !after.exists + ? 'deleted' + : 'modified', + }; + debugLogger.info(`LSP config changed: ${event.changeType} ${event.path}`); + const notified = await this.notifyListener(event); + if (notified) { + this.lastSnapshot = after; + } + } + + /** + * Reads `.lsp.json` as a single operation. ENOENT is treated as deletion so a + * file removed during a filesystem race still reconciles servers to empty. + */ + private readSnapshot(): LspConfigSnapshot { + let raw: string; + try { + raw = fs.readFileSync(this.configPath, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { exists: false }; + } + debugLogger.warn('Failed to read .lsp.json:', error); + return { + exists: true, + error: + 'Failed to read .lsp.json; existing LSP runtime state is unchanged.', + }; + } + + try { + const parsed = JSON.parse(raw) as unknown; + return { exists: true, canonical: canonicalize(parsed) }; + } catch (error) { + debugLogger.warn('Failed to parse .lsp.json:', error); + return { + exists: true, + error: + 'Invalid JSON in .lsp.json; existing LSP runtime state is unchanged.', + }; + } + } + + /** + * Runs the listener with timeout isolation so a hung reload cannot stall CLI. + * + * Returns whether the listener completed successfully; callers use this to + * decide whether the semantic snapshot can advance or should be retried. + */ + private async notifyListener(event: LspConfigChangeEvent): Promise { + if (!this.listener) return true; + const TIMEOUT_MS = LspConfigWatcher.LISTENER_TIMEOUT_MS; + let timerId: ReturnType | undefined; + const timeoutPromise = new Promise((_, reject) => { + timerId = setTimeout( + () => + reject( + new Error( + `LSP config change listener timeout after ${TIMEOUT_MS}ms`, + ), + ), + TIMEOUT_MS, + ); + if ( + typeof timerId === 'object' && + timerId !== null && + 'unref' in timerId + ) { + (timerId as { unref: () => void }).unref(); + } + }); + const listenerPromise = Promise.resolve().then(() => + this.listener?.(event), + ); + try { + await Promise.race([listenerPromise, timeoutPromise]); + return true; + } catch (error) { + debugLogger.warn('LSP config change listener error:', error); + return false; + } finally { + if (timerId !== undefined) clearTimeout(timerId); + void listenerPromise.catch(() => undefined); + } + } +} + +function canonicalize(value: unknown): string { + return JSON.stringify(sortJsonValue(value)); +} + +function sortJsonValue(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(sortJsonValue); + } + if (value && typeof value === 'object') { + // Object.create(null) avoids prototype pollution from __proto__ keys + const sorted = Object.create(null) as Record; + for (const key of Object.keys(value).sort()) { + sorted[key] = sortJsonValue((value as Record)[key]); + } + return sorted; + } + return value; +} diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 7cb03e5cd2..b91dc34b55 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -17,6 +17,7 @@ import { readFileSync } from 'node:fs'; import { createNonInteractivePromptId, main, + registerLspHotReload, setupUnhandledRejectionHandler, validateDnsResolutionOrder, } from './gemini.js'; @@ -29,6 +30,13 @@ import { ApprovalMode, OutputFormat } from '@qwen-code/qwen-code-core'; const mockWriteStderrLine = vi.hoisted(() => vi.fn()); const mockHandleListExtensions = vi.hoisted(() => vi.fn()); +const lspConfigWatcherMock = vi.hoisted(() => ({ + instances: [] as Array<{ + listener?: (event: unknown) => void | Promise; + startWatching: ReturnType; + stopWatching: ReturnType; + }>, +})); describe('gemini import boundary', () => { it('does not statically import ACP or noninteractive auth branches', () => { @@ -78,6 +86,8 @@ vi.mock('./config/config.js', () => ({ getSandbox: vi.fn(() => false), getQuestion: vi.fn(() => ''), isInteractive: () => false, + isLspEnabled: () => false, + getLspClient: () => undefined, getWarnings: vi.fn(() => []), isSafeMode: vi.fn(() => false), getModelsConfig: vi.fn(() => ({ getCurrentAuthType: () => null })), @@ -161,6 +171,35 @@ vi.mock('./config/settingsWatcher.js', () => ({ }, })); +vi.mock('./config/lsp-config-watcher.js', () => ({ + LspConfigWatcher: class { + listener?: (event: unknown) => void | Promise; + startWatching = vi.fn( + (listener: (event: unknown) => void | Promise) => { + this.listener = listener; + }, + ); + stopWatching = vi.fn(); + + constructor() { + lspConfigWatcherMock.instances.push(this); + } + }, +})); + +function withLspDisabledConfig( + config: T, +): T & { + isLspEnabled: () => boolean; + getLspClient: () => undefined; +} { + return { + isLspEnabled: () => false, + getLspClient: () => undefined, + ...config, + }; +} + describe('gemini.tsx main function', () => { let originalEnvGeminiSandbox: string | undefined; let originalEnvSandbox: string | undefined; @@ -169,6 +208,7 @@ describe('gemini.tsx main function', () => { []; beforeEach(() => { + lspConfigWatcherMock.instances.length = 0; // Store and clear sandbox-related env variables to ensure a consistent test environment originalEnvGeminiSandbox = process.env['QWEN_SANDBOX']; originalEnvSandbox = process.env['SANDBOX']; @@ -417,6 +457,187 @@ describe('gemini.tsx main function', () => { ); }); + describe('registerLspHotReload', () => { + it('does not register a watcher when LSP is disabled', () => { + const registerCleanup = vi.fn(); + + registerLspHotReload( + withLspDisabledConfig({ + getProjectRoot: () => '/workspace', + }) as unknown as Config, + registerCleanup, + ); + + expect(lspConfigWatcherMock.instances).toHaveLength(0); + expect(registerCleanup).not.toHaveBeenCalled(); + }); + + it('does not register a watcher when the client cannot reinitialize', () => { + const registerCleanup = vi.fn(); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({}), + getProjectRoot: () => '/workspace', + } as unknown as Config, + registerCleanup, + ); + + expect(lspConfigWatcherMock.instances).toHaveLength(0); + expect(registerCleanup).not.toHaveBeenCalled(); + }); + + it('emits an LSP status update after successful reload', async () => { + const registerCleanup = vi.fn(); + const reinitializeLsp = vi.fn(async () => ({ + reconcile: { + added: ['clangd'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }, + skipped: [], + })); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({ reinitialize: vi.fn() }), + getProjectRoot: () => '/workspace', + reinitializeLsp, + } as unknown as Config, + registerCleanup, + ); + + await lspConfigWatcherMock.instances[0]?.listener?.({ + path: '/workspace/.lsp.json', + changeType: 'modified', + }); + + expect(reinitializeLsp).toHaveBeenCalledOnce(); + expect(appEvents.emit).toHaveBeenCalledWith(AppEvent.LspStatusChanged); + }); + + it('emits an LSP status update when reload is skipped by the config', async () => { + const reinitializeLsp = vi.fn(async () => undefined); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({ reinitialize: vi.fn() }), + getProjectRoot: () => '/workspace', + reinitializeLsp, + } as unknown as Config, + vi.fn(), + ); + + await lspConfigWatcherMock.instances[0]?.listener?.({ + path: '/workspace/.lsp.json', + changeType: 'modified', + }); + + expect(reinitializeLsp).toHaveBeenCalledOnce(); + expect(appEvents.emit).not.toHaveBeenCalledWith( + AppEvent.LogError, + expect.any(String), + ); + expect(appEvents.emit).toHaveBeenCalledWith(AppEvent.LspStatusChanged); + }); + + it('emits a user-visible error and rejects when reload fails', async () => { + const reinitializeLsp = vi.fn(async () => { + throw new Error('invalid lsp json'); + }); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({ reinitialize: vi.fn() }), + getProjectRoot: () => '/workspace', + reinitializeLsp, + } as unknown as Config, + vi.fn(), + ); + + await expect( + lspConfigWatcherMock.instances[0]?.listener?.({ + path: '/workspace/.lsp.json', + changeType: 'modified', + }), + ).rejects.toThrow('invalid lsp json'); + + expect(appEvents.emit).toHaveBeenCalledWith( + AppEvent.LogError, + 'Failed to reload LSP server settings: invalid lsp json. Some LSP servers may have been partially updated. Run with --debug for details.', + ); + }); + + it('emits a user-visible error and rejects when reload has failed servers', async () => { + const reinitializeLsp = vi.fn(async () => ({ + reconcile: { + added: [], + removed: [], + restarted: [], + unchanged: [], + failed: ['clangd'], + }, + skipped: [], + })); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({ reinitialize: vi.fn() }), + getProjectRoot: () => '/workspace', + reinitializeLsp, + } as unknown as Config, + vi.fn(), + ); + + await expect( + lspConfigWatcherMock.instances[0]?.listener?.({ + path: '/workspace/.lsp.json', + changeType: 'modified', + }), + ).rejects.toThrow('LSP reload partially completed'); + + expect(appEvents.emit).toHaveBeenCalledWith( + AppEvent.LogError, + 'LSP reload partially completed: changed=, failed=clangd. Run with --debug for details.', + ); + expect(appEvents.emit).toHaveBeenCalledWith(AppEvent.LspStatusChanged); + }); + + it('surfaces invalid config without reinitializing LSP', async () => { + const reinitializeLsp = vi.fn(); + + registerLspHotReload( + { + isLspEnabled: () => true, + getLspClient: () => ({ reinitialize: vi.fn() }), + getProjectRoot: () => '/workspace', + reinitializeLsp, + } as unknown as Config, + vi.fn(), + ); + + await lspConfigWatcherMock.instances[0]?.listener?.({ + path: '/workspace/.lsp.json', + changeType: 'invalid', + error: + 'Invalid JSON in .lsp.json; existing LSP runtime state is unchanged.', + }); + + expect(reinitializeLsp).not.toHaveBeenCalled(); + expect(appEvents.emit).toHaveBeenCalledWith( + AppEvent.LogError, + 'Invalid JSON in .lsp.json; existing LSP runtime state is unchanged.', + ); + }); + }); + it('writes non-interactive warnings discovered during config initialization', async () => { const originalNoRelaunch = process.env['QWEN_CODE_NO_RELAUNCH']; const originalIsTTY = Object.getOwnPropertyDescriptor( diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 7620b4e428..18e2d1df0f 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -6,6 +6,7 @@ import { AuthType, + type Config, InputFormat, isDebugLoggingDegraded, isBareMode, @@ -40,6 +41,7 @@ import { } from './config/settings.js'; import { SettingsWatcher } from './config/settingsWatcher.js'; import { registerMcpHotReload } from './config/hot-reload.js'; +import { LspConfigWatcher } from './config/lsp-config-watcher.js'; import { initializeI18n, resolveLanguageSetting } from './i18n/index.js'; import { setupStartupWorktree, @@ -611,6 +613,8 @@ export async function main() { registerCleanup(disposeMcpHotReload); } + registerLspHotReload(config, registerCleanup); + // Phase D-1: persist the WorktreeSession sidecar so Phase C's restore // machinery on a subsequent `--resume` picks the worktree back up, and // capture any override of a previously-resumed session's worktree so @@ -1049,3 +1053,107 @@ export async function main() { export function createNonInteractivePromptId(sessionId: string): string { return `${sessionId}########0`; } + +/** + * Watches `.lsp.json` for changes and reconciles running LSP servers + * (add / remove / restart) without requiring a session restart. + * + * Silently no-ops when LSP is disabled or the active client does not + * support runtime reinitialization. + * + * Emits {@link AppEvent.LspStatusChanged} after every successful reload + * so the UI can reflect the new server state. + */ +export function registerLspHotReload( + config: Config, + registerCleanup: (fn: () => void | Promise) => void, +): void { + if ( + config.isLspEnabled?.() !== true || + !config.getLspClient?.()?.reinitialize + ) { + return; + } + const lspConfigWatcher = new LspConfigWatcher(config.getProjectRoot()); + debugLogger.info( + `Registering LSP config hot reload watcher for ${config.getProjectRoot()}`, + ); + lspConfigWatcher.startWatching(async (event) => { + if (event.changeType === 'invalid') { + debugLogger.warn(`Invalid LSP config file ${event.path}: ${event.error}`); + appEvents.emit(AppEvent.LogError, event.error); + return; + } + debugLogger.info( + `Reloading LSP server settings: changeType=${event.changeType}, path=${event.path}`, + ); + let errorReported = false; + try { + const result = await config.reinitializeLsp(); + if (result) { + const failedServers = getRuntimeReloadFailedNames(result.reconcile); + debugLogger.info( + `Reloaded LSP server settings: added=${formatRuntimeReloadNames( + result.reconcile.added, + )}, removed=${formatRuntimeReloadNames( + result.reconcile.removed, + )}, restarted=${formatRuntimeReloadNames( + result.reconcile.restarted, + )}, unchanged=${formatRuntimeReloadNames( + result.reconcile.unchanged, + )}, failed=${formatRuntimeReloadNames( + failedServers, + )}, skipped=${formatRuntimeReloadNames( + result.skipped.map((server) => server.name), + )}`, + ); + if (failedServers.length > 0) { + appEvents.emit(AppEvent.LspStatusChanged); + const changedServers = [ + ...result.reconcile.added, + ...result.reconcile.removed, + ...result.reconcile.restarted, + ]; + const message = `LSP reload partially completed: changed=${formatRuntimeReloadNames( + changedServers, + )}, failed=${formatRuntimeReloadNames( + failedServers, + )}. Run with --debug for details.`; + appEvents.emit(AppEvent.LogError, message); + errorReported = true; + throw new Error(message); + } + } else { + debugLogger.info( + 'Skipped LSP server settings reload because LSP is disabled or no client is available', + ); + } + appEvents.emit(AppEvent.LspStatusChanged); + } catch (error) { + debugLogger.warn('Failed to reload LSP server settings:', error); + if (!errorReported) { + const message = + error instanceof Error + ? `Failed to reload LSP server settings: ${error.message}. Some LSP servers may have been partially updated. Run with --debug for details.` + : 'Failed to reload LSP server settings; some LSP servers may have been partially updated. Run with --debug for details.'; + appEvents.emit(AppEvent.LogError, message); + } + throw error; + } + }); + registerCleanup(() => lspConfigWatcher.stopWatching()); +} + +function formatRuntimeReloadNames(names: readonly string[]): string { + return names.length === 0 ? '' : names.join(','); +} + +/** + * Reads the optional failed bucket defensively because the CLI may typecheck + * against stale core dist declarations during local development. + */ +function getRuntimeReloadFailedNames(reconcile: { + failed?: readonly string[]; +}): readonly string[] { + return reconcile.failed ?? []; +} diff --git a/packages/cli/src/utils/events.ts b/packages/cli/src/utils/events.ts index 5a6275f6a8..f0e40cbfad 100644 --- a/packages/cli/src/utils/events.ts +++ b/packages/cli/src/utils/events.ts @@ -18,6 +18,7 @@ export enum AppEvent { * re-evaluate mid-session instead of only at startup. See issue #4615. */ McpPendingApprovalChanged = 'mcp-pending-approval-changed', + LspStatusChanged = 'lsp-status-changed', } export const appEvents = new EventEmitter(); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 101892d690..16a1ab2170 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1965,6 +1965,158 @@ describe('Server Config (config.ts)', () => { }); }); + it('should no-op LSP reinitialize when disabled or unavailable', async () => { + const disabledConfig = new Config({ + ...baseParams, + lsp: { enabled: false }, + }); + await expect(disabledConfig.reinitializeLsp()).resolves.toBeUndefined(); + + const noClientConfig = new Config({ + ...baseParams, + lsp: { enabled: true }, + }); + await expect(noClientConfig.reinitializeLsp()).resolves.toBeUndefined(); + }); + + it('should delegate LSP reinitialize to the configured client', async () => { + const result = { + reconcile: { + added: ['tsserver'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }, + skipped: [], + }; + const reinitialize = vi.fn().mockResolvedValue(result); + const config = new Config({ + ...baseParams, + lsp: { enabled: true }, + lspClient: { + reinitialize, + } as unknown as ConfigParameters['lspClient'], + }); + + await expect(config.reinitializeLsp()).resolves.toBe(result); + expect(reinitialize).toHaveBeenCalledOnce(); + }); + + it('should surface partial LSP reinitialize failures in status snapshot', async () => { + const result = { + reconcile: { + added: ['tsserver'], + removed: [], + restarted: [], + unchanged: [], + failed: ['clangd'], + }, + skipped: [], + }; + const config = new Config({ + ...baseParams, + lsp: { enabled: true }, + lspClient: { + reinitialize: vi.fn().mockResolvedValue(result), + } as unknown as ConfigParameters['lspClient'], + }); + + await expect(config.reinitializeLsp()).resolves.toBe(result); + expect(config.getLspStatusSnapshot()).toMatchObject({ + initializationError: 'LSP reload partially failed: clangd', + }); + }); + + it('should surface LSP reinitialize failures in status snapshot', async () => { + const config = new Config({ + ...baseParams, + lsp: { enabled: true }, + lspClient: { + reinitialize: vi.fn().mockRejectedValue(new Error('invalid lsp json')), + } as unknown as ConfigParameters['lspClient'], + }); + + await expect(config.reinitializeLsp()).rejects.toThrow('invalid lsp json'); + expect(config.getLspStatusSnapshot()).toMatchObject({ + initializationError: 'invalid lsp json', + }); + }); + + it('should clear previous LSP reinitialize failures after recovery', async () => { + const result = { + reconcile: { + added: [], + removed: [], + restarted: [], + unchanged: ['tsserver'], + failed: [], + }, + skipped: [], + }; + const reinitialize = vi + .fn() + .mockRejectedValueOnce(new Error('invalid lsp json')) + .mockResolvedValueOnce(result); + const config = new Config({ + ...baseParams, + lsp: { enabled: true }, + lspClient: { + reinitialize, + } as unknown as ConfigParameters['lspClient'], + }); + + await expect(config.reinitializeLsp()).rejects.toThrow('invalid lsp json'); + expect(config.getLspStatusSnapshot()).toMatchObject({ + initializationError: 'invalid lsp json', + }); + + await expect(config.reinitializeLsp()).resolves.toBe(result); + expect(config.getLspStatusSnapshot().initializationError).toBeUndefined(); + }); + + it('should clear partial LSP reinitialize failures after full recovery', async () => { + const partialFailure = { + reconcile: { + added: [], + removed: [], + restarted: [], + unchanged: [], + failed: ['clangd'], + }, + skipped: [], + }; + const success = { + reconcile: { + added: [], + removed: [], + restarted: [], + unchanged: ['clangd'], + failed: [], + }, + skipped: [], + }; + const reinitialize = vi + .fn() + .mockResolvedValueOnce(partialFailure) + .mockResolvedValueOnce(success); + const config = new Config({ + ...baseParams, + lsp: { enabled: true }, + lspClient: { + reinitialize, + } as unknown as ConfigParameters['lspClient'], + }); + + await expect(config.reinitializeLsp()).resolves.toBe(partialFailure); + expect(config.getLspStatusSnapshot()).toMatchObject({ + initializationError: 'LSP reload partially failed: clangd', + }); + + await expect(config.reinitializeLsp()).resolves.toBe(success); + expect(config.getLspStatusSnapshot().initializationError).toBeUndefined(); + }); + describe('initialize', () => { it('should throw an error if initialized more than once', async () => { const config = new Config({ diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 8400eda3bd..f0c4e19b8b 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -81,7 +81,11 @@ import type { ArtifactHostConfig, ArtifactOssConfig, } from '../tools/artifact/publisher.js'; -import type { LspClient, LspStatusSnapshot } from '../lsp/types.js'; +import type { + LspClient, + LspServiceReinitializeResult, + LspStatusSnapshot, +} from '../lsp/types.js'; import type { InstructionLoadReason } from '../hooks/types.js'; import { ApprovalMode } from './approval-mode.js'; @@ -4455,7 +4459,7 @@ export class Config { if (this.lspClient) { return { - ...this.createLspStatusSnapshot(true), + ...this.createLspStatusSnapshot(true, this.lspInitializationError), statusUnavailable: true, }; } @@ -4496,10 +4500,38 @@ export class Config { if (this.initialized) { throw new Error('Cannot set LSP status after initialization'); } + this.setRuntimeLspInitializationError(error); + } + + private setRuntimeLspInitializationError( + error: Error | string | undefined, + ): void { this.lspInitializationError = error instanceof Error ? error.message : error; } + async reinitializeLsp(): Promise { + if (!this.isLspEnabled() || !this.lspClient?.reinitialize) { + return undefined; + } + try { + const result = await this.lspClient.reinitialize(); + if (result.reconcile.failed.length > 0) { + this.setRuntimeLspInitializationError( + `LSP reload partially failed: ${result.reconcile.failed.join(', ')}`, + ); + } else { + this.setRuntimeLspInitializationError(undefined); + } + return result; + } catch (error) { + this.setRuntimeLspInitializationError( + error instanceof Error ? error : String(error), + ); + throw error; + } + } + getSessionSubagents(): SubagentConfig[] { return this.sessionSubagents; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c93ff5d0f1..b34b6967e6 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -316,6 +316,7 @@ export * from './ide/types.js'; // ============================================================================ export * from './lsp/constants.js'; +export * from './lsp/configHash.js'; export * from './lsp/LspConfigLoader.js'; export * from './lsp/LspConnectionFactory.js'; export * from './lsp/LspResponseNormalizer.js'; diff --git a/packages/core/src/lsp/LspConfigLoader.test.ts b/packages/core/src/lsp/LspConfigLoader.test.ts index 479b45676e..96733af9be 100644 --- a/packages/core/src/lsp/LspConfigLoader.test.ts +++ b/packages/core/src/lsp/LspConfigLoader.test.ts @@ -154,6 +154,109 @@ describe('LspConfigLoader config-driven behavior', () => { mock.restore(); } }); + + it('strict user config loading rejects invalid server entries', async () => { + mock({ + [workspaceRoot]: { + '.lsp.json': JSON.stringify({ + typescript: { + transport: 'stdio', + }, + }), + }, + }); + + const loader = new LspConfigLoader(workspaceRoot); + const result = await loader.loadUserConfigsStrict(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.message).toContain( + 'Invalid LSP server config in /workspace/.lsp.json: typescript', + ); + } + }); + + it('strict user config loading accepts empty object as explicit empty config', async () => { + mock({ + [workspaceRoot]: { + '.lsp.json': JSON.stringify({}), + }, + }); + + const loader = new LspConfigLoader(workspaceRoot); + const result = await loader.loadUserConfigsStrict(); + + expect(result).toEqual({ ok: true, configs: [] }); + }); + + it('strict user config loading treats deleted config as empty', async () => { + mock({ + [workspaceRoot]: {}, + }); + + const loader = new LspConfigLoader(workspaceRoot); + const result = await loader.loadUserConfigsStrict(); + + expect(result).toEqual({ ok: true, configs: [] }); + }); + + it('non-strict user config loading skips invalid entries without rejecting all configs', async () => { + mock({ + [workspaceRoot]: { + '.lsp.json': JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + invalid: { + transport: 'stdio', + }, + }), + }, + }); + + const loader = new LspConfigLoader(workspaceRoot); + const configs = await loader.loadUserConfigs(); + + expect(configs).toHaveLength(1); + expect(configs[0]?.name).toBe('typescript-language-server'); + }); + + it('non-strict user config loading returns empty configs for malformed JSON', async () => { + mock({ + [workspaceRoot]: { + '.lsp.json': '{', + }, + }); + + const loader = new LspConfigLoader(workspaceRoot); + const configs = await loader.loadUserConfigs(); + + expect(configs).toEqual([]); + }); + + it('forces user configs to require trusted workspaces', async () => { + mock({ + [workspaceRoot]: { + '.lsp.json': JSON.stringify({ + typescript: { + command: 'typescript-language-server', + trustRequired: false, + }, + }), + }, + }); + + const loader = new LspConfigLoader(workspaceRoot); + + await expect(loader.loadUserConfigs()).resolves.toEqual([ + expect.objectContaining({ trustRequired: true }), + ]); + await expect(loader.loadUserConfigsStrict()).resolves.toEqual({ + ok: true, + configs: [expect.objectContaining({ trustRequired: true })], + }); + }); }); describe('LspConfigLoader extension configs', () => { diff --git a/packages/core/src/lsp/LspConfigLoader.ts b/packages/core/src/lsp/LspConfigLoader.ts index 1a0050b7ee..1c338e4fbf 100644 --- a/packages/core/src/lsp/LspConfigLoader.ts +++ b/packages/core/src/lsp/LspConfigLoader.ts @@ -22,6 +22,10 @@ import { createDebugLogger } from '../utils/debugLogger.js'; const debugLogger = createDebugLogger('LSP'); const MAX_TCP_PORT = 65_535; +export type LspUserConfigLoadResult = + | { ok: true; configs: LspServerConfig[] } + | { ok: false; error: Error }; + export class LspConfigLoader { constructor(private readonly workspaceRoot: string) {} @@ -38,13 +42,32 @@ export class LspConfigLoader { try { const configContent = fs.readFileSync(lspConfigPath, 'utf-8'); const data = JSON.parse(configContent); - return this.parseConfigSource(data, lspConfigPath); + return this.parseConfigSource(data, lspConfigPath, { + forceTrustRequired: true, + }); } catch (error) { debugLogger.warn('Failed to load user .lsp.json config:', error); return []; } } + async loadUserConfigsStrict(): Promise { + const lspConfigPath = path.join(this.workspaceRoot, '.lsp.json'); + try { + const configContent = fs.readFileSync(lspConfigPath, 'utf-8'); + const data = JSON.parse(configContent); + return this.parseUserConfigSourceStrict(data, lspConfigPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { ok: true, configs: [] }; + } + return { + ok: false, + error: error instanceof Error ? error : new Error(String(error)), + }; + } + } + /** * Load LSP configurations declared by extensions (Claude plugins). */ @@ -168,6 +191,7 @@ export class LspConfigLoader { private parseConfigSource( source: unknown, origin: string, + options: { forceTrustRequired?: boolean } = {}, ): LspServerConfig[] { if (!this.isRecord(source)) { return []; @@ -185,7 +209,9 @@ export class LspConfigLoader { const name = typeof spec['command'] === 'string' ? (spec['command'] as string) : key; - const config = this.buildServerConfig(name, languages, spec, origin); + const config = this.buildServerConfig(name, languages, spec, origin, { + forceTrustRequired: options.forceTrustRequired, + }); if (config) { configs.push(config); } @@ -194,6 +220,45 @@ export class LspConfigLoader { return configs; } + private parseUserConfigSourceStrict( + source: unknown, + origin: string, + ): LspUserConfigLoadResult { + if (!this.isRecord(source)) { + return { + ok: false, + error: new Error(`LSP config in ${origin} must be an object`), + }; + } + + const configs: LspServerConfig[] = []; + for (const [key, spec] of Object.entries(source)) { + if (!this.isRecord(spec)) { + return { + ok: false, + error: new Error( + `LSP config error in ${origin}: ${key} must be an object`, + ), + }; + } + const languages = [key]; + const name = + typeof spec['command'] === 'string' ? (spec['command'] as string) : key; + const config = this.buildServerConfig(name, languages, spec, origin, { + forceTrustRequired: true, + }); + if (!config) { + return { + ok: false, + error: new Error(`Invalid LSP server config in ${origin}: ${key}`), + }; + } + configs.push(config); + } + + return { ok: true, configs }; + } + private resolveExtensionConfigPath( extensionPath: string, configPath: string, @@ -221,6 +286,7 @@ export class LspConfigLoader { languages: string[], spec: Record, origin: string, + options: { forceTrustRequired?: boolean } = {}, ): LspServerConfig | null { const transport = this.normalizeTransport(spec['transport']); const command = @@ -249,8 +315,9 @@ export class LspConfigLoader { ? (spec['restartOnCrash'] as boolean) : undefined; const maxRestarts = this.normalizeMaxRestarts(spec['maxRestarts']); - const trustRequired = - typeof spec['trustRequired'] === 'boolean' + const trustRequired = options.forceTrustRequired + ? true + : typeof spec['trustRequired'] === 'boolean' ? (spec['trustRequired'] as boolean) : true; const socket = this.normalizeSocketOptions(spec); diff --git a/packages/core/src/lsp/LspServerManager.test.ts b/packages/core/src/lsp/LspServerManager.test.ts index 99c09bf325..bd8ec0302f 100644 --- a/packages/core/src/lsp/LspServerManager.test.ts +++ b/packages/core/src/lsp/LspServerManager.test.ts @@ -5,12 +5,19 @@ */ import path from 'node:path'; -import { describe, expect, it, vi } from 'vitest'; +import { pathToFileURL } from 'node:url'; +import type { ChildProcess } from 'node:child_process'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { Config as CoreConfig } from '../config/config.js'; import type { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import type { WorkspaceContext } from '../utils/workspaceContext.js'; import { LspServerManager } from './LspServerManager.js'; -import type { LspConnectionResult, LspServerConfig } from './types.js'; +import { LspConnectionFactory } from './LspConnectionFactory.js'; +import type { + LspConnectionInterface, + LspConnectionResult, + LspServerConfig, +} from './types.js'; const debugLoggerMock = vi.hoisted(() => ({ debug: vi.fn(), @@ -37,6 +44,11 @@ type PathSafeManager = { isPathSafe(command: string, workspacePath: string, cwd?: string): boolean; }; +type ReconcilePrivateView = { + startServer(name: string, handle: unknown): Promise; + stopServer(name: string, handle: unknown): Promise; +}; + function createManager(workspaceRoot: string): PathSafeManager { return new LspServerManager( {} as CoreConfig, @@ -49,7 +61,297 @@ function createManager(workspaceRoot: string): PathSafeManager { ) as unknown as PathSafeManager; } +function createReconcileManager(): { + manager: LspServerManager; + privateView: ReconcilePrivateView; +} { + const manager = createTrustedManager(); + const privateView = manager as unknown as ReconcilePrivateView; + vi.spyOn(privateView, 'startServer').mockImplementation( + async (_name, handle) => { + (handle as { status: string }).status = 'READY'; + }, + ); + vi.spyOn(privateView, 'stopServer').mockImplementation( + async (_name, handle) => { + (handle as { status: string }).status = 'NOT_STARTED'; + }, + ); + return { manager, privateView }; +} + +function createTrustedManager(): LspServerManager { + return new LspServerManager( + { + isTrustedFolder: vi.fn().mockReturnValue(true), + } as unknown as CoreConfig, + {} as WorkspaceContext, + {} as FileDiscoveryService, + { + requireTrustedWorkspace: false, + workspaceRoot: '/workspace', + }, + ); +} + +function pathToRootUri(rootPath: string): string { + return pathToFileURL(rootPath).toString(); +} + describe('LspServerManager', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('reconcileServerConfigs', () => { + it('starts added servers', async () => { + const { manager, privateView } = createReconcileManager(); + + const result = await manager.reconcileServerConfigs([serverConfig]); + + expect(result).toEqual({ + added: ['clangd'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }); + expect(privateView.startServer).toHaveBeenCalledOnce(); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + expect(debugLoggerMock.info).toHaveBeenCalledWith( + 'Reconciling LSP server configs: desired=clangd', + ); + expect(debugLoggerMock.info).toHaveBeenCalledWith( + 'LSP reconcile result: added=clangd, removed=, restarted=, unchanged=, failed=', + ); + }); + + it('removes missing servers', async () => { + const { manager, privateView } = createReconcileManager(); + manager.setServerConfigs([serverConfig]); + + const result = await manager.reconcileServerConfigs([]); + + expect(result.removed).toEqual(['clangd']); + expect(privateView.stopServer).toHaveBeenCalledOnce(); + expect(manager.getHandles().has('clangd')).toBe(false); + expect(debugLoggerMock.info).toHaveBeenCalledWith( + 'LSP reconcile result: added=, removed=clangd, restarted=, unchanged=, failed=', + ); + }); + + it('restarts changed servers and preserves unchanged handles', async () => { + const { manager, privateView } = createReconcileManager(); + const otherConfig = { + ...serverConfig, + name: 'pyright', + languages: ['python'], + command: 'pyright-langserver', + }; + manager.setServerConfigs([serverConfig, otherConfig]); + const originalOtherHandle = manager.getHandles().get('pyright'); + + const result = await manager.reconcileServerConfigs([ + { ...serverConfig, args: ['--log=verbose'] }, + otherConfig, + ]); + + expect(result.restarted).toEqual(['clangd']); + expect(result.unchanged).toEqual(['pyright']); + expect(privateView.stopServer).toHaveBeenCalledOnce(); + expect(privateView.startServer).toHaveBeenCalledOnce(); + expect(manager.getHandles().get('pyright')).toBe(originalOtherHandle); + expect(debugLoggerMock.info).toHaveBeenCalledWith( + 'LSP reconcile result: added=, removed=, restarted=clangd, unchanged=pyright, failed=', + ); + }); + + it('reports added server startup failures without caching the failed hash', async () => { + const { manager, privateView } = createReconcileManager(); + vi.mocked(privateView.startServer) + .mockImplementationOnce(async (_name, handle) => { + (handle as { status: string }).status = 'FAILED'; + }) + .mockImplementationOnce(async (_name, handle) => { + (handle as { status: string }).status = 'READY'; + }); + + const first = await manager.reconcileServerConfigs([serverConfig]); + const second = await manager.reconcileServerConfigs([serverConfig]); + + expect(first).toMatchObject({ + added: [], + failed: ['clangd'], + unchanged: [], + }); + expect(second).toMatchObject({ + added: [], + restarted: ['clangd'], + failed: [], + unchanged: [], + }); + expect(privateView.startServer).toHaveBeenCalledTimes(2); + }); + + it('reports restarted server startup failures without caching the failed hash', async () => { + const { manager, privateView } = createReconcileManager(); + manager.setServerConfigs([serverConfig]); + vi.mocked(privateView.startServer) + .mockImplementationOnce(async (_name, handle) => { + (handle as { status: string }).status = 'FAILED'; + }) + .mockImplementationOnce(async (_name, handle) => { + (handle as { status: string }).status = 'READY'; + }); + const changedConfig = { ...serverConfig, args: ['--log=verbose'] }; + + const first = await manager.reconcileServerConfigs([changedConfig]); + const second = await manager.reconcileServerConfigs([changedConfig]); + + expect(first).toMatchObject({ + restarted: [], + failed: ['clangd'], + unchanged: [], + }); + expect(second).toMatchObject({ + restarted: ['clangd'], + failed: [], + unchanged: [], + }); + expect(privateView.startServer).toHaveBeenCalledTimes(2); + }); + + it('serializes concurrent reconcile calls', async () => { + const { manager, privateView } = createReconcileManager(); + const order: string[] = []; + vi.mocked(privateView.startServer).mockImplementation( + async (name, handle) => { + order.push(`start:${name}`); + await new Promise((resolve) => setTimeout(resolve, 10)); + (handle as { status: string }).status = 'READY'; + }, + ); + + await Promise.all([ + manager.reconcileServerConfigs([serverConfig]), + manager.reconcileServerConfigs([ + { ...serverConfig, command: 'clangd-next' }, + ]), + ]); + + expect(order).toEqual(['start:clangd', 'start:clangd']); + expect(manager.getHandles().get('clangd')?.config.command).toBe( + 'clangd-next', + ); + }); + + it('aborts a server startup before removing it', async () => { + const { manager, privateView } = createReconcileManager(); + manager.setServerConfigs([serverConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + let resolveStartup!: () => void; + handle!.startingPromise = new Promise((resolve) => { + resolveStartup = resolve; + }); + handle!.startupAbortController = new AbortController(); + const abortSpy = vi.spyOn(handle!.startupAbortController, 'abort'); + + const reconcile = manager.reconcileServerConfigs([]); + await Promise.resolve(); + expect(abortSpy).toHaveBeenCalledOnce(); + expect(privateView.stopServer).not.toHaveBeenCalled(); + + resolveStartup(); + await reconcile; + + expect(privateView.stopServer).toHaveBeenCalledOnce(); + expect(manager.getHandles().has('clangd')).toBe(false); + }); + + it('aborts a server startup before restarting it', async () => { + const { manager, privateView } = createReconcileManager(); + manager.setServerConfigs([serverConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + let resolveStartup!: () => void; + handle!.startingPromise = new Promise((resolve) => { + resolveStartup = resolve; + }); + handle!.startupAbortController = new AbortController(); + const abortSpy = vi.spyOn(handle!.startupAbortController, 'abort'); + + const reconcile = manager.reconcileServerConfigs([ + { ...serverConfig, args: ['--log=verbose'] }, + ]); + await Promise.resolve(); + expect(abortSpy).toHaveBeenCalledOnce(); + expect(privateView.stopServer).not.toHaveBeenCalled(); + + resolveStartup(); + await reconcile; + + expect(privateView.stopServer).toHaveBeenCalledOnce(); + expect(privateView.startServer).toHaveBeenCalledOnce(); + }); + + it('stopAll waits for an active reconcile before clearing handles', async () => { + const { manager, privateView } = createReconcileManager(); + let resolveStart!: () => void; + vi.mocked(privateView.startServer).mockImplementation( + async (_name, handle) => { + await new Promise((resolve) => { + resolveStart = resolve; + }); + (handle as { status: string }).status = 'READY'; + }, + ); + + const reconcile = manager.reconcileServerConfigs([serverConfig]); + await Promise.resolve(); + const stopAll = manager.stopAll(); + await Promise.resolve(); + + expect(manager.getHandles().has('clangd')).toBe(true); + resolveStart(); + await Promise.all([reconcile, stopAll]); + + expect(privateView.stopServer).toHaveBeenCalledOnce(); + expect(manager.getHandles().size).toBe(0); + }); + + it('serializes stopAll with later reconcile calls', async () => { + const { manager, privateView } = createReconcileManager(); + const order: string[] = []; + manager.setServerConfigs([serverConfig]); + vi.mocked(privateView.stopServer).mockImplementation( + async (_name, handle) => { + order.push('stop'); + await new Promise((resolve) => setTimeout(resolve, 10)); + (handle as { status: string }).status = 'NOT_STARTED'; + }, + ); + vi.mocked(privateView.startServer).mockImplementation( + async (_name, handle) => { + order.push('start'); + (handle as { status: string }).status = 'READY'; + }, + ); + + await Promise.all([ + manager.stopAll(), + manager.reconcileServerConfigs([{ ...serverConfig, command: 'next' }]), + ]); + + expect(order).toEqual(['stop', 'start']); + expect(manager.getHandles().get('clangd')?.config.command).toBe('next'); + }); + }); + describe('isPathSafe', () => { it('allows bare commands resolved through PATH', () => { const workspaceRoot = path.resolve('/workspace/project'); @@ -149,6 +451,8 @@ describe('LspServerManager', () => { }, 'isPathSafe', ).mockReturnValue(true); + const connection = createMockConnection(); + const process = createMockProcess(); vi.spyOn( manager as unknown as { createLspConnection: ( @@ -157,7 +461,8 @@ describe('LspServerManager', () => { }, 'createLspConnection', ).mockResolvedValue({ - connection: {}, + connection, + process, processDiagnostics, } as unknown as LspConnectionResult); vi.spyOn( @@ -170,6 +475,13 @@ describe('LspServerManager', () => { manager.setServerConfigs([serverConfig]); await manager.startAll(); + expect(connection.end).toHaveBeenCalledOnce(); + expect(process.kill).toHaveBeenCalledOnce(); + expect(manager.getHandles().get('clangd')?.connection).toBeUndefined(); + expect(manager.getHandles().get('clangd')?.process).toBeUndefined(); + expect(manager.getHandles().get('clangd')?.processDiagnostics).toBe( + processDiagnostics, + ); expect(debugLoggerMock.error).toHaveBeenCalledWith( 'LSP server clangd process diagnostics:', processDiagnostics, @@ -179,4 +491,1245 @@ describe('LspServerManager', () => { expect.any(Error), ); }); + + it('logs when workspace trust check blocks startup', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(false); + + manager.setServerConfigs([serverConfig]); + await manager.startAll(); + + expect(manager.getHandles().get('clangd')?.status).toBe('FAILED'); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'Workspace trust check failed, not starting LSP server clangd', + ); + }); + + it('does not probe command existence for unsafe command paths', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + const isPathSafe = vi + .spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ) + .mockReturnValue(false); + const commandExists = vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ); + + manager.setServerConfigs([ + { ...serverConfig, command: '../../outside/payload' }, + ]); + await manager.startAll(); + + expect(isPathSafe).toHaveBeenCalledOnce(); + expect(commandExists).not.toHaveBeenCalled(); + expect(manager.getHandles().get('clangd')?.status).toBe('FAILED'); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'LSP server clangd command path is unsafe: ../../outside/payload', + ); + }); + + it('retries the same config after initial command lookup failure', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true); + vi.spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ).mockResolvedValue({ + connection: createMockConnection(), + process: createMockProcess() as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + + manager.setServerConfigs([serverConfig]); + await manager.startAll(); + const result = await manager.reconcileServerConfigs([serverConfig]); + + expect(result.restarted).toEqual(['clangd']); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + }); + + it('passes LSP config env through command probe filtering', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + const commandExists = vi + .spyOn( + manager as unknown as { + commandExists: ( + command: string, + env?: Record, + cwd?: string, + ) => Promise; + }, + 'commandExists', + ) + .mockResolvedValue(false); + + manager.setServerConfigs([ + { + ...serverConfig, + env: { PATH: '/tmp/fake-bin', SAFE_VALUE: '1' }, + }, + ]); + await manager.startAll(); + + expect(commandExists).toHaveBeenCalledWith( + 'clangd', + { PATH: '/tmp/fake-bin', SAFE_VALUE: '1' }, + '/workspace', + ); + }); + + it('retries the same config after a crash restart failure', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true); + vi.spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ).mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true }; + + manager.setServerConfigs([config]); + await manager.startAll(); + expect(exitHandler).toBeDefined(); + + exitHandler?.(1); + const result = await manager.reconcileServerConfigs([config]); + + expect(result.restarted).toEqual(['clangd']); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + }); + + it('does not restart a crashed server while stopping all servers', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true }; + + manager.setServerConfigs([config]); + await manager.startAll(); + expect(exitHandler).toBeDefined(); + + exitHandler?.(1); + await manager.stopAll(); + + expect(createLspConnection).toHaveBeenCalledOnce(); + expect(manager.getHandles().size).toBe(0); + }); + + it('does not restart a crashed stale handle', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true }; + + manager.setServerConfigs([config]); + await manager.startAll(); + expect(exitHandler).toBeDefined(); + + exitHandler?.(1); + manager.clearServerHandles(); + await Promise.resolve(); + + expect(createLspConnection).toHaveBeenCalledOnce(); + }); + + it('does not restart a crashed handle already being stopped', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true }; + + manager.setServerConfigs([config]); + await manager.startAll(); + const handle = manager.getHandles().get('clangd'); + expect(exitHandler).toBeDefined(); + expect(handle).toBeDefined(); + + exitHandler?.(1); + handle!.stopRequested = true; + await Promise.resolve(); + + expect(createLspConnection).toHaveBeenCalledOnce(); + }); + + it('retries the same config after a crash without restartOnCrash', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + + manager.setServerConfigs([serverConfig]); + await manager.startAll(); + expect(exitHandler).toBeDefined(); + + exitHandler?.(1); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'LSP server clangd exited but restartOnCrash is disabled', + ); + const result = await manager.reconcileServerConfigs([serverConfig]); + + expect(result.restarted).toEqual(['clangd']); + expect(createLspConnection).toHaveBeenCalledTimes(2); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + }); + + it('logs when a crashed server has zero restart attempts configured', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ).mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true, maxRestarts: 0 }; + + manager.setServerConfigs([config]); + await manager.startAll(); + expect(exitHandler).toBeDefined(); + + exitHandler?.(1); + + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'LSP server clangd exited but maxRestarts is 0', + ); + expect(manager.getHandles().get('clangd')?.status).toBe('FAILED'); + }); + + it('retries the same config after crash restart attempts are exhausted', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + const config = { ...serverConfig, restartOnCrash: true, maxRestarts: 1 }; + + manager.setServerConfigs([config]); + await manager.startAll(); + const handle = manager.getHandles().get('clangd'); + expect(exitHandler).toBeDefined(); + expect(handle).toBeDefined(); + handle!.restartAttempts = 1; + + exitHandler?.(1); + const result = await manager.reconcileServerConfigs([config]); + + expect(result.restarted).toEqual(['clangd']); + expect(createLspConnection).toHaveBeenCalledTimes(2); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + }); + + it('filters security-sensitive LSP environment overrides', () => { + const manager = createTrustedManager(); + const env = ( + manager as unknown as { + buildProcessEnv(env: Record): NodeJS.ProcessEnv; + } + ).buildProcessEnv({ + PATH: '/tmp/fake-bin', + NODE_OPTIONS: '--require /tmp/hook.js', + node_options: '--require /tmp/lowercase-hook.js', + Ld_PreLoad: '/tmp/preload.so', + SAFE_VALUE: '1', + }); + + expect(env['PATH']).toBe('/tmp/fake-bin'); + expect(env['NODE_OPTIONS']).toBe(process.env['NODE_OPTIONS']); + expect(env['node_options']).toBeUndefined(); + expect(env['Ld_PreLoad']).toBeUndefined(); + expect(env['SAFE_VALUE']).toBe('1'); + }); + + it('does not use LSP config PATH when probing command existence', () => { + const manager = createTrustedManager(); + const env = ( + manager as unknown as { + buildCommandProbeEnv(env: Record): NodeJS.ProcessEnv; + } + ).buildCommandProbeEnv({ + PATH: '/tmp/fake-bin', + Path: '/tmp/fake-bin-windows', + JAVA_HOME: '/opt/java', + NODE_OPTIONS: '--require /tmp/hook.js', + SAFE_VALUE: '1', + }); + + expect(env['PATH']).not.toBe('/tmp/fake-bin'); + expect(env['Path']).not.toBe('/tmp/fake-bin-windows'); + expect(env['JAVA_HOME']).toBe('/opt/java'); + expect(env['NODE_OPTIONS']).toBe(process.env['NODE_OPTIONS']); + expect(env['SAFE_VALUE']).toBe('1'); + }); + + it('ignores reset errors while queueing a crash restart', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess({ + kill: vi.fn(() => { + throw new Error('kill failed'); + }), + }); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + const connection = createMockConnection({ + end: vi.fn(() => { + throw new Error('end failed'); + }), + }); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValueOnce({ + connection, + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult) + .mockResolvedValueOnce({ + connection: createMockConnection(), + process: createMockProcess() as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockResolvedValue(undefined); + + manager.setServerConfigs([{ ...serverConfig, restartOnCrash: true }]); + await manager.startAll(); + exitHandler?.(1); + await manager.reconcileServerConfigs([ + { ...serverConfig, restartOnCrash: true }, + ]); + + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'Error closing LSP connection during reset:', + expect.any(Error), + ); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'Error killing LSP process during reset:', + expect.any(Error), + ); + expect(manager.getHandles().get('clangd')?.status).toBe('READY'); + }); + + it('kills owned process after graceful shutdown for socket transports', async () => { + const manager = createTrustedManager(); + const connection = createMockConnection(); + const process = createMockProcess(); + const socketConfig: LspServerConfig = { + ...serverConfig, + transport: 'tcp', + socket: { host: '127.0.0.1', port: 9876 }, + }; + manager.setServerConfigs([socketConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.process = process as unknown as ChildProcess; + handle!.status = 'READY'; + + await manager.stopAll(); + + expect(connection.shutdown).toHaveBeenCalledOnce(); + expect(connection.end).toHaveBeenCalledOnce(); + expect(process.kill).toHaveBeenCalledOnce(); + }); + + it('cancels an in-flight socket startup retry when stopped', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + vi.spyOn(LspConnectionFactory, 'createSocketConnection').mockReturnValue( + new Promise(() => {}), + ); + manager.setServerConfigs([ + { + ...serverConfig, + command: process.execPath, + args: ['-e', 'setTimeout(() => {}, 10000);'], + transport: 'tcp', + socket: { host: '127.0.0.1', port: 65534 }, + workspaceFolder: process.cwd(), + rootUri: pathToRootUri(process.cwd()), + startupTimeout: 30_000, + }, + ]); + + const startAll = manager.startAll(); + await vi.waitFor(() => { + expect(LspConnectionFactory.createSocketConnection).toHaveBeenCalled(); + }); + + const stopped = await Promise.race([ + manager.stopAll().then(() => true), + new Promise((resolve) => setTimeout(() => resolve(false), 1000)), + ]); + await startAll; + + expect(stopped).toBe(true); + expect(manager.getHandles().size).toBe(0); + }); + + it('cancels socket command spawn wait when startup is aborted', async () => { + const manager = createTrustedManager(); + const controller = new AbortController(); + const childProcess = { + exitCode: null, + kill: vi.fn(), + once: vi.fn( + (_event: string, _handler: (...args: unknown[]) => void) => + childProcess, + ), + off: vi.fn( + (_event: string, _handler: (...args: unknown[]) => void) => + childProcess, + ), + }; + const privateView = manager as unknown as { + waitForSocketProcessSpawn( + process: ChildProcess, + signal: AbortSignal, + ): Promise; + }; + + const wait = privateView.waitForSocketProcessSpawn( + childProcess as unknown as ChildProcess, + controller.signal, + ); + controller.abort(); + + await expect(wait).rejects.toThrow('LSP server startup cancelled'); + expect(childProcess.kill).toHaveBeenCalledOnce(); + expect(childProcess.off).toHaveBeenCalledWith( + 'spawn', + expect.any(Function), + ); + expect(childProcess.off).toHaveBeenCalledWith( + 'error', + expect.any(Function), + ); + }); + + it('cleans up a socket connection that resolves after startup abort wins', async () => { + const manager = createTrustedManager(); + const controller = new AbortController(); + const connection = { connection: { end: vi.fn() } }; + let resolveConnection!: (value: typeof connection) => void; + const privateView = manager as unknown as { + raceStartupAbort( + promise: Promise, + signal: AbortSignal, + cleanupAfterAbort: (value: T) => void, + ): Promise; + }; + const connectionPromise = new Promise((resolve) => { + resolveConnection = resolve; + }); + + const wait = privateView.raceStartupAbort( + connectionPromise, + controller.signal, + (value) => value.connection.end(), + ); + controller.abort(); + await expect(wait).rejects.toThrow('LSP server startup cancelled'); + + resolveConnection(connection); + await Promise.resolve(); + + expect(connection.connection.end).toHaveBeenCalledOnce(); + }); + + it('observes a startup promise that rejects after abort wins', async () => { + const manager = createTrustedManager(); + const controller = new AbortController(); + let rejectStartup!: (error: Error) => void; + const privateView = manager as unknown as { + raceStartupAbort(promise: Promise, signal: AbortSignal): Promise; + }; + const startupPromise = new Promise((_resolve, reject) => { + rejectStartup = reject; + }); + + const wait = privateView.raceStartupAbort( + startupPromise, + controller.signal, + ); + controller.abort(); + await expect(wait).rejects.toThrow('LSP server startup cancelled'); + + rejectStartup(new Error('late socket failure')); + await Promise.resolve(); + }); + + it('cancels a startup that is waiting for protocol initialization', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ).mockResolvedValue({ + connection: createMockConnection(), + process: createMockProcess() as unknown as ChildProcess, + } as unknown as LspConnectionResult); + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockReturnValue(new Promise(() => {})); + + manager.setServerConfigs([serverConfig]); + const startAll = manager.startAll(); + await vi.waitFor(() => { + expect(manager.getHandles().get('clangd')?.status).toBe('IN_PROGRESS'); + }); + + const stopped = await Promise.race([ + manager.stopAll().then(() => true), + new Promise((resolve) => setTimeout(() => resolve(false), 1000)), + ]); + await startAll; + + expect(stopped).toBe(true); + expect(manager.getHandles().size).toBe(0); + }); + + it('fails socket startup early when the child exits before connect', async () => { + const manager = createTrustedManager(); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + vi.spyOn(LspConnectionFactory, 'createSocketConnection').mockReturnValue( + new Promise(() => {}), + ); + manager.setServerConfigs([ + { + ...serverConfig, + command: process.execPath, + args: [ + '-e', + 'process.stderr.write("socket startup failed\\n"); process.exit(7);', + ], + transport: 'tcp', + socket: { host: '127.0.0.1', port: 65533 }, + workspaceFolder: process.cwd(), + rootUri: pathToRootUri(process.cwd()), + startupTimeout: 30_000, + }, + ]); + + await manager.startAll(); + + const handle = manager.getHandles().get('clangd'); + expect(handle?.status).toBe('FAILED'); + expect(handle?.error?.message).toContain( + 'LSP server exited before socket connection was ready', + ); + expect(handle?.processDiagnostics).toMatchObject({ + stderrTail: 'socket startup failed\n', + exitCode: 7, + exitSignal: null, + }); + }); + + it('does not crash-restart a server that exits during protocol initialization', async () => { + const manager = createTrustedManager(); + let exitHandler: ((code: number | null) => void) | undefined; + const process = createMockProcess(); + process.once = vi.fn( + (event: string, handler: (code: number | null) => void) => { + if (event === 'exit') { + exitHandler = handler; + } + return process; + }, + ); + vi.spyOn( + manager as unknown as { + checkWorkspaceTrust: () => Promise; + }, + 'checkWorkspaceTrust', + ).mockResolvedValue(true); + vi.spyOn( + manager as unknown as { + isPathSafe: () => boolean; + }, + 'isPathSafe', + ).mockReturnValue(true); + vi.spyOn( + manager as unknown as { + commandExists: () => Promise; + }, + 'commandExists', + ).mockResolvedValue(true); + const createLspConnection = vi + .spyOn( + manager as unknown as { + createLspConnection: ( + config: LspServerConfig, + ) => Promise; + }, + 'createLspConnection', + ) + .mockResolvedValue({ + connection: createMockConnection(), + process: process as unknown as ChildProcess, + } as unknown as LspConnectionResult); + let resolveInitialize!: () => void; + vi.spyOn( + manager as unknown as { + initializeLspServer: () => Promise; + }, + 'initializeLspServer', + ).mockReturnValue( + new Promise((resolve) => { + resolveInitialize = resolve; + }), + ); + + manager.setServerConfigs([{ ...serverConfig, restartOnCrash: true }]); + const startAll = manager.startAll(); + await vi.waitFor(() => { + expect(exitHandler).toBeDefined(); + }); + exitHandler?.(1); + resolveInitialize(); + await startAll; + + expect(createLspConnection).toHaveBeenCalledOnce(); + expect(manager.getHandles().get('clangd')?.status).toBe('FAILED'); + }); + + it('logs and continues when killing an owned process throws', async () => { + const manager = createTrustedManager(); + const connection = createMockConnection(); + const killError = new Error('kill failed'); + const process = createMockProcess({ + kill: vi.fn(() => { + throw killError; + }), + }); + manager.setServerConfigs([serverConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.process = process as unknown as ChildProcess; + handle!.status = 'READY'; + + await expect(manager.stopAll()).resolves.toBeUndefined(); + + expect(process.kill).toHaveBeenCalledOnce(); + expect(debugLoggerMock.warn).toHaveBeenCalledWith( + 'Error killing LSP server clangd process:', + killError, + ); + expect(manager.getHandles().size).toBe(0); + }); + + it('clears shutdown timeout when shutdown completes first', async () => { + vi.useFakeTimers(); + const manager = createTrustedManager(); + const connection = createMockConnection(); + manager.setServerConfigs([{ ...serverConfig, shutdownTimeout: 30_000 }]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.status = 'READY'; + + await manager.stopAll(); + + expect(connection.shutdown).toHaveBeenCalledOnce(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('unrefs the shutdown timeout', async () => { + const originalSetTimeout = globalThis.setTimeout; + let shutdownTimer: ReturnType | undefined; + vi.spyOn(globalThis, 'setTimeout').mockImplementation( + (handler, timeout, ...args) => { + shutdownTimer = originalSetTimeout(handler, timeout, ...args); + return shutdownTimer; + }, + ); + const manager = createTrustedManager(); + const connection = createMockConnection(); + manager.setServerConfigs([{ ...serverConfig, shutdownTimeout: 30_000 }]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.status = 'READY'; + + await manager.stopAll(); + + expect(shutdownTimer).toBeDefined(); + expect(shutdownTimer?.hasRef()).toBe(false); + }); + + it('unrefs and clears command probe timeout when the command errors first', async () => { + const manager = createTrustedManager(); + const timer = { + unref: vi.fn(), + } as unknown as ReturnType; + vi.spyOn(globalThis, 'setTimeout').mockReturnValue(timer); + const clearTimeout = vi + .spyOn(globalThis, 'clearTimeout') + .mockImplementation(() => undefined); + const commandExists = ( + manager as unknown as { + commandExists(command: string): Promise; + } + ).commandExists('__qwen_lsp_missing_command__'); + + await expect(commandExists).resolves.toBe(false); + expect(timer.unref).toHaveBeenCalledOnce(); + expect(clearTimeout).toHaveBeenCalledWith(timer); + }); + + it('ends the connection when shutdown timeout fires', async () => { + vi.useFakeTimers(); + const manager = createTrustedManager(); + const connection = createMockConnection({ + shutdown: vi.fn(() => new Promise(() => {})), + }); + manager.setServerConfigs([{ ...serverConfig, shutdownTimeout: 30_000 }]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.status = 'READY'; + + const stopAll = manager.stopAll(); + await vi.advanceTimersByTimeAsync(30_000); + await stopAll; + + expect(connection.end).toHaveBeenCalledOnce(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('uses the default shutdown timeout when none is configured', async () => { + vi.useFakeTimers(); + const manager = createTrustedManager(); + const connection = createMockConnection({ + shutdown: vi.fn(() => new Promise(() => {})), + }); + manager.setServerConfigs([serverConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + handle!.connection = connection; + handle!.status = 'READY'; + + const stopAll = manager.stopAll(); + await vi.advanceTimersByTimeAsync(5000); + await stopAll; + + expect(connection.end).toHaveBeenCalledOnce(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('waits for an in-flight startup before releasing server resources', async () => { + const manager = createTrustedManager(); + const connection = createMockConnection(); + const process = createMockProcess(); + manager.setServerConfigs([serverConfig]); + const handle = manager.getHandles().get('clangd'); + expect(handle).toBeDefined(); + let resolveStartup!: () => void; + handle!.startingPromise = new Promise((resolve) => { + resolveStartup = () => { + handle!.connection = connection; + handle!.process = process as unknown as ChildProcess; + resolve(); + }; + }); + + const stopAll = manager.stopAll(); + await Promise.resolve(); + expect(connection.end).not.toHaveBeenCalled(); + expect(process.kill).not.toHaveBeenCalled(); + + resolveStartup(); + await stopAll; + + expect(connection.shutdown).toHaveBeenCalledOnce(); + expect(connection.end).toHaveBeenCalledOnce(); + expect(process.kill).toHaveBeenCalledOnce(); + }); }); + +function createMockConnection( + overrides: Partial = {}, +): LspConnectionInterface { + return { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(async () => {}), + end: vi.fn(), + ...overrides, + }; +} + +function createMockProcess( + overrides: { + exitCode?: number | null; + kill?: ReturnType; + once?: ReturnType; + off?: ReturnType; + } = {}, +): { + exitCode: number | null; + kill: ReturnType; + once: ReturnType; + off: ReturnType; +} { + return { + exitCode: overrides.exitCode ?? null, + kill: overrides.kill ?? vi.fn(), + once: overrides.once ?? vi.fn(), + off: overrides.off ?? vi.fn(), + }; +} diff --git a/packages/core/src/lsp/LspServerManager.ts b/packages/core/src/lsp/LspServerManager.ts index fa3beb1958..02243961d0 100644 --- a/packages/core/src/lsp/LspServerManager.ts +++ b/packages/core/src/lsp/LspServerManager.ts @@ -16,6 +16,7 @@ import { LspConnectionFactory } from './LspConnectionFactory.js'; import { DEFAULT_LSP_COMMAND_CHECK_TIMEOUT_MS, DEFAULT_LSP_MAX_RESTARTS, + DEFAULT_LSP_SHUTDOWN_TIMEOUT_MS, DEFAULT_LSP_SOCKET_MAX_RETRY_DELAY_MS, DEFAULT_LSP_SOCKET_RETRY_DELAY_MS, DEFAULT_LSP_STARTUP_TIMEOUT_MS, @@ -23,22 +24,45 @@ import { } from './constants.js'; import type { LspConnectionResult, + LspProcessDiagnostics, + LspReconcileResult, LspServerConfig, LspServerHandle, LspServerStatus, LspSocketOptions, } from './types.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { lspServerConfigHash } from './configHash.js'; const debugLogger = createDebugLogger('LSP'); +const SECURITY_SENSITIVE_ENV_KEYS = new Set([ + 'DYLD_INSERT_LIBRARIES', + 'DYLD_LIBRARY_PATH', + 'LD_AUDIT', + 'LD_LIBRARY_PATH', + 'LD_PRELOAD', + 'NODE_OPTIONS', +]); export interface LspServerManagerOptions { requireTrustedWorkspace: boolean; workspaceRoot: string; } +/** + * Owns the per-session lifecycle of configured LSP servers. + * + * The manager is deliberately session-local: it stores one handle per server + * name, starts/stops subprocess or socket-backed connections, and reconciles + * config changes without replacing unchanged handles. Callers must pass only + * configs that have already passed service-level admission checks. + */ export class LspServerManager { private serverHandles: Map = new Map(); + private serverConfigHashes: Map = new Map(); + /** Serializes hot-reload reconcile calls so stop/start operations do not race. */ + private reconcileQueue: Promise = Promise.resolve(); + private stoppingAll = false; private requireTrustedWorkspace: boolean; private workspaceRoot: string; @@ -54,16 +78,32 @@ export class LspServerManager { setServerConfigs(configs: LspServerConfig[]): void { this.serverHandles.clear(); + this.serverConfigHashes.clear(); for (const config of configs) { this.serverHandles.set(config.name, { config, status: 'NOT_STARTED', }); + this.serverConfigHashes.set(config.name, lspServerConfigHash(config)); } + debugLogger.info( + `Prepared ${configs.length} LSP server config(s): ${formatServerNames( + configs.map((config) => config.name), + )}`, + ); } + /** Drops all prepared handles without attempting process shutdown. */ clearServerHandles(): void { + if (this.serverHandles.size > 0) { + debugLogger.info( + `Clearing ${this.serverHandles.size} LSP server handle(s): ${formatServerNames( + Array.from(this.serverHandles.keys()), + )}`, + ); + } this.serverHandles.clear(); + this.serverConfigHashes.clear(); } getHandles(): ReadonlyMap { @@ -84,11 +124,141 @@ export class LspServerManager { } } + /** + * Stops every server after any in-flight reconcile has drained. + * + * This prevents shutdown from clearing handles while a queued reconcile is + * still able to start a new process. + */ async stopAll(): Promise { - for (const [name, handle] of Array.from(this.serverHandles)) { - await this.stopServer(name, handle); + this.stoppingAll = true; + const stop = async () => { + try { + for (const [name, handle] of Array.from(this.serverHandles)) { + await this.stopServer(name, handle); + } + this.serverHandles.clear(); + this.serverConfigHashes.clear(); + } finally { + this.stoppingAll = false; + } + }; + const next = this.reconcileQueue.then(stop, stop); + this.reconcileQueue = next.catch(() => undefined); + return next; + } + + async reconcileServerConfigs( + configs: LspServerConfig[], + ): Promise { + // Keep the returned promise as the caller-visible result, but store a + // swallowed version in the queue so one failed reconcile does not poison + // every future hot reload. + const run = async () => this.doReconcileServerConfigs(configs); + const next = this.reconcileQueue.then(run, run); + this.reconcileQueue = next.catch(() => undefined); + return next; + } + + /** + * Applies a desired config set incrementally. + * + * Hashes identify semantic config changes. Unchanged servers keep their + * existing connection and warm state; removed or changed servers are stopped + * before their handles are deleted or replaced. + */ + private async doReconcileServerConfigs( + configs: LspServerConfig[], + ): Promise { + debugLogger.info( + `Reconciling LSP server configs: desired=${formatServerNames( + configs.map((config) => config.name), + )}`, + ); + const desiredConfigs = new Map(); + const desiredHashes = new Map(); + for (const config of configs) { + desiredConfigs.set(config.name, config); + desiredHashes.set(config.name, lspServerConfigHash(config)); } - this.serverHandles.clear(); + + const result: LspReconcileResult = { + added: [], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }; + + for (const [name, handle] of Array.from(this.serverHandles)) { + const nextConfig = desiredConfigs.get(name); + if (!nextConfig) { + await this.abortAndWaitForStartup(handle); + await this.stopServer(name, handle); + this.serverHandles.delete(name); + this.serverConfigHashes.delete(name); + result.removed.push(name); + continue; + } + + const nextHash = desiredHashes.get(name); + if (this.serverConfigHashes.get(name) !== nextHash) { + await this.abortAndWaitForStartup(handle); + await this.stopServer(name, handle); + const nextHandle: LspServerHandle = { + config: nextConfig, + status: 'NOT_STARTED', + }; + this.serverHandles.set(name, nextHandle); + await this.startServer(name, nextHandle); + if (nextHandle.status === 'FAILED') { + this.serverConfigHashes.delete(name); + result.failed.push(name); + } else { + if (nextHash) { + this.serverConfigHashes.set(name, nextHash); + } + result.restarted.push(name); + } + } else { + result.unchanged.push(name); + } + } + + for (const [name, config] of desiredConfigs) { + if (this.serverHandles.has(name)) { + continue; + } + const handle: LspServerHandle = { + config, + status: 'NOT_STARTED', + }; + this.serverHandles.set(name, handle); + await this.startServer(name, handle); + if (handle.status === 'FAILED') { + this.serverConfigHashes.delete(name); + result.failed.push(name); + } else { + const hash = desiredHashes.get(name); + if (hash) { + this.serverConfigHashes.set(name, hash); + } + result.added.push(name); + } + } + + debugLogger.info( + `LSP reconcile result: added=${formatServerNames( + result.added, + )}, removed=${formatServerNames( + result.removed, + )}, restarted=${formatServerNames( + result.restarted, + )}, unchanged=${formatServerNames( + result.unchanged, + )}, failed=${formatServerNames(result.failed)}`, + ); + return result; } /** @@ -173,7 +343,8 @@ export class LspServerManager { name: string, handle: LspServerHandle, ): Promise { - // If already starting, wait for the existing promise + // A handle can be reached by startAll(), reconcile, or crash restart. Share + // one startup promise so concurrent callers cannot spawn duplicate servers. if (handle.startingPromise) { return handle.startingPromise; } @@ -182,17 +353,22 @@ export class LspServerManager { return; } handle.stopRequested = false; + handle.processExitedUnexpectedly = false; - // Create a promise to lock concurrent calls handle.startingPromise = this.doStartServer(name, handle).finally(() => { handle.startingPromise = undefined; + handle.startupAbortController = undefined; }); return handle.startingPromise; } /** - * Internal method that performs the actual server startup. + * Performs startup after the per-handle startup lock is installed. + * + * All admission and command safety checks happen before process creation. + * If creation or initialize fails after resources exist, the catch path tears + * them down and leaves a FAILED handle for status reporting. * * @param name - The name of the LSP server * @param handle - The LSP server handle @@ -210,6 +386,7 @@ export class LspServerManager { `LSP server ${name} requires trusted workspace, skipping startup`, ); handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } @@ -220,16 +397,30 @@ export class LspServerManager { workspaceTrusted, ); if (!trusted) { - debugLogger.info( + debugLogger.warn( `Workspace trust check failed, not starting LSP server ${name}`, ); handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } // Check if command exists if (handle.config.command) { const commandCwd = handle.config.workspaceFolder ?? this.workspaceRoot; + // Check path safety before any command probe can spawn the configured + // executable. + if ( + !this.isPathSafe(handle.config.command, this.workspaceRoot, commandCwd) + ) { + debugLogger.warn( + `LSP server ${name} command path is unsafe: ${handle.config.command}`, + ); + handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); + return; + } + if ( !(await this.commandExists( handle.config.command, @@ -241,34 +432,47 @@ export class LspServerManager { `LSP server ${name} command not found: ${handle.config.command}`, ); handle.status = 'FAILED'; - return; - } - - // Check path safety - if ( - !this.isPathSafe(handle.config.command, this.workspaceRoot, commandCwd) - ) { - debugLogger.warn( - `LSP server ${name} command path is unsafe: ${handle.config.command}`, - ); - handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } } try { + const startupAbortController = new AbortController(); + handle.startupAbortController = startupAbortController; handle.error = undefined; + handle.processDiagnostics = undefined; handle.warmedUp = false; handle.status = 'IN_PROGRESS'; + debugLogger.info( + `Starting LSP server ${name}: command=${ + handle.config.command ?? '' + }, transport=${handle.config.transport}, languages=${formatServerNames( + handle.config.languages, + )}`, + ); // Create LSP connection - const connection = await this.createLspConnection(handle.config); + const connection = await this.createLspConnection( + handle.config, + startupAbortController.signal, + ); handle.connection = connection.connection; handle.process = connection.process; handle.processDiagnostics = connection.processDiagnostics; - // Initialize LSP server - await this.initializeLspServer(connection, handle.config); + const startupExit = this.createStartupExitWatcher(name, handle); + try { + // Initialize LSP server + await this.raceStartupAbort( + this.initializeLspServer(connection, handle.config), + startupAbortController.signal, + undefined, + startupExit?.promise, + ); + } finally { + startupExit?.dispose(); + } handle.status = 'READY'; this.attachRestartHandler(name, handle); @@ -276,59 +480,133 @@ export class LspServerManager { } catch (error) { handle.status = 'FAILED'; handle.error = error as Error; + const processDiagnostics = + typeof error === 'object' && error !== null + ? (error as { processDiagnostics?: LspProcessDiagnostics }) + .processDiagnostics + : undefined; + if (processDiagnostics) { + handle.processDiagnostics = processDiagnostics; + } + this.serverConfigHashes.delete(name); + if (!handle.processExitedUnexpectedly) { + handle.stopRequested = true; + } + await this.releaseServerResources(name, handle, false); if (handle.processDiagnostics) { debugLogger.error( `LSP server ${name} process diagnostics:`, handle.processDiagnostics, ); } + handle.connection = undefined; + handle.process = undefined; debugLogger.error(`LSP server ${name} failed to start:`, error); } } /** - * Stop individual LSP server + * Stops a server and resets runtime-only handle state. */ private async stopServer( name: string, handle: LspServerHandle, ): Promise { + debugLogger.info(`Stopping LSP server ${name}`); handle.stopRequested = true; + handle.startupAbortController?.abort(); - if (handle.connection) { - try { - await this.shutdownConnection(handle); - } catch (error) { - debugLogger.error(`Error closing LSP server ${name}:`, error); - } - } else if (handle.process && handle.process.exitCode === null) { - handle.process.kill(); + if (handle.startingPromise) { + await handle.startingPromise; } + await this.releaseServerResources(name, handle, true); handle.connection = undefined; handle.process = undefined; handle.processDiagnostics = undefined; handle.status = 'NOT_STARTED'; handle.warmedUp = false; handle.restartAttempts = 0; + debugLogger.info(`LSP server ${name} stopped`); } + private async abortAndWaitForStartup(handle: LspServerHandle): Promise { + if (!handle.startingPromise) { + return; + } + handle.startupAbortController?.abort(); + await handle.startingPromise.catch(() => undefined); + } + + /** + * Releases runtime resources for a handle without changing its logical config. + * + * Connection shutdown and process termination are intentionally isolated so a + * broken JSON-RPC stream cannot prevent killing an owned server process. + */ + private async releaseServerResources( + name: string, + handle: LspServerHandle, + graceful: boolean, + ): Promise { + // Connection teardown and process kill are separate on purpose: a broken + // pipe during end()/shutdown() must not prevent killing an owned process. + if (handle.connection) { + try { + if (graceful) { + await this.shutdownConnection(handle); + } else { + handle.connection.end(); + } + } catch (error) { + debugLogger.error(`Error closing LSP server ${name}:`, error); + } + } + + if (handle.process && handle.process.exitCode === null) { + try { + handle.process.kill(); + } catch (error) { + debugLogger.warn(`Error killing LSP server ${name} process:`, error); + } + } + } + + /** + * Performs graceful LSP shutdown with a bounded wait, then always closes the + * underlying JSON-RPC connection to avoid retaining streams or sockets. + */ private async shutdownConnection(handle: LspServerHandle): Promise { if (!handle.connection) { return; } try { const shutdownPromise = handle.connection.shutdown(); - if (typeof handle.config.shutdownTimeout === 'number') { + void shutdownPromise.catch(() => undefined); + const timeout = + handle.config.shutdownTimeout ?? DEFAULT_LSP_SHUTDOWN_TIMEOUT_MS; + let timerId: ReturnType | undefined; + try { await Promise.race([ shutdownPromise, - new Promise((resolve) => - setTimeout(resolve, handle.config.shutdownTimeout), - ), + new Promise((resolve) => { + timerId = setTimeout(resolve, timeout); + if ( + typeof timerId === 'object' && + timerId !== null && + 'unref' in timerId + ) { + timerId.unref(); + } + }), ]); - } else { - await shutdownPromise; + } finally { + if (timerId !== undefined) { + clearTimeout(timerId); + } } } finally { + // Always end the JSON-RPC connection, even if shutdown rejects or times + // out, so streams and socket handles are not retained. handle.connection.end(); } } @@ -341,13 +619,24 @@ export class LspServerManager { if (handle.stopRequested) { return; } + handle.processExitedUnexpectedly = true; + // Only unexpected process exits can trigger restart. Explicit stops set + // stopRequested before terminating the process. if (!handle.config.restartOnCrash) { + debugLogger.warn( + `LSP server ${name} exited but restartOnCrash is disabled`, + ); handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } const maxRestarts = handle.config.maxRestarts ?? DEFAULT_LSP_MAX_RESTARTS; if (maxRestarts <= 0) { + debugLogger.warn( + `LSP server ${name} exited but maxRestarts is ${maxRestarts}`, + ); handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } const attempts = handle.restartAttempts ?? 0; @@ -356,23 +645,96 @@ export class LspServerManager { `LSP server ${name} reached max restart attempts (${maxRestarts}), stopping restarts`, ); handle.status = 'FAILED'; + this.serverConfigHashes.delete(name); return; } handle.restartAttempts = attempts + 1; debugLogger.warn( `LSP server ${name} exited (code ${code ?? 'unknown'}), restarting (${handle.restartAttempts}/${maxRestarts})`, ); + this.enqueueCrashRestart(name, handle); + }); + } + + private createStartupExitWatcher( + name: string, + handle: LspServerHandle, + ): + | { + promise: Promise; + dispose: () => void; + } + | undefined { + if (!handle.process) { + return undefined; + } + let onExit: + | ((code: number | null, signal: NodeJS.Signals | null) => void) + | undefined; + const promise = new Promise((_, reject) => { + onExit = (code, signal) => { + handle.processExitedUnexpectedly = true; + reject( + new Error( + `LSP server ${name} exited before initialization completed (code ${ + code ?? 'unknown' + }, signal ${signal ?? 'unknown'})`, + ), + ); + }; + handle.process!.once('exit', onExit); + }); + return { + promise, + dispose: () => { + if (onExit) { + handle.process?.off('exit', onExit); + } + }, + }; + } + + private enqueueCrashRestart(name: string, handle: LspServerHandle): void { + const restart = async () => { + if (handle.startingPromise) { + await handle.startingPromise.catch(() => undefined); + } + if ( + this.stoppingAll || + this.serverHandles.get(name) !== handle || + handle.stopRequested + ) { + return; + } this.resetHandle(handle); - void this.startServer(name, handle); + await this.startServer(name, handle); + if (handle.status === 'FAILED') { + this.serverConfigHashes.delete(name); + } + }; + const next = this.reconcileQueue.then(restart, restart); + this.reconcileQueue = next.catch(() => undefined); + void next.catch((error) => { + debugLogger.warn(`LSP server ${name} crash restart failed:`, error); }); } private resetHandle(handle: LspServerHandle): void { + // Crash restart reuses the same logical handle, so clear runtime resources + // while preserving config and restartAttempts. if (handle.connection) { - handle.connection.end(); + try { + handle.connection.end(); + } catch (error) { + debugLogger.warn('Error closing LSP connection during reset:', error); + } } if (handle.process && handle.process.exitCode === null) { - handle.process.kill(); + try { + handle.process.kill(); + } catch (error) { + debugLogger.warn('Error killing LSP process during reset:', error); + } } handle.connection = undefined; handle.process = undefined; @@ -381,6 +743,7 @@ export class LspServerManager { handle.error = undefined; handle.warmedUp = false; handle.stopRequested = false; + handle.processExitedUnexpectedly = false; } private buildProcessEnv( @@ -389,26 +752,73 @@ export class LspServerManager { if (!env || Object.keys(env).length === 0) { return undefined; } - return { ...process.env, ...env }; + const filteredEnv: Record = {}; + for (const [key, value] of Object.entries(env)) { + if (SECURITY_SENSITIVE_ENV_KEYS.has(key.toUpperCase())) { + debugLogger.warn( + `Ignoring security-sensitive LSP server env override: ${key}`, + ); + continue; + } + filteredEnv[key] = value; + } + return { ...process.env, ...filteredEnv }; + } + + /** + * Builds the environment used only for the pre-start command probe. + * + * The probe runs `command --version`, so a workspace-controlled PATH could + * redirect a bare command such as `clangd` to an unintended executable before + * the real LSP server startup path is reached. Keep regular env values that + * probes may need, but always resolve commands through the current process + * PATH instead of `.lsp.json`. + */ + private buildCommandProbeEnv( + env: Record | undefined, + ): NodeJS.ProcessEnv | undefined { + if (!env || Object.keys(env).length === 0) { + return undefined; + } + const filteredEnv: Record = {}; + for (const [key, value] of Object.entries(env)) { + const normalizedKey = key.toUpperCase(); + if ( + normalizedKey === 'PATH' || + SECURITY_SENSITIVE_ENV_KEYS.has(normalizedKey) + ) { + debugLogger.warn( + `Ignoring security-sensitive LSP command probe env override: ${key}`, + ); + continue; + } + filteredEnv[key] = value; + } + return { ...process.env, ...filteredEnv }; } private async connectSocketWithRetry( socket: LspSocketOptions, timeoutMs: number, + signal?: AbortSignal, ): Promise< Awaited> > { + // Socket-based servers may need a short boot window after the command is + // spawned. Retry until the startup deadline instead of failing immediately. const deadline = Date.now() + timeoutMs; let attempt = 0; while (true) { + this.throwIfStartupAborted(signal); const remaining = deadline - Date.now(); if (remaining <= 0) { throw new Error('LSP server connection timeout'); } try { - return await LspConnectionFactory.createSocketConnection( - socket, - remaining, + return await this.raceStartupAbort( + LspConnectionFactory.createSocketConnection(socket, remaining), + signal, + (connection) => connection.connection.end(), ); } catch (error) { attempt += 1; @@ -419,16 +829,105 @@ export class LspServerManager { DEFAULT_LSP_SOCKET_RETRY_DELAY_MS * attempt, DEFAULT_LSP_SOCKET_MAX_RETRY_DELAY_MS, ); - await new Promise((resolve) => setTimeout(resolve, delay)); + await this.delayWithAbort(delay, signal); + } + } + } + + private throwIfStartupAborted(signal: AbortSignal | undefined): void { + if (signal?.aborted) { + throw new Error('LSP server startup cancelled'); + } + } + + private async delayWithAbort( + delayMs: number, + signal: AbortSignal | undefined, + ): Promise { + if (!signal) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + return; + } + this.throwIfStartupAborted(signal); + await new Promise((resolve, reject) => { + const onAbort = () => { + clearTimeout(timerId); + signal.removeEventListener('abort', onAbort); + reject(new Error('LSP server startup cancelled')); + }; + const timerId = setTimeout(() => { + signal.removeEventListener('abort', onAbort); + resolve(); + }, delayMs); + signal.addEventListener('abort', onAbort, { once: true }); + }); + } + + private async raceStartupAbort( + promise: Promise, + signal: AbortSignal | undefined, + cleanupAfterAbort?: (value: T) => void, + failurePromise?: Promise, + ): Promise { + if (!signal) { + if (!failurePromise) { + return promise; + } + void promise.catch(() => undefined); + void failurePromise.catch(() => undefined); + return Promise.race([promise, failurePromise]); + } + this.throwIfStartupAborted(signal); + let abortWon = false; + const observedPromise = promise.then( + (value) => { + if (abortWon) { + cleanupAfterAbort?.(value); + } + return value; + }, + (error: unknown) => { + throw error; + }, + ); + void observedPromise.catch(() => undefined); + if (failurePromise) { + void failurePromise.catch(() => undefined); + } + let onAbort: (() => void) | undefined; + try { + const raceInputs: Array | Promise> = [ + observedPromise, + new Promise((_, reject) => { + onAbort = () => { + abortWon = true; + signal.removeEventListener('abort', onAbort!); + reject(new Error('LSP server startup cancelled')); + }; + signal.addEventListener('abort', onAbort, { once: true }); + }), + ]; + if (failurePromise) { + raceInputs.push(failurePromise); + } + return await Promise.race(raceInputs); + } finally { + if (onAbort) { + signal.removeEventListener('abort', onAbort); } } } /** - * Create LSP connection + * Creates a transport-specific LSP connection. + * + * For stdio, the spawned process is always owned by this manager. For + * tcp/socket, the process is owned only when a command was provided; otherwise + * the connection is to an externally managed daemon. */ private async createLspConnection( config: LspServerConfig, + signal?: AbortSignal, ): Promise { const workspaceFolder = config.workspaceFolder ?? this.workspaceRoot; const startupTimeout = @@ -470,29 +969,50 @@ export class LspServerManager { } let process: ChildProcess | undefined; + let processDiagnostics: LspProcessDiagnostics | undefined; + let earlyExit: + | { + promise: Promise; + dispose: () => void; + } + | undefined; if (config.command) { + this.throwIfStartupAborted(signal); + processDiagnostics = { stderrTail: '' }; process = spawn(config.command, config.args ?? [], { cwd: workspaceFolder, env, - stdio: 'ignore', + stdio: ['ignore', 'ignore', 'pipe'], }); - await new Promise((resolve, reject) => { - process?.once('spawn', () => resolve()); - process?.once('error', (error) => { - reject(new Error(`Failed to spawn LSP server: ${error.message}`)); - }); + process.stderr?.on('data', (chunk: Buffer | string) => { + processDiagnostics!.stderrTail = ( + processDiagnostics!.stderrTail + chunk.toString() + ).slice(-4000); }); + earlyExit = this.watchSocketProcessEarlyExit( + process, + processDiagnostics, + ); + await this.waitForSocketProcessSpawn(process, signal); } try { - const lspConnection = await this.connectSocketWithRetry( + const socketConnection = this.connectSocketWithRetry( config.socket, startupTimeout, + signal, ); + const lspConnection = process + ? await this.waitForSocketConnectionOrProcessExit( + socketConnection, + earlyExit!, + ) + : await socketConnection; return { connection: lspConnection.connection, process, + processDiagnostics, shutdown: async () => { await lspConnection.connection.shutdown(); }, @@ -503,6 +1023,15 @@ export class LspServerManager { lspConnection.connection.initialize(params), }; } catch (error) { + if ( + processDiagnostics && + error instanceof Error && + !('processDiagnostics' in error) + ) { + ( + error as Error & { processDiagnostics: LspProcessDiagnostics } + ).processDiagnostics = processDiagnostics; + } if (process && process.exitCode === null) { process.kill(); } @@ -513,6 +1042,110 @@ export class LspServerManager { } } + private async waitForSocketConnectionOrProcessExit( + socketConnection: Promise< + Awaited> + >, + earlyExit: { promise: Promise; dispose: () => void }, + ): Promise< + Awaited> + > { + try { + return await Promise.race([socketConnection, earlyExit.promise]); + } finally { + earlyExit.dispose(); + } + } + + /** + * Waits for the LSP server child process to successfully spawn. + * + * Resolves when the 'spawn' event fires, rejects if an error occurs during + * spawning or if the provided abort signal is triggered. Ensures all event + * listeners are cleaned up regardless of outcome to prevent memory leaks. + * + * @param process - The child process to monitor for successful spawn + * @param signal - Optional AbortSignal to cancel the wait; if already aborted, + * the process is killed immediately and the promise rejects + */ + private async waitForSocketProcessSpawn( + process: ChildProcess, + signal: AbortSignal | undefined, + ): Promise { + this.throwIfStartupAborted(signal); + await new Promise((resolve, reject) => { + let settled = false; + const cleanup = () => { + process.off('spawn', onSpawn); + process.off('error', onError); + signal?.removeEventListener('abort', onAbort); + }; + const settle = (complete: () => void) => { + if (settled) { + return; + } + settled = true; + cleanup(); + complete(); + }; + const onSpawn = () => settle(resolve); + const onError = (error: Error) => + settle(() => + reject(new Error(`Failed to spawn LSP server: ${error.message}`)), + ); + const onAbort = () => + settle(() => { + if (process.exitCode === null) { + try { + process.kill(); + } catch (error) { + debugLogger.warn( + 'Error killing aborted LSP socket process:', + error, + ); + } + } + reject(new Error('LSP server startup cancelled')); + }); + + process.once('spawn', onSpawn); + process.once('error', onError); + signal?.addEventListener('abort', onAbort, { once: true }); + if (signal?.aborted) { + onAbort(); + } + }); + } + + private watchSocketProcessEarlyExit( + process: ChildProcess, + diagnostics: LspProcessDiagnostics | undefined, + ): { promise: Promise; dispose: () => void } { + let onExit: (code: number | null, signal: NodeJS.Signals | null) => void; + const promise = new Promise((_, reject) => { + onExit = (code, signal) => { + if (diagnostics) { + diagnostics.exitCode = code; + diagnostics.exitSignal = signal; + } + reject( + new Error( + `LSP server exited before socket connection was ready: code=${ + code ?? 'unknown' + }, signal=${signal ?? 'none'}`, + ), + ); + }; + process.once('exit', onExit); + }); + return { + promise, + dispose: () => { + process.off('exit', onExit); + }, + }; + } + /** * Initialize LSP server */ @@ -601,36 +1234,45 @@ export class LspServerManager { ): Promise { return new Promise((resolve) => { let settled = false; - const child = spawn(command, ['--version'], { - stdio: ['ignore', 'ignore', 'ignore'], - cwd: cwd ?? this.workspaceRoot, - env: this.buildProcessEnv(env), - }); - - child.on('error', () => { - settled = true; - resolve(false); - }); - - child.on('exit', (code) => { + const settle = (value: boolean) => { if (settled) { return; } settled = true; + clearTimeout(timerId); + resolve(value); + }; + const child = spawn(command, ['--version'], { + stdio: ['ignore', 'ignore', 'ignore'], + cwd: cwd ?? this.workspaceRoot, + env: this.buildCommandProbeEnv(env), + }); + + child.on('error', () => { + settle(false); + }); + + child.on('exit', (code) => { // 127 typically indicates command not found in shell - resolve(code !== 127); + settle(code !== 127); }); // If the process is still running after the timeout, it means the // command was found and started — it just didn't finish in time. // This is expected for servers like jdtls that don't support --version. - setTimeout(() => { + const timerId = setTimeout(() => { if (!settled) { - settled = true; child.kill(); - resolve(true); + settle(true); } }, DEFAULT_LSP_COMMAND_CHECK_TIMEOUT_MS); + if ( + typeof timerId === 'object' && + timerId !== null && + 'unref' in timerId + ) { + timerId.unref(); + } }); } @@ -739,3 +1381,7 @@ export class LspServerManager { return undefined; } } + +function formatServerNames(names: readonly string[]): string { + return names.length === 0 ? '' : names.join(','); +} diff --git a/packages/core/src/lsp/NativeLspClient.ts b/packages/core/src/lsp/NativeLspClient.ts index e78af742d7..1a137f4880 100644 --- a/packages/core/src/lsp/NativeLspClient.ts +++ b/packages/core/src/lsp/NativeLspClient.ts @@ -30,6 +30,7 @@ import type { LspSymbolInformation, LspWorkspaceEdit, LspServerStatusInfo, + LspServiceReinitializeResult, } from './types.js'; import type { NativeLspService } from './NativeLspService.js'; @@ -81,6 +82,10 @@ export class NativeLspClient implements LspClient { return result; } + reinitialize(): Promise { + return this.service.reinitialize(); + } + /** * Search for symbols across the workspace. * diff --git a/packages/core/src/lsp/NativeLspService.test.ts b/packages/core/src/lsp/NativeLspService.test.ts index 377d5f5723..68dbab5058 100644 --- a/packages/core/src/lsp/NativeLspService.test.ts +++ b/packages/core/src/lsp/NativeLspService.test.ts @@ -144,6 +144,963 @@ describe('NativeLspService', () => { expect(status).toBeDefined(); }); + test('reinitialize reconciles valid .lsp.json configs', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-reinit-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + args: ['--stdio'], + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const reconcileServerConfigs = vi.fn(async () => ({ + added: ['typescript-language-server'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + const result = await service.reinitialize(); + + expect(reconcileServerConfigs).toHaveBeenCalledWith([ + expect.objectContaining({ + name: 'typescript-language-server', + languages: ['typescript'], + }), + ]); + expect(result.reconcile.added).toEqual(['typescript-language-server']); + expect(result.skipped).toEqual([]); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize preserves runtime state on invalid .lsp.json', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-invalid-')); + try { + fs.writeFileSync(path.join(tempDir, '.lsp.json'), '{'); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const reconcileServerConfigs = vi.fn(); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + await expect(service.reinitialize()).rejects.toThrow(); + expect(reconcileServerConfigs).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize preserves runtime state on invalid server entries', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-invalid-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + transport: 'stdio', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const reconcileServerConfigs = vi.fn(); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + await expect(service.reinitialize()).rejects.toThrow( + 'Invalid LSP server config', + ); + expect(reconcileServerConfigs).not.toHaveBeenCalled(); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize queue continues after a failed reload', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-queue-error-')); + try { + fs.writeFileSync(path.join(tempDir, '.lsp.json'), '{'); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const reconcileServerConfigs = vi.fn(async () => ({ + added: ['typescript-language-server'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + await expect(service.reinitialize()).rejects.toThrow(); + expect(reconcileServerConfigs).not.toHaveBeenCalled(); + + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const result = await service.reinitialize(); + + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + expect(result.reconcile.added).toEqual(['typescript-language-server']); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize replays open documents after restarting servers', async () => { + vi.useFakeTimers(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-replay-')); + try { + const filePath = path.join(tempDir, 'main.ts'); + const uri = pathToFileURL(filePath).toString(); + fs.writeFileSync(filePath, 'const value = 1;\n', 'utf-8'); + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const connection = { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const handle = { + config: { + name: 'typescript-language-server', + languages: ['typescript'], + command: 'typescript-language-server', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection, + }; + const reconcileServerConfigs = vi.fn(async () => ({ + added: [], + removed: [], + restarted: ['typescript-language-server'], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + getHandles: () => new Map([['typescript-language-server', handle]]), + }; + const internals = service as unknown as { + openedDocuments: Map>; + }; + internals.openedDocuments.set( + 'typescript-language-server', + new Set([uri]), + ); + + const reinitialize = service.reinitialize(); + await vi.runAllTimersAsync(); + await reinitialize; + + expect(connection.send).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'textDocument/didOpen', + params: { + textDocument: expect.objectContaining({ + uri, + languageId: 'typescript', + text: 'const value = 1;\n', + }), + }, + }), + ); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize continues replaying documents after one server send fails', async () => { + vi.useFakeTimers(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-replay-')); + try { + const firstPath = path.join(tempDir, 'first.ts'); + const secondPath = path.join(tempDir, 'second.py'); + const firstUri = pathToFileURL(firstPath).toString(); + const secondUri = pathToFileURL(secondPath).toString(); + fs.writeFileSync(firstPath, 'const value = 1;\n', 'utf-8'); + fs.writeFileSync(secondPath, 'value = 1\n', 'utf-8'); + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + python: { + command: 'pyright-langserver', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const firstConnection = { + listen: vi.fn(), + send: vi.fn(() => { + throw new Error('broken pipe'); + }), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const secondConnection = { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const handles = new Map([ + [ + 'typescript-language-server', + { + config: { + name: 'typescript-language-server', + languages: ['typescript'], + command: 'typescript-language-server', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection: firstConnection, + }, + ], + [ + 'pyright-langserver', + { + config: { + name: 'pyright-langserver', + languages: ['python'], + command: 'pyright-langserver', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection: secondConnection, + }, + ], + ]); + const reconcileServerConfigs = vi.fn(async () => ({ + added: [], + removed: [], + restarted: ['typescript-language-server', 'pyright-langserver'], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + getHandles: () => handles, + }; + const internals = service as unknown as { + openedDocuments: Map>; + lastConnections: Map; + }; + internals.openedDocuments.set( + 'typescript-language-server', + new Set([firstUri]), + ); + internals.openedDocuments.set('pyright-langserver', new Set([secondUri])); + + const reinitialize = service.reinitialize(); + await vi.runAllTimersAsync(); + await expect(reinitialize).resolves.toBeDefined(); + + expect(firstConnection.send).toHaveBeenCalledOnce(); + expect(internals.lastConnections.has('typescript-language-server')).toBe( + false, + ); + expect(internals.lastConnections.get('pyright-langserver')).toBe( + secondConnection, + ); + expect(secondConnection.send).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'textDocument/didOpen', + params: { + textDocument: expect.objectContaining({ + uri: secondUri, + languageId: 'python', + text: 'value = 1\n', + }), + }, + }), + ); + } finally { + vi.useRealTimers(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize preserves open documents for failed servers until a later restart succeeds', async () => { + vi.useFakeTimers(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-failed-')); + try { + const filePath = path.join(tempDir, 'index.ts'); + const uri = pathToFileURL(filePath).toString(); + fs.writeFileSync(filePath, 'const value = 1;\n', 'utf-8'); + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const connection = { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const handle = { + config: { + name: 'typescript-language-server', + languages: ['typescript'], + command: 'typescript-language-server', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection, + }; + const reconcileServerConfigs = vi + .fn() + .mockResolvedValueOnce({ + added: [], + removed: [], + restarted: [], + unchanged: [], + failed: ['typescript-language-server'], + }) + .mockResolvedValueOnce({ + added: [], + removed: [], + restarted: ['typescript-language-server'], + unchanged: [], + failed: [], + }); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + getHandles: () => new Map([['typescript-language-server', handle]]), + }; + const internals = service as unknown as { + openedDocuments: Map>; + }; + internals.openedDocuments.set( + 'typescript-language-server', + new Set([uri]), + ); + + await service.reinitialize(); + expect( + internals.openedDocuments.get('typescript-language-server'), + ).toEqual(new Set([uri])); + + const reinitialize = service.reinitialize(); + await vi.runAllTimersAsync(); + await reinitialize; + + expect(connection.send).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'textDocument/didOpen', + params: { + textDocument: expect.objectContaining({ + uri, + languageId: 'typescript', + text: 'const value = 1;\n', + }), + }, + }), + ); + } finally { + vi.useRealTimers(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize serializes the full snapshot reconcile and replay flow', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-queue-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + let resolveFirstReconcile: ( + value: Awaited>, + ) => void; + const firstReconcile = new Promise<{ + added: string[]; + removed: string[]; + restarted: string[]; + unchanged: string[]; + failed: string[]; + }>((resolve) => { + resolveFirstReconcile = resolve; + }); + const reconcileServerConfigs = vi + .fn() + .mockReturnValueOnce(firstReconcile) + .mockResolvedValueOnce({ + added: [], + removed: [], + restarted: [], + unchanged: ['typescript-language-server'], + failed: [], + }); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + const first = service.reinitialize(); + await vi.waitFor(() => { + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + }); + const second = service.reinitialize(); + await Promise.resolve(); + + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + + resolveFirstReconcile!({ + added: ['typescript-language-server'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }); + await first; + await vi.waitFor(() => { + expect(reconcileServerConfigs).toHaveBeenCalledTimes(2); + }); + await second; + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize snapshots documents opened while reconcile is pending', async () => { + vi.useFakeTimers(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-snapshot-')); + try { + const filePath = path.join(tempDir, 'index.ts'); + const uri = pathToFileURL(filePath).toString(); + fs.writeFileSync(filePath, 'const value = 1;\n', 'utf-8'); + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const connection = { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const handle = { + config: { + name: 'typescript-language-server', + languages: ['typescript'], + command: 'typescript-language-server', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection, + }; + let resolveReconcile: ( + value: Awaited>, + ) => void; + const reconcilePromise = new Promise<{ + added: string[]; + removed: string[]; + restarted: string[]; + unchanged: string[]; + failed: string[]; + }>((resolve) => { + resolveReconcile = resolve; + }); + const reconcileServerConfigs = vi.fn(() => reconcilePromise); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + getHandles: () => new Map([['typescript-language-server', handle]]), + }; + const internals = service as unknown as { + openedDocuments: Map>; + }; + + const reinitialize = service.reinitialize(); + await vi.waitFor(() => { + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + }); + internals.openedDocuments.set( + 'typescript-language-server', + new Set([uri]), + ); + resolveReconcile!({ + added: [], + removed: [], + restarted: ['typescript-language-server'], + unchanged: [], + failed: [], + }); + await vi.runAllTimersAsync(); + await reinitialize; + + expect(connection.send).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'textDocument/didOpen', + params: { + textDocument: expect.objectContaining({ + uri, + languageId: 'typescript', + text: 'const value = 1;\n', + }), + }, + }), + ); + } finally { + vi.useRealTimers(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('stop cancels an in-flight reinitialize replay delay', async () => { + vi.useFakeTimers(); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-stop-')); + try { + const filePath = path.join(tempDir, 'index.ts'); + const uri = pathToFileURL(filePath).toString(); + fs.writeFileSync(filePath, 'const value = 1;\n', 'utf-8'); + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + const connection = { + listen: vi.fn(), + send: vi.fn(), + onNotification: vi.fn(), + onRequest: vi.fn(), + request: vi.fn(), + initialize: vi.fn(), + shutdown: vi.fn(), + end: vi.fn(), + }; + const handle = { + config: { + name: 'typescript-language-server', + languages: ['typescript'], + command: 'typescript-language-server', + args: [], + transport: 'stdio', + }, + status: 'READY', + connection, + }; + const stopAll = vi.fn(async () => {}); + const reconcileServerConfigs = vi.fn(async () => ({ + added: [], + removed: [], + restarted: ['typescript-language-server'], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + getHandles: () => new Map([['typescript-language-server', handle]]), + stopAll, + }; + const internals = service as unknown as { + openedDocuments: Map>; + lastConnections: Map; + }; + internals.openedDocuments.set( + 'typescript-language-server', + new Set([uri]), + ); + internals.lastConnections.set('typescript-language-server', connection); + + const reinitialize = service.reinitialize(); + await vi.waitFor(() => { + expect(connection.send).toHaveBeenCalledOnce(); + }); + await service.stop(); + + await expect(reinitialize).rejects.toThrow('LSP reinitialize cancelled'); + expect(stopAll).toHaveBeenCalledOnce(); + expect(vi.getTimerCount()).toBe(0); + expect(internals.openedDocuments.size).toBe(0); + expect(internals.lastConnections.size).toBe(0); + } finally { + vi.useRealTimers(); + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('stop cancels queued reinitialize calls before they start', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-stop-queue-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + typescript: { + command: 'typescript-language-server', + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { workspaceRoot: tempDir }, + ); + let resolveFirstReconcile: ( + value: Awaited>, + ) => void; + const firstReconcile = new Promise<{ + added: string[]; + removed: string[]; + restarted: string[]; + unchanged: string[]; + failed: string[]; + }>((resolve) => { + resolveFirstReconcile = resolve; + }); + const reconcileServerConfigs = vi.fn(() => firstReconcile); + const stopAll = vi.fn(async () => {}); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + stopAll, + }; + + const first = service.reinitialize(); + await vi.waitFor(() => { + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + }); + const second = service.reinitialize(); + await service.stop(); + + resolveFirstReconcile!({ + added: ['typescript-language-server'], + removed: [], + restarted: [], + unchanged: [], + failed: [], + }); + + await expect(first).rejects.toThrow('LSP reinitialize cancelled'); + await expect(second).rejects.toThrow('LSP reinitialize cancelled'); + expect(reconcileServerConfigs).toHaveBeenCalledOnce(); + expect(stopAll).toHaveBeenCalledOnce(); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('reinitialize stops all servers when trusted workspace is required but unavailable', async () => { + const tempConfig = new MockConfig(); + vi.spyOn(tempConfig, 'isTrustedFolder').mockReturnValue(false); + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { requireTrustedWorkspace: true }, + ); + const stopAll = vi.fn(async () => {}); + const internals = service as unknown as { + serverManager: unknown; + openedDocuments: Map>; + lastConnections: Map; + }; + internals.serverManager = { + getHandles: () => new Map([['tsserver', {}]]), + stopAll, + }; + internals.openedDocuments.set('tsserver', new Set(['file:///a.ts'])); + internals.lastConnections.set('tsserver', {}); + + const result = await service.reinitialize(); + + expect(stopAll).toHaveBeenCalledOnce(); + expect(result.reconcile.removed).toEqual(['tsserver']); + expect(internals.openedDocuments.has('tsserver')).toBe(false); + expect(internals.lastConnections.has('tsserver')).toBe(false); + }); + + test('reinitialize skips all user-configured servers in untrusted workspaces', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-untrusted-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + trusted: { + command: 'trusted-language-server', + languages: ['typescript'], + trustRequired: true, + }, + untrusted: { + command: 'untrusted-language-server', + languages: ['javascript'], + trustRequired: false, + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + vi.spyOn(tempConfig, 'isTrustedFolder').mockReturnValue(false); + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { requireTrustedWorkspace: false, workspaceRoot: tempDir }, + ); + const reconcileServerConfigs = vi.fn(async () => ({ + added: [], + removed: [], + restarted: [], + unchanged: [], + failed: [], + })); + (service as unknown as { serverManager: unknown }).serverManager = { + reconcileServerConfigs, + }; + + const result = await service.reinitialize(); + + expect(reconcileServerConfigs).toHaveBeenCalledWith([]); + expect(result.skipped).toEqual([ + { + name: 'trusted-language-server', + reason: 'server_trust_required', + }, + { + name: 'untrusted-language-server', + reason: 'server_trust_required', + }, + ]); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('discoverAndPrepare skips trust-required servers in untrusted workspaces', async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lsp-discover-')); + try { + fs.writeFileSync( + path.join(tempDir, '.lsp.json'), + JSON.stringify({ + trusted: { + command: 'trusted-language-server', + languages: ['typescript'], + trustRequired: true, + }, + untrusted: { + command: 'untrusted-language-server', + languages: ['javascript'], + trustRequired: false, + }, + }), + ); + const tempConfig = new MockConfig(); + tempConfig.rootPath = tempDir; + vi.spyOn(tempConfig, 'isTrustedFolder').mockReturnValue(false); + const service = new NativeLspService( + tempConfig as unknown as CoreConfig, + mockWorkspace as unknown as WorkspaceContext, + eventEmitter, + mockFileDiscovery as unknown as FileDiscoveryService, + mockIdeStore as unknown as IdeContextStore, + { requireTrustedWorkspace: false, workspaceRoot: tempDir }, + ); + + await service.discoverAndPrepare(); + + const handles = Array.from( + ( + service as unknown as { + serverManager: { getHandles: () => Map }; + } + ).serverManager + .getHandles() + .keys(), + ); + expect(handles).toEqual([]); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + test('stop clears document tracking caches', async () => { + const stopAll = vi.fn(async () => {}); + const internals = lspService as unknown as { + serverManager: { stopAll: () => Promise }; + openedDocuments: Map>; + lastConnections: Map; + }; + internals.serverManager = { stopAll }; + internals.openedDocuments.set('tsserver', new Set(['file:///a.ts'])); + internals.lastConnections.set('tsserver', {}); + + await lspService.stop(); + + expect(stopAll).toHaveBeenCalledOnce(); + expect(internals.openedDocuments.size).toBe(0); + expect(internals.lastConnections.size).toBe(0); + }); + test('should expose a detailed status snapshot for configured servers', () => { const serverManager = { getHandles: () => diff --git a/packages/core/src/lsp/NativeLspService.ts b/packages/core/src/lsp/NativeLspService.ts index aa7794a9d1..d9caf014a1 100644 --- a/packages/core/src/lsp/NativeLspService.ts +++ b/packages/core/src/lsp/NativeLspService.ts @@ -38,6 +38,9 @@ import { LspServerManager } from './LspServerManager.js'; import type { LspConnectionInterface, LspServerHandle, + LspServerConfig, + LspServiceReinitializeResult, + LspSkippedServer, LspServerStatus, LspStatusSnapshot, NativeLspServiceOptions, @@ -86,6 +89,9 @@ export class NativeLspService { private normalizer: LspResponseNormalizer; private openedDocuments = new Map>(); private lastConnections = new Map(); + private reinitializeQueue: Promise = Promise.resolve(); + private reinitializeAbortController: AbortController | undefined; + private stopping = false; constructor( config: CoreConfig, @@ -141,7 +147,204 @@ export class NativeLspService { extensionConfigs, userConfigs, ); - this.serverManager.setServerConfigs(serverConfigs); + const { admitted, skipped } = this.filterServerConfigs( + serverConfigs, + workspaceTrusted, + ); + debugLogger.info( + `Discovered ${admitted.length} LSP server config(s): ${formatServerNames( + admitted.map((config) => config.name), + )}, skipped=${formatServerNames(skipped.map((server) => server.name))}`, + ); + this.serverManager.setServerConfigs(admitted); + } + + async reinitialize(): Promise { + if (this.stopping) { + throw new Error('LSP reinitialize cancelled'); + } + const controller = new AbortController(); + const run = async () => { + this.throwIfReinitializeAborted(controller.signal); + this.reinitializeAbortController = controller; + try { + return await this.doReinitialize(controller.signal); + } finally { + if (this.reinitializeAbortController === controller) { + this.reinitializeAbortController = undefined; + } + } + }; + const next = this.reinitializeQueue.then(run, run); + this.reinitializeQueue = next.catch(() => undefined); + return next; + } + + private throwIfReinitializeAborted(signal: AbortSignal): void { + if (this.stopping || signal.aborted) { + throw new Error('LSP reinitialize cancelled'); + } + } + + private async doReinitialize( + signal: AbortSignal, + ): Promise { + this.throwIfReinitializeAborted(signal); + const workspaceTrusted = this.config.isTrustedFolder(); + debugLogger.info( + `Reinitializing LSP servers: workspaceRoot=${this.workspaceRoot}, trusted=${workspaceTrusted}`, + ); + if (this.requireTrustedWorkspace && !workspaceTrusted) { + this.throwIfReinitializeAborted(signal); + const removed = Array.from(this.serverManager.getHandles().keys()); + await this.serverManager.stopAll(); + this.throwIfReinitializeAborted(signal); + this.clearDocumentTrackingForServers(removed); + const result = { + reconcile: { + added: [], + removed, + restarted: [], + unchanged: [], + failed: [], + }, + skipped: [], + }; + debugLogger.info( + `LSP reinitialize result: added=, removed=${formatServerNames( + removed, + )}, restarted=, unchanged=, failed=, skipped=`, + ); + return result; + } + + this.throwIfReinitializeAborted(signal); + const userConfigs = await this.configLoader.loadUserConfigsStrict(); + this.throwIfReinitializeAborted(signal); + if (!userConfigs.ok) { + throw userConfigs.error; + } + + const extensionConfigs = await this.configLoader.loadExtensionConfigs( + this.getActiveExtensions(), + ); + this.throwIfReinitializeAborted(signal); + const serverConfigs = this.configLoader.mergeConfigs( + [], + extensionConfigs, + userConfigs.configs, + ); + const { admitted, skipped } = this.filterServerConfigs( + serverConfigs, + workspaceTrusted, + ); + this.throwIfReinitializeAborted(signal); + const reconcile = await this.serverManager.reconcileServerConfigs(admitted); + this.throwIfReinitializeAborted(signal); + const restartedOpenDocuments = this.snapshotOpenDocuments( + reconcile.restarted, + ); + this.clearDocumentTrackingForServers([ + ...reconcile.removed, + ...reconcile.restarted, + ]); + await this.replayOpenDocuments( + reconcile.restarted, + restartedOpenDocuments, + signal, + ); + this.throwIfReinitializeAborted(signal); + debugLogger.info( + `LSP reinitialize result: added=${formatServerNames( + reconcile.added, + )}, removed=${formatServerNames( + reconcile.removed, + )}, restarted=${formatServerNames( + reconcile.restarted, + )}, unchanged=${formatServerNames( + reconcile.unchanged, + )}, failed=${formatServerNames( + reconcile.failed, + )}, skipped=${formatServerNames(skipped.map((server) => server.name))}`, + ); + return { reconcile, skipped }; + } + + private filterServerConfigs( + configs: LspServerConfig[], + workspaceTrusted: boolean, + ): { admitted: LspServerConfig[]; skipped: LspSkippedServer[] } { + const admitted: LspServerConfig[] = []; + const skipped: LspSkippedServer[] = []; + for (const config of configs) { + if (!workspaceTrusted && config.trustRequired) { + debugLogger.warn( + `LSP server ${config.name} requires trusted workspace, skipping`, + ); + skipped.push({ name: config.name, reason: 'server_trust_required' }); + continue; + } + admitted.push(config); + } + return { admitted, skipped }; + } + + private clearDocumentTrackingForServers(serverNames: string[]): void { + for (const name of serverNames) { + this.openedDocuments.delete(name); + this.lastConnections.delete(name); + } + } + + private snapshotOpenDocuments( + serverNames: string[], + ): Map> { + const snapshots = new Map>(); + for (const name of serverNames) { + const documents = this.openedDocuments.get(name); + if (documents) { + snapshots.set(name, new Set(documents)); + } + } + return snapshots; + } + + private async replayOpenDocuments( + serverNames: string[], + snapshots: Map>, + signal: AbortSignal, + ): Promise { + this.throwIfReinitializeAborted(signal); + if ( + serverNames.length === 0 || + !serverNames.some((name) => snapshots.has(name)) + ) { + return; + } + const readyHandles = new Map(this.getReadyHandles()); + for (const name of serverNames) { + this.throwIfReinitializeAborted(signal); + const handle = readyHandles.get(name); + const documents = snapshots.get(name); + if (!handle || !documents) { + continue; + } + let openedAny = false; + for (const uri of documents) { + this.throwIfReinitializeAborted(signal); + try { + openedAny = this.sendDocumentOpen(name, handle, uri) || openedAny; + } catch (error) { + debugLogger.warn( + `Failed to replay document ${uri} for LSP server ${name}:`, + error, + ); + } + } + if (openedAny) { + await this.delay(DEFAULT_LSP_DOCUMENT_OPEN_DELAY_MS, signal); + } + } } private getActiveExtensions(): Extension[] { @@ -164,7 +367,11 @@ export class NativeLspService { * Stop all LSP servers */ async stop(): Promise { + this.stopping = true; + this.reinitializeAbortController?.abort(); await this.serverManager.stopAll(); + this.openedDocuments.clear(); + this.lastConnections.clear(); } /** @@ -286,11 +493,28 @@ export class NativeLspService { if (lastConnection && lastConnection !== handle.connection) { this.openedDocuments.delete(serverName); } - this.lastConnections.set(serverName, handle.connection); + if (!this.sendDocumentOpen(serverName, handle, uri)) { + return false; + } + + // Wait for the LSP server to process the newly opened document. + // Without this delay, requests sent immediately after didOpen may return + // empty results because the server hasn't finished analyzing the file. + await this.delay(DEFAULT_LSP_DOCUMENT_OPEN_DELAY_MS); + + return true; + } + + private sendDocumentOpen( + serverName: string, + handle: LspServerHandle & { connection: LspConnectionInterface }, + uri: string, + ): boolean { if (!uri.startsWith('file://')) { return false; } + const openedForServer = this.openedDocuments.get(serverName); if (openedForServer?.has(uri)) { return false; @@ -330,15 +554,11 @@ export class NativeLspService { }, }); + this.lastConnections.set(serverName, handle.connection); const nextOpened = openedForServer ?? new Set(); nextOpened.add(uri); this.openedDocuments.set(serverName, nextOpened); - // Wait for the LSP server to process the newly opened document. - // Without this delay, requests sent immediately after didOpen may return - // empty results because the server hasn't finished analyzing the file. - await this.delay(DEFAULT_LSP_DOCUMENT_OPEN_DELAY_MS); - return true; } @@ -520,8 +740,24 @@ export class NativeLspService { return justOpened && !this.serverManager.isTypescriptServer(handle); } - private async delay(ms: number): Promise { - await new Promise((resolve) => setTimeout(resolve, ms)); + private async delay(ms: number, signal?: AbortSignal): Promise { + if (!signal) { + await new Promise((resolve) => setTimeout(resolve, ms)); + return; + } + this.throwIfReinitializeAborted(signal); + await new Promise((resolve, reject) => { + const onAbort = () => { + clearTimeout(timer); + signal.removeEventListener('abort', onAbort); + reject(new Error('LSP reinitialize cancelled')); + }; + const timer = setTimeout(() => { + signal.removeEventListener('abort', onAbort); + resolve(); + }, ms); + signal.addEventListener('abort', onAbort, { once: true }); + }); } /** @@ -1408,3 +1644,7 @@ export class NativeLspService { return message.includes('No Project'); } } + +function formatServerNames(names: readonly string[]): string { + return names.length === 0 ? '' : names.join(','); +} diff --git a/packages/core/src/lsp/configHash.test.ts b/packages/core/src/lsp/configHash.test.ts new file mode 100644 index 0000000000..3f46358cc9 --- /dev/null +++ b/packages/core/src/lsp/configHash.test.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import type { LspServerConfig } from './types.js'; +import { lspServerConfigHash } from './configHash.js'; + +const baseConfig: LspServerConfig = { + name: 'tsserver', + languages: ['typescript'], + command: 'typescript-language-server', + args: ['--stdio'], + transport: 'stdio', + env: { B: '2', A: '1' }, + settings: { beta: true, alpha: { z: 1, a: 2 } }, + rootUri: 'file:///workspace', + workspaceFolder: '/workspace', + trustRequired: true, +}; + +describe('lspServerConfigHash', () => { + it('ignores object key order', () => { + const reordered: LspServerConfig = { + ...baseConfig, + env: { A: '1', B: '2' }, + settings: { alpha: { a: 2, z: 1 }, beta: true }, + }; + + expect(lspServerConfigHash(reordered)).toBe( + lspServerConfigHash(baseConfig), + ); + }); + + it('treats argument order as semantic', () => { + expect( + lspServerConfigHash({ + ...baseConfig, + args: ['--stdio', '--log'], + }), + ).not.toBe( + lspServerConfigHash({ + ...baseConfig, + args: ['--log', '--stdio'], + }), + ); + }); + + it('changes when runtime config fields change', () => { + expect( + lspServerConfigHash({ + ...baseConfig, + command: 'other-server', + }), + ).not.toBe(lspServerConfigHash(baseConfig)); + + expect( + lspServerConfigHash({ + ...baseConfig, + trustRequired: false, + }), + ).not.toBe(lspServerConfigHash(baseConfig)); + }); +}); diff --git a/packages/core/src/lsp/configHash.ts b/packages/core/src/lsp/configHash.ts new file mode 100644 index 0000000000..bacd3110bf --- /dev/null +++ b/packages/core/src/lsp/configHash.ts @@ -0,0 +1,49 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { createHash } from 'node:crypto'; +import type { LspServerConfig } from './types.js'; + +function sortJsonValue(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(sortJsonValue); + } + if (value && typeof value === 'object') { + // Object.create(null) avoids prototype pollution from __proto__ keys + const sorted = Object.create(null) as Record; + for (const key of Object.keys(value).sort()) { + sorted[key] = sortJsonValue((value as Record)[key]); + } + return sorted; + } + return value; +} + +export function lspServerConfigHash(config: LspServerConfig): string { + const hashInput = { + name: config.name, + languages: config.languages, + transport: config.transport, + command: config.command, + args: config.args, + env: config.env, + initializationOptions: config.initializationOptions, + settings: config.settings, + extensionToLanguage: config.extensionToLanguage, + workspaceFolder: config.workspaceFolder, + rootUri: config.rootUri, + startupTimeout: config.startupTimeout, + shutdownTimeout: config.shutdownTimeout, + restartOnCrash: config.restartOnCrash, + maxRestarts: config.maxRestarts, + trustRequired: config.trustRequired, + socket: config.socket, + } satisfies Record; + const stableConfig = sortJsonValue(hashInput); + return createHash('sha256') + .update(JSON.stringify(stableConfig)) + .digest('hex'); +} diff --git a/packages/core/src/lsp/constants.ts b/packages/core/src/lsp/constants.ts index aa70435a0e..80ff19ff8a 100644 --- a/packages/core/src/lsp/constants.ts +++ b/packages/core/src/lsp/constants.ts @@ -13,6 +13,9 @@ import type { LspCodeActionKind, LspDiagnosticSeverity } from './types.js'; /** Default timeout for LSP server startup in milliseconds */ export const DEFAULT_LSP_STARTUP_TIMEOUT_MS = 10000; +/** Default timeout for graceful LSP server shutdown in milliseconds */ +export const DEFAULT_LSP_SHUTDOWN_TIMEOUT_MS = 5000; + /** Default timeout for LSP requests in milliseconds */ export const DEFAULT_LSP_REQUEST_TIMEOUT_MS = 15000; diff --git a/packages/core/src/lsp/types.ts b/packages/core/src/lsp/types.ts index 5b90423521..b7c9fbb69f 100644 --- a/packages/core/src/lsp/types.ts +++ b/packages/core/src/lsp/types.ts @@ -254,6 +254,26 @@ export interface LspServerStatusInfo { error?: string; } +export type LspServerSkipReason = 'server_trust_required'; + +export interface LspSkippedServer { + name: string; + reason: LspServerSkipReason; +} + +export interface LspReconcileResult { + added: string[]; + removed: string[]; + restarted: string[]; + unchanged: string[]; + failed: string[]; +} + +export interface LspServiceReinitializeResult { + reconcile: LspReconcileResult; + skipped: LspSkippedServer[]; +} + export interface LspClient { /** * Get the status of all configured LSP servers. @@ -376,6 +396,11 @@ export interface LspClient { * Get a point-in-time snapshot of LSP server connection status. */ getStatusSnapshot?(): LspStatusSnapshot; + + /** + * Re-read LSP configuration and reconcile live server connections. + */ + reinitialize?(): Promise; } // ============================================================================ @@ -586,6 +611,10 @@ export interface LspServerHandle { processDiagnostics?: LspProcessDiagnostics; /** Lock to prevent concurrent startup attempts */ startingPromise?: Promise; + /** Cancels an in-flight startup attempt */ + startupAbortController?: AbortController; + /** Whether the owned process exited unexpectedly during the current startup */ + processExitedUnexpectedly?: boolean; } /**