fix: sync open files to disk before rename to prevent silent corruption#141
Open
Roxxik wants to merge 1 commit into
Open
fix: sync open files to disk before rename to prevent silent corruption#141Roxxik wants to merge 1 commit into
Roxxik wants to merge 1 commit into
Conversation
rename_symbol planned its WorkspaceEdit from rust-analyzer's in-memory
buffer, which goes stale when a file is edited on disk by a non-LSP path
after it was opened: OpenFile is a no-op once a file is open, and the
workspace watcher's didChange is debounced (300ms) and races the rename.
The server then returns edit ranges computed for the pre-edit content,
and the bridge applies those {line,character} ranges to the current
(grown) file -- overwriting unrelated text at stale positions and
missing the real references, while reporting success. The clobbered
positions are in-bounds, so no range/bounds check can catch them; the
buffer must be made current before the rename is planned.
Fix: add Client.SyncOpenFiles, which re-sends every open file's current
on-disk content via didChange, and call it in RenameSymbol before
issuing the rename. The didChange notifications and the rename request
are ordered on the connection, so the rename is planned against current
content.
Add a regression test that reproduces the corruption faithfully: watcher
ON (production config), rust-analyzer warmed so the rename completes in
~1ms -- far inside the 300ms debounce -- and wins the race against the
watcher, exactly as in production. It fails (silent corruption) before
the fix and passes after.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
rename_symbolcan silently corrupt files. If a file is edited on disk by a non-LSP path (an editor or agent writing directly) after the server has opened it, and a rename is then issued, the server plans theWorkspaceEditagainst its stale in-memory buffer. The bridge applies those{line, character}ranges to the current on-disk file, overwriting unrelated text at stale positions and missing the real references, while reporting success.Concretely, renaming
SHARED_CONSTANTafter prepending lines to a referencing file produced:with the real references left unrenamed and the tool reporting
Successfully renamed.Root cause
Client.OpenFileis a no-op once a file is open, so the server keeps the version it saw atdidOpen.OpenFile; it never re-syncs.didChangeis debounced (300ms) and races the rename, so it routinely loses.The corrupted positions are in-bounds (valid coordinates, wrong place), so no range/bounds check can catch this. The buffer must be made current before the rename is planned.
Fix
Add
Client.SyncOpenFiles, which re-sends every open file's current on-disk content viadidChange, and call it inRenameSymbolbefore issuing the rename. Notifications and the rename request are ordered on the connection, so the rename is planned against current content.Test
TestRenameSymbolStaleBufferreproduces the corruption faithfully under the real configuration (watcher on). rust-analyzer is warmed so the rename completes in ~1ms, well inside the 300ms debounce, and wins the race exactly as in production. The ~200x margin keeps it deterministic; if the rename ever lost the race it fails loudly withContentModifiedrather than flaking. It fails (silent corruption) before the fix and passes after; existing rename tests are unaffected.Known limitations
SyncOpenFilesandApplyWorkspaceEdit's disk re-read; it is inherent to the architecture and strictly smaller than before.🤖 Generated with Claude Code