fix: lsp on change encounter deadlock by Dsaquel · Pull Request #2511 · ast-grep/ast-grep

Dsaquel · 2026-03-01T15:55:17Z

fix #2509

On the on_change request, the lock of get_mut is waiting until the 2 requests finish.

Thanks @jkbz64 for spotting the bug in the patch.

Summary by CodeRabbit

Refactor
- Reorganized internal diagnostic computation logic to centralize how diagnostics are generated and published, improving code maintainability while preserving existing functionality.

Co-authored-by: Kamil Jakubus <kamil.jakubus@usagi.coffee>

coderabbitai · 2026-03-01T15:55:44Z

📝 Walkthrough

Walkthrough

Refactored diagnostic publishing to release mutable locks before async operations, preventing potential deadlocks when holding DashMap guards across await boundaries in concurrent LSP scenarios.

Changes

Cohort / File(s)	Summary
Async Lock Safety `crates/lsp/src/lib.rs`	Restructured message handlers to compute diagnostics within scoped lock blocks, releasing mutable DashMap guards before async `.await` calls. Updated `on_open`/`on_change` and `republish_all_diagnostics` to collect data, compute diagnostics/notes/fixes, then call `client.publish_diagnostics()` outside lock scope instead of passing mutable references across await boundaries.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit saw locks held too long,
Across awaits where they don't belong!
Released them early, scoped with care,
No more stalls in the async air! 🚀

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: lsp on change encounter deadlock' clearly describes the primary change—fixing a deadlock issue in LSP's on_change handling.
Linked Issues check	✅ Passed	The PR directly addresses all coding requirements from issue `#2509`: restructuring guard scoping in on_change/republish_all_diagnostics to avoid holding DashMap guards across await points.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the deadlock issue: refactoring diagnostic computation, adjusting publish_diagnostics signature, and improving code action handling.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-03-01T15:58:12Z

Codecov Report

❌ Patch coverage is 53.12500% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.42%. Comparing base (e5f9010) to head (ecfb28d).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/lsp/src/lib.rs	53.12%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2511      +/-   ##
==========================================
+ Coverage   84.40%   84.42%   +0.01%     
==========================================
  Files         113      113              
  Lines       18665    18675      +10     
==========================================
+ Hits        15755    15766      +11     
+ Misses       2910     2909       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/lsp/src/lib.rs (1)

733-747: ⚠️ Potential issue | 🔴 Critical

DashMap guard still held across .await — potential deadlock remains.

The iter_mut() call returns guards that are held for each iteration. The entry guard is held while awaiting publish_diagnostics on line 744, which is the same pattern this PR aims to fix. This can still cause deadlocks during rule reloads while other LSP operations are in flight.

🐛 Proposed fix: collect data first, then publish outside the loop

   /// Republish diagnostics for all currently open files
   async fn republish_all_diagnostics(&self) {
-    // Get all currently open file URIs
-    for mut entry in self.map.iter_mut() {
-      let (uri_str, versioned) = entry.pair_mut();
-      let Ok(uri) = uri_str.parse::<Uri>() else {
-        continue;
-      };
-      // Republish diagnostics for this file
-      let diagnostics = self.compute_diagnostics(uri.clone(), versioned);
-      self
-        .client
-        .publish_diagnostics(uri, diagnostics, Some(versioned.version))
-        .await;
-    }
+    // Collect URIs first to avoid holding guards across awaits
+    let uris: Vec<String> = self.map.iter().map(|e| e.key().clone()).collect();
+
+    for uri_str in uris {
+      let Some((uri, diagnostics, version)) = ({
+        let mut versioned = self.map.get_mut(&uri_str)?;
+        let uri = uri_str.parse::<Uri>().ok()?;
+        let diagnostics = self.compute_diagnostics(uri.clone(), &mut versioned);
+        Some((uri, diagnostics, versioned.version))
+      }) else {
+        continue;
+      };
+      self
+        .client
+        .publish_diagnostics(uri, diagnostics, Some(version))
+        .await;
+    }
   }

