Skip to content

Conversation

gaauwe
Copy link

@gaauwe gaauwe commented Oct 15, 2025

Summary

Fixed a race condition between initialized and did_open that caused the requireConfiguration setting to sometimes be ignored in editor extensions.

Related: zed-industries/zed#39803, biomejs/biome-zed#59, #6589, #6287

The Problem

There was a race condition where did_open could execute before initialized completed loading extension settings. This manifested differently in different editors:

Zed: The LSP server starts when a file is opened, causing did_open to execute before extension settings are loaded from initialized, consistently ignoring the requireConfiguration setting.

VS Code: The LSP server starts even without open files. The bug would occur when:

  • Opening VS Code with files already open (both handlers race)
  • Restarting the LSP with files already open (both handlers race)

It would work correctly only when opening VS Code without files first (allowing initialized to complete), then opening files afterward.

The Solution

This fix ensures load_extension_settings() is called in did_open when creating a new project, before loading the Biome configuration file. This guarantees extension settings are available regardless of the timing between initialized and did_open, maintaining consistency with other configuration loading flows.

Test Plan

Manual testing in Zed:

  1. Configure the require_config_file setting in Zed's Biome extension settings
  2. Open a file in a project
  3. Verify that the requireConfiguration setting is respected

Manual testing in VS Code:

  1. Configure the requireConfiguration setting in VS Code's Biome extension settings
  2. Test scenario A: Open VS Code with files already open
  3. Test scenario B: Restart the LSP server with files open
  4. Verify that the requireConfiguration setting is respected in both scenarios

Copy link

changeset-bot bot commented Oct 15, 2025

🦋 Changeset detected

Latest commit: 44b0f92

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

Walkthrough

Adds a synchronous initialization marker and explicit initialization tracking to the LSP session (an AtomicBool plus set_initialized/has_initialized APIs). Ensures extension settings are loaded (awaiting session.load_extension_settings()) before loading the Biome configuration when a text document is opened, and calls session.set_initialized() during server initialization. Also bumps @biomejs/biome to a patch version. No exported/public API signatures were changed.

Possibly related PRs

Suggested labels

A-Project

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly and concisely summarises the main change by stating that the LSP fix loads extension settings before configuration on document open, which aligns with the core race-condition resolution described in the changeset.
Description Check ✅ Passed The description directly addresses the race condition between initialized and did_open, outlines the problem scenarios, the implemented solution, and a manual test plan, all of which correspond closely to the changes in the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 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.

@github-actions github-actions bot added the A-LSP Area: language server protocol label Oct 15, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (2)
crates/biome_lsp/src/handlers/text_document.rs (2)

35-35: Remove debug statement.

This eprintln! appears to be leftover debug code and should be removed or converted to proper logging via the tracing crate.

Apply this diff:

-    eprintln!("Session id {:?}", session.key);

74-77: Remove or replace debug output.

Similar to line 35, this eprintln! should be removed or converted to proper structured logging using the tracing crate (e.g., debug! or info!).

Apply this diff:

-            eprintln!(
-                "Loading configuration from text_document {:?}",
-                &project_path
-            );
+            debug!(
+                "Loading configuration from text_document {:?}",
+                &project_path
+            );
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d37638d and 758e841.

📒 Files selected for processing (2)
  • .changeset/swift-apples-greet.md (1 hunks)
  • crates/biome_lsp/src/handlers/text_document.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place core crates under /crates/biome_*/

Files:

  • crates/biome_lsp/src/handlers/text_document.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Format Rust files before committing (e.g., via just f which formats Rust)
Document rules, assists, and options with inline rustdoc in source

Files:

  • crates/biome_lsp/src/handlers/text_document.rs
.changeset/*.md

📄 CodeRabbit inference engine (CONTRIBUTING.md)

.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period

Files:

  • .changeset/swift-apples-greet.md
🔇 Additional comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)

78-78: load_extension_settings handles errors internally and is idempotent
Client configuration failures and JSON parse errors are caught and logged without panicking; each invocation safely overwrites the stored settings.

@gaauwe gaauwe force-pushed the fix/did-open-load-extension-settings branch from 758e841 to e736d9e Compare October 15, 2025 22:20
@gaauwe gaauwe force-pushed the fix/did-open-load-extension-settings branch from e736d9e to 5786194 Compare October 15, 2025 22:27
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
crates/biome_lsp/src/session.rs (1)

630-638: Use Acquire/Release for initialized atomics

Apply this diff to explicitly encode the happens-before relationship:

@@ crates/biome_lsp/src/session.rs
-    self.initialized.store(true, Ordering::Relaxed);
+    self.initialized.store(true, Ordering::Release);

-    self.initialized.load(Ordering::Relaxed)
+    self.initialized.load(Ordering::Acquire)

The RwLock on extension_settings makes Relaxed safe, but Release/Acquire is clearer. Confirmed that did_open’s has_initialized() check already handles the false case correctly.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5786194 and 44b0f92.

📒 Files selected for processing (3)
  • crates/biome_lsp/src/handlers/text_document.rs (1 hunks)
  • crates/biome_lsp/src/server.rs (1 hunks)
  • crates/biome_lsp/src/session.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/biome_lsp/src/handlers/text_document.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place core crates under /crates/biome_*/

Files:

  • crates/biome_lsp/src/session.rs
  • crates/biome_lsp/src/server.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Format Rust files before committing (e.g., via just f which formats Rust)
Document rules, assists, and options with inline rustdoc in source

Files:

  • crates/biome_lsp/src/session.rs
  • crates/biome_lsp/src/server.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/session.rs (1)
crates/biome_lsp/src/server.rs (2)
  • new (49-61)
  • new (548-564)
🔇 Additional comments (3)
crates/biome_lsp/src/session.rs (2)

89-90: Field addition looks good.

The initialized flag with AtomicBool is appropriate for tracking the LSP initialization state in a thread-safe manner.


219-219: Initialization is correct.

Properly initialised to false since the session hasn't received the initialized() notification at construction time.

crates/biome_lsp/src/server.rs (1)

317-317: Correct placement to fix the race condition.

Calling set_initialized() after loading extension and workspace settings ensures that did_open can reliably check whether initialization has completed, preventing the race condition where did_open runs before settings are loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LSP Area: language server protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant