refactor(lsp): Phase 0 latency and mutex containment#3969
Conversation
Implement the LSP server rebuild pipeline from docs/design/lsp-latency-and-mutex-fix.md: source revision and owned candidates, revision-conditional engine commits, versioned diagnostics fencing, bounded request waits with typed LSP errors, negotiated position encoding, debounced native tails, and coherent run/test snapshots. Memoize inlay hints and semantic tokens as Salsa queries.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change introduces revision-gated project state, debounced and fenced LSP refreshes, negotiated position encoding, Salsa-tracked editor queries, generation-aware run cancellation, deferred profiling activation, and updated development and testing documentation. ChangesLSP runtime modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LSPClient
participant MultiProjectLSP
participant BexProject
participant PositionCodec
participant PlaygroundServer
LSPClient->>MultiProjectLSP: initialize(position encodings)
MultiProjectLSP->>PositionCodec: negotiate encoding
MultiProjectLSP-->>LSPClient: capabilities with negotiated encoding
LSPClient->>MultiProjectLSP: document change or request
MultiProjectLSP->>BexProject: mutate sources or read current revision
BexProject-->>MultiProjectLSP: revision-tagged candidate or snapshot
MultiProjectLSP->>PositionCodec: convert LSP positions and ranges
MultiProjectLSP-->>LSPClient: diagnostics, symbols, tokens, or edits
PlaygroundServer->>BexProject: prepare_function_run(...)
BexProject-->>PlaygroundServer: generation-pinned engine
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Binary size checks failed❌ 1 violations · ✅ 6 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 14763593
gzip_bytes = 4188237Generated by |
- DiagnosticsFence::admit now compares candidates against the live source revision (read under the fence lock), so a superseded rebuild's diagnostics or a candidate racing mark_dirty can never regress markers a newer revision already published (design 0B "Ready(stale revision) -> discard"; acceptance: no v7 publication after v8 arrives). - Add a commit_engine_if_current_with_hook seam and port the commit-boundary rendezvous test: an edit placed exactly against the conditional commit is serialized after it and immediately makes the installed engine non-current. - Extract classify_request_timeout and cover the request lane's timeout classification (revision moved -> ContentModified-stale; unchanged -> congestion) with channel-sequenced tests, no sleep margins. - Correct false lock/revision comments: run_cfgs is a leaf mutex taken under the source gate in prepare_function_run; the source revision advances on every applied batch; playground cancel is best-effort and cannot reach a run pinned to a superseded engine (D8 registration deferred). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VEMxti5WQ8bCgJUuX9AQvo
|
Pushed Correctness fixes
Test hardening (ported from #3961's strongest asset)
Verification
Explicit Remaining known trade-off for reviewers: D7 fail-closed run admission means playground runs return |
Semantic conflict fix: canary's #3973 added list_tests_with_metadata(db) in build_project_update, where this branch replaced the raw db binding with the source-guard read. The tests listing now rides the same guard as the functions listing, so the payload stays one coherent snapshot.
|
CI note: the failures on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_lsp2_actions_tests/src/runner.rs`:
- Line 7: The annotation collection currently bypasses Salsa memoization because
file_annotations is a plain function. Update file_annotations in annotations.rs
to be the tracked returns(ref) query, or delegate to that tracked query and
clone only at the ownership boundary; ensure the production LSP handlers and
runner use this tracked path rather than recomputing annotation aggregation.
In `@baml_language/TEST_INSTRUCTIONS.md`:
- Around line 71-76: Add the required `cargo test --lib` command to the “Verify
all tests pass” section, alongside the existing Rust test commands, so the
documented workflow covers library tests for Rust changes.
- Line 111: Correct the environment variable reference in the “DO NOT EDIT”
instruction to use the case-sensitive name UPDATE_EXPECT=1, matching the
commands documented above.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c938a392-17fe-414e-9d51-9b4113bc0ea4
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
CONTRIBUTING.mdREADME-DEV.mdbaml_language/TEST_INSTRUCTIONS.mdbaml_language/crates/baml_cli/src/paint.rsbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/tokens.rsbaml_language/crates/baml_lsp2_actions_tests/src/runner.rsbaml_language/crates/baml_lsp_server/src/playground_server.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex_lsp/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/diagnostics.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/notification.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/bex_project/src/bex_lsp/position_codec.rsbaml_language/crates/bex_project/src/bex_lsp/request.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_project/src/project.rsdocs/design/lsp-latency-and-mutex-fix.md
| use baml_compiler_diagnostics::{RenderConfig, render_diagnostic}; | ||
| use baml_lsp2_actions::{ | ||
| annotations::{AnnotationKind, InlineAnnotation, annotations}, | ||
| annotations::{AnnotationKind, InlineAnnotation, file_annotations}, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Keep inlay-hint collection on the Salsa-tracked query path.
The supplied baml_lsp2_actions/src/annotations.rs defines file_annotations as a plain pub fn, so this change appears to bypass the B1 memoized query. The production LSP handlers also call this API while claiming the results are Salsa-memoized, which can re-run annotation aggregation on every request.
Make file_annotations the tracked returns(ref) query, or delegate to the tracked query and clone only at the ownership boundary.
Also applies to: 168-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/baml_lsp2_actions_tests/src/runner.rs` at line 7, The
annotation collection currently bypasses Salsa memoization because
file_annotations is a plain function. Update file_annotations in annotations.rs
to be the tracked returns(ref) query, or delegate to that tracked query and
clone only at the ownership boundary; ensure the production LSP handlers and
runner use this tracked path rather than recomputing annotation aggregation.
| ### 7. Verify all tests pass | ||
|
|
||
| ```bash | ||
| # Run all tests (can skip slow parser_stress with --skip parser_stress) | ||
| cargo test --package baml_tests -- --skip parser_stress | ||
| cargo test --package lsp_actions_tests | ||
| cargo test --package baml_lsp2_actions_tests |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the required library test command.
This verification block omits cargo test --lib. Because this PR changes Rust code, add that command to the documented verification workflow.
As per coding guidelines, always run cargo test --lib when Rust code changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/TEST_INSTRUCTIONS.md` around lines 71 - 76, Add the required
`cargo test --lib` command to the “Verify all tests pass” section, alongside the
existing Rust test commands, so the documented workflow covers library tests for
Rust changes.
Source: Coding guidelines
|
|
||
|
|
||
| DO NOT EDIT the diagnostics manually in lsp_actions_tests. Use update_expect=1 | ||
| DO NOT EDIT the diagnostics manually in baml_lsp2_actions_tests. Use update_expect=1 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the correct environment variable name.
update_expect=1 is lowercase here, but the commands above use the case-sensitive variable UPDATE_EXPECT=1.
Proposed fix
-DO NOT EDIT the diagnostics manually in baml_lsp2_actions_tests. Use update_expect=1
+DO NOT EDIT the diagnostics manually in baml_lsp2_actions_tests. Use UPDATE_EXPECT=1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DO NOT EDIT the diagnostics manually in baml_lsp2_actions_tests. Use update_expect=1 | |
| DO NOT EDIT the diagnostics manually in baml_lsp2_actions_tests. Use UPDATE_EXPECT=1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/TEST_INSTRUCTIONS.md` at line 111, Correct the environment
variable reference in the “DO NOT EDIT” instruction to use the case-sensitive
name UPDATE_EXPECT=1, matching the commands documented above.
1 participant
Summary
Implements Phase 0 of the LSP latency and mutex containment design (
docs/design/lsp-latency-and-mutex-fix.md):SourceRevision, detachedCompilationOutcome/DiagnosticCandidate/EngineCandidate, and revision-conditional engine commits so superseded rebuilds change no runtime state.RuntimeStatewith collection epoch fencing, registry leases, and atomic run snapshots (prepare_function_run).DiagnosticsFencewith compare-and-clear dirty tracking, stale-candidate discard, and bounded busy retry on native.ContentModified/RequestFailed/InternalErrorinstead of generic-32001.PositionCodecfor UTF-8/UTF-16 at the LSP boundary.file_annotationsandsemantic_tokensas tracked queries.Draft for early review and comparison with an alternate approach. Further performance work (snapshot reads off the source gate, startup prewarm) is planned after measurement.
Test plan
cargo test --lib -p bex_projectcargo test --lib(workspace; SDK harness failures pre-existing)cargo check --target wasm32-unknown-unknown -p bridge_wasmcargo clippy(pre-commit hook)baml-clion sandbox project-32803behavior vs alternate PR (later)/tmp/baml-inlay-reproharness after merge candidate selectionSummary by CodeRabbit