Based on learnings: "avoid relying on mutable global state across await boundaries."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/lsp/src/lib.rs` around lines 733 - 747, The republish_all_diagnostics
function currently holds DashMap entry guards from self.map.iter_mut() across an
await when calling self.client.publish_diagnostics, which can deadlock; change
the implementation to first collect the necessary data (e.g., clone or extract
(uri, versioned) pairs and compute diagnostics via compute_diagnostics into a
temporary Vec) while not awaiting, drop the DashMap guards, then iterate that
Vec and call self.client.publish_diagnostics(uri, diagnostics, Some(version))
.await outside the map iteration so no DashMap guard from iter_mut() is held
across await points.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@crates/lsp/src/lib.rs`:
- Around line 733-747: The republish_all_diagnostics function currently holds
DashMap entry guards from self.map.iter_mut() across an await when calling
self.client.publish_diagnostics, which can deadlock; change the implementation
to first collect the necessary data (e.g., clone or extract (uri, versioned)
pairs and compute diagnostics via compute_diagnostics into a temporary Vec)
while not awaiting, drop the DashMap guards, then iterate that Vec and call
self.client.publish_diagnostics(uri, diagnostics, Some(version)) .await outside
the map iteration so no DashMap guard from iter_mut() is held across await
points.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2e8d4 and ecfb28d.

📒 Files selected for processing (1)

crates/lsp/src/lib.rs

HerringtonDarkholme · 2026-03-01T16:24:55Z

Hi thanks for the contribution. However I think I need some thinking for making this change

Dsaquel · 2026-03-01T16:30:07Z

Take your time, we can close this PR if it's not appropriate.

HerringtonDarkholme

the fix looks okay. However, the overall code organization can be improved. The improvement is out of the scope so I am approving this first

Dsaquel · 2026-03-02T15:30:50Z

Ok thank you.

Do you want want me to revert the refactor ?

HerringtonDarkholme · 2026-03-02T17:29:50Z

The curret code looks okay, thank @Dsaquel

##### [\`v0.42.0\`](https://github.com/ast-grep/ast-grep/blob/HEAD/CHANGELOG.md#0420) - chore(deps): update dependency dprint to v0.53.0 [`#2547`](ast-grep/ast-grep#2547) - chore(deps): update rust crate tree-sitter to v0.26.7 [`#2541`](ast-grep/ast-grep#2541) - chore(deps): update dependency web-tree-sitter to v0.26.7 [`#2540`](ast-grep/ast-grep#2540) - chore(deps): update dependency [@types/node](https://github.com/types/node) to v24.12.0 [`#2518`](ast-grep/ast-grep#2518) - fix(deps): update rust crate tree-sitter-lua to 0.5.0 [`#2506`](ast-grep/ast-grep#2506) - chore(deps): update rust crate clap to v4.6.0 [`#2538`](ast-grep/ast-grep#2538) - fix(deps): update rust crate tree-sitter-scala to 0.25.0 [`#2536`](ast-grep/ast-grep#2536) - chore(deps): update dependency oxlint to v1.55.0 [`#2533`](ast-grep/ast-grep#2533) - chore(deps): update rust crate clap\_complete to v4.6.0 [`#2539`](ast-grep/ast-grep#2539) - chore(deps): update rust crate bit-set to v0.9.1 [`#2537`](ast-grep/ast-grep#2537) - chore(deps): update rust crate bit-set to 0.9.0 [`#2527`](ast-grep/ast-grep#2527) - chore(deps): update rust crate assert\_cmd to v2.2.0 [`#2529`](ast-grep/ast-grep#2529) - chore(deps): update rust crate tempfile to v3.27.0 [`#2531`](ast-grep/ast-grep#2531) - fix(lsp): scan injected languages for diagnostics [`#2528`](ast-grep/ast-grep#2528) - chore(deps): update dependency [@ast-grep/napi](https://github.com/ast-grep/napi) to v0.41.1 [`#2526`](ast-grep/ast-grep#2526) - chore(deps): update dependency oxlint to v1.52.0 [`#2524`](ast-grep/ast-grep#2524) - feat: support nth-child esquery [`#2546`](ast-grep/ast-grep#2546) - feat: support :is selector [`#2545`](ast-grep/ast-grep#2545) - feat: support :not selector [`#2544`](ast-grep/ast-grep#2544) - feat: support :has rule [`#2543`](ast-grep/ast-grep#2543) - fix(lsp): scan injected languages for diagnostics ([#2528](ast-grep/ast-grep#2528)) [`#2522`](ast-grep/ast-grep#2522) - feat: add parameterized util [`3d90372`](ast-grep/ast-grep@3d90372) - refactor: limit parameterized utils to globals [`2d69a34`](ast-grep/ast-grep@2d69a34) - refactor: move parameterized\_util [`c77e38d`](ast-grep/ast-grep@c77e38d) ##### [\`v0.41.1\`](https://github.com/ast-grep/ast-grep/blob/HEAD/CHANGELOG.md#0411) > 10 March 2026 - fix: lsp on change encounter deadlock [`#2511`](ast-grep/ast-grep#2511) - chore(deps): update dependency oxlint to v1.51.0 [`#2512`](ast-grep/ast-grep#2512) - chore(deps): update rust crate tempfile to v3.26.0 [`#2497`](ast-grep/ast-grep#2497) - chore(deps): update rust crate inquire to v0.9.4 [`#2498`](ast-grep/ast-grep#2498) - chore(deps): update dependency [@types/node](https://github.com/types/node) to v24.11.0 [`#2502`](ast-grep/ast-grep#2502) - chore(deps): update dependency dprint to v0.52.0 [`#2499`](ast-grep/ast-grep#2499) - fix: override severity on `rule` & `inline-rules` flags [`#2505`](ast-grep/ast-grep#2505) - chore(deps): update github artifact actions [`#2507`](ast-grep/ast-grep#2507) - chore(deps): update rust crate tree-sitter to v0.26.6 [`#2501`](ast-grep/ast-grep#2501) - chore(deps): update dependency web-tree-sitter to v0.26.6 [`#2500`](ast-grep/ast-grep#2500) - chore(deps): update dependency ava to v7 [`#2508`](ast-grep/ast-grep#2508) - chore(deps): update dependency oxlint to v1.50.0 [`#2495`](ast-grep/ast-grep#2495) - chore(deps): update dependency [@ast-grep/napi](https://github.com/ast-grep/napi) to v0.41.0 [`#2494`](ast-grep/ast-grep#2494) - fix: bump ls-types version [`#2525`](ast-grep/ast-grep#2525) - refactor: change versioned ast work [`a86b2ab`](ast-grep/ast-grep@a86b2ab) - fix: bump wasm-bindgen [`d23e334`](ast-grep/ast-grep@d23e334) - fix: fix wasm package [`0b92e94`](ast-grep/ast-grep@0b92e94) Renovate-Branch: renovate/2024.6-ast-grep-cli-0.x Change-Id: I91bcfa93167c7af0833afeb42df774621bad595c Priv-Id: ef06dc14816a639036bb167212251f765ce38ad8

