Eagerly compute SHA's for watched files (#2861)

### Motivation

This PR fixes two issues related to automated restarts. The first one is preventing undesired restarts immediately after the server finishes booting.

The second one is preventing unwanted restarts if a create event is fired for an existing file. In theory, this shouldn't happen, but our tests tell a different story and we consistently see a create event getting fired for updates.

### Implementation

The two fixes are:

1. Unified all of our watchers to only check the top level configuration files and then started eagerly computing the SHAs for all of them (if they are present). This will prevent unwanted restarts if these files are touched immediately after boot
2. Started using the hash check for `didCreate` as well. If the file being updated fires a create event and the content SHA matches

### Automated Tests

Added a new test and reduced some of the sleeps on the other test, which is taking a bit too long.

Author: vinistock

vscode/src/test/suite/workspace.test.ts

@@ -59,15 +59,15 @@ suite("Workspace", () => {
 
     await workspace.activate();
 
-    for (let i = 0; i < 5; i++) {
-      await new Promise((resolve) => setTimeout(resolve, 200));
+    for (let i = 0; i < 4; i++) {
+      await new Promise((resolve) => setTimeout(resolve, 50));
       fs.writeFileSync(path.join(gitDir, "rebase-apply"), "1");
-      await new Promise((resolve) => setTimeout(resolve, 200));
+      await new Promise((resolve) => setTimeout(resolve, 50));
       fs.rmSync(path.join(gitDir, "rebase-apply"));
     }
 
     // Give enough time for all watchers to fire and all debounces to run off
-    await new Promise((resolve) => setTimeout(resolve, 10000));
+    await new Promise((resolve) => setTimeout(resolve, 6000));
 
     startStub.restore();
     restartSpy.restore();
@@ -77,4 +77,40 @@ suite("Workspace", () => {
     // The restart call only happens once because of the debounce
     assert.strictEqual(restartSpy.callCount, 1);
   }).timeout(60000);
+
+  test("does not restart when watched files are touched without modifying contents", async () => {
+    const lockfileUri = vscode.Uri.file(
+      path.join(workspacePath, "Gemfile.lock"),
+    );
+    const contents = Buffer.from("hello");
+    await vscode.workspace.fs.writeFile(lockfileUri, contents);
+
+    const workspace = new Workspace(
+      context,
+      workspaceFolder,
+      FAKE_TELEMETRY,
+      () => {},
+      new Map(),
+    );
+
+    await workspace.activate();
+
+    const startStub = sinon.stub(workspace, "start");
+    const restartSpy = sinon.spy(workspace, "restart");
+
+    for (let i = 0; i < 4; i++) {
+      const updateDate = new Date();
+      fs.utimesSync(lockfileUri.fsPath, updateDate, updateDate);
+      await new Promise((resolve) => setTimeout(resolve, 50));
+    }
+
+    // Give enough time for all watchers to fire and all debounces to run off
+    await new Promise((resolve) => setTimeout(resolve, 6000));
+
+    startStub.restore();
+    restartSpy.restore();
+
+    assert.strictEqual(startStub.callCount, 0);
+    assert.strictEqual(restartSpy.callCount, 0);
+  }).timeout(10000);
 });

vscode/src/workspace.ts

@@ -14,6 +14,13 @@ import {
 } from "./common";
 import { WorkspaceChannel } from "./workspaceChannel";
 
+const WATCHED_FILES = [
+  "Gemfile.lock",
+  "gems.locked",
+  ".rubocop.yml",
+  ".rubocop",
+];
+
 export class Workspace implements WorkspaceInterface {
   public lspClient?: Client;
   public readonly ruby: Ruby;
@@ -58,8 +65,6 @@ export class Workspace implements WorkspaceInterface {
     this.createTestItems = createTestItems;
     this.isMainWorkspace = isMainWorkspace;
     this.virtualDocuments = virtualDocuments;
-
-    this.registerRestarts(context);
   }
 
   // Activate this workspace. This method is intended to be invoked only once, unlikely `start` which may be invoked
@@ -82,6 +87,20 @@ export class Workspace implements WorkspaceInterface {
       rootGitUri,
       ".git/{rebase-merge,rebase-apply,BISECT_START,CHERRY_PICK_HEAD}",
     );
+
+    this.registerRestarts();
+
+    // Eagerly calculate SHAs for the watched files to avoid unnecessary restarts
+    for (const file of WATCHED_FILES) {
+      const uri = vscode.Uri.joinPath(this.workspaceFolder.uri, file);
+      const currentSha = await this.fileContentsSha(uri);
+
+      if (!currentSha) {
+        continue;
+      }
+
+      this.restartDocumentShas.set(uri.fsPath, currentSha);
+    }
   }
 
   async start(debugMode?: boolean) {
@@ -164,7 +183,9 @@ export class Workspace implements WorkspaceInterface {
       // server can now handle shutdown requests
       if (this.needsRestart) {
         this.needsRestart = false;
-        await this.restart();
+        await this.debouncedRestart(
+          "a restart was requested while the server was still booting",
+        );
       }
     } catch (error: any) {
       this.error = true;
@@ -342,67 +363,49 @@ export class Workspace implements WorkspaceInterface {
     return result;
   }
 
-  private registerRestarts(context: vscode.ExtensionContext) {
-    this.createRestartWatcher(context, "Gemfile.lock");
-    this.createRestartWatcher(context, "gems.locked");
-    this.createRestartWatcher(context, "**/.rubocop.yml");
-    this.createRestartWatcher(context, ".rubocop");
+  private registerRestarts() {
+    this.createRestartWatcher(`{${WATCHED_FILES.join(",")}}`);
 
     // If a configuration that affects the Ruby LSP has changed, update the client options using the latest
     // configuration and restart the server
-    context.subscriptions.push(
+    this.context.subscriptions.push(
       vscode.workspace.onDidChangeConfiguration(async (event) => {
         if (event.affectsConfiguration("rubyLsp")) {
-          // Re-activate Ruby if the version manager changed
-          if (
-            event.affectsConfiguration("rubyLsp.rubyVersionManager") ||
-            event.affectsConfiguration("rubyLsp.bundleGemfile") ||
-            event.affectsConfiguration("rubyLsp.customRubyCommand")
-          ) {
-            await this.ruby.activateRuby();
-          }
-
-          this.outputChannel.info(
-            "Restarting the Ruby LSP because configuration changed",
-          );
-          await this.restart();
+          await this.debouncedRestart("configuration changed");
         }
       }),
     );
   }
 
-  private createRestartWatcher(
-    context: vscode.ExtensionContext,
-    pattern: string,
-  ) {
+  private createRestartWatcher(pattern: string) {
     const watcher = vscode.workspace.createFileSystemWatcher(
       new vscode.RelativePattern(this.workspaceFolder, pattern),
     );
 
     // Handler for only triggering restart if the contents of the file have been modified. If the file was just touched,
     // but the contents are the same, we don't want to restart
     const debouncedRestartWithHashCheck = async (uri: vscode.Uri) => {
-      const fileContents = await vscode.workspace.fs.readFile(uri);
       const fsPath = uri.fsPath;
+      const currentSha = await this.fileContentsSha(uri);
 
-      const hash = createHash("sha256");
-      hash.update(fileContents.toString());
-      const currentSha = hash.digest("hex");
-
-      if (this.restartDocumentShas.get(fsPath) !== currentSha) {
+      if (currentSha && this.restartDocumentShas.get(fsPath) !== currentSha) {
         this.restartDocumentShas.set(fsPath, currentSha);
-        await this.debouncedRestart(`${pattern} changed`);
+        await this.debouncedRestart(`${fsPath} changed, matching ${pattern}`);
       }
     };
 
-    const debouncedRestart = async () => {
-      await this.debouncedRestart(`${pattern} changed`);
+    const debouncedRestart = async (uri: vscode.Uri) => {
+      this.restartDocumentShas.delete(uri.fsPath);
+      await this.debouncedRestart(`${uri.fsPath} changed, matching ${pattern}`);
     };
 
-    context.subscriptions.push(
+    this.context.subscriptions.push(
       watcher,
       watcher.onDidChange(debouncedRestartWithHashCheck),
-      watcher.onDidCreate(debouncedRestart),
+      // Interestingly, we are seeing create events being fired even when the file already exists. If a create event is
+      // fired for an update to an existing file, then we need to check if the contents still match to prevent unwanted
+      // restarts
+      watcher.onDidCreate(debouncedRestartWithHashCheck),
       watcher.onDidDelete(debouncedRestart),
     );
   }
@@ -416,9 +419,9 @@ export class Workspace implements WorkspaceInterface {
       this.#inhibitRestart = true;
     };
 
-    const stop = async () => {
+    const stop = async (uri: vscode.Uri) => {
       this.#inhibitRestart = false;
-      await this.debouncedRestart(`${glob} changed`);
+      await this.debouncedRestart(`${uri.fsPath} changed, matching ${glob}`);
     };
 
     this.context.subscriptions.push(
@@ -429,4 +432,18 @@ export class Workspace implements WorkspaceInterface {
       workspaceWatcher.onDidDelete(stop),
     );
   }
+
+  private async fileContentsSha(uri: vscode.Uri): Promise<string | undefined> {
+    let fileContents;
+
+    try {
+      fileContents = await vscode.workspace.fs.readFile(uri);
+    } catch (error: any) {
+      return undefined;
+    }
+
+    const hash = createHash("sha256");
+    hash.update(fileContents.toString());
+    return hash.digest("hex");
+  }
 }