From d367557eb1949bbfe9d497badc23b1029b027a30 Mon Sep 17 00:00:00 2001 From: Kent Sibilev Date: Fri, 6 Nov 2026 23:32:47 -0500 Subject: [PATCH] Parse documents in the worker thread after queued edits are applied Since text synchronization notifications were moved to the worker queue (#3637), the reader thread could parse a document for an incoming request before the didChange edits queued ahead of it were applied. The worker then applied the edits (clearing the cache) and ran the request against the outdated parse result, caching the stale response until the next edit. This shows up as syntax error diagnostics that persist after the code is fixed. Move the parse (and the ERB virtual state delegation) into the worker, right before the request is processed, so it always reflects every edit received before the request. Co-Authored-By: Claude Opus 5.5 --- lib/ruby_lsp/base_server.rb | 84 +++++++++++++++++++------------------ test/server_test.rb | 37 ++++++++++++++++ 2 files changed, 81 insertions(+), 40 deletions(-) diff --git a/lib/ruby_lsp/base_server.rb b/lib/ruby_lsp/base_server.rb index 2a5a1c9717..14e630d711 100644 --- a/lib/ruby_lsp/base_server.rb +++ b/lib/ruby_lsp/base_server.rb @@ -39,46 +39,13 @@ def start @reader.each_message do |message| method = message[:method] - # We must parse the document under a mutex lock or else we might switch threads and accept text edits in the - # source. Altering the source reference during parsing will put the parser in an invalid internal state, since - # it started parsing with one source but then it changed in the middle. We don't want to do this for text - # synchronization notifications - @global_state.synchronize do - uri = message.dig(:params, :textDocument, :uri) - - if uri - begin - parsed_uri = URI(uri) - message[:params][:textDocument][:uri] = parsed_uri - - # We don't want to try to parse documents on text synchronization notifications - unless method.start_with?("textDocument/did") - document = @store.get(parsed_uri) - - # If the client supports request delegation and we're working with an ERB document and there was - # something to parse, then we have to maintain the client updated about the virtual state of the host - # language source - if document.parse! && @global_state.client_capabilities.supports_request_delegation && - document.is_a?(ERBDocument) - - send_message( - Notification.new( - method: "delegate/textDocument/virtualState", - params: { - textDocument: { - uri: uri, - text: document.host_language_source, - }, - }, - ), - ) - end - end - rescue URI::Error - # A client can send a document URI with a scheme Ruby's URI parser rejects, so we don't want to fail - rescue Store::NonExistingDocumentError - # If we receive a request for a file that no longer exists, we don't want to fail - end + uri = message.dig(:params, :textDocument, :uri) + + if uri + begin + message[:params][:textDocument][:uri] = URI(uri) + rescue URI::Error + # A client can send a document URI with a scheme Ruby's URI parser rejects, so we don't want to fail end end @@ -178,11 +145,48 @@ def handle_incoming_message(message) end end + parse_document(message) process_message(message) # Ensure we remove the request if it got cancelled while it was being processed @cancelled_requests.delete(id) end + # Parse the document targeted by a request right before processing it. This must happen in the worker thread, after + # any text synchronization notifications that were queued ahead of the request have been applied. Parsing when the + # message is read would use the source from before those edits, and the request would then compute (and cache) its + # response from an outdated parse result + #: (Hash[Symbol, untyped] message) -> void + def parse_document(message) + method = message[:method] + uri = message.dig(:params, :textDocument, :uri) + return if method.nil? || method.start_with?("textDocument/did") || !uri.is_a?(URI::Generic) + + @global_state.synchronize do + document = @store.get(uri) + + # If the client supports request delegation and we're working with an ERB document and there was + # something to parse, then we have to maintain the client updated about the virtual state of the host + # language source + if document.parse! && @global_state.client_capabilities.supports_request_delegation && + document.is_a?(ERBDocument) + + send_message( + Notification.new( + method: "delegate/textDocument/virtualState", + params: { + textDocument: { + uri: uri.to_s, + text: document.host_language_source, + }, + }, + ), + ) + end + rescue Store::NonExistingDocumentError + # If we receive a request for a file that no longer exists, we don't want to fail + end + end + #: ((Result | Error | Notification | Request) message) -> void def send_message(message) # When we're shutting down the server, there's a small race condition between closing the thread queues and diff --git a/test/server_test.rb b/test/server_test.rb index 1301c8b59c..fe51916361 100644 --- a/test/server_test.rb +++ b/test/server_test.rb @@ -1779,6 +1779,43 @@ def test_does_not_crash_when_a_document_uri_cannot_be_parsed server&.run_shutdown end + def test_requests_parse_the_document_after_queued_edits_are_applied + # Text synchronization notifications are processed in the worker thread. A request read right after an edit must + # not be computed (and cached) from a parse result that predates that edit + uri = URI::Generic.from_path(path: File.join(Dir.pwd, "fake.rb")).to_s + messages = [ + { + method: "textDocument/didOpen", + params: { textDocument: { uri: uri, text: "def foo", version: 1, languageId: "ruby" } }, + }, + { + method: "textDocument/didChange", + params: { + textDocument: { uri: uri, version: 2 }, + contentChanges: [ + { range: { start: { line: 0, character: 7 }, end: { line: 0, character: 7 } }, text: "\nend" }, + ], + }, + }, + { id: 1, method: "textDocument/diagnostic", params: { textDocument: { uri: uri } } }, + ] + input = messages.map do |message| + body = message.to_json + "Content-Length: #{body.bytesize}\r\n\r\n#{body}" + end.join + + @server.run_shutdown + @server = RubyLsp::Server.new(test_mode: true, reader: StringIO.new(input), writer: StringIO.new) + @server.global_state.stubs(:active_linters).returns([]) + + capture_subprocess_io { @server.start } + + result = find_message(RubyLsp::Result, id: 1) + items = result.response #: as RubyLsp::Interface::FullDocumentDiagnosticReport + .items + assert_empty(items.map(&:message)) + end + private def run_initialize_server_with_setup_error(error)