Repository navigation
refactor(lsp): Phase 0 latency and mutex containment - #3969
Closed
rossirpaulo wants to merge 4 commits into
Closed
rossirpaulo wants to merge 4 commits into
rossirpaulo wants to merge 4 commits into
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):
|
Contributor
|
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 |
rossirpaulo
marked this pull request as ready for review
July 10, 2026 21:04
- 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01VEMxti5WQ8bCgJUuX9AQvo
Contributor
Author
|
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.
Contributor
Author
|
CI note: the failures on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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}, |
Contributor
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.
Comment on lines
71
to
+76
| ### 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 |
Contributor
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 |
Contributor
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.
Suggested change
| 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.
This was referenced Jul 11, 2026
Contributor
Author
tsenart
pushed a commit
to perfloop-oss/baml
that referenced
this pull request
Jul 13, 2026
…ment + Phase 1 bounded ingress (BoundaryML#4000) Combines BoundaryML#3969 (Phase 0) and BoundaryML#3976 (Phase 1) into one PR against canary, with latest canary merged in (including the BoundaryML#3965 session-token removal and BoundaryML#3973 test-metadata work, both semantically reconciled). Supersedes both stacked PRs. Implements `docs/design/lsp-latency-and-mutex-fix.md` (committed in this PR) through Phase 1; Phase 2 (salsa snapshot reads) remains behind the doc's spike gate. ## Root cause The 0.14.0 debounced engine rebuild holds the project DB mutex across compiles while `try_lock_db` maps any contention to `-32001`; stale build candidates could install over newer edits, busy diagnostic reads looked empty, and playground/browser routing could leak stale session output. ## Phase 0 — correctness containment (from BoundaryML#3969) - Monotonic `SourceRevision`; owned compilation/diagnostic/engine candidates; **revision-conditional engine commit** through the same gate as source mutations — stale installs impossible by construction (rendezvous-tested at the exact commit boundary). - `DiagnosticsFence` judging staleness against the **authoritative revision**: a superseded rebuild's late diagnostics or a candidate racing `mark_dirty` can never regress newer markers. - Bounded request-lane waits (1s) mapping to typed errors — `ContentModified`/`RequestFailed`; **`-32001` is never emitted**. Busy is never conflated with empty; poison is terminal. - Salsa-memoized `file_annotations`/`semantic_tokens`; 150/300 ms debounced tails; single-flight rebuilds; UTF-8/UTF-16 `PositionCodec` (C1). ## Phase 1 — bounded ingress, cancellation, client gating (from BoundaryML#3976) - **IngressScheduler + LspRuntime** in `baml_lsp_server`: stdio and browser LSP sessions through one bounded scheduler (per-class byte/item budgets, lifecycle barriers, response ownership); `$/cancelRequest` handled on the transport thread — queued requests answer **`-32800`**, running cancellable requests observe a dispatch-scoped token (entry + post-source-guard safe points; never salsa unwinding); browser takeover tombstones the old session's sink and replays overlays. - Hardened framing/output: 16 KiB header cap; oversized messages/responses degrade **per-message** (`-32803`) instead of killing the session/process; serialize-once shared outbound frames with accurate budget accounting; connection-scoped sessions (per-connection encoding negotiation). - **Playground run-gating (D6)**: during rebuild windows (`is_bex_current == false` or a `projectNotReady` rejection) Run/test/preview disable with "Preparing current build…" and re-enable on the next current update — no protocol version bump. Closes the fail-closed D7 UX window Phase 0 introduces. - **Reconnect correctness**: fail-closed hello; bounded first-connect queue; session-scoped commands never replay into a new session (synthesized rejections); the desired-runtime lease survives reconnects. - **Track A**: canonical multi-root ownership (`projectRoots.ts`) + routing; closing the last `.baml` document no longer stops the owner server; aggregate status preserves startup failures. ## Measured (same harness/machine, 400 generated functions) | Metric | 0.14.1 (regressed) | this PR | |---|---:|---:| | Typing inlayHint p50 / p95 | 313.6 / 379.4 ms | **~2 / ~52 ms** | | Post-pause `-32001` failures | 46–57 per feature/window | **0** | | Unanswered requests | 144–160 | **0** | | Rebuild-window inlay/hover/completion | mass failures | 218/218 each | | Explicit `$/cancelRequest` | rejected/ignored | **`-32800`** | | Refresh storms per 10-edit burst | — | 0 | ## Deliberately not included - **Phase 2 salsa snapshots** (removes the residual post-pause hitch on very large projects): gated by the design doc's own spike-and-signoff decision — `panic=abort` release profile vs salsa's unwinding cancellation must be resolved first. - **D5 demand-gating server side**: client seam ships dormant (typed + tested, nothing sends it). - The alternate implementation's five-lock core rewrite (redundant with Phase 0's invariants), playground protocol-v3 lockstep (compat hazard; equivalent wire-compatible fixes included), and stdlib `testing.*` API additions (unauthorized surface). ## Validation - `bex_project` 45/45 (+1 pre-existing ignored) incl. commit-boundary rendezvous, fence, bounded-wait classification; `baml_lsp_server` 64/64 (22 scheduler, 9 runtime, framing/budget); workspace `cargo check --lib` clean; clippy `-D warnings` clean; wasm check clean. - TS: playground 125/125, extension 13/13, webview 19/19 (incl. gating), all typechecks. - Latency harness re-run on this exact merged head (numbers above). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VEMxti5WQ8bCgJUuX9AQvo <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved LSP responsiveness and stability with bounded messaging, cancellation handling, session recovery, and safer browser takeover behavior. * Added negotiated UTF-8/UTF-16 position handling for more accurate diagnostics, ranges, formatting, and semantic highlighting. * Added project-aware VS Code routing for nested projects, symlinks, and changing project markers. * Added runtime leasing support for playground connections. * **Bug Fixes** * Prevented runs and tests from starting while a project build is stale or unavailable, with clearer preparation feedback. * Improved cancellation, error reporting, diagnostics updates, and run generation consistency. * **Documentation** * Expanded contributor and Rust workspace setup, testing, formatting, and debugging guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <[email protected]>
pull Bot
pushed a commit
to justinlietz93/baml
that referenced
this pull request
Jul 14, 2026
…oc fixes (BoundaryML#4007) Stacked on BoundaryML#4000 (retargets to canary when it merges). Batches the four small items deferred from the BoundaryML#4000 review cycle so the approved, CI-green main PR stayed untouched. 1. **Per-file `PositionCodec` in `references` and `workspace/symbol`** — CodeRabbit's one valid Major on BoundaryML#4000: codec construction scans the entire target file to build its line table, and both handlers built one per *result*; a symbol with N usages in one file did N identical scans. Now one codec per distinct file via a lazy map inside each handler (the same pattern `document_symbol` already used). 2. **Memoization contract tests** (ported from the alternate implementation's test suite): `file_annotations` and `semantic_tokens` execute exactly once cold, zero times warm, and once again after a source edit — asserted by counting salsa `WillExecute` events, so future implementation rewrites (including the BoundaryML#3867 re-land) keep the tests meaningful. This also formally answers the stale "bypasses Salsa memoization" review claim on BoundaryML#3969 by pinning the invariant. 3. **`TEST_INSTRUCTIONS.md`**: the "Verify all tests pass" section now includes `cargo test --lib` (previously the documented flow skipped the workspace unit tests entirely — where all of BoundaryML#4000's new tests live), and two case-sensitive `UPDATE_EXPECT` references fixed (lowercase `update_expect=1` is a silent no-op for expect-test). Validation: `bex_project` 45/45, `baml_lsp2_actions_tests` 377/377 (incl. the 2 new memoization tests), clippy `-D warnings` clean, wasm check clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VEMxti5WQ8bCgJUuX9AQvo <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Performance** * Improved responsiveness for “Find References” and workspace symbol searches, especially when results span multiple locations in the same file. * **Bug Fixes** * Strengthened language-server behavior verification to ensure cached results are reused correctly and refreshed after source changes. * **Documentation** * Clarified testing guidance for Rust changes and language-server snapshot updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Fable 5 <[email protected]>
This branch was previously deployed
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 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