From e212948bcbb7baab2e686f5e0ad4243efca5016c Mon Sep 17 00:00:00 2001 From: Stefan Nitz Date: Sun, 7 Jun 2026 09:57:11 +0200 Subject: [PATCH] fix: sync open files to disk before rename to prevent silent corruption 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 --- .../rust/rename_symbol/stale_buffer_test.go | 90 +++++++++++++++++++ internal/lsp/client.go | 21 +++++ internal/tools/rename-symbol.go | 5 ++ 3 files changed, 116 insertions(+) create mode 100644 integrationtests/tests/rust/rename_symbol/stale_buffer_test.go diff --git a/integrationtests/tests/rust/rename_symbol/stale_buffer_test.go b/integrationtests/tests/rust/rename_symbol/stale_buffer_test.go new file mode 100644 index 00000000..bb1e0096 --- /dev/null +++ b/integrationtests/tests/rust/rename_symbol/stale_buffer_test.go @@ -0,0 +1,90 @@ +package rename_symbol_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/isaacphi/mcp-language-server/integrationtests/tests/rust/internal" + "github.com/isaacphi/mcp-language-server/internal/tools" +) + +// TestRenameSymbolStaleBuffer reproduces silent file corruption: when a file is +// edited on disk by a non-LSP path after the server opened it, and a rename is +// issued before the server's buffer is re-synced, the server plans the +// WorkspaceEdit against its stale (pre-edit) buffer. Those ranges point at where +// references used to be; applied to the now-larger on-disk file they overwrite +// unrelated text and miss the real references, while the tool reports success. +// +// It runs the real configuration (watcher ON). rust-analyzer is warmed first so +// the rename completes in ~1ms, far inside the watcher's 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 with ContentModified rather than +// flake. A correct bridge must sync open files to disk before renaming. +func TestRenameSymbolStaleBuffer(t *testing.T) { + suite := internal.GetTestSuite(t) + + ctx, cancel := context.WithTimeout(suite.Context, 30*time.Second) + defer cancel() + + for _, f := range []string{ + "src/main.rs", "src/types.rs", "src/helper.rs", + "src/consumer.rs", "src/another_consumer.rs", "src/clean.rs", + } { + if err := suite.Client.OpenFile(ctx, filepath.Join(suite.WorkspaceDir, f)); err != nil { + t.Logf("open %s: %v", f, err) + } + } + time.Sleep(3 * time.Second) + + // Force rust-analyzer's first (slow) analysis now so the timed rename is fast. + for i := 0; i < 3; i++ { + if _, err := tools.FindReferences(ctx, suite.Client, "SHARED_CONSTANT"); err != nil { + t.Logf("warm-up references: %v", err) + } + } + + consumerPath := filepath.Join(suite.WorkspaceDir, "src/consumer.rs") + original, err := os.ReadFile(consumerPath) + if err != nil { + t.Fatalf("read consumer.rs: %v", err) + } + + // Edit consumer.rs on disk without notifying the server: prepend marker lines + // so every SHARED_CONSTANT reference shifts down, off the server's stale view. + const markerCount = 8 + var markers strings.Builder + for i := 0; i < markerCount; i++ { + fmt.Fprintf(&markers, "// REPRO MARKER %d - must be preserved verbatim\n", i) + } + if err := os.WriteFile(consumerPath, append([]byte(markers.String()), original...), 0644); err != nil { + t.Fatalf("write consumer.rs: %v", err) + } + + // Rename the constant at its definition in types.rs (line 78, col 11). + typesPath := filepath.Join(suite.WorkspaceDir, "src/types.rs") + t0 := time.Now() + result, err := tools.RenameSymbol(ctx, suite.Client, typesPath, 78, 11, "RENAMED_CONSTANT") + t.Logf("rename took %v (watcher debounce is 300ms)", time.Since(t0)) + if err != nil { + t.Fatalf("RenameSymbol returned error (likely ContentModified, watcher beat the rename): %v", err) + } + t.Logf("rename result: %s", result) + + got, err := os.ReadFile(consumerPath) + if err != nil { + t.Fatalf("re-read consumer.rs: %v", err) + } + gotStr := string(got) + + // A correct rename replaces both SHARED_CONSTANT references and touches nothing + // else; any other bytes are the stale-position corruption. + want := markers.String() + strings.ReplaceAll(string(original), "SHARED_CONSTANT", "RENAMED_CONSTANT") + if gotStr != want { + t.Errorf("consumer.rs corrupted by a stale-position rename.\n--- got ---\n%s\n--- want ---\n%s", gotStr, want) + } +} diff --git a/internal/lsp/client.go b/internal/lsp/client.go index fc07059d..55880bf7 100644 --- a/internal/lsp/client.go +++ b/internal/lsp/client.go @@ -366,6 +366,27 @@ func (c *Client) NotifyChange(ctx context.Context, filepath string) error { return c.Notify(ctx, "textDocument/didChange", params) } +// SyncOpenFiles re-sends every open file's current on-disk content to the server +// via didChange. Callers that plan edits from the server's view of a document +// must call this first (see RenameSymbol). +func (c *Client) SyncOpenFiles(ctx context.Context) { + c.openFilesMu.RLock() + uris := make([]string, 0, len(c.openFiles)) + for uri := range c.openFiles { + uris = append(uris, uri) + } + c.openFilesMu.RUnlock() + + for _, uri := range uris { + path := strings.TrimPrefix(uri, "file://") + if err := c.NotifyChange(ctx, path); err != nil { + // Unreadable (e.g. deleted): leave it stale. ApplyWorkspaceEdit will + // then error on it rather than corrupt it. + lspLogger.Debug("SyncOpenFiles: skipping %s: %v", path, err) + } + } +} + func (c *Client) CloseFile(ctx context.Context, filepath string) error { uri := fmt.Sprintf("file://%s", filepath) diff --git a/internal/tools/rename-symbol.go b/internal/tools/rename-symbol.go index f51e3e73..2b61fbcf 100644 --- a/internal/tools/rename-symbol.go +++ b/internal/tools/rename-symbol.go @@ -39,6 +39,11 @@ func RenameSymbol(ctx context.Context, client *lsp.Client, filePath string, line // Skip the PrepareRename check as it might not be supported by all language servers // Execute the rename directly + // Sync the server's view of every open file to disk before planning the + // rename: an out-of-band on-disk edit after the file was opened otherwise + // leaves a stale buffer, and the rename's ranges land at stale positions. + client.SyncOpenFiles(ctx) + // Execute the rename operation workspaceEdit, err := client.Rename(ctx, params) if err != nil {