feat: report ready in experimental/serverStatus - #23362
Open
tagawa0525 wants to merge 2 commits into
Open
tagawa0525 wants to merge 2 commits into
tagawa0525 wants to merge 2 commits into
Conversation
`quiescent` answers "is there pending background work?", which is not the question a client that wants complete answers to workspace-wide requests asks. Before the first workspace has been loaded the server is trivially quiescent (nothing is in flight yet), so a client that reads `quiescent` as "ready" reads it wrongly at exactly the moment it matters most; and the documentation reserves the notification for the end user. Add a `ready` field: `false` while the workspaces are being (re)loaded, `true` once they are, including while caches are being primed (priming only warms what a request computes on demand anyway, so answers during priming are slower but complete). Document that this field, unlike the others, is meant for clients that decide whether to trust an answer. The initial `last_reported_status` starts at `ready: false`, so the notification traffic does not change: the first notification is still the one that reports the first load.
The slow-test helper only waits for `quiescent`, so an incorrect `ready` transition would go unnoticed. Add two tests: on a one-crate project the first status (sent while the workspace is still being fetched) says `ready: false`, the load ends in `ready: true`, and `ready` does not go back during the first load (in particular not while caches are primed); on a directory without `Cargo.toml` the trivially quiescent first status says `ready: false`, and the failed load is settled as `ready: true` with `health: error`.
2 tasks
There was a problem hiding this comment.
🔵 Needs a closer look
Add a test assertion proving ready is true while quiescent remains false during cache priming.
Pull request overview
Adds a ready signal to experimental/serverStatus, distinguishing workspace loading from cache priming and settled failures.
Changes:
- Updates Rust, TypeScript, and documentation protocol definitions.
- Computes readiness from workspace and reload state.
- Adds slow-test coverage for readiness transitions.
File summaries
| File | Description |
|---|---|
editors/code/src/lsp_ext.ts |
Adds the TypeScript ready field. |
docs/book/src/contributing/lsp-extensions.md |
Documents readiness semantics. |
crates/rust-analyzer/tests/slow-tests/support.rs |
Adds status test helpers. |
crates/rust-analyzer/tests/slow-tests/main.rs |
Tests readiness transitions. |
crates/rust-analyzer/src/reload.rs |
Computes and reports readiness. |
crates/rust-analyzer/src/lsp/ext.rs |
Extends the server-status payload. |
crates/rust-analyzer/src/global_state.rs |
Initializes readiness state. |
Review details
Suppressed comments (1)
crates/rust-analyzer/tests/slow-tests/main.rs:1645
- This only proves that
readynever falls after its firsttrue; it would also pass ifreadyremainedfalsethroughout cache priming and changed totrueonly whenquiescentbecametrue. Since the new contract explicitly promises readiness during priming, assert that the suffix contains aready && !quiescentstatus so that transition is covered.
let first_ready = statuses.iter().position(|status| status.ready).unwrap();
assert!(statuses[first_ready..].iter().all(|status| status.ready), "{statuses:?}");
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tagawa0525
added a commit
to tagawa0525/lsp-det
that referenced
this pull request
Sep 14, 2026
## Why rust-lang/rust-analyzer#23331 の返答を受けて作り直した fork の `server-status-ready` を、fork の GitHub Actions で全ジョブ通してから rust-lang/rust-analyzer#23362 として提出した。経緯と出した本文を文書に残す必要があった ## What - upstream-submissions.md の一覧を提出済みにし、「提出後の反応」に fork での素振り(CI の `github.repository` の条件を外した CI 専用ブランチの PR)、fork の Copilot の 3 点への対応、出した本文を記す - 測定報告に fork の sha(rebase 後と slow-tests 追加)と提出を記す - scripts/upstream/README の行、CLAUDE.md、CHANGELOG の該当行を更新 ## Impact 文書のみ。`docs/upstream-submissions.md`、`docs/research/rust-analyzer-quiescent-measurement.md`、`scripts/upstream/README*.md`、`CLAUDE.md`、`CHANGELOG.md` PR: #108
Contributor
|
CC @rust-lang/rust-analyzer for opinions on the new LSP extension. Also, @tagawa0525, I suspect you've used LLM for this. Please read our AI policy. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the simplified option A from #23331: a single
ready: boolfield inexperimental/serverStatus.quiescentanswers "is there pending background work?", which is not the question a client that wants complete answers to workspace-wide requests asks. Before the first workspace has been loaded the server is trivially quiescent (nothing is in flight yet), so a client that readsquiescentas "ready" reads it wrongly at exactly the moment it matters most; and the documentation reserves the notification for the end user.Changes
readyisfalsewhile the workspaces are being (re)loaded andtrueonce they are, including while caches are being primed (priming only warms what a request computes on demand anyway, so answers during priming are slower but complete), as suggested in #23331. It is computed from the same facts asquiescentminus the priming queue, plus "at least one workspace has been loaded or the load has failed": a failed load is reported throughhealth, andreadysays the failure is settled. A reload that is only queued (cargo.autoreload = false) is not a load in progress:readystaystrueandhealthcarries the warning, as today. The initiallast_reported_statusstarts atready: false, so the notification traffic does not change.The doc paragraph is amended: this field, unlike the others, is meant for clients that decide whether to trust an answer.
lsp-extensions.mdand its hash are updated, andeditors/code/src/lsp_ext.tsgets the field.ServerStatusParamsderivesDebugfor the test messages.Measured over stdio with the script from the issue:
{quiescent: false, ready: false}at 11 ms →{quiescent: false, ready: true}at 0.56 s (caches being primed) →{quiescent: true, ready: true}1 ms laterCargo.toml:{health: "warning", quiescent: true, ready: false}at 11 ms →{health: "error", quiescent: true, ready: true}at 12 msTests
Two slow tests observe the transitions: on a one-crate project the first status says
ready: false, the load ends inready: trueandreadydoes not go back during the first load; on a directory withoutCargo.tomlthe trivially quiescent first status saysready: false, and the failed load settles asready: truewithhealth: error.cargo xtask tidyand therust-analyzertests pass locally and on my fork's CI (all jobs, with thegithub.repositorygate lifted there).