Dsaquel and others added 2 commits March 1, 2026 16:30

refact: compute diagnostics independently of publish_diagnostics

ccdea4c

fix: drop versioned before sending request

ecfb28d

Co-authored-by: Kamil Jakubus <kamil.jakubus@usagi.coffee>

coderabbitai Bot reviewed Mar 1, 2026

View reviewed changes

HerringtonDarkholme approved these changes Mar 2, 2026

View reviewed changes

HerringtonDarkholme added this pull request to the merge queue Mar 2, 2026

HerringtonDarkholme removed this pull request from the merge queue due to a manual request Mar 2, 2026

HerringtonDarkholme merged commit d0c539d into ast-grep:main Mar 2, 2026
5 of 6 checks passed

Dsaquel deleted the fix/lsp-on-change-encounter-deadlock branch March 2, 2026 17:35

This was referenced May 12, 2026

Bug: LSP diagnostics silently dropped - missing tracing subscriber hides tower-lsp transport errors #2637

Closed

Bug: LSP diagnostics lost during initialization — publishDiagnostics suppressed before server enters Initialized state #2638

Closed

coderabbitai Bot mentioned this pull request May 12, 2026

fix(lsp): buffer diagnostics during initialization race (#2638) #2640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: lsp on change encounter deadlock#2511

fix: lsp on change encounter deadlock#2511
HerringtonDarkholme merged 2 commits into
ast-grep:mainfrom
Dsaquel:fix/lsp-on-change-encounter-deadlock

Dsaquel commented Mar 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Mar 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

HerringtonDarkholme commented Mar 1, 2026

Uh oh!

Dsaquel commented Mar 1, 2026

Uh oh!

HerringtonDarkholme left a comment

Uh oh!

Dsaquel commented Mar 2, 2026

Uh oh!

HerringtonDarkholme commented Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Dsaquel commented Mar 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

HerringtonDarkholme commented Mar 1, 2026

Uh oh!

Dsaquel commented Mar 1, 2026

Uh oh!

HerringtonDarkholme left a comment

Choose a reason for hiding this comment

Uh oh!

Dsaquel commented Mar 2, 2026

Uh oh!

HerringtonDarkholme commented Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dsaquel commented Mar 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 1, 2026 •

edited

Loading

codecov Bot commented Mar 1, 2026 •

edited

Loading