Skip to content

Commit 6872268

Browse files
committed
fix(lsp): address race condition and misleading comments in version tracking
Addresses three Copilot review comments: 1. Fixed TOCTOU race in validate_from_content_and_publish: capture version at the same time as content snapshot to ensure diagnostics are published with the version matching the content they were validated against, not a newer version from concurrent did_change events. 2. Corrected misleading comments in handle_did_open/did_change about lock guarantees. The comments suggested locks prevent readers from seeing inconsistency, but that's only true if readers atomically capture both values together (which they now do). 3. Clarified integration test comment to explain why it doesn't assert version field in publish_diagnostics (requires consuming socket messages). Referenced unit tests in backend/tests.rs that verify version lifecycle.
1 parent 714476a commit 6872268

3 files changed

Lines changed: 20 additions & 10 deletions

File tree

crates/agnix-lsp/src/backend.rs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -200,13 +200,15 @@ impl Backend {
200200
}
201201
}
202202

203-
// Get content from cache
204-
let (content, expected_content) = {
203+
// Get content from cache and capture version at same time to avoid TOCTOU
204+
// between content validation and version publish
205+
let (content, expected_content, captured_version) = {
205206
let docs = self.documents.read().await;
206207
match docs.get(&uri) {
207208
Some(cached) => {
208209
let snapshot = Arc::clone(cached);
209-
(Arc::clone(&snapshot), Some(snapshot))
210+
let version = self.get_document_version(&uri).await;
211+
(Arc::clone(&snapshot), Some(snapshot), version)
210212
}
211213
None => {
212214
// Fall back to file-based validation
@@ -267,8 +269,9 @@ impl Backend {
267269
return;
268270
}
269271

270-
// Read version just-in-time to minimize TOCTOU window
271-
let version = self.get_document_version(&uri).await;
272+
// Use version captured at time of content snapshot to avoid publishing
273+
// newer version with older (already-validated) diagnostics
274+
let version = captured_version;
272275
self.client
273276
.publish_diagnostics(uri, diagnostics, version)
274277
.await;

crates/agnix-lsp/src/backend/events.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@ impl Backend {
1414
std::borrow::Cow::Borrowed(_) => raw,
1515
std::borrow::Cow::Owned(normalized) => normalized,
1616
};
17-
// Acquire both locks before releasing either so readers never see a
18-
// state where content is updated but version is not (or vice versa).
17+
// Acquire both locks atomically to update content and version together.
18+
// Readers that need both values must capture them in a single operation
19+
// (see validate_from_content_and_publish).
1920
{
2021
let mut docs = self.documents.write().await;
2122
let mut versions = self.document_versions.write().await;
@@ -38,8 +39,9 @@ impl Backend {
3839
std::borrow::Cow::Borrowed(_) => raw,
3940
std::borrow::Cow::Owned(normalized) => normalized,
4041
};
41-
// Acquire both locks before releasing either so readers never see a
42-
// state where content is updated but version is not (or vice versa).
42+
// Acquire both locks atomically to update content and version together.
43+
// Readers that need both values must capture them in a single operation
44+
// (see validate_from_content_and_publish).
4345
{
4446
let mut docs = self.documents.write().await;
4547
let mut versions = self.document_versions.write().await;

crates/agnix-lsp/tests/lsp_integration.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1579,7 +1579,12 @@ mod document_version_tests {
15791579
// This test exercises the version lifecycle through the LanguageServer
15801580
// trait boundary and uses hover as a behavioral indicator of document
15811581
// cache state (hover returns Some when cached, None after close).
1582-
1582+
//
1583+
// Note: This test verifies that versions are tracked and available
1584+
// for publishing. The actual assertion that diagnostics are published
1585+
// with the correct version field requires consuming LSP notifications
1586+
// from the socket. See backend/tests.rs for unit tests that verify
1587+
// version lifecycle (test_document_version_tracked_on_open, etc.).
15831588
// Phase 1: Open with version 1
15841589
service
15851590
.inner()

0 commit comments

Comments
 (